Feature/auth flow #453
Feature/auth flow #453
Conversation
- Add AuthConfig, AuthCredential, AuthScheme types - Add AuthHandler for OAuth2 flow orchestration - Add CredentialManager and CredentialService for token management - Add OAuth2 token exchange helpers - Add unit tests for auth package
Summary of ChangesHello @cpunion, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the system's authentication capabilities by introducing a robust and extensible framework for managing various authentication types, with a strong focus on OAuth2 flows. It provides structured components for defining, handling, and persisting credentials, and seamlessly integrates these capabilities into the tool execution pipeline, allowing agents to dynamically request and utilize user authorization for API interactions. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive authentication framework, including support for various auth schemes like OAuth2, and integrates it with a new OpenAPI toolset. The implementation is extensive and well-structured, covering credential management, token exchange, and refresh logic. The new auth package is the core of this feature, and the openapitoolset provides a practical application of it. I've identified several areas for improvement, primarily concerning error handling and robustness. Specifically, there are cases where errors from JSON parsing and credential loading/refreshing are silently ignored, which could lead to unexpected behavior. Additionally, the example OAuth2 handler re-implements logic that is already available in the standard oauth2 library, making it more fragile. Addressing these points will significantly improve the reliability and maintainability of this new auth flow.
cpunion
force-pushed
the
feature/auth-flow
branch
4 times, most recently
from
bc2af3e to
695a733
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive authentication flow, primarily for OAuth2/OIDC, by adding a new auth package and integrating it into the existing agent and tool infrastructure. The changes are extensive, including new data structures for auth configurations and credentials, handlers for the auth lifecycle, and modifications to the core agent flow to support auth-related tool re-execution. While the overall structure is sound and aligns with the goals, I've identified several critical and high-severity issues related to error handling and security that need to be addressed. Specifically, there are cases of ignored errors in security-sensitive areas like random state generation and credential persistence, which could lead to vulnerabilities or runtime failures.
auth/auth_handler.go
Outdated
| // generateRandomState generates a random state string for OAuth CSRF protection. | ||
| func generateRandomState() string { | ||
| b := make([]byte, 16) | ||
| rand.Read(b) |
There was a problem hiding this comment.
The error from crypto/rand.Read is ignored. If reading from the OS's entropy source fails, the generated state for OAuth will not be random, which could compromise CSRF protection. This is a critical security risk. The error must be handled. Panicking would be appropriate here, as this indicates a critical failure in the environment.
if _, err := rand.Read(b); err != nil {
// This is a critical failure. The OS's entropy source is not working.
// Panicking is the only safe option.
panic(fmt.Sprintf("failed to generate random state for OAuth: %v", err))
}
auth/credential_manager.go
Outdated
| if len(credentialService) > 0 && credentialService[0] != nil { | ||
| svc = credentialService[0] | ||
| if credential == nil { | ||
| loaded, _ := svc.LoadCredential(ctx, m.authConfig) |
There was a problem hiding this comment.
The error returned from svc.LoadCredential is being ignored. If loading the credential from the service fails due to a transient issue, the error is silently swallowed. This could lead to unnecessary re-authentication flows. The error should be propagated and handled by the caller.
| loaded, _ := svc.LoadCredential(ctx, m.authConfig) | |
| loaded, err := svc.LoadCredential(ctx, m.authConfig) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to load credential from service: %w", err) | |
| } |
auth/credential_manager.go
Outdated
| m.authConfig.ExchangedAuthCredential = credential | ||
| // Save to credential service for persistence across requests | ||
| if svc != nil { | ||
| svc.SaveCredential(ctx, m.authConfig) |
There was a problem hiding this comment.
The error from svc.SaveCredential is ignored. If saving the credential fails, it won't be persisted for subsequent requests, potentially forcing the user to re-authenticate unnecessarily. This error should be handled, for example by returning it from GetAuthCredential.
if err := svc.SaveCredential(ctx, m.authConfig); err != nil {
// Consider logging the error or propagating it.
}| refreshed, err := ref.Refresh(ctx, cred, m.authConfig.AuthScheme) | ||
| if err != nil { | ||
| // On refresh failure, return original | ||
| return cred, false | ||
| } |
There was a problem hiding this comment.
The error returned by ref.Refresh is swallowed. If a token refresh fails, the original (likely expired) credential is returned without any indication of the failure. This can lead to subsequent API calls failing unexpectedly. The error should be propagated up to the caller of GetAuthCredential so it can be handled appropriately.
refreshed, err := ref.Refresh(ctx, cred, m.authConfig.AuthScheme)
if err != nil {
// On refresh failure, return an error instead of the original credential.
return nil, fmt.Errorf("failed to refresh credential: %w", err)
}
auth/helpers.go
Outdated
| if err := json.Unmarshal(credentialJSON, &saCred); err == nil { | ||
| cred.ServiceAccount.ServiceAccountCredential = &saCred | ||
| } |
There was a problem hiding this comment.
The error from json.Unmarshal is silently ignored. If the provided credentialJSON is invalid, this function will return a credential with a nil ServiceAccountCredential field without any indication of failure. This can lead to subtle bugs later on. The function should return an error to the caller to indicate that parsing failed.
if err := json.Unmarshal(credentialJSON, &saCred); err != nil {
// Consider changing the function signature to return an error.
} else {
cred.ServiceAccount.ServiceAccountCredential = &saCred
}
auth/auth_config.go
Outdated
| var schemePart string | ||
| if c.AuthScheme != nil { | ||
| schemeType := c.AuthScheme.GetType() | ||
| schemeJSON, _ := json.Marshal(c.AuthScheme) |
There was a problem hiding this comment.
The error returned by json.Marshal is being ignored. While marshalling this struct is unlikely to fail, it's a good practice to handle potential errors. A failure here could lead to silent issues like credential key collisions if schemeJSON is nil. Consider panicking if marshalling fails, as this would indicate a programming error (e.g., an unmarshallable type in the auth config).
auth/auth_config.go
Outdated
|
|
||
| var credPart string | ||
| if c.RawAuthCredential != nil { | ||
| credJSON, _ := json.Marshal(c.RawAuthCredential) |
There was a problem hiding this comment.
Similar to the AuthScheme marshalling, the error from json.Marshal for RawAuthCredential is ignored. This could lead to silent failures and key collisions. It's safer to handle this error, for instance by panicking, to prevent unexpected behavior.
| cred, err := parseAuthCredentialFromMap(credData) | ||
| if err == nil { | ||
| config.ExchangedAuthCredential = cred | ||
| } |
There was a problem hiding this comment.
The error returned from parseAuthCredentialFromMap is ignored. If parsing the credential data fails, the error is silently swallowed, and config.ExchangedAuthCredential remains nil. This could cause the authentication flow to fail silently. The error should be handled, at least by logging it, to aid in debugging.
cred, err := parseAuthCredentialFromMap(credData)
if err != nil {
// Consider logging this error for debugging purposes.
continue
}
config.ExchangedAuthCredential = credAdd support for tools to request authentication credentials during execution: - Add RequestedAuthConfigs field to EventActions for storing auth requests - Implement GenerateAuthEvent to convert auth requests to adk_request_credential function calls - Add RequestAuthConfig method to ToolContext for tools to request authentication - Add AuthConfig field to Tool interface for tools to declare auth requirements - Integrate auth event generation into the LLM base flow Also fix test comparisons to ignore RequestedAuthConfigs field initialization and add UTC timezone setup in database tests for consistent timestamp formatting.
cpunion
force-pushed
the
feature/auth-flow
branch
from
695a733 to
654e57b
Compare
1 participant
Base on:
#452 feat(auth): add OAuth2/OIDC authentication package