fix(api): Google OAuth provider#26
Open
kelkawi-a wants to merge 7 commits intotuannvm:mainfrom
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Google-specific opaque-token validation via tokeninfo fallback, refactors token response generation to prefer id_token for Google opaque access tokens, consolidates WWW-Authenticate header formatting and error codes, and updates/introduces tests validating these behaviors. Changes
Sequence Diagram(s)sequenceDiagram
actor Client as Client
participant Handler as OAuth2Handler
participant Validator as OIDCValidator
participant GoogleAPI as Google TokenInfo / JWKS
Client->>Handler: Request with Bearer token
Handler->>Validator: ValidateToken(token)
Validator->>Validator: Trim token, detect JWT via dot-count
alt token looks like JWT
Validator->>GoogleAPI: Verify JWT via JWKS
GoogleAPI-->>Validator: Verified claims
Validator-->>Handler: User(from JWT)
else Google opaque access token
Validator->>GoogleAPI: GET /tokeninfo?access_token=...
GoogleAPI-->>Validator: JSON tokeninfo claims
Validator->>Validator: runTokenValidators(claims)
Validator-->>Handler: User(from tokeninfo)
end
Handler->>Handler: buildTokenResponse(oauth2.Token)
alt Google + opaque access token
Handler-->>Client: 200 + JSON (access_token set to id_token)
else JWT or non-Google
Handler-->>Client: 200 + JSON (access_token preserved)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Ali Kelkawi <ali.kelkawi@canonical.com>
kelkawi-a
force-pushed
the
fix/google-oauth
branch
from
2fb05f8 to
470322b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
oauth.go (1)
265-279:⚠️ Potential issue | 🟡 MinorUse
invalid_requestfor the missing/malformed Authorization path.This branch is the “no usable Bearer token” case, but it still reports
error="invalid_token".Return401()usesinvalid_requestfor the same condition, so callers now get different OAuth semantics depending on which wrapper they hit.Suggested fix
- `Bearer realm="OAuth", error="invalid_token", error_description="Missing or invalid access token", resource_metadata="%s"`, + `Bearer realm="OAuth", error="invalid_request", error_description="Bearer token required", resource_metadata="%s"`, ... - Error: "invalid_token", - ErrorDescription: "Missing or invalid access token", + Error: "invalid_request", + ErrorDescription: "Bearer token required",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oauth.go` around lines 265 - 279, Change the missing/malformed Authorization branch to use OAuth error "invalid_request" instead of "invalid_token": update the WWW-Authenticate header string and the oauthErrorResponse payload created in the authHeader == "" || len(authHeader) < 7 || authHeader[:7] != "Bearer " branch so the error field is "invalid_request" (keep ErrorDescription the same and continue setting Content-Type, status and metadata via GetProtectedResourceMetadataURL). This will align this path with Return401() semantics and ensure callers see a consistent error for no usable Bearer token.
🧹 Nitpick comments (1)
handlers_token_response_test.go (1)
10-83: Collapse these into a table-driven subtest.These three tests are the same harness with different fixtures, so a single
tests := []struct{...}loop witht.Run(tt.name, ...)will be easier to extend as Google edge cases grow.As per coding guidelines, "
**/*_test.go: Use table-driven subtests pattern with t.Run() for test organization`".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@handlers_token_response_test.go` around lines 10 - 83, Collapse the three nearly-identical tests into a single table-driven subtest in handlers_token_response_test.go by creating a slice of test cases (e.g., fields: name, provider, accessToken, idToken, refreshToken, expectedAccessToken, expectedIDToken, expectedRefreshToken) and iterating with for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ... }) }; inside each subtest instantiate OAuth2Handler with config.Provider set from tt.provider, build the oauth2.Token with tt.accessToken and WithExtra including tt.idToken/tt.refreshToken, call handler.buildTokenResponse(token), and assert expectations (compare response["access_token"], response["id_token"], response["refresh_token"] against tt.expected* values) to preserve current assertions for buildTokenResponse behavior across providers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@provider/provider.go`:
- Around line 329-343: The function userFromGoogleTokenInfoClaims currently
fails when claims["sub"] is empty because Google tokeninfo access-token
responses use "user_id" instead of "sub"; update userFromGoogleTokenInfoClaims
to attempt subject := claims["sub"].(string) and if empty, fall back to subject
= claims["user_id"].(string) before returning the "missing subject" error, and
adjust any error text accordingly; update or add a unit test that constructs a
tokeninfo-style claims map containing "user_id" (and not "sub") to assert
successful user creation and that behavior for audience checks
(skipAudienceCheck/audience) remains unchanged.
---
Outside diff comments:
In `@oauth.go`:
- Around line 265-279: Change the missing/malformed Authorization branch to use
OAuth error "invalid_request" instead of "invalid_token": update the
WWW-Authenticate header string and the oauthErrorResponse payload created in the
authHeader == "" || len(authHeader) < 7 || authHeader[:7] != "Bearer " branch so
the error field is "invalid_request" (keep ErrorDescription the same and
continue setting Content-Type, status and metadata via
GetProtectedResourceMetadataURL). This will align this path with Return401()
semantics and ensure callers see a consistent error for no usable Bearer token.
---
Nitpick comments:
In `@handlers_token_response_test.go`:
- Around line 10-83: Collapse the three nearly-identical tests into a single
table-driven subtest in handlers_token_response_test.go by creating a slice of
test cases (e.g., fields: name, provider, accessToken, idToken, refreshToken,
expectedAccessToken, expectedIDToken, expectedRefreshToken) and iterating with
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ... }) }; inside
each subtest instantiate OAuth2Handler with config.Provider set from
tt.provider, build the oauth2.Token with tt.accessToken and WithExtra including
tt.idToken/tt.refreshToken, call handler.buildTokenResponse(token), and assert
expectations (compare response["access_token"], response["id_token"],
response["refresh_token"] against tt.expected* values) to preserve current
assertions for buildTokenResponse behavior across providers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 25688543-4a72-454e-985d-33a7faa2b63d
📒 Files selected for processing (6)
api_test.gohandlers.gohandlers_token_response_test.gooauth.goprovider/provider.goprovider/provider_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
handlers_token_response_test.go (1)
10-83: Refactor to table-driven subtests per coding guidelines.The tests correctly validate
buildTokenResponsebehavior for Google opaque tokens, Google JWT tokens, and non-Google providers. However, the coding guidelines require table-driven subtests witht.Run()for test organization.♻️ Proposed refactor to table-driven subtests
-func TestBuildTokenResponseGoogleOpaqueAccessTokenUsesIDToken(t *testing.T) { - handler := &OAuth2Handler{ - config: &OAuth2Config{Provider: "google"}, - logger: &defaultLogger{}, - } - - idToken := "header.payload.signature" - token := (&oauth2.Token{ - AccessToken: "ya29.a0ARW5m7Opaque", - TokenType: "Bearer", - RefreshToken: "refresh-token", - Expiry: time.Now().Add(time.Hour), - }).WithExtra(map[string]interface{}{ - "id_token": idToken, - "scope": "openid profile email", - }) - - response := handler.buildTokenResponse(token) - - if response["access_token"] != idToken { - t.Fatalf("expected access_token to be mapped to id_token for Google opaque token") - } - - if response["id_token"] != idToken { - t.Fatalf("expected id_token in response") - } - - if response["refresh_token"] != "refresh-token" { - t.Fatalf("expected refresh_token in response") - } -} - -func TestBuildTokenResponseGoogleJWTAccessTokenPreserved(t *testing.T) { - handler := &OAuth2Handler{ - config: &OAuth2Config{Provider: "google"}, - logger: &defaultLogger{}, - } - - jwtAccessToken := "jwt.access.token" - token := (&oauth2.Token{ - AccessToken: jwtAccessToken, - TokenType: "Bearer", - Expiry: time.Now().Add(time.Hour), - }).WithExtra(map[string]interface{}{ - "id_token": "id.token.value", - }) - - response := handler.buildTokenResponse(token) - - if response["access_token"] != jwtAccessToken { - t.Fatalf("expected JWT access_token to remain unchanged") - } -} - -func TestBuildTokenResponseNonGoogleAccessTokenPreserved(t *testing.T) { - handler := &OAuth2Handler{ - config: &OAuth2Config{Provider: "okta"}, - logger: &defaultLogger{}, - } - - token := (&oauth2.Token{ - AccessToken: "opaque-token-value", - TokenType: "Bearer", - Expiry: time.Now().Add(time.Hour), - }).WithExtra(map[string]interface{}{ - "id_token": "id.token.value", - }) - - response := handler.buildTokenResponse(token) - - if response["access_token"] != "opaque-token-value" { - t.Fatalf("expected non-Google access_token to remain unchanged") - } -} +func TestBuildTokenResponse(t *testing.T) { + tests := []struct { + name string + provider string + accessToken string + refreshToken string + idToken string + expectedAccessToken string + expectedRefreshToken string + expectIDTokenInResponse bool + }{ + { + name: "Google opaque access token uses id_token", + provider: "google", + accessToken: "ya29.a0ARW5m7Opaque", + refreshToken: "refresh-token", + idToken: "header.payload.signature", + expectedAccessToken: "header.payload.signature", + expectedRefreshToken: "refresh-token", + expectIDTokenInResponse: true, + }, + { + name: "Google JWT access token preserved", + provider: "google", + accessToken: "jwt.access.token", + idToken: "id.token.value", + expectedAccessToken: "jwt.access.token", + expectIDTokenInResponse: true, + }, + { + name: "Non-Google access token preserved", + provider: "okta", + accessToken: "opaque-token-value", + idToken: "id.token.value", + expectedAccessToken: "opaque-token-value", + expectIDTokenInResponse: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + handler := &OAuth2Handler{ + config: &OAuth2Config{Provider: tc.provider}, + logger: &defaultLogger{}, + } + + token := (&oauth2.Token{ + AccessToken: tc.accessToken, + TokenType: "Bearer", + RefreshToken: tc.refreshToken, + Expiry: time.Now().Add(time.Hour), + }).WithExtra(map[string]interface{}{ + "id_token": tc.idToken, + }) + + response := handler.buildTokenResponse(token) + + if response["access_token"] != tc.expectedAccessToken { + t.Errorf("access_token = %v, want %v", response["access_token"], tc.expectedAccessToken) + } + + if tc.expectedRefreshToken != "" && response["refresh_token"] != tc.expectedRefreshToken { + t.Errorf("refresh_token = %v, want %v", response["refresh_token"], tc.expectedRefreshToken) + } + + if tc.expectIDTokenInResponse && response["id_token"] != tc.idToken { + t.Errorf("id_token = %v, want %v", response["id_token"], tc.idToken) + } + }) + } +}As per coding guidelines: "Use table-driven subtests pattern with t.Run() for test organization" for files matching
**/*_test.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@handlers_token_response_test.go` around lines 10 - 83, Combine the three separate tests (TestBuildTokenResponseGoogleOpaqueAccessTokenUsesIDToken, TestBuildTokenResponseGoogleJWTAccessTokenPreserved, TestBuildTokenResponseNonGoogleAccessTokenPreserved) into a single table-driven test that iterates cases and uses t.Run for each case; for each row include fields for name, Provider (OAuth2Config), AccessToken, Extra (id_token/scope/refresh_token), and expected response keys, create an OAuth2Handler per case, call handler.buildTokenResponse(token) and assert expected fields, and keep references to buildTokenResponse, OAuth2Handler, and OAuth2Config so the test logic is easy to locate and maintain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@handlers_token_response_test.go`:
- Around line 10-83: Combine the three separate tests
(TestBuildTokenResponseGoogleOpaqueAccessTokenUsesIDToken,
TestBuildTokenResponseGoogleJWTAccessTokenPreserved,
TestBuildTokenResponseNonGoogleAccessTokenPreserved) into a single table-driven
test that iterates cases and uses t.Run for each case; for each row include
fields for name, Provider (OAuth2Config), AccessToken, Extra
(id_token/scope/refresh_token), and expected response keys, create an
OAuth2Handler per case, call handler.buildTokenResponse(token) and assert
expected fields, and keep references to buildTokenResponse, OAuth2Handler, and
OAuth2Config so the test logic is easy to locate and maintain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31c4f6e8-5ab3-4b86-a609-5f0b36198081
📒 Files selected for processing (6)
api_test.gohandlers.gohandlers_token_response_test.gooauth.goprovider/provider.goprovider/provider_test.go
✅ Files skipped from review due to trivial changes (1)
- api_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- oauth.go
- provider/provider_test.go
- handlers.go
- provider/provider.go
Signed-off-by: Ali Kelkawi <ali.kelkawi@canonical.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
provider/provider.go (1)
297-359: Reuse the existing validator pipeline for tokeninfo claims.The JWT path runs
v.TokenValidators, but the Google opaque-token path returns before those hooks execute. That makes opaque Google tokens bypass any extra validator added toOIDCValidator.TokenValidatorsand leaves audience checks duplicated in a second path. Unmarshal intojwt.MapClaimsand run the shared validators before constructing theUser.Suggested refactor
- var claims map[string]interface{} + var claims jwt.MapClaims if err := json.Unmarshal(body, &claims); err != nil { return nil, fmt.Errorf("failed parsing google tokeninfo response: %w", err) } + for i, fn := range v.TokenValidators { + if err := fn(claims); err != nil { + return nil, fmt.Errorf("validation function %d failed with error: %w", i, err) + } + } + return v.userFromGoogleTokenInfoClaims(claims) } -func (v *OIDCValidator) userFromGoogleTokenInfoClaims(claims map[string]interface{}) (*User, error) { - aud, _ := claims["aud"].(string) - if !v.skipAudienceCheck { - if aud == "" { - return nil, fmt.Errorf("missing audience claim") - } - if aud != v.audience { - return nil, fmt.Errorf("invalid audience: expected %s, got %s", v.audience, aud) - } - } - +func (v *OIDCValidator) userFromGoogleTokenInfoClaims(claims jwt.MapClaims) (*User, error) { subject, _ := claims["sub"].(string) if subject == "" { subject, _ = claims["user_id"].(string) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider/provider.go` around lines 297 - 359, The Google opaque-token path should reuse the shared validator pipeline instead of duplicating audience logic: in validateGoogleOpaqueToken, unmarshal body into jwt.MapClaims (not map[string]interface{}), run the configured validators in v.TokenValidators against that claims object (iterating and returning on error), and only after successful validation call userFromGoogleTokenInfoClaims to build the User; remove or skip the duplicate audience validation in userFromGoogleTokenInfoClaims so audience checking happens via the shared validators.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@handlers_token_response_test.go`:
- Around line 74-84: The failing test logs raw tokens; change t.Fatalf calls to
log only SHA-256 hashes (hex) truncated to the first 16 characters instead of
raw token strings: compute hashes for response["access_token"] and
tt.expectedAccessToken, response["id_token"] and tt.expectedIDToken, and
response["refresh_token"] and tt.expectedRefreshToken, and use those hash
strings in the fmt message inside the checks in handlers_token_response_test
(the response map and tt.* vars are the symbols to update); ensure you import
crypto/sha256 and fmt and compute fmt.Sprintf("%x",
sha256.Sum256([]byte(token)))[:16] for each token before passing to t.Fatalf.
---
Nitpick comments:
In `@provider/provider.go`:
- Around line 297-359: The Google opaque-token path should reuse the shared
validator pipeline instead of duplicating audience logic: in
validateGoogleOpaqueToken, unmarshal body into jwt.MapClaims (not
map[string]interface{}), run the configured validators in v.TokenValidators
against that claims object (iterating and returning on error), and only after
successful validation call userFromGoogleTokenInfoClaims to build the User;
remove or skip the duplicate audience validation in
userFromGoogleTokenInfoClaims so audience checking happens via the shared
validators.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57836190-a512-4306-98c4-471646f637c2
📒 Files selected for processing (5)
api_test.gohandlers_token_response_test.gooauth.goprovider/provider.goprovider/provider_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- provider/provider_test.go
- oauth.go
Signed-off-by: Ali Kelkawi <ali.kelkawi@canonical.com>
kelkawi-a
force-pushed
the
fix/google-oauth
branch
from
55d390e to
99dac51
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@provider/provider_test.go`:
- Around line 447-459: The test "skip audience check" is exercising
userFromGoogleTokenInfoClaims() which ignores audience fields, so move or add
this subtest to the Google opaque-token validation path that actually checks
audiences: update the test to call validateGoogleOpaqueToken() (or
runTokenValidators() with validateAudience wired) using an OIDCValidator
instance with audience="my-client-id.apps.googleusercontent.com" and
skipAudienceCheck=true and a token/claims where aud ≠ audience to ensure the
audience branch is exercised; ensure the test asserts success when
skipAudienceCheck=true and failure when skipAudienceCheck=false so
validateAudience logic is covered.
- Around line 501-504: The httptest handler is calling t.Fatalf from its
goroutine (inside the httptest.NewServer handler) which is unsafe; instead
capture the mismatch by writing an error string/boolean into a channel or a
shared variable protected by the test (e.g., errCh or gotErr) from inside the
http.HandlerFunc and then, after the request completes (or after
server.Close()/defer close), read that channel or inspect the variable in the
main test goroutine and call t.Fatalf or t.Errorf there; update the handler in
the server := httptest.NewServer(http.HandlerFunc(...)) block to send the
mismatch to the channel and add an assertion in the test function after the
handler runs.
In `@provider/provider.go`:
- Around line 244-245: Currently any non-JWT bearer string is sent to Google
when v.providerName == "google"; change this so opaque-token fallback to
validateGoogleOpaqueToken is only invoked after a positive Google-token check or
explicit opt-in flag (e.g., add a isGoogleOpaqueCandidate(tokenString) predicate
or check a config flag like allowGoogleOpaqueValidation) instead of relying
solely on looksLikeJWT, update both call sites (the branch around providerName
== "google" and the similar block at the later section) to use that
predicate/flag, and add a regression test that supplies an opaque token from a
different IdP and asserts validateGoogleOpaqueToken is not called (mock the
outbound call) to ensure non-Google opaque inputs never trigger the external
request.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56966f1b-6a4f-4cda-ae75-fdf322fc0bf9
📒 Files selected for processing (5)
api_test.gohandlers_token_response_test.gooauth.goprovider/provider.goprovider/provider_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- oauth.go
- handlers_token_response_test.go
Signed-off-by: Ali Kelkawi <ali.kelkawi@canonical.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
handlers.go (1)
628-662: Consider documenting the semantic implications of using id_token as access_token.The Google-specific logic correctly addresses the opaque token issue by substituting the id_token (a JWT) for the access_token. However, there are a few considerations:
- The
id_tokenappears twice in the response for Google opaque tokens (as bothaccess_tokenandid_tokenfields)- The
id_tokenandaccess_tokenhave different intended purposes in OIDC - the id_token is for client authentication while access_token is for resource accessThis approach should work for MCP clients that need a JWT to validate, but downstream resources expecting standard access token claims (e.g.,
scope,audfor the resource) may behave unexpectedly if they receive an id_token instead.Consider adding a comment explaining when this substitution is appropriate and any limitations.
📝 Suggested documentation
// buildTokenResponse builds an RFC 6749-style token response with provider-specific behavior. +// For Google provider with opaque access tokens: Google issues opaque access tokens that cannot +// be validated as JWTs. When detected, this function substitutes the id_token (which IS a JWT) +// as the access_token for downstream JWT validators. Note: This is suitable for MCP clients +// but may not be appropriate for all OAuth resource servers expecting standard access token claims. func (h *OAuth2Handler) buildTokenResponse(token *oauth2.Token) map[string]interface{} {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@handlers.go` around lines 628 - 662, In buildTokenResponse, add a concise comment above the Google-specific substitution (the h.config.Provider == "google" block referencing token, accessToken, id_token and looksLikeJWT) that explains: why we substitute id_token for an opaque access_token (to provide a JWT for downstream validation), that this results in the same JWT appearing in both access_token and id_token fields, and the semantic limitations (id_token is meant for client identity, not resource access—downstream resource servers expecting access_token claims like aud/scope may misbehave). Also note acceptable usage scenarios (MCP clients needing a JWT) and suggest revisiting to emit a derived token or inform consumers if stricter semantics are required.provider/provider_test.go (1)
481-554: Use a case table for the new tokeninfo scenario tests.These two tests both have paired scenarios and repeated harness setup. Converting them to table-driven subtests would match the repo convention and make it cheaper to add more tokeninfo cases without duplicating server/validator wiring.
As per coding guidelines,
**/*_test.go: Use table-driven subtests pattern with t.Run() for test organization.Also applies to: 556-600
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider/provider_test.go` around lines 481 - 554, The two subtests in TestOIDCValidator_ValidateGoogleOpaqueTokenRunsTokenValidators duplicate setup and should be converted to a table-driven t.Run loop: extract the httptest server and requestErrCh setup (and googleTokenInfoURL override/cleanup) once, then define a slice of test cases (fields: name, TokenValidators for OIDCValidator, wantErr bool, wantErrContains string, wantCalled bool, wantSubject string) and iterate with t.Run(tc.name, func(t *testing.T) { ... }); inside each iteration construct the OIDCValidator with tc.TokenValidators, call validateGoogleOpaqueToken, assert error presence/content, assert token validator called state, and assert returned user.Subject when applicable; ensure you drain/reset requestErrCh between cases to preserve the original request check semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@provider/provider.go`:
- Around line 295-305: The client.Do call may return a *url.Error that embeds
the full request URL (including the access token), so replace the direct
fmt.Errorf("%w", err) wrapping around the client.Do error with logic that uses
errors.As to detect a *url.Error and wrap a sanitized error message (omitting
the URL) instead; import "errors" and in the block handling the client.Do error
check errors.As(err, &urlErr) and return a generic "google tokeninfo request
failed" error (wrapping urlErr.Err or err without the URL) to avoid leaking
tokenString/endpoint, and add a regression test simulating a network failure to
assert no token appears in the returned/logged error.
---
Nitpick comments:
In `@handlers.go`:
- Around line 628-662: In buildTokenResponse, add a concise comment above the
Google-specific substitution (the h.config.Provider == "google" block
referencing token, accessToken, id_token and looksLikeJWT) that explains: why we
substitute id_token for an opaque access_token (to provide a JWT for downstream
validation), that this results in the same JWT appearing in both access_token
and id_token fields, and the semantic limitations (id_token is meant for client
identity, not resource access—downstream resource servers expecting access_token
claims like aud/scope may misbehave). Also note acceptable usage scenarios (MCP
clients needing a JWT) and suggest revisiting to emit a derived token or inform
consumers if stricter semantics are required.
In `@provider/provider_test.go`:
- Around line 481-554: The two subtests in
TestOIDCValidator_ValidateGoogleOpaqueTokenRunsTokenValidators duplicate setup
and should be converted to a table-driven t.Run loop: extract the httptest
server and requestErrCh setup (and googleTokenInfoURL override/cleanup) once,
then define a slice of test cases (fields: name, TokenValidators for
OIDCValidator, wantErr bool, wantErrContains string, wantCalled bool,
wantSubject string) and iterate with t.Run(tc.name, func(t *testing.T) { ... });
inside each iteration construct the OIDCValidator with tc.TokenValidators, call
validateGoogleOpaqueToken, assert error presence/content, assert token validator
called state, and assert returned user.Subject when applicable; ensure you
drain/reset requestErrCh between cases to preserve the original request check
semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8c17286-3735-4182-9b19-9a3d044066a7
📒 Files selected for processing (6)
api_test.gofixed_redirect_test.gohandlers.gooauth.goprovider/provider.goprovider/provider_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- api_test.go
Signed-off-by: Ali Kelkawi <ali.kelkawi@canonical.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR solves the issue reported in #16 . In summary, Google OAuth does not return JWTs, but rather opaque tokens. This PR introduces the logic needed to fetch the user's identity from Google's token info endpoint. The changes were tested locally against an instance of Google OAuth.
Changes
When the provider is set to
google, fetch the user's identity using Google's token info endpoint.Testing
Related Issues
Fixes #16 .
Summary by CodeRabbit