Conversation
0e55db2 to
60d70d0
Compare
Summary of ChangesHello @dgellow, 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 refactors the authentication system by introducing a robust Identity Provider (IDP) abstraction. This change moves away from a hardcoded Google OAuth implementation to a flexible, pluggable model that can integrate with various IDPs like Google, Azure AD, GitHub, and any OIDC-compliant service. The update involves substantial changes to configuration, validation, and core authentication handlers to support this new multi-IDP architecture, enhancing the system's extensibility and security. Highlights
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
|
There was a problem hiding this comment.
Code Review
This pull request is an excellent refactoring effort that replaces a Google-specific OAuth implementation with a flexible, provider-based abstraction. The introduction of the internal/idp package with a Provider interface is well-executed and allows for easy extension to other identity providers. The configuration changes, while breaking, are clearly documented and the new structure is more logical. The validation logic has been updated to be provider-aware, which is a great improvement. I've identified a couple of areas for improvement, mainly around code duplication in the configuration parsing and test coverage for the new GitHub provider. Overall, this is a high-quality change that significantly improves the authentication capabilities of the application.
b4851d1 to
c580b4e
Compare
Replaces Google-specific OAuth with a provider abstraction pattern
supporting Google, Azure AD, GitHub, and generic OIDC providers.
Changes:
- Create internal/idp package with Provider interface and implementations
- Update config from googleClientId/googleClientSecret to structured idp block
- Add provider tracking to browser session cookies for multi-IDP readiness
- Delete internal/googleauth package
- Add comprehensive tests for idp package (including GitHub UserInfo tests)
- Update all example configs and documentation
- Refactor parseIDPConfig to use helper function for cleaner code
Config format change:
"idp": {
"provider": "google|azure|github|oidc",
"clientId": "...",
"clientSecret": {"$env": "..."},
"redirectUri": "...",
// Provider-specific: tenantId (azure), allowedOrgs (github), discoveryUrl (oidc)
}
Co-authored-by: Claude <noreply@anthropic.com>
Previously allowedDomains was passed at call time to UserInfo(), while allowedOrgs was configured at construction. This inconsistency made the interface harder to understand and didn't follow the principle that access control should be configured when the provider is created. Changes: - Provider.UserInfo() now takes only context and token - All providers store allowedDomains internally - Factory accepts allowedDomains parameter - All tests updated to reflect new interface
f092c4d to
9ccd1ff
Compare
Providers now only fetch identity — access control (domain/org checks) is centralized in a single validateAccess method on the auth handler. This means IDP errors (unreachable provider) produce ErrServerError while policy rejections produce ErrAccessDenied, and adding new access rules no longer requires touching every provider. GitHub always fetches orgs now (scope was already requested unconditionally). UserInfo struct renamed to Identity to avoid collision with the method name. ParseClientRequest relocated to oauth package as ParseClientRegistration. Deleted deprecated ProtectedResourceMetadata and inlined the workaround logic into the handler that used it.
Fix duplicate isStdioServer with diverging implementations by adding IsStdio() method on MCPClientConfig. Unify CSRF strategy across admin and token handlers using stateless HMAC-based tokens (removes sync.Map approach). Make StoreAuthorizeRequest return an error so Firestore failures surface clearly. Use cookie.SetSession consistently (fixes SameSite mismatch from Strict to Lax for OAuth callbacks). Split http.go into focused files (user_token_service.go, session_handler.go). Extract handleBrowserCallback and handleOAuthClientCallback from 190-line IDPCallbackHandler. Add GetUser to Storage interface for O(1) admin checks. Consolidate browserauth + oauthsession into internal/session, move envutil.IsDev into config package. Replace hardcoded Firestore collection names with constants. Delete unused GetErrorName. Update CLAUDE.md to reference ./scripts/ instead of make.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly improves the authentication architecture by introducing a well-designed abstraction for multiple identity providers. The separation of authentication (identity fetching) from authorization (access policy) is a great design choice that enhances clarity and flexibility. The codebase quality pass has resulted in numerous improvements, such as fixing the stateful CSRF implementation, improving database lookups for admin checks, and general code cleanup by splitting oversized files and consolidating small packages. The changes are consistently applied across the configuration, implementation, tests, and documentation. I have one minor suggestion to improve the clarity of a validation error message.
| if len(oauth.JWTSecret) < 32 { | ||
| return fmt.Errorf("jwtSecret must be at least 32 characters (got %d). Generate with: openssl rand -base64 32", len(oauth.JWTSecret)) | ||
| } |
There was a problem hiding this comment.
The suggested command openssl rand -base64 32 generates a 44-character string, which might be confusing given the 32-character minimum requirement. To avoid this confusion and provide a command that generates a key closer to the minimum length, you could suggest openssl rand -base64 24 which produces exactly 32 characters.
| if len(oauth.JWTSecret) < 32 { | |
| return fmt.Errorf("jwtSecret must be at least 32 characters (got %d). Generate with: openssl rand -base64 32", len(oauth.JWTSecret)) | |
| } | |
| if len(oauth.JWTSecret) < 32 { | |
| return fmt.Errorf("jwtSecret must be at least 32 characters (got %d). Generate with: openssl rand -base64 24", len(oauth.JWTSecret)) | |
| } |
Replace the removed mcp/postgres Docker image with Google's MCP Toolbox for Databases. Convert 8 static JSON test configs to programmatic generation via Go builder functions so the image reference lives in one place (ToolboxImage constant). Key changes: - GoogleProvider now accepts optional endpoint overrides for authorizationUrl, tokenUrl, and userInfoUrl, replacing the broken GOOGLE_OAUTH_* env vars that were never actually read - Integration tests use execute_sql tool instead of query (toolbox API) - Remove dead code: TestEnvironment, SetupTestEnvironment, execDockerCompose helpers - Pull toolbox image in TestMain to prevent first-test timeout - Update all example configs and --config-init default
Add endpoint overrides to GitHub and Azure providers so they can point at local fake servers during testing, matching the existing Google provider pattern. Create FakeGitHubServer (port 9092) and FakeOIDCServer (port 9093) alongside FakeGCPServer to simulate all four supported IDPs. New integration tests exercise the full OAuth flow for GitHub, OIDC, and Azure providers end-to-end (register client → authorize → IDP redirect → callback → token exchange → MCP tools/list). Also tests org denial for GitHub and domain denial across all three providers. Reorganize integration directory: split test_utils.go (993 lines) into test_helpers.go, test_clients.go, test_fakes.go. Split oauth_test.go (1576 lines) into oauth_flow_test.go, oauth_user_tokens_test.go, oauth_service_test.go, oauth_rfc8707_test.go, oauth_idp_test.go.
Summary
Breaking change
Config format changes from flat
googleClientId/googleClientSecretfields to a structuredidpblock:Quality improvements
isStdioServerwith diverging implementationssync.Mapleak)StoreAuthorizeRequestreturn error (Firestore failures were silent)GetUserto Storage interface for O(1) admin checksCloses #31, closes #29