Feat/account api helpers jwt improvements - #49
Conversation
|
Warning Rate limit exceeded@BrandtKruger has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 41 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (21)
WalkthroughAdds an Account API client with pagination and deep-merge utilities; expands JWT token parsing/helpers and feature-flag/entitlement APIs; extends OAuth2 flows/options (PKCE, device flow, GetToken, session hooks); implements Gin session-backed storage and many tests across packages. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Token as jwt.Token
participant APIClient as account_api.Client
participant Account as Account API
App->>Token: GetPermissionsWithAPI(ctx, apiClient, opts)
alt opts.ForceAPI == true
Token->>APIClient: CallAccountAPIPaginated(ctx, "/permissions")
APIClient->>Account: GET /permissions?limit=N
Account-->>APIClient: {items:[...], pagination:{starting_after:X}}
loop fetch more pages
APIClient->>Account: GET /permissions?starting_after=X&limit=N
Account-->>APIClient: {items:[...], pagination:...}
end
APIClient-->>Token: merged + deduplicated items
else
Token->>Token: extract permissions from token claims
end
Token-->>App: PermissionsWithOrg result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
jwt/jwt_options.go (2)
90-93: Silent failure returns nil Option, which may cause nil pointer dereference.When
keyfunc.Newfails, returningnilinstead of an Option that records the error (like the HTTP client error handling at lines 80-84) can cause a nil pointer panic if users iterate over options without nil checks.Apply this diff to return an error-recording Option instead of nil:
jwks, err := keyfunc.New(options) if err != nil { - return nil + return func(s *Token) { + s.validationErrors = append(s.validationErrors, fmt.Errorf("failed to create JWKS keyfunc: %w", err)) + } }
320-338: Code uses Go 1.20+ feature with incompatible minimum version.The
newErrorfunction uses multiple%wformat specifiers infmt.Errorf, which is only supported in Go 1.20+. However, the project's go.mod specifiesgo 1.18as the minimum version. This creates a runtime incompatibility for users on Go 1.18 and 1.19. Either raise the minimum Go version to 1.20+ in go.mod or refactornewErrorto avoid multiple%wverbs for Go 1.18 compatibility.oauth2/authorization_code/options.go (2)
267-280: Silent error handling when generating PKCE artifacts.If
generateCodeVerifier()fails, the error is silently ignored, which could leave PKCE in an inconsistent state (enabled but without a valid code challenge). Consider logging or returning an error.Since
Optionreturnsvoid, consider either:
- Logging the error for debugging
- Storing the error state for later retrieval
- Setting
usePKCE = falseon failure to prevent inconsistent state
304-328: Same silent error handling issue in WithPKCEChallengeMethod.Same concern as
WithPKCE- errors fromgenerateCodeVerifier()andSetCodeVerifier()are silently ignored.Additionally, there's code duplication between
WithPKCE()andWithPKCEChallengeMethod(). Consider havingWithPKCE()delegate toWithPKCEChallengeMethod("S256"):func WithPKCE() Option { return func(s *AuthorizationCodeFlow) { - s.usePKCE = true - s.challengeMethod = "S256" // Explicitly set recommended default - // Generate code verifier and challenge when PKCE is enabled - if codeVerifier, err := generateCodeVerifier(); err == nil { - // Store code verifier in session hooks - if s.sessionHooks != nil { - _ = s.sessionHooks.SetCodeVerifier(codeVerifier) - } - s.codeChallenge = generateCodeChallenge(codeVerifier) - } + WithPKCEChallengeMethod("S256")(s) } }
🧹 Nitpick comments (16)
frameworks/gin_kinde/gin_kinde_test.go (2)
15-214: Consider extracting a test helper to reduce duplication.The session setup pattern (creating store, router, middleware, and capturing session) is repeated across all subtests. A helper function would improve maintainability:
func setupTestSession(t *testing.T) (sessions.Session, *gin.Engine) { store := cookie.NewStore([]byte("secret")) router := gin.New() router.Use(sessions.Sessions("test", store)) var session sessions.Session router.GET("/", func(c *gin.Context) { session = sessions.Default(c) }) w := httptest.NewRecorder() req := httptest.NewRequest("GET", "/", nil) router.ServeHTTP(w, req) return session, router }Then each test could start with:
session, _ := setupTestSession(t) storage := &SessionStorage{session: session}
221-243: Strengthen assertion to verify kinde_client in context.The test name indicates it should verify that the kinde client is created in context, but line 242's assertion (
assert.NotNil(t, w)) only checks that the response recorder exists, which will always be true. Consider verifying the actual behavior by adding a route handler that checks for the client:err := UseKindeAuth( privateGroup, "https://test.kinde.com", "test_client_id", "test_client_secret", "http://localhost:8080", ) assert.Nil(t, err) + + var clientFound bool + privateGroup.GET("/test", func(c *gin.Context) { + _, clientFound = c.Get("kinde_client") + c.String(200, "ok") + }) w := httptest.NewRecorder() - req := httptest.NewRequest("GET", "/private/test", nil) + req := httptest.NewRequest("GET", "/private/test", nil) router.ServeHTTP(w, req) - // The middleware should set the kinde_client in context - // This is tested indirectly through the middleware execution - assert.NotNil(t, w) + assert.True(t, clientFound, "kinde_client should be set in context")oauth2/authorization_code/example_middleware.go (1)
84-89: Placeholder implementation always returns false, making the example non-functional.The
hasRequiredScopesfunction is a placeholder that always returnsfalse, meaningExampleCustomMiddlewarewill always return a 403 Forbidden response. While documented as a placeholder, consider providing a basic working implementation that demonstrates actual scope checking from the token's claims.// hasRequiredScopes is a helper function to check if a token has required scopes func hasRequiredScopes(token *jwt.Token, requiredScope string) bool { - // This is a simplified example - you would implement your own scope checking logic - // based on your application's requirements - return false // Placeholder implementation + // This is a simplified example - check if the token has the required scope + claims := token.GetClaims() + if claims == nil { + return false + } + scopes, ok := claims["scope"].(string) + if !ok { + return false + } + for _, s := range strings.Split(scopes, " ") { + if s == requiredScope { + return true + } + } + return false }Note: You'll need to add
"strings"to imports if you apply this suggestion.oauth2/authorization_code/options_test.go (1)
197-219: Consider verifying the code verifier is stored in session hooks.The PKCE tests verify that
flow.codeChallengeis set, but don't verify that the code verifier was stored viasessionHooks. Consider adding an assertion to verify the verifier was properly stored.t.Run("generates code verifier and challenge", func(t *testing.T) { - flow := &AuthorizationCodeFlow{ - sessionHooks: newTestSessionHooks(), - } + mockHooks := newTestSessionHooks() + flow := &AuthorizationCodeFlow{ + sessionHooks: mockHooks, + } WithPKCE()(flow) assert.NotEmpty(t, flow.codeChallenge) + // Verify code verifier was stored (if mockHooks supports inspection) })oauth2/client_credentials/options_test.go (1)
108-119: Assert the expected audience value for hostname extraction.The test verifies
audiencesis not nil but doesn't assert the actual expected value. This makes it harder to catch regressions.t.Run("extracts hostname correctly", func(t *testing.T) { flow := &ClientCredentialsFlow{ config: clientcredentials.Config{ EndpointParams: make(map[string][]string), }, } WithKindeManagementAPI("https://subdomain.my_kinde_tenant.kinde.com:8080")(flow) audiences := flow.config.EndpointParams["audience"] - // Should extract the hostname correctly - assert.NotNil(t, audiences) + // Should extract the hostname correctly and form the audience URL + assert.Contains(t, audiences, "https://subdomain.my_kinde_tenant.kinde.com/api") })oauth2/client_credentials/token_source_test.go (1)
60-109: Test assertions only verify errors occur, not specific behaviors.Both subtests assert
err != nilwithout verifying the token caching or session hook error handling logic works correctly. While the comments explain this is due to incomplete test infrastructure, consider using a mock HTTP server to provide more meaningful assertions.This is acceptable for initial coverage, but consider enhancing with
httptest.Serverfor more comprehensive validation in the future.oauth2/authorization_code/account_api.go (1)
10-38: Consider more descriptive error messages.The error messages on lines 21 and 32 could provide additional context about where these values are expected to come from (e.g., "issuer claim not found in access token" or "access token field is empty in OAuth2 token").
For example:
issuer := token.GetIssuer() if issuer == "" { - return nil, fmt.Errorf("issuer claim not found in token") + return nil, fmt.Errorf("issuer claim not found in access token") } getTokenFunc := func(ctx context.Context) (string, error) { token, err := flow.GetToken(ctx) if err != nil { return "", err } accessToken, ok := token.GetAccessToken() if !ok { - return "", fmt.Errorf("access token not found") + return "", fmt.Errorf("access token field is empty in OAuth2 token") } return accessToken, nil }jwt/account_api_helpers.go (4)
46-55: Consider nil check for apiClient when ForceAPI is true.If
options.ForceAPIis true butapiClientis nil, the code will panic when callingapiClient.CallAccountAPIPaginated. Add a nil check for better error handling.func (j *Token) GetPermissionsWithAPI(ctx context.Context, apiClient *account_api.Client, options GetPermissionsOptions) (*PermissionsWithOrg, error) { if !options.ForceAPI { // Read from token permissions := j.GetPermissions() orgCode := j.GetOrganizationCode() return &PermissionsWithOrg{ OrgCode: orgCode, Permissions: permissions, }, nil } + + if apiClient == nil { + return nil, fmt.Errorf("apiClient is required when ForceAPI is true") + } // Fetch from Account API
99-103: Consider nil check for apiClient when forceAPI is true.Same concern as
GetPermissionsWithAPI- ifforceAPIis true butapiClientis nil, the code will panic.func (j *Token) GetRolesWithAPI(ctx context.Context, apiClient *account_api.Client, forceAPI bool) ([]Role, error) { if !forceAPI { // Read from token return j.GetRoles(), nil } + + if apiClient == nil { + return nil, fmt.Errorf("apiClient is required when forceAPI is true") + } // Fetch from Account API
149-153: Consider nil check for apiClient when forceAPI is true.Same pattern - add nil check for consistency and safety.
func (j *Token) GetFeatureFlagsWithAPI(ctx context.Context, apiClient *account_api.Client, forceAPI bool) (map[string]FeatureFlag, error) { if !forceAPI { // Read from token return j.GetFeatureFlags(), nil } + + if apiClient == nil { + return nil, fmt.Errorf("apiClient is required when forceAPI is true") + } // Fetch from Account API
236-258: Add nil check for apiClient parameter.Unlike other methods,
GetEntitlementsalways requires an API call. Add nil check at the start.func (j *Token) GetEntitlements(ctx context.Context, apiClient *account_api.Client) (*EntitlementsResult, error) { + if apiClient == nil { + return nil, fmt.Errorf("apiClient is required") + } + type AccountEntitlementsData struct {kinde/account_api/client.go (4)
62-77: NewClient always returns nil error.The function signature returns
(*Client, error)but never returns an error. Either add validation (e.g., for empty baseURL, nil getToken) or simplify the signature to return*Clientonly.func NewClient(baseURL string, getToken func(ctx context.Context) (string, error), opts ...ClientOption) (*Client, error) { + if baseURL == "" { + return nil, fmt.Errorf("baseURL is required") + } + if getToken == nil { + return nil, fmt.Errorf("getToken function is required") + } + // Remove trailing slash baseURL = strings.TrimSuffix(baseURL, "/")
103-105: Consider adding Accept header for API best practices.While the server likely defaults to JSON, explicitly requesting JSON is a good practice.
// Set authorization header req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", accessToken)) - req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json")Note:
Content-Typeis typically used for request bodies. For GET requests without a body,Acceptis more appropriate.
244-256: URL is constructed twice - once unused.Lines 246-252 build a URL but only use
u.RawQuery. Line 256 reconstructs the route with query string. The URL parsing is redundant.for currentResponse.Metadata.HasMore { - // Build URL with pagination parameter - u, err := url.Parse(fmt.Sprintf("%s/%s", c.baseURL, route)) - if err != nil { - return fmt.Errorf("failed to parse URL: %w", err) - } - q := u.Query() + // Build query parameter for pagination + q := url.Values{} q.Set("starting_after", nextPageStartingAfter) - u.RawQuery = q.Encode() // Make request with pagination var pageResponse BaseAccountResponse - pageBody, err := c.callAPI(ctx, fmt.Sprintf("%s?%s", route, u.RawQuery)) + pageBody, err := c.callAPI(ctx, fmt.Sprintf("%s?%s", route, q.Encode()))
301-313: Same redundant URL construction pattern.Same issue as in
paginateArray- simplify the URL building.for currentResponse.Metadata.HasMore { - // Build URL with pagination parameter - u, err := url.Parse(fmt.Sprintf("%s/%s", c.baseURL, route)) - if err != nil { - return fmt.Errorf("failed to parse URL: %w", err) - } - q := u.Query() + // Build query parameter for pagination + q := url.Values{} q.Set("starting_after", nextPageStartingAfter) - u.RawQuery = q.Encode() // Make request with pagination var pageResponse BaseAccountResponse - pageBody, err := c.callAPI(ctx, fmt.Sprintf("%s?%s", route, u.RawQuery)) + pageBody, err := c.callAPI(ctx, fmt.Sprintf("%s?%s", route, q.Encode()))jwt/token.go (1)
556-559: Role objects without a Key are silently dropped.Roles with an
idandnamebut nokeyare excluded. This is documented in the comment, but consider logging or including a warning since this silently loses data.The comment explains the reasoning, but consumers might be confused if their API returns roles without keys.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
frameworks/gin_kinde/gin_kinde_test.go(1 hunks)jwt/account_api_helpers.go(1 hunks)jwt/account_api_helpers_test.go(1 hunks)jwt/jwt.go(4 hunks)jwt/jwt_options.go(6 hunks)jwt/jwt_test.go(1 hunks)jwt/token.go(18 hunks)kinde/account_api/client.go(1 hunks)kinde/account_api/client_test.go(1 hunks)oauth2/authorization_code/account_api.go(1 hunks)oauth2/authorization_code/account_api_test.go(1 hunks)oauth2/authorization_code/authorization_code.go(5 hunks)oauth2/authorization_code/example_middleware.go(2 hunks)oauth2/authorization_code/options.go(3 hunks)oauth2/authorization_code/options_test.go(1 hunks)oauth2/authorization_code/token_source_test.go(1 hunks)oauth2/client_credentials/options_test.go(1 hunks)oauth2/client_credentials/token_source_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
kinde/account_api/client_test.go (1)
kinde/account_api/client.go (5)
NewClient(62-77)Client(17-24)WithHTTPClient(37-41)BaseAccountResponse(139-145)Metadata(129-135)
jwt/jwt_options.go (4)
jwt/jwt.go (1)
Token(29-34)oauth2/authorization_code/options.go (1)
Option(10-10)oauth2/client_credentials/options.go (1)
Option(12-12)kinde/management_api/oas_cfg_gen.go (1)
Option(191-194)
oauth2/authorization_code/account_api.go (1)
kinde/account_api/client.go (2)
Client(17-24)NewClient(62-77)
oauth2/authorization_code/options_test.go (3)
oauth2/authorization_code/authorization_code.go (1)
AuthorizationCodeFlow(78-90)oauth2/authorization_code/options.go (9)
WithPrompt(82-86)WithOffline(101-105)WithCustomStateGenerator(127-131)WithClientID(168-172)WithClientSecret(184-188)WithScopes(206-210)WithAdditionalScope(228-232)WithPKCE(267-280)WithPKCEChallengeMethod(304-329)jwt/jwt_options.go (1)
WillValidateAlgorithm(201-209)
oauth2/client_credentials/options_test.go (3)
oauth2/client_credentials/client_credentials.go (1)
ClientCredentialsFlow(30-35)oauth2/client_credentials/options.go (1)
WithKindeManagementAPI(41-60)jwt/jwt_options.go (1)
WillValidateAlgorithm(201-209)
oauth2/authorization_code/token_source_test.go (3)
oauth2/client_credentials/token_source_test.go (4)
MockSessionHooksForTokenSource(17-19)TestSessionTokenSource_validateToken(34-58)TestSessionTokenSource_Token(60-109)TestSessionTokenSource_getValidatedToken(111-131)oauth2/authorization_code/authorization_code.go (1)
AuthorizationCodeFlow(78-90)jwt/jwt_options.go (1)
WillValidateWithJWKSUrl(73-98)
jwt/jwt_test.go (1)
jwt/jwt.go (1)
Token(29-34)
frameworks/gin_kinde/gin_kinde_test.go (1)
frameworks/gin_kinde/gin_kinde.go (1)
UseKindeAuth(99-167)
jwt/account_api_helpers_test.go (2)
jwt/account_api_helpers.go (1)
GetPermissionsOptions(12-16)jwt/jwt.go (1)
Token(29-34)
jwt/account_api_helpers.go (4)
kinde/management_api/oas_schemas_gen.go (2)
Permissions(17566-17575)Roles(18925-18936)jwt/jwt.go (1)
Token(29-34)kinde/account_api/client.go (1)
Client(17-24)jwt/token.go (2)
Role(491-495)FeatureFlag(347-350)
oauth2/authorization_code/authorization_code.go (2)
jwt/jwt.go (1)
Token(29-34)oauth2/authorization_code/options.go (2)
Option(10-10)WithScopes(206-210)
jwt/token.go (1)
jwt/jwt.go (2)
Token(29-34)ParseIDTokenUnverified(110-123)
🔇 Additional comments (44)
frameworks/gin_kinde/gin_kinde_test.go (3)
1-13: LGTM!Package declaration and imports are appropriate for the test suite.
245-250: LGTM! Reasonable to defer callback testing to integration tests.Skipping the OAuth callback route in unit tests is appropriate given the complexity of the full OAuth flow setup.
310-364: LGTM! Good defensive testing for type safety.The invalid type tests appropriately verify that the SessionStorage methods handle type mismatches gracefully and return descriptive errors.
jwt/jwt_options.go (3)
254-268: LGTM!The audience validation logic correctly handles both single-string and array audience claims using
slices.Contains. Consider using%qinstead of%vin the error message for clearer string formatting, but this is optional.
302-310: LGTM!Good defensive programming with the nil check for the validator function, preventing subtle runtime issues.
229-233: LGTM!Clean delegation to the underlying JWT library's issuer validation.
oauth2/authorization_code/example_middleware.go (2)
10-46: LGTM!Clear documentation example demonstrating the middleware integration pattern with token extraction from context.
62-82: LGTM!The middleware structure follows the correct pattern with appropriate HTTP status codes (401 for authentication failure, 403 for authorization failure).
oauth2/authorization_code/options_test.go (3)
11-44: LGTM!Comprehensive test coverage for
WithAuthParametercovering new parameter addition, appending, and deduplication.
140-170: LGTM!Clear and concise tests for scope management options.
102-112: VerifynewTestSessionHooks()helper is defined in this package.The test uses
newTestSessionHooks()on line 109, which should be defined in the test package. Ensure this helper function exists in the test files.oauth2/client_credentials/options_test.go (1)
12-53: LGTM!Well-structured tests for
WithAuthParameterandWithAudienceoptions.oauth2/client_credentials/token_source_test.go (3)
16-32: LGTM!Well-implemented mock with proper nil handling for the
GetRawTokenmethod.
111-131: LGTM!Good error propagation test that validates the error message content.
47-48: No action needed. ThetestclientCredentialsToken()helper function is defined inoauth2/client_credentials/client_credentials_test.goat line 83 and is properly accessible within the test package.oauth2/authorization_code/token_source_test.go (2)
14-60: LGTM!The mock implementation correctly provides all required session hooks for testing the authorization code flow's token source behavior.
62-140: No action needed—testJwtToken()is properly defined in the package.The function is defined in
oauth2/authorization_code/authorization_code_test.goat line 112 and is correctly accessible totoken_source_test.gowithin the same package. This is a standard Go testing pattern where helper functions are shared across test files in the same package.oauth2/authorization_code/account_api_test.go (1)
7-14: LGTM!The skipped integration test is appropriately marked with a clear explanation. Integration tests requiring full OAuth flow setup are reasonable to skip in unit test suites.
jwt/account_api_helpers_test.go (2)
17-245: LGTM!Comprehensive test coverage for account API helper methods with clear test cases for both token-based and API-based data retrieval. Good use of
httptestservers for mocking API responses.
247-263: LGTM!The
createTestTokenhelper provides a clean way to construct test tokens with custom claims. The structure properly initializes both the processing and rawToken fields.kinde/account_api/client_test.go (1)
14-317: LGTM!Excellent test coverage for the account API client including:
- Client initialization and options
- Authorization headers and token handling
- Single-page and multi-page pagination for both array and object responses
- Array merging with deduplication
- Deep object merging with nested structures
The tests are well-structured and clearly validate the expected behaviors.
jwt/jwt.go (4)
14-26: LGTM!The enhanced documentation for the
tokenProcessingstruct clearly explains the purpose of each field, improving code maintainability.
37-87: LGTM!Excellent documentation for
ParseFromAuthorizationHeaderandParseFromString. The enhanced docs include parameter descriptions, return values, error conditions, and usage examples.
89-123: LGTM - Security considerations well-documented.The
ParseIDTokenUnverifiedfunction includes clear warnings about when it should and should not be used. The documentation explicitly states it should only be used for ID tokens that have already been validated during the OAuth flow, which is the correct security posture.
125-239: LGTM!The enhanced documentation for
ParseFromSessionStorageandParseOAuth2Tokenis comprehensive, including detailed parameter descriptions, return value explanations, and usage examples. The docs clearly explain the validation options and error handling behavior.jwt/jwt_test.go (3)
1130-1329: LGTM!Comprehensive test coverage for
GetRolesandHasRolesmethods, including:
- Standard and Hasura role claims
- String and object role formats
- Precedence rules (standard over Hasura)
- Edge cases (missing claims, empty roles)
The tests thoroughly validate the expected behavior.
1331-1425: LGTM!Excellent test coverage for
GetUserProfileincluding:
- Profile extraction from ID token extras
- Required vs. optional claims (sub is required)
- Handling of missing ID tokens
The use of unsigned JWT tokens with
SigningMethodNoneis appropriate for unit tests.
1427-1688: LGTM!Thorough test coverage for:
- Generic claim access via
GetClaim- User organizations with standard and Hasura claims
- Permissions, org codes, and feature flags with Hasura fallback
- Precedence rules (standard claims preferred over Hasura)
The tests validate both happy paths and edge cases effectively.
oauth2/authorization_code/authorization_code.go (5)
57-58: LGTM!The addition of
GetToken(context.Context) (*jwt.Token, error)to bothIAuthorizationCodeFlowandIDeviceAuthorizationFlowinterfaces provides a consistent API for retrieving validated tokens across different OAuth2 flows.Also applies to: 74-74
100-107: LGTM!The
GetTokenimplementation correctly delegates to the token source'sgetValidatedTokenmethod, maintaining separation of concerns and proper error wrapping.
117-174: LGTM!Excellent additions:
NewAuthorizationCodeFlownow has comprehensive documentation explaining the flow, parameters, and usageNewDeviceAuthorizationFlowprovides a clean API for the device authorization grant, correctly delegating to the shared constructor with empty credentials- Both functions properly prepend default scopes (openid, profile, email)
183-248: LGTM!The enhanced documentation for
GetAuthURLandAuthorizationCodeReceivedHandleris comprehensive, clearly explaining:
- What each method does
- Parameters and their purposes
- Expected callback structure
- Security considerations (CSRF protection via state)
- Usage examples
412-437: LGTM!The enhanced documentation for
TokenFromContextclearly explains how to extract the token from the request context, including parameter descriptions, return values, and a practical usage example.jwt/account_api_helpers.go (1)
182-220: LGTM!The
Entitlement,Plan, andEntitlementsResulttypes are well-documented with clear field descriptions. Good use of descriptive comments explaining the purpose of each field.kinde/account_api/client.go (1)
347-370: LGTM!The
mergeArraysfunction correctly usesjson.RawMessagebytes as keys for deduplication, which provides reliable comparison for JSON objects.oauth2/authorization_code/options.go (3)
254-263: LGTM!The
WithTokenValidationoption correctly chains validation options and conditionally adds JWKS validation. Good documentation and flexibility.
206-232: LGTM!
WithScopesandWithAdditionalScopeprovide clear, complementary APIs for scope management. Good documentation distinguishes their behavior.
101-105: The "offline" scope is correct for Kinde; no code changes needed.Kinde uses the "offline" scope (not "offline_access") to request refresh tokens. The code correctly implements this. If any docstring mentions "offline_access," update it to accurately describe the "offline" scope instead.
jwt/token.go (6)
565-570: Verify intentional behavior: HasRoles returns true for empty roleKeys.When
roleKeysis empty, the function returnstrue. This might be intentional (vacuous truth - user trivially has "none of" the roles), but it could be surprising. Document this behavior or consider returningfalse.The current logic means
token.HasRoles()(no arguments) always returnstrue. Is this the intended behavior? Typically, checking "has any of these roles" with no roles would returnfalse.
626-667: LGTM!The
GetUserProfilemethod is well-implemented with proper nil checks, required field validation (sub claim), and graceful handling of optional fields. Good documentation explains when nil is returned.
709-736: LGTM!The
GetUserOrganizationsmethod correctly handles both standard and Hasura claim formats with proper fallback logic. The use ofextractStringArrayhelper promotes code reuse.
822-827: LGTM!The
GetValidationErrorsmethod correctly aggregates validation errors using thenewErrorhelper. Returningnilwhen there are no errors is the expected behavior.
240-271: LGTM!Good addition of Hasura format support for permissions claim (
x-hasura-permissions). The fallback pattern is consistent with other claim accessors in this file.
354-397: LGTM!The
GetFeatureFlagsmethod andextractFeatureFlagshelper are well-implemented with proper Hasura format fallback and type-safe extraction using thetoStringhelper.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
oauth2/client_credentials/options_test.go (2)
108-119: Strengthen the hostname extraction assertion.The test verifies that audiences is not nil but doesn't check the actual extracted hostname value. Consider asserting the expected audience to verify correct hostname extraction.
Apply this diff to add a proper assertion:
WithKindeManagementAPI("https://subdomain.my_kinde_tenant.kinde.com:8080")(flow) audiences := flow.config.EndpointParams["audience"] - // Should extract the hostname correctly - assert.NotNil(t, audiences) + // Should extract the hostname correctly, stripping port and .kinde.com suffix + assert.Contains(t, audiences, "https://subdomain.my_kinde_tenant.kinde.com/api")
207-213: Test the actual function behavior rather than the standard library.This test directly calls
url.Parseinstead of testingWithKindeManagementAPIbehavior with a port in the URL. Consider moving this toTestWithKindeManagementAPIas a sub-test that verifies the actual function handles ports correctly.Consider refactoring like this:
- t.Run("handles URL with port", func(t *testing.T) { - // This test just checks URL parsing, doesn't need a flow - - parsedURL, _ := url.Parse("https://my_kinde_tenant.kinde.com:8080") - hostname := parsedURL.Hostname() - assert.Equal(t, "my_kinde_tenant.kinde.com", hostname) - })And add to
TestWithKindeManagementAPI:t.Run("handles URL with port", func(t *testing.T) { flow := &ClientCredentialsFlow{ config: clientcredentials.Config{ EndpointParams: make(map[string][]string), }, } WithKindeManagementAPI("https://my_kinde_tenant.kinde.com:8080")(flow) audiences := flow.config.EndpointParams["audience"] assert.Contains(t, audiences, "https://my_kinde_tenant.kinde.com/api") })kinde/account_api/client.go (2)
96-98: Route normalization could prevent double slashes.If
routestarts with/, the URL will contain double slashes (e.g.,https://example.com//api/v1/...). While most servers handle this, normalizing would be cleaner.// Build URL - apiURL := fmt.Sprintf("%s/%s", c.baseURL, route) + apiURL := fmt.Sprintf("%s/%s", c.baseURL, strings.TrimPrefix(route, "/"))
447-470: Consider handlingjson.Marshalerrors to prevent silent data loss.The previous review suggestion to use JSON marshaling has been addressed. However, if
json.Marshalfails, all such items would share an empty key and be incorrectly deduplicated. While unlikely for API response data, a fallback would be safer.for _, item := range arr1 { - keyBytes, _ := json.Marshal(item) - key := string(keyBytes) + keyBytes, err := json.Marshal(item) + key := string(keyBytes) + if err != nil { + key = fmt.Sprintf("%v", item) // Fallback for unmarshalable items + } if !seen[key] { seen[key] = true result = append(result, item) } } for _, item := range arr2 { - keyBytes, _ := json.Marshal(item) - key := string(keyBytes) + keyBytes, err := json.Marshal(item) + key := string(keyBytes) + if err != nil { + key = fmt.Sprintf("%v", item) // Fallback for unmarshalable items + } if !seen[key] { seen[key] = true result = append(result, item) } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
kinde/account_api/client.go(1 hunks)oauth2/client_credentials/options.go(2 hunks)oauth2/client_credentials/options_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
oauth2/client_credentials/options_test.go (3)
oauth2/client_credentials/client_credentials.go (1)
ClientCredentialsFlow(30-35)oauth2/client_credentials/options.go (5)
WithAuthParameter(16-24)WithAudience(27-31)WithSessionHooks(34-38)WithKindeManagementAPI(41-69)WithTokenValidation(72-81)jwt/jwt_options.go (1)
WillValidateAlgorithm(201-209)
🔇 Additional comments (12)
oauth2/client_credentials/options.go (2)
43-46: LGTM!The empty string guard prevents invalid audience construction and aligns with the new test coverage.
60-64: LGTM!The guard correctly prevents adding an invalid audience when the host becomes empty after processing (e.g., input ".kinde.com").
oauth2/client_credentials/options_test.go (5)
12-38: LGTM!The test properly covers both adding a new parameter and appending to an existing one. The use of
t.Parallelis appropriate.
40-53: LGTM!The test correctly verifies that the audience parameter is added via the
WithAudiencehelper.
122-145: LGTM!The test properly covers both JWKS validation enablement and custom token option application.
147-188: LGTM!The table-driven test approach provides good coverage for various URL input formats.
194-205: LGTM!The assertion on line 204 properly verifies that empty input does not add an audience, addressing the previous review feedback.
kinde/account_api/client.go (5)
127-169: LGTM!The pagination metadata types and single-page fetch implementation are well-structured with clear documentation.
197-228: LGTM!The dispatch logic correctly handles single-page responses efficiently and properly detects array vs object responses for appropriate pagination handling.
346-369: LGTM!Using raw JSON bytes for deduplication is appropriate here since API responses from the same endpoint should have consistent formatting.
377-391: LGTM!Efficient manual construction is appropriate since
json.RawMessageitems are already valid JSON.
403-438: LGTM!The recursive deep merge correctly handles nested maps and arrays with proper fallback behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
kinde/account_api/client.go(1 hunks)oauth2/client_credentials/options.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- oauth2/client_credentials/options.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: GitGuardian Security Checks
🔇 Additional comments (4)
kinde/account_api/client.go (4)
1-84: Well-structured client implementation with proper input validation.The Account API client is well-designed with good documentation, proper use of functional options, and comprehensive input validation. The past review concerns about missing validation have been properly addressed.
154-346: Pagination implementation correctly handles query parameters.The pagination methods properly parse routes and merge query parameters, addressing the concerns from previous reviews. The logic correctly handles both array and object responses with appropriate merging strategies.
348-400: Efficient array merging and marshaling helpers.The
mergeArraysandmarshalArrayhelpers are well-implemented with efficient deduplication forjson.RawMessageand optimized array construction.
402-447: Deep merge logic correctly handles nested structures.The recursive object merging properly handles nested maps and delegates array merging appropriately. The logic preserves data from both objects as intended.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
oauth2/authorization_code/authorization_code.go (2)
239-249: Missing error handling for SetRawToken in callback handler.The
SetRawTokencall at line 247 ignores the returned error. If session storage fails, the user appears authenticated but has no stored token, leading to confusing behavior on subsequent requests.func (flow *AuthorizationCodeFlow) AuthorizationCodeReceivedHandler(w http.ResponseWriter, r *http.Request) { receivedState := r.URL.Query().Get("state") if flow.stateVerifier(flow, receivedState) { token, err := flow.config.Exchange(r.Context(), r.URL.Query().Get("code")) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return } - flow.sessionHooks.SetRawToken(token) + if err := flow.sessionHooks.SetRawToken(token); err != nil { + http.Error(w, "failed to store token", http.StatusInternalServerError) + return + } } }
239-249: State verification failure is silently ignored.When state verification fails (line 241 returns false), the handler completes without any response or error, leaving the client with an empty 200 OK response. This should return an error for invalid/mismatched state (potential CSRF attack).
func (flow *AuthorizationCodeFlow) AuthorizationCodeReceivedHandler(w http.ResponseWriter, r *http.Request) { receivedState := r.URL.Query().Get("state") - if flow.stateVerifier(flow, receivedState) { + if !flow.stateVerifier(flow, receivedState) { + http.Error(w, "invalid state parameter", http.StatusBadRequest) + return + } - token, err := flow.config.Exchange(r.Context(), r.URL.Query().Get("code")) + token, err := flow.config.Exchange(r.Context(), r.URL.Query().Get("code")) - if err != nil { + if err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) + http.Error(w, err.Error(), http.StatusInternalServerError) - return + return - } + } - flow.sessionHooks.SetRawToken(token) + if err := flow.sessionHooks.SetRawToken(token); err != nil { + http.Error(w, "failed to store token", http.StatusInternalServerError) + return } }frameworks/gin_kinde/gin_kinde.go (6)
63-72: Inconsistent error handling for session save.
SetRawTokenignores the error returned bysession.Save()at line 70, whileSetCodeVerifier(lines 45-47) correctly handles it. This inconsistency could lead to silent failures when storing tokens.func (storage *SessionStorage) SetRawToken(token *oauth2.Token) error { if token == nil { storage.session.Set("kinde_token", nil) } else { storage.session.Set("kinde_token", token) } - storage.session.Save() - return nil + if err := storage.session.Save(); err != nil { + return fmt.Errorf("failed to save session: %w", err) + } + return nil }
74-82: Nil pointer panic risk in GetPostAuthRedirect and GetState.Both methods perform direct type assertions without nil checks. If the session value is nil, this will panic.
// GetPostAuthRedirect implements authorization_code.SessionHooks. func (storage *SessionStorage) GetPostAuthRedirect() (string, error) { - return storage.session.Get("post_auth_redirect").(string), nil + v := storage.session.Get("post_auth_redirect") + if v == nil { + return "", nil + } + s, ok := v.(string) + if !ok { + return "", fmt.Errorf("invalid post_auth_redirect type in session") + } + return s, nil } // GetState implements authorization_code.SessionHooks. func (storage *SessionStorage) GetState() (string, error) { - return storage.session.Get("auth_state").(string), nil + v := storage.session.Get("auth_state") + if v == nil { + return "", nil + } + s, ok := v.(string) + if !ok { + return "", fmt.Errorf("invalid auth_state type in session") + } + return s, nil }
104-110: Nil pointer panic risk in GetItem.Direct type assertion at line 109 will panic if the value is not a string (e.g., stored as a different type).
func (storage *SessionStorage) GetItem(key string) string { value := storage.session.Get(key) if value == nil { return "" } - return value.(string) + if s, ok := value.(string); ok { + return s + } + return "" }
161-193: Kinde client is recreated on every request.The middleware at lines 163-193 creates a new
AuthorizationCodeFlowinstance for every request. This is inefficient and can cause issues with state management since PKCE code challenges and state are stored on the flow instance.Consider initializing the flow once during
UseKindeAuthsetup and storing only the session-specific storage in context.func UseKindeAuth(router *gin.RouterGroup, kindeDomain, clientID, clientSecret, baseRedirectURL string, options ...authorization_code.Option) error { + basePath := router.BasePath() + if basePath == "/" { + basePath = "" + } + redirectURI := fmt.Sprintf("%s%s%s", baseRedirectURL, basePath, "/kinde/callback") router.Use(func(ctx *gin.Context) { session := sessions.Default(ctx) sessionStorage := &SessionStorage{session: session} - basePath := router.BasePath() - if basePath == "/" { - basePath = "" - } - - options = append(options, + reqOptions := append(options, authorization_code.WithSessionHooks(sessionStorage), authorization_code.WithTokenValidation(true), ) - redirectURI := fmt.Sprintf("%s%s%s", baseRedirectURL, basePath, "/kinde/callback") kindeClient, err := authorization_code.NewAuthorizationCodeFlow(kindeDomain, clientID, clientSecret, redirectURI, - options..., + reqOptions..., )Note: While moving initialization outside the middleware would be ideal, the session hooks need to be per-request. However, the current code mutates the
optionsslice on each request (line 172), which causes options to accumulate. This is a bug that should be fixed by using a copy.
195-208: Type assertion to concrete type breaks interface abstraction.Line 197 asserts to
*authorization_code.AuthorizationCodeFlow(concrete type), but the client is returned asIAuthorizationCodeFlow(interface). This assertion will always fail, causing a 500 error on every callback.router.GET("/kinde/callback", func(ctx *gin.Context) { if client, ok := ctx.Get("kinde_client"); ok { - if kindeClient, ok := client.(*authorization_code.AuthorizationCodeFlow); ok { + if kindeClient, ok := client.(authorization_code.IAuthorizationCodeFlow); ok { err := kindeClient.ExchangeCode(context.Background(), ctx.Query("code"), ctx.Query("state"))
210-226: Same type assertion issue in authentication middleware.Line 213 has the same concrete type assertion problem as the callback handler.
router.Use(func(ctx *gin.Context) { if client, ok := ctx.Get("kinde_client"); ok { - if kindeClient, ok := client.(*authorization_code.AuthorizationCodeFlow); ok { + if kindeClient, ok := client.(authorization_code.IAuthorizationCodeFlow); ok {
🧹 Nitpick comments (3)
oauth2/authorization_code/authorization_code.go (1)
117-148: Documentation example shows redundant scopes.The documentation states that default scopes (
openid,profile,WithScopes("openid", "profile", "email")again, which would result in duplicate scopes.Consider updating the example to show additional scopes or remove the
WithScopescall:// Example: // // flow, err := NewAuthorizationCodeFlow( // "https://yourdomain.kinde.com", // "your-client-id", // "your-client-secret", // "https://yourapp.com/callback", -// WithScopes("openid", "profile", "email"), +// WithScopes("offline_access"), // Additional scopes beyond defaults // WithPKCE(), // )frameworks/gin_kinde/gin_kinde.go (2)
84-96: Ignored session save errors in SetPostAuthRedirect and SetState.Both methods call
session.Save()but ignore errors, inconsistent withSetCodeVerifier.// SetPostAuthRedirect implements authorization_code.SessionHooks. func (storage *SessionStorage) SetPostAuthRedirect(redirect string) error { storage.session.Set("post_auth_redirect", redirect) - storage.session.Save() - return nil + if err := storage.session.Save(); err != nil { + return fmt.Errorf("failed to save session: %w", err) + } + return nil } // SetState implements authorization_code.SessionHooks. func (storage *SessionStorage) SetState(state string) error { storage.session.Set("auth_state", state) - storage.session.Save() - return nil + if err := storage.session.Save(); err != nil { + return fmt.Errorf("failed to save session: %w", err) + } + return nil }
198-198: Usingcontext.Background()discards request context.Both
ExchangeCode(line 198) andIsAuthenticated(line 215) usecontext.Background()instead ofctx.Request.Context(). This loses request-scoped values (tracing, timeouts, cancellation).- err := kindeClient.ExchangeCode(context.Background(), ctx.Query("code"), ctx.Query("state")) + err := kindeClient.ExchangeCode(ctx.Request.Context(), ctx.Query("code"), ctx.Query("state"))- if isAuthenticated, _ := kindeClient.IsAuthenticated(context.Background()); !isAuthenticated { + if isAuthenticated, _ := kindeClient.IsAuthenticated(ctx.Request.Context()); !isAuthenticated {Also applies to: 215-215
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.modis excluded by!**/*.mod
📒 Files selected for processing (3)
frameworks/gin_kinde/gin_kinde.go(3 hunks)kinde/account_api/client.go(1 hunks)oauth2/authorization_code/authorization_code.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- kinde/account_api/client.go
🧰 Additional context used
🧬 Code graph analysis (2)
frameworks/gin_kinde/gin_kinde.go (1)
examples/gin-chat/main.go (1)
SessionStorage(15-17)
oauth2/authorization_code/authorization_code.go (2)
jwt/jwt.go (1)
Token(29-34)oauth2/authorization_code/options.go (2)
Option(10-10)WithScopes(206-210)
🔇 Additional comments (7)
oauth2/authorization_code/authorization_code.go (5)
57-58: LGTM - Interface extension for GetToken.The
GetTokenmethod addition toIAuthorizationCodeFlowis consistent with the implementation at lines 100-107 and aligns with theIDeviceAuthorizationFlowinterface.
100-107: LGTM - GetToken implementation.The implementation correctly delegates to the token source and returns a validated token. Error wrapping provides useful context.
151-174: LGTM - NewDeviceAuthorizationFlow documentation and implementation.The documentation clearly explains the device flow use case and the implementation correctly delegates to the internal constructor with empty credentials.
183-217: LGTM - GetAuthURL documentation and PKCE parameter handling.The enhanced documentation is comprehensive. The PKCE code challenge parameters are correctly added when
usePKCEis enabled.
413-438: LGTM - TokenFromContext documentation.The enhanced documentation clearly explains the purpose, parameters, and provides a practical usage example.
frameworks/gin_kinde/gin_kinde.go (2)
13-27: LGTM - SessionStorage type documentation.The documentation clearly describes the purpose and capabilities of the SessionStorage type.
29-49: LGTM - GetCodeVerifier and SetCodeVerifier implementations.Both methods properly handle nil checks, type assertions, and save errors.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frameworks/gin_kinde/gin_kinde.go (2)
125-137: Fix error handling in SetPostAuthRedirect and SetState.Both methods call
session.Save()but ignore the returned error. This could lead to silent failures when session data cannot be persisted.Apply this diff to properly handle errors:
func (storage *SessionStorage) SetPostAuthRedirect(redirect string) error { storage.session.Set("post_auth_redirect", redirect) - storage.session.Save() - return nil + if err := storage.session.Save(); err != nil { + return fmt.Errorf("failed to save session: %w", err) + } + return nil } func (storage *SessionStorage) SetState(state string) error { storage.session.Set("auth_state", state) - storage.session.Save() - return nil + if err := storage.session.Save(); err != nil { + return fmt.Errorf("failed to save session: %w", err) + } + return nil }
212-235: Performance issue: Kinde client created on every request.The middleware creates a new
AuthorizationCodeFlowclient on every request (lines 220-225). This is inefficient as it involves:
- Constructing OAuth2 config
- Setting up JWKS URL
- Initializing state generators/verifiers
This overhead is unnecessary since the client configuration is static per application instance.
Refactor to create the client once during initialization:
func UseKindeAuth(router *gin.RouterGroup, kindeDomain, clientID, clientSecret, baseRedirectURL string, options ...authorization_code.Option) error { basePath := router.BasePath() if basePath == "/" { basePath = "" } redirectURI := fmt.Sprintf("%s%s%s", baseRedirectURL, basePath, "/kinde/callback") + + // Create base options (without session hooks, which are request-specific) + baseOptions := append(options, authorization_code.WithTokenValidation(true)) router.Use(func(ctx *gin.Context) { session := sessions.Default(ctx) sessionStorage := &SessionStorage{session: session} - reqOptions := append(options, + // Create client with request-specific session hooks + reqOptions := append(baseOptions, authorization_code.WithSessionHooks(sessionStorage), - authorization_code.WithTokenValidation(true), ) kindeClient, err := authorization_code.NewAuthorizationCodeFlow(kindeDomain, clientID, clientSecret, redirectURI, reqOptions..., )Note: A better approach would be to refactor the
AuthorizationCodeFlowto separate static configuration from per-request session management, but that would require changes to the authorization_code package. The above diff is a minimal improvement, though it still recreates the client per request.Would you like me to propose a more comprehensive refactoring that moves client creation outside the request handler entirely?
🧹 Nitpick comments (4)
oauth2/authorization_code/authorization_code.go (1)
239-254: Consider more specific error messaging for state verification failures.The state verifier (line 241) can fail for two distinct reasons: session retrieval errors or actual state mismatch. Currently, both cases return "invalid state parameter", which may make debugging session-related issues harder.
Consider handling these cases separately:
- if !flow.stateVerifier(flow, receivedState) { - http.Error(w, "invalid state parameter", http.StatusBadRequest) - return - } + storedState, err := flow.sessionHooks.GetState() + if err != nil { + http.Error(w, "failed to retrieve state from session", http.StatusInternalServerError) + return + } + if storedState != receivedState { + http.Error(w, "invalid state parameter", http.StatusBadRequest) + return + }frameworks/gin_kinde/gin_kinde.go (3)
52-79: Consider clarifying the error message for JSON unmarshal failures.The backwards compatibility logic is well-implemented, but line 76 returns "invalid token type in session" when
json.Unmarshalfails. This could be misleading—the actual issue might be malformed JSON rather than an incorrect type.Consider making the error message more specific:
var t oauth2.Token if err := json.Unmarshal(tokenBytes, &t); err != nil { - return nil, fmt.Errorf("invalid token type in session") + return nil, fmt.Errorf("failed to unmarshal token from session: %w", err) }
161-164: Consider adding error handling to SetItem for consistency.Unlike other setter methods (e.g.,
SetRawToken,SetCodeVerifier),SetItemdoesn't return an error even thoughsession.Save()can fail. This inconsistency could make error handling unpredictable for consumers.Consider updating the signature:
-func (storage *SessionStorage) SetItem(key, value string) { +func (storage *SessionStorage) SetItem(key, value string) error { storage.session.Set(key, value) - storage.session.Save() + if err := storage.session.Save(); err != nil { + return fmt.Errorf("failed to save session: %w", err) + } + return nil }
237-250: Consider using the post_auth_redirect for callback redirects.Line 245 hard-codes a redirect to "/" after successful authentication. The
SessionStoragehasGetPostAuthRedirect()andSetPostAuthRedirect()methods that could be used to redirect users back to their original destination.If post-auth redirect is intended to be used:
router.GET("/kinde/callback", func(ctx *gin.Context) { if client, ok := ctx.Get("kinde_client"); ok { if kindeClient, ok := client.(authorization_code.IAuthorizationCodeFlow); ok { err := kindeClient.ExchangeCode(context.Background(), ctx.Query("code"), ctx.Query("state")) if err != nil { ctx.AbortWithError(500, err) return } - ctx.Redirect(302, "/") + + // Get the stored post-auth redirect or default to "/" + session := sessions.Default(ctx) + sessionStorage := &SessionStorage{session: session} + redirectURL, _ := sessionStorage.GetPostAuthRedirect() + if redirectURL == "" { + redirectURL = "/" + } + ctx.Redirect(302, redirectURL) return } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frameworks/gin_kinde/gin_kinde.go(6 hunks)oauth2/authorization_code/authorization_code.go(5 hunks)
🔇 Additional comments (7)
oauth2/authorization_code/authorization_code.go (3)
57-58: LGTM! Clean interface addition.The
GetTokenmethod is a valuable addition to the interface, providing a clean way to retrieve validated JWT tokens from the flow.
100-107: LGTM! Clean implementation.The implementation correctly retrieves the token source and validates the token with proper error wrapping.
117-144: Excellent documentation improvements.The enhanced documentation is comprehensive, clear, and includes practical examples. The note about default scopes being prepended is particularly helpful.
frameworks/gin_kinde/gin_kinde.go (4)
14-25: LGTM! Clear and comprehensive documentation.The documentation clearly explains the purpose and capabilities of SessionStorage.
81-97: LGTM! JSON serialization is a good approach.Using JSON serialization avoids gob serialization issues and is more portable.
99-123: LGTM! Proper nil handling.Both
GetPostAuthRedirectandGetStatecorrectly handle nil values by returning empty strings, which is appropriate for optional session data.
166-204: Excellent documentation with practical examples.The documentation clearly explains the middleware's functionality, parameters, and provides a helpful usage example.
5a85e6a to
6a52861
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frameworks/gin_kinde/gin_kinde.go (1)
304-327: PKCE verifier regenerated on every request causes token exchange failures when PKCE is enabled.The middleware creates a new
AuthorizationCodeFlowon every request (line 312). WhenWithPKCE()is included in the options passed toUseKindeAuth, a new code verifier and challenge are generated each time this middleware runs. Since the session persists across requests but the verifier value is overwritten on each middleware invocation, any request that arrives between the initial auth redirect and the callback will replace the verifier in the session. When the callback arrives, the token exchange fails because the stored verifier no longer matches the challenge sent to Kinde.To fix: Either create the flow once at initialization time (not per-request) or defer verifier/challenge generation until
GetAuthURLis called rather than during flow construction.
🧹 Nitpick comments (2)
oauth2/authorization_code/authorization_code.go (1)
198-217: Consider handling the URL parse error.Line 201 ignores the error from
url.Parse. While the URL is constructed programmatically and unlikely to be malformed, returning an error or logging would aid debugging if issues arise.oauth2/authorization_code/options.go (1)
267-280: Errors from SetCodeVerifier are silently ignored.Both
WithPKCE(line 275) andWithPKCEChallengeMethod(line 320) discard errors fromSetCodeVerifier. If session storage fails, the flow will proceed without a stored verifier, causing token exchange to fail later with a confusing error.func WithPKCE() Option { return func(s *AuthorizationCodeFlow) { s.usePKCE = true s.challengeMethod = "S256" if codeVerifier, err := generateCodeVerifier(); err == nil { if s.sessionHooks != nil { - _ = s.sessionHooks.SetCodeVerifier(codeVerifier) + if err := s.sessionHooks.SetCodeVerifier(codeVerifier); err != nil { + // Consider storing error or logging - flow will fail at exchange + } } s.codeChallenge = generateCodeChallenge(codeVerifier) } } }At minimum, consider storing the error or adding a mechanism to surface it. Alternatively, defer PKCE artifact generation to
GetAuthURLtime when failure can be reported.Also applies to: 304-329
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.modis excluded by!**/*.mod
📒 Files selected for processing (7)
frameworks/gin_kinde/gin_kinde.go(6 hunks)kinde/account_api/client.go(1 hunks)oauth2/authorization_code/authorization_code.go(5 hunks)oauth2/authorization_code/example_middleware.go(2 hunks)oauth2/authorization_code/options.go(3 hunks)oauth2/client_credentials/options.go(2 hunks)oauth2/client_credentials/options_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- oauth2/client_credentials/options_test.go
- oauth2/authorization_code/example_middleware.go
🧰 Additional context used
🧬 Code graph analysis (2)
frameworks/gin_kinde/gin_kinde.go (3)
examples/gin-chat/main.go (1)
SessionStorage(15-17)oauth2/authorization_code/options.go (2)
Option(10-10)WithSessionHooks(153-157)oauth2/authorization_code/authorization_code.go (1)
IAuthorizationCodeFlow(42-59)
oauth2/authorization_code/authorization_code.go (2)
jwt/jwt.go (1)
Token(29-34)oauth2/authorization_code/options.go (2)
Option(10-10)WithScopes(206-210)
🔇 Additional comments (9)
oauth2/client_credentials/options.go (1)
43-66: Good defensive validation added.The early returns for empty
kindeDomainand emptyhostafter processing prevent malformed audience URLs. UsingWithAudienceinstead of directWithAuthParametercorrectly eliminates the duplicate audience issue flagged previously.kinde/account_api/client.go (4)
62-84: Input validation properly implemented.The
NewClientfunction now correctly validates bothbaseURLandgetTokenparameters, addressing the documentation contract and preventing nil pointer panics downstream.
244-294: Pagination URL handling looks correct.The route is now properly parsed with
url.Parse, and query parameters are correctly merged usingrouteURL.Query()before encoding. This preserves any existing query parameters in the route.
460-493: JSON marshaling error handling correctly implemented.The
mergeInterfaceArraysfunction now handlesjson.Marshalerrors by including the item without deduplication rather than silently dropping it. This prevents data loss for unmarshalable items.
355-378: Deduplication using raw JSON bytes is reliable.Using
string(item)onjson.RawMessagefor deduplication keys is appropriate since these are already normalized JSON bytes, avoiding the ordering issues that would occur withfmt.Sprintf("%v")on complex objects.oauth2/authorization_code/authorization_code.go (1)
57-59: GetToken method correctly added to interface and implemented.The
GetTokenmethod provides a clean public API for retrieving validated tokens from the flow, delegating to the internal token source. This aligns well with the Account API integration that needs token access.Also applies to: 100-107
frameworks/gin_kinde/gin_kinde.go (1)
87-113: Good backwards compatibility handling for token storage.The
GetRawTokenmethod handles multiple storage formats (JSON string, byte slice, and direct token pointer) gracefully, ensuring smooth migration between storage formats.oauth2/authorization_code/options.go (2)
29-40: Good duplicate prevention for auth parameters.The
slices.Containscheck prevents adding duplicate values for the same parameter key, which is a sensible safeguard.
101-105: No issues found. The scope name is correct.Kinde's documentation confirms that "offline" is the correct scope name to request refresh tokens, not "offline_access". The implementation in
WithOffline()is correct.Likely an incorrect or invalid review comment.
- Add account API helpers for roles, permissions, and feature flags - Improve JWT token parsing and validation - Add comprehensive error handling in session storage - Implement JSON serialization for OAuth2 tokens in Gin sessions - Enhance documentation coverage across all packages - Fix multiple CodeRabbit review issues - Add extensive test coverage for new features - Improve token handling and error management throughout
- Bump versions for several dependencies including: - github.com/MicahParks/keyfunc/v3 to v3.7.0 - github.com/golang-jwt/jwt/v5 to v5.3.0 - github.com/stretchr/testify to v1.11.1 - golang.org/x/oauth2 to v0.34.0 - golang.org/x/term to v0.38.0 - Update indirect dependencies for bytedance/sonic, cloudwego/base64x, and others. - Ensure compatibility with the latest versions and improve overall stability.
- Added error handling for session.Save() in SetPostAuthRedirect and SetState methods to ensure failures are reported. - Updated SetItem method to ignore errors from session.Save() for backward compatibility, with a note in the documentation. - Refactored AuthorizationCodeReceivedHandler to use ExchangeCode for better PKCE support and removed unused token handling code.
6a52861 to
7e0f47e
Compare
KeeganBeuthin
left a comment
KeeganBeuthin
left a comment
There was a problem hiding this comment.
Really great work Brandt, just finished reviewing and testing.
All of my concerns would be quite minor. Approved :)
3 participants
Explain your changes
Adds an Account API client with pagination and deep-merge utilities; extends JWT parsing and token helpers (claims, roles, feature flags, entitlements); expands OAuth2 flows/options (PKCE, device flow, GetToken, session hooks); adds Gin session-backed storage and many tests across packages.
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.