Add role and permission handling in JWT token - #47
Conversation
- Implemented `GetRoles` and `HasRoles` methods to retrieve user roles from both standard and Hasura claims. - Enhanced `GetPermissions`, `GetOrganizationCode`, and `GetFeatureFlags` methods to support fallback between standard and Hasura claim formats. - Added comprehensive unit tests for role and permission retrieval, ensuring correct functionality across various scenarios. This update improves the flexibility and robustness of token handling in the application.
WalkthroughAdds extensive tests across Gin and OAuth2 flows; implements a Kinde Account API client with pagination and merging; enriches JWT token parsing and claim helpers (roles, feature flags, profile, ParseIDTokenUnverified); adds token-to-Account-API helper methods and exposes Account API client creation from the authorization-code flow. Changes
*Note: multiple JWT test additions consolidated under "JWT Tests". Sequence Diagram(s)sequenceDiagram
participant Caller as Test/Caller
participant Token as jwt.Token
participant AccountAPI as account_api.Client
participant HTTP as Account API Server
Note over Caller,AccountAPI: Token.GetPermissionsWithAPI(forceAPI=true)
Caller->>Token: GetPermissionsWithAPI(ctx, apiClient, ForceAPI=true)
Token->>AccountAPI: CallAccountAPIPaginated("/organizations/:org/permissions")
AccountAPI->>HTTP: GET /... (Authorization: Bearer <access_token>)
HTTP-->>AccountAPI: 200 JSON page + metadata
AccountAPI-->>Token: aggregated pages
Token->>Token: merge/dedupe/transform pages -> PermissionsWithOrg
Token-->>Caller: PermissionsWithOrg
sequenceDiagram
participant Client as Browser
participant Router as Gin Router
participant Middleware as UseKindeAuth
participant Session as Session Store
participant Flow as AuthorizationCodeFlow
participant Handler as Protected Handler
Client->>Router: Request private route
Router->>Middleware: invoke middleware
Middleware->>Session: GetRawToken()
alt token present
Session-->>Middleware: oauth2.Token
Middleware->>Flow: GetAccountAPIClient(ctx)
Flow-->>Middleware: account_api.Client
Middleware->>Router: inject client into context
Router->>Handler: forward request
Handler-->>Client: 200 OK
else token missing/invalid
Session-->>Middleware: nil/error
Middleware->>Client: 302 Redirect to auth endpoint
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (12)
frameworks/gin_kinde/gin_kinde_test.go (2)
15-214: Suggest extracting test setup into a helper function.The test setup pattern is repeated in every test case (creating store, router, configuring middleware, and extracting session). This duplication can be reduced by extracting a common helper function.
Consider adding a helper function:
func setupTestSession(t *testing.T) (sessions.Session, *httptest.ResponseRecorder) { t.Helper() 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, w }Then simplify tests:
- 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) - + session, _ := setupTestSession(t) storage := &SessionStorage{session: session}
221-243: Strengthen test assertion for kinde_client in context.The test "creates kinde client in context" only verifies that the response recorder is not nil (line 242), which doesn't actually validate that the kinde_client was set in the context. Consider adding a test route that accesses the context value to verify the client was properly set.
privateGroup := router.Group("/private") 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) 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/client_credentials/token_source_test.go (2)
34-58: Test name vs behavior forvalidateTokenThe subtest is named
"successfully validates token"but explicitly expects validation to fail due to missing JWKS, only asserting thaterris non‑nil. Consider renaming the test (e.g. to “fails validation without JWKS”) or tightening the assertion (e.g. checking an error substring) so the intent matches the behavior under test.
60-109: Token source tests are structurally OK but only assert failure casesBoth subtests in
TestSessionTokenSource_Tokenand thegetValidatedTokentest primarily assert that “something failed” (non‑nil error) under misconfigured or error conditions. That’s fine as smoke coverage, but if you want these to guard behavior more strongly, adding a happy‑path unit test (with a fake token source / stubbed JWKS) would better exercise caching and validation logic without depending on integration tests.oauth2/authorization_code/token_source_test.go (1)
62-140: Authorization code token source tests focus only on error pathsSimilar to the client‑credentials tests, these subtests validate error propagation when JWKS/config or session hooks fail, while names like
"successfully validates token"might suggest a passing validation scenario. If/when you add more coverage, consider:
- Renaming error‑path tests to reflect the expected failure.
- Adding at least one happy‑path unit (with a stubbed JWKS/keyfunc) so
validateToken,Token, andgetValidatedTokenare exercised under success conditions as well.jwt/account_api_helpers.go (1)
61-95: Roles API helper returns normalizedRoleobjects
GetRolesWithAPImirrorsGetRoles()semantics:
- Uses token roles when
forceAPIis false.- On
forceAPI == true, callsaccount_api/v1/rolesand converts each entry into the exportedRoletype.If you expect org‑specific role information to matter, consider later extending the return type (or adding a parallel helper) to surface
OrgCodeas well, but as‑is this is coherent with howGetRoles()works.jwt/token.go (1)
259-299: Feature flag extraction helper is a nice reuse point and filters malformed entriesThe refactored
GetFeatureFlagsplusextractFeatureFlags:
- Prefer the standard
"feature_flags"claim, then fallback to"x-hasura-feature-flags".- Build a
map[string]FeatureFlagonly from entries that contain both"t"and"v".- Use
toStringfor the type so non‑string type markers don’t panic.This is consistent with existing feature flag usage and matches the new tests that ensure malformed flags are ignored.
kinde/account_api/client.go (5)
33-47: Consider adding input validation for required parameters.The constructor doesn't validate that
baseURLis non-empty or thatgetTokenis non-nil. Adding early validation would provide clearer error messages at construction time rather than later during API calls.Apply this diff to add validation:
func NewClient(baseURL string, getToken func(ctx context.Context) (string, error), opts ...ClientOption) (*Client, error) { + if baseURL == "" { + return nil, fmt.Errorf("baseURL cannot be empty") + } + if getToken == nil { + return nil, fmt.Errorf("getToken function cannot be nil") + } + // Remove trailing slash baseURL = strings.TrimSuffix(baseURL, "/")
61-62: Handle leading slashes in route to prevent double slashes in URL.If
routestarts with a/, the URL construction will produce a double slash (e.g.,https://api.example.com//v1/users). While many servers tolerate this, it's not RFC-compliant and could cause issues.Apply this diff to normalize the route:
// Build URL - apiURL := fmt.Sprintf("%s/%s", c.baseURL, route) + route = strings.TrimPrefix(route, "/") + apiURL := fmt.Sprintf("%s/%s", c.baseURL, route)
251-275: Deduplication relies on exact byte-matching of JSON.The deduplication uses
string(item)as the key, which means JSON objects with identical content but different key ordering would be treated as duplicates. While API responses typically have consistent key ordering, this approach is fragile.For more robust deduplication, consider unmarshaling to a comparable format and using semantic comparison, or add a comment documenting this assumption:
// mergeArrays merges two arrays and removes duplicates. +// Note: Deduplication assumes identical JSON byte representation; +// objects with the same content but different key order are treated as distinct. func mergeArrays(arr1, arr2 []json.RawMessage) []json.RawMessage {
294-302: Document the fallback behavior when inputs aren't maps.When either
obj1orobj2is not amap[string]interface{}, the function returnsobj2, which discardsobj1entirely. For the pagination use case where both should be maps, this is acceptable, but it could be documented to clarify the expected behavior.Add a comment explaining this:
// deepMergeObjects deeply merges two objects. +// If either input is not a map[string]interface{}, returns obj2. func deepMergeObjects(obj1, obj2 interface{}) interface{} {
332-354: Deduplication usingfmt.Sprintf("%v", item)is fragile for complex types.Using
fmt.Sprintf("%v", item)to generate deduplication keys has limitations:
- Maps with identical content but different iteration order produce different string representations
- Complex nested structures may not format consistently
- This can cause incorrect deduplication or failure to deduplicate semantically identical items
For more reliable deduplication, consider marshaling items to JSON and comparing the byte representation (similar to
mergeArrays):func mergeInterfaceArrays(arr1, arr2 []interface{}) []interface{} { seen := make(map[string]bool) result := []interface{}{} for _, item := range arr1 { - key := fmt.Sprintf("%v", item) + itemBytes, _ := json.Marshal(item) + key := string(itemBytes) if !seen[key] { seen[key] = true result = append(result, item) } } for _, item := range arr2 { - key := fmt.Sprintf("%v", item) + itemBytes, _ := json.Marshal(item) + key := string(itemBytes) if !seen[key] { seen[key] = true result = append(result, item) } } return result }Note: This still has the JSON key-ordering limitation, but it's more consistent with the
mergeArraysapproach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
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_test.go(1 hunks)jwt/token.go(5 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(1 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 (11)
oauth2/authorization_code/account_api.go (2)
oauth2/authorization_code/authorization_code.go (1)
AuthorizationCodeFlow(78-90)kinde/account_api/client.go (2)
Client(15-19)NewClient(33-48)
jwt/account_api_helpers_test.go (2)
jwt/account_api_helpers.go (1)
GetPermissionsOptions(11-13)jwt/jwt.go (1)
Token(22-27)
oauth2/client_credentials/token_source_test.go (2)
oauth2/client_credentials/client_credentials.go (1)
ClientCredentialsFlow(30-35)jwt/jwt_options.go (1)
WillValidateWithJWKSUrl(28-53)
oauth2/authorization_code/authorization_code.go (1)
jwt/jwt.go (1)
Token(22-27)
frameworks/gin_kinde/gin_kinde_test.go (1)
frameworks/gin_kinde/gin_kinde.go (1)
UseKindeAuth(99-167)
oauth2/authorization_code/token_source_test.go (2)
oauth2/authorization_code/authorization_code.go (1)
AuthorizationCodeFlow(78-90)jwt/jwt_options.go (1)
WillValidateWithJWKSUrl(28-53)
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(77-85)
jwt/jwt_test.go (1)
jwt/jwt.go (1)
Token(22-27)
kinde/account_api/client_test.go (1)
kinde/account_api/client.go (5)
NewClient(33-48)Client(15-19)WithHTTPClient(25-29)BaseAccountResponse(99-102)Metadata(93-96)
jwt/token.go (1)
jwt/jwt.go (2)
Token(22-27)ParseFromString(42-44)
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(22-27)kinde/account_api/client.go (1)
Client(15-19)jwt/token.go (2)
Role(353-357)FeatureFlag(254-257)
⏰ 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: build
🔇 Additional comments (20)
oauth2/authorization_code/account_api_test.go (1)
7-14: LGTM - Appropriate test placeholder for integration testing.The skipped test appropriately recognizes that GetAccountAPIClient requires a full OAuth flow setup. This placeholder documents the need for integration tests while allowing the PR to proceed with unit-testable components.
oauth2/authorization_code/authorization_code.go (1)
57-58: LGTM - Interface extension aligns with PR objectives.Adding GetToken to the IAuthorizationCodeFlow interface is a logical extension that supports the new Account API integration. The implementation already exists and is correctly implemented.
oauth2/authorization_code/account_api.go (1)
10-38: LGTM - Well-structured Account API client integration.The implementation correctly:
- Retrieves and validates the token
- Extracts the issuer for the base URL
- Creates a token provider function for API authentication
- Handles all error paths appropriately
The getTokenFunc closure creates a fresh token on each API call, which ensures token validity but may re-validate unnecessarily. This is acceptable for the Chill review mode, as it prioritizes correctness over micro-optimizations.
oauth2/client_credentials/options_test.go (1)
1-213: LGTM - Comprehensive test coverage for option configurators.The test suite thoroughly covers all option functions with both happy paths and edge cases. The tests are well-organized, use parallel execution appropriately, and handle error scenarios gracefully.
oauth2/authorization_code/options_test.go (1)
1-254: LGTM - Comprehensive authorization code flow options testing.The test suite provides excellent coverage of all configuration options, including important security features like PKCE. The tests appropriately:
- Verify deduplication of auth parameters
- Test custom state generators
- Validate PKCE setup with both S256 and plain methods
- Ensure secure defaults (S256) for invalid challenge methods
kinde/account_api/client_test.go (1)
1-318: LGTM - Excellent test coverage for Account API client.The test suite comprehensively covers:
- Client initialization and configuration
- Successful API calls with proper headers and authentication
- Error handling for HTTP errors and missing tokens
- Complex pagination scenarios with cursor-based navigation
- Data merging utilities for both arrays and objects
The pagination tests are particularly thorough, validating multi-page flows and ensuring correct data aggregation.
jwt/account_api_helpers_test.go (1)
1-234: LGTM - Comprehensive JWT API helper tests.The test suite thoroughly validates all API helper methods with:
- Dual-path testing for token-based and API-based data retrieval
- Proper mock server setup for API responses
- Verification of complex data structures (permissions, roles, feature flags, entitlements)
- Clean test helper function for token creation
The consistent test patterns and comprehensive coverage ensure the API helpers work correctly in both fallback and forced API modes.
oauth2/client_credentials/token_source_test.go (1)
16-32: MockSessionHooksForTokenSource implementation looks solidThe mock correctly wires
SetRawTokenandGetRawToken, and the nil check before type assertion inGetRawTokenavoids panics. No changes needed here.oauth2/authorization_code/token_source_test.go (1)
14-60: MockSessionHooksForTokenSource covers the full session hooks surfaceThe mock cleanly implements all required session hook methods and uses testify/mock idiomatically, including safe return value handling for getters. This is a good reusable test helper.
jwt/jwt_test.go (3)
1130-1329: Role and HasRoles tests cover the key scenarios wellThe
GetRolesandHasRolestests exercise:
- Standard vs Hasura role claims.
- Object vs string role representations.
- Precedence of standard over Hasura roles.
- Presence/absence checks in
HasRoles, including the empty‑argument behavior.These align with the implementation in
jwt/token.goand provide good coverage of the new role API.
1393-1463:GetClaimtests are clear and accurateThe new
GetClaimtests correctly cover:
- The
parsed == nilcase.- Existing claims (including custom ones).
- Non‑existent claims.
They match the simple map‑lookup semantics and are sufficient for this helper.
1466-1593: Hasura claim fallback tests look goodThe
*_WithHasuratests for permissions, organization code, and feature flags correctly validate:
- Fallback to Hasura claims when standard claims are absent.
- Preference for standard claims when both formats are present.
- For feature flags, the correct extraction of type/value and exclusion of Hasura flags when standard flags exist.
These align with the updated implementations in
jwt/token.goand give good coverage of the precedence rules.jwt/account_api_helpers.go (2)
10-59: Permissions API helper is consistent with existing token behavior
GetPermissionsWithAPI:
- Sensibly returns token‑derived permissions/org_code when
ForceAPIis false.- For
ForceAPI == true, mapsaccount_api/v1/permissionsintoPermissionsWithOrg, returning just the permission keys, which is consistent withGetPermissions().This is a clean separation between token‑only and API‑backed behavior; no issues spotted here.
97-131: Feature flags API helper matches the in-token flag structure
GetFeatureFlagsWithAPI:
- Respects the
forceAPIflag similarly to roles/permissions.- Maps the API’s richer
feature_flagsentries down to your existingFeatureFlagtype using the sameType/Valuepattern.That keeps the external API format decoupled from consumers. No functional issues noted.
jwt/token.go (6)
159-192: Hasura-aware permissions logic is correct and backward compatible
GetPermissionsnow:
- Prefers the standard
"permissions"claim when present.- Falls back to
"x-hasura-permissions"only when the standard claim is absent or not of the expected type.- Continues to ignore non‑string entries.
This matches the tests and keeps existing behavior intact while adding Hasura support.
215-236: Organization code fallback to Hasura is implemented sensibly
GetOrganizationCode:
- First attempts
"org_code"and returns it if it’s a string.- Falls back to
"x-hasura-org-code"when the standard claim is missing or of a wrong type.This matches the new tests, maintains old behavior, and provides the Hasura compatibility you want.
352-418: Role type and extraction handle both object and string formats correctlyThe new
Rolestruct andGetRoles/extractRoles:
- Support both full role objects (
id,name,key) and simple string roles (key only).- Prefer
"roles"over"x-hasura-roles", as tested.- Guard against malformed entries by requiring at least a key or ID before appending.
This provides a robust abstraction over the raw claims and is well covered by tests.
420-448: HasRoles implementation is straightforward and efficient
HasRoles:
- Returns true when no role keys are passed (useful for guard logic).
- Builds a map of user role keys for O(1) lookup.
- Correctly returns false when there are no roles or no requested keys are present.
This is idiomatic and matches the test expectations.
508-519: GetClaim is a thin but useful wrapper around MapClaims
GetClaimcorrectly:
- Short‑circuits when
parsedorClaimsis nil.- Performs a direct lookup on
golangjwt.MapClaims.The tests in
jwt/jwt_test.gocover the main scenarios; no issues here.
556-576: extractStringArray is a safe, reusable helper
extractStringArray:
- Handles
[]stringand[]interface{}(the two main cases from JSON/MapClaims).- Returns
nilfor everything else, avoiding panics.- Is used in
GetUserOrganizations, which keeps that code clean.Looks good and idiomatic.
Fixes critical bugs and test coverage issues identified by CodeRabbit: 1. **Added ParseIDTokenUnverified() function** (jwt/jwt.go) - Parses ID tokens without signature validation - Safe for tokens already validated during OAuth flow - Uses jwt.ParseUnverified() to extract claims 2. **Fixed GetUserProfile()** (jwt/token.go) - Changed from ParseFromString() to ParseIDTokenUnverified() - Now correctly extracts user profile from ID tokens - Previously always returned nil due to missing keyfunc 3. **Fixed GetUserOrganizations()** (jwt/token.go) - Changed from ParseFromString() to ParseIDTokenUnverified() - Now correctly extracts org_codes from ID tokens - Previously always returned nil due to missing keyfunc 4. **Enhanced test coverage** (jwt/jwt_test.go) - Replaced incomplete test stubs with real assertions - Added comprehensive GetUserProfile tests with actual JWT tokens - Added comprehensive GetUserOrganizations tests - Tests both standard and Hasura claim formats - Tests fallback behavior and edge cases 5. **Enhanced GetEntitlements test** (jwt/account_api_helpers_test.go) - Added assertions for all 8 Entitlement fields - Added error case coverage for API failures - Validates Plans fields (Key and SubscribedOn) Testing: ✅ All JWT tests pass (36 test cases) ✅ Code compiles without errors ✅ GetUserProfile now works correctly ✅ GetUserOrganizations now works correctly ✅ GetEntitlements has comprehensive coverage Addresses CodeRabbit review comments: - jwt/token.go lines 450-507 (GetUserProfile) - jwt/token.go lines 521-554 (GetUserOrganizations) - jwt/jwt_test.go lines 1331-1391 (GetUserProfile tests) - jwt/jwt_test.go lines 1439-1497 (GetUserOrganizations tests) - jwt/account_api_helpers_test.go lines 133-210 (GetEntitlements)
- Removed unnecessary blank lines in `account_api_helpers_test.go`, `jwt_test.go`, and `jwt.go` to improve code readability. - Ensured consistent formatting across test cases and function implementations. This change does not affect functionality but enhances code maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
jwt/account_api_helpers_test.go (4)
17-64: Good coverage of token vs API permissions paths; consider asserting NewClient never failsThe two
GetPermissionsWithAPIsubtests clearly exercise the token‑only and API code paths and validate bothOrgCodeand permissions. Sinceaccount_api.NewClientcan, in principle, evolve to return an error, you may want torequire.NoError(t, err)instead of discarding it to keep tests robust against future changes.
66-114: Avoid relying on role slice ordering from the APIThe
forceAPI=trueroles test assertsresult[0].Key == "admin"andresult[1].Key == "user", which couples the test to the ordering behavior ofGetRolesWithAPI/the API response. To make the test resilient if roles are later collected or merged in a different order (e.g., via maps or pagination), consider asserting on the set of keys (e.g., using a map orassert.ElementsMatch) instead of fixed indices.
116-167: Feature flag tests effectively cover token and API formatsThe feature‑flag tests nicely validate both in‑token
feature_flagsand API‑provided flags, including type and value for a boolean flag. You might optionally add an assertion on the second flag’s value ("flag2" == "test") to fully exercise the string path, but current coverage is already solid.
169-245: Entitlements tests give strong coverage; pagination/metadata path could be added laterThe entitlements tests thoroughly verify org code, plan fields, and all entitlement properties on the happy path, plus error handling for a 500 response with an informative error message. If
Metadata.HasMore/ pagination behavior inGetEntitlementsis non‑trivial, a future test that exercises multiple pages (withHasMore=trueandnext_page_starting_after) would round out coverage, but it’s not a blocker.jwt/token.go (3)
159-236: Hasura fallbacks in GetPermissions and GetOrganizationCode are correct; consider reusing extractStringArrayThe fallbacks to
"x-hasura-permissions"and"x-hasura-org-code"are implemented safely and only used when the standard claims are absent, which is the right precedence. The per‑function loops that turn[]interface{}into[]stringmirror the newextractStringArrayhelper; if you ever want to support both[]interface{}and[]stringfor permissions as you already do for org codes, you could refactor to reuseextractStringArrayhere to reduce duplication and broaden type support.
352-418: Role parsing covers both string and object roles; align behavior with comment and consider empty‑arg HasRoles semantics
extractRolescorrectly supports both simple string roles and richer role objects withid/name/key, ignoring malformed entries. Two minor points:
- The
GetRolescomment says “Returns an empty slice if no roles are found,” but the function currently returnsnil; callers usinglen()are fine, but you may want either to return[]Role{}explicitly or adjust the comment.- Since
extractStringArrayalready handles[]stringand[]interface{}, you could reuse a similar pattern for roles in the future if you ever accept other slice types.
420-448: HasRoles logic is efficient; double‑check the behavior when called with no role keysBuilding a
map[string]boolof role keys and checking requested keys against it is a good O(n+m) approach. The special caselen(roleKeys) == 0returningtrueis a design choice that effectively treats “no roles requested” as “no restriction”; ensure callers never rely onHasRoles()with no args to mean “user has any roles,” or consider returningfalsein that case if you want a stricter interpretation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
jwt/account_api_helpers_test.go(1 hunks)jwt/jwt.go(1 hunks)jwt/jwt_test.go(1 hunks)jwt/token.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- jwt/jwt_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
jwt/account_api_helpers_test.go (2)
jwt/account_api_helpers.go (1)
GetPermissionsOptions(11-13)jwt/jwt.go (1)
Token(22-27)
jwt/token.go (1)
jwt/jwt.go (2)
Token(22-27)ParseIDTokenUnverified(50-63)
🔇 Additional comments (5)
jwt/jwt.go (1)
46-63: Non‑validating ID token parse helper looks correct and resolves prior limitationUsing
ParseUnverifiedwithgolangjwt.MapClaims{}and a defensiveMapClaimstype assertion cleanly supports extracting ID token claims without requiring a keyfunc, matching the documented “already validated in OAuth flow” assumption and unblocking callers likeGetUserProfile/GetUserOrganizations.jwt/account_api_helpers_test.go (1)
247-263: createTestToken helper is straightforward and appropriate for claim‑centric testsDirectly seeding
processing.parsedwith aMapClaims‑backedgolangjwt.Tokenis a clean way to focus these tests on claim extraction logic without involving signing/parsing, and the minimaloauth2.Tokenstub is sufficient for the helpers under test.jwt/token.go (3)
259-299: Feature flag extraction logic is robust and shared for standard/Hasura claimsUsing a shared
extractFeatureFlagshelper and trying"feature_flags"before"x-hasura-feature-flags"gives clear, predictable behavior, and the helper gracefully skips malformed entries by requiring bothtandv. ReturningTypeviatoStringensures only string types are treated as valid flag‑types, which keeps downstream getters (GetFeatureFlagBool/String/Int) well‑guarded.
450-504: GetUserProfile now correctly uses non‑validating ID token parsing and enforces requiredsubSwitching
GetUserProfileto useParseIDTokenUnverifiedfixes the earlier issue where parsing always failed without a keyfunc, and the method now gracefully handles missing/invalid tokens and requires a non‑empty"sub"before returning a profile. Optional fields (given_name,family_name,picture) are populated only when present, which is a sensible, backwards‑compatible shape forUserProfile.
506-573: Generic claim access and organization extraction from ID token are well-factored
GetClaimprovides a simple, nil‑safe way to fetch arbitrary access‑token claims, andGetUserOrganizationscorrectly mirrorsGetUserProfileby usingParseIDTokenUnverifiedand supporting both"org_codes"and"x-hasura-org-codes". The sharedextractStringArrayhelper cleanly handles both[]stringand[]interface{}inputs and ignores non‑string elements, which should be robust against minor claim shape variations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
jwt/account_api_helpers_test.go (1)
229-244: Consider additional error scenarios for better coverage.The error test currently only covers HTTP 500 responses. To improve test robustness, consider adding test cases for:
- Malformed JSON responses
- Missing required fields in the response
- Network timeout scenarios (if applicable)
This would help ensure the error handling is resilient across various failure modes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
jwt/account_api_helpers_test.go(1 hunks)jwt/jwt.go(1 hunks)jwt/jwt_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- jwt/jwt.go
🧰 Additional context used
🧬 Code graph analysis (2)
jwt/jwt_test.go (1)
jwt/jwt.go (1)
Token(22-27)
jwt/account_api_helpers_test.go (3)
kinde/account_api/client.go (3)
NewClient(33-48)BaseAccountResponse(99-102)Metadata(93-96)jwt/account_api_helpers.go (1)
GetPermissionsOptions(11-13)jwt/jwt.go (1)
Token(22-27)
🔇 Additional comments (13)
jwt/account_api_helpers_test.go (5)
17-64: LGTM! Well-structured API integration tests.The test coverage for both claim-based and API-based permission retrieval is solid. The mock server correctly simulates the API response structure.
66-114: LGTM! Consistent test pattern for roles.The test appropriately validates both token-based and API-based role retrieval with proper assertions on the role structure.
116-167: LGTM! Comprehensive feature flag testing.The test validates both boolean and string feature flag types, ensuring proper extraction of both Type and Value fields.
169-227: LGTM! Thorough validation of entitlement fields.The success case comprehensively validates all entitlement fields including org_code, plans, and all entitlement properties. The assertions provide good regression protection.
247-263: LGTM! Clean test helper implementation.The helper function properly uses
t.Helper()and provides a convenient way to create test tokens with custom claims. The direct struct construction is appropriate for unit testing.jwt/jwt_test.go (8)
1130-1251: LGTM! Comprehensive role extraction tests.The test suite thoroughly covers role retrieval scenarios including standard and Hasura claim formats, string vs. object representations, and proper precedence handling. The coverage ensures robust role extraction functionality.
1253-1329: LGTM! Well-designed role authorization tests.The tests cover all relevant scenarios for role checking, including the edge case where no roles are specified (returns true). This provides good validation of the authorization logic.
1331-1425: LGTM! Profile extraction tests now fully implemented.The tests properly call
GetUserProfile()and validate the returned profile data. The previous no-op issue has been successfully resolved. Edge cases for missing ID token, missing sub claim, and minimal profile data are well-covered.
1427-1471: LGTM! Clean generic claim accessor tests.The tests validate the claim retrieval pattern with proper existence checks. Coverage is appropriate for a generic accessor method.
1473-1559: LGTM! Organization extraction tests now fully functional.The tests properly call
GetUserOrganizations()and validate the returned organization codes. The previous no-op issue has been successfully addressed. Tests cover standard claims, Hasura fallback, and proper precedence handling.
1561-1599: LGTM! Hasura permissions fallback properly tested.The tests validate that Hasura-format permissions are used when standard claims are missing, and that standard claims take precedence when both are present. This ensures proper fallback behavior.
1601-1632: LGTM! Organization code Hasura fallback validated.The tests ensure proper fallback to Hasura-format organization codes and correct precedence handling. The implementation follows the same pattern as other Hasura claim tests.
1634-1688: LGTM! Feature flags Hasura support thoroughly tested.The tests validate Hasura-format feature flags as a fallback and ensure standard claims take precedence. The assertions correctly verify both the presence of expected flags and absence of Hasura flags when standard claims exist.
- Add comprehensive documentation to all exported types and functions - Document struct fields with detailed descriptions - Enhance function docs with parameters, return values, and examples - Add context and use case information for critical functions - Document internal helper functions for better code understanding This addresses CodeRabbit's docstring coverage warning, improving coverage from 25.40% towards the 80% threshold.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
kinde/account_api/client.go (1)
441-469: Consider JSON-based deduplication for better accuracy with complex objects.The current implementation uses
fmt.Sprintf("%v")for deduplication, which may produce identical strings for different complex objects. While the comment acknowledges this limitation and it works well for primitive types, consider using JSON marshaling for the deduplication key to handle nested structures more reliably.Apply this diff for more robust deduplication:
func mergeInterfaceArrays(arr1, arr2 []interface{}) []interface{} { seen := make(map[string]bool) result := []interface{}{} for _, item := range arr1 { - key := fmt.Sprintf("%v", item) + keyBytes, _ := json.Marshal(item) + key := string(keyBytes) if !seen[key] { seen[key] = true result = append(result, item) } } for _, item := range arr2 { - key := fmt.Sprintf("%v", item) + keyBytes, _ := json.Marshal(item) + key := string(keyBytes) if !seen[key] { seen[key] = true result = append(result, item) } } return result }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
jwt/account_api_helpers.go(1 hunks)jwt/jwt.go(1 hunks)jwt/token.go(5 hunks)kinde/account_api/client.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
jwt/token.go (1)
jwt/jwt.go (2)
Token(22-27)ParseIDTokenUnverified(67-80)
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(22-27)kinde/account_api/client.go (1)
Client(17-24)jwt/token.go (2)
Role(358-362)FeatureFlag(254-257)
🔇 Additional comments (6)
jwt/jwt.go (1)
46-80: Excellent implementation with clear security documentation.The function correctly uses
ParseUnverifiedfor ID tokens that have already been validated during the OAuth flow. The comprehensive documentation clearly warns against misuse, specifying appropriate use cases and explicitly listing scenarios where this function should NOT be used.jwt/account_api_helpers.go (1)
1-288: Clean API-backed helpers with consistent patterns.The implementations follow a consistent pattern:
- Check forceAPI flag to decide between token claims or API fetch
- Use
CallAccountAPIPaginatedfor automatic pagination- Define internal DTOs matching API response structure
- Map to exported SDK types
- Include contextual error messages
The separation between internal API DTOs and exported types is good practice, and error handling properly wraps errors with context.
jwt/token.go (4)
159-192: LGTM: Hasura fallback maintains backward compatibility.The enhancement properly tries the standard "permissions" claim first, then falls back to "x-hasura-permissions". This maintains backward compatibility while adding Hasura support.
357-460: Well-designed role handling with flexible format support.The implementation handles multiple role formats gracefully:
- String arrays (simplified format with keys only)
- Object arrays (detailed format with ID, name, and key)
HasRolesuses an efficient map-based lookup, and the Hasura fallback pattern is consistent with other methods. The validation that roles have at least a key or ID before adding them (line 424) is good defensive programming.
478-534: Correctly uses unverified parsing for already-validated ID tokens.The implementation now properly uses
ParseIDTokenUnverifiedto extract claims from the ID token without requiring a keyfunc. This is appropriate since the ID token was already validated during the OAuth flow. The required "sub" field validation and optional field handling are correct.
549-593: Correct implementation with consistent Hasura fallback pattern.Like
GetUserProfile, this method now correctly usesParseIDTokenUnverifiedto extract organization codes from the validated ID token. The fallback from standard "org_codes" to Hasura "x-hasura-org-codes" follows the same pattern used throughout the codebase.
- Read response body once before checking status code - Fixes critical issue where second read would fail since HTTP response bodies can only be read once - Addresses CodeRabbit review comment on PR kinde-oss#47
- Expanded docstrings for JWT options, including detailed descriptions, parameters, return values, and examples for functions like WillValidateWithPublicKey, WillValidateWithJWKSUrl, and others. - Improved documentation for parsing functions such as ParseFromAuthorizationHeader, ParseFromString, and ParseFromSessionStorage, providing clarity on usage and expected behavior. - Added comprehensive comments for internal helper functions to facilitate better understanding of the codebase. This update significantly improves the documentation coverage and usability of the JWT library, addressing previous coverage warnings.
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)
jwt/jwt_options.go (1)
320-338:newErrordoesn't handle nil primary error gracefully.When
newErroris called with a non-empty message and nilerr, the format string becomes"%w: %s"with args[nil, message]. Using%wwith a nil error can produce unexpected behavior in Go's error wrapping. This becomes problematic whenGetValidationErrorsin token.go callsnewError("token validation errors", nil, j.validationErrors...).If
j.validationErrorsis empty, this creates an error message even when there are no errors, contradicting the documented behavior that the method should return nil when validation succeeds.Consider this fix:
func newError(message string, err error, more ...error) error { + // If no primary error and no additional errors, return nil + if err == nil && len(more) == 0 { + return nil + } + + // If no primary error but we have additional errors, use the first as primary + if err == nil && len(more) > 0 { + err = more[0] + more = more[1:] + } + var format string var args []any if message != "" { format = "%w: %s" args = []any{err, message} } else { format = "%w" args = []any{err} } for _, e := range more { format += ": %w" args = append(args, e) } err = fmt.Errorf(format, args...) return err }
🧹 Nitpick comments (3)
jwt/token.go (3)
565-593: Document the vacuous truth behavior ofHasRoleswith empty input.Line 569 returns
truewhenlen(roleKeys) == 0. This vacuous truth behavior (checking if the user has "any of zero roles" returns true) may be counterintuitive. Some developers might expect this to return false or to check whether the user has any roles at all.Add a clear comment explaining this behavior:
// HasRoles checks if the token contains any of the specified roles. -// Returns true if the user has at least one of the provided role keys. +// Returns true if the user has at least one of the provided role keys. +// If no role keys are provided (empty input), returns true (vacuously true). func (j *Token) HasRoles(roleKeys ...string) bool { if len(roleKeys) == 0 { + // Vacuously true: user has "any of zero roles" return true }
738-766:extractStringArrayreturns inconsistent nil vs empty slice.The function returns
nilfor nil input (line 749) but an empty slice[]stringfor empty arrays (e.g.,[]interface{}{}). This inconsistency could cause issues for callers that uselen()checks vs nil checks.Consider always returning an empty slice for clarity and consistency:
func extractStringArray(value interface{}) []string { if value == nil { - return nil + return []string{} } switch arr := value.(type) { case []string: + if arr == nil { + return []string{} + } return arr case []interface{}: result := make([]string, 0, len(arr)) for _, item := range arr { if str, ok := item.(string); ok { result = append(result, str) } } return result } - return nil + return []string{} }
626-736: Consider caching parsed ID token claims to avoid duplicate parsing.Both
GetUserProfile(line 633) andGetUserOrganizations(line 716) independently parse the ID token usingParseIDTokenUnverified. If both methods are called on the same token, the ID token is parsed twice, which is inefficient.Consider caching the parsed ID token claims in the Token struct:
type Token struct { rawToken *oauth2.Token processing tokenProcessing isValid bool validationErrors []error idTokenClaims golangjwt.MapClaims // Cache parsed ID token claims } func (j *Token) getIDTokenClaims() (golangjwt.MapClaims, error) { if j.idTokenClaims != nil { return j.idTokenClaims, nil } idTokenStr, exists := j.GetIdToken() if !exists || idTokenStr == "" { return nil, fmt.Errorf("ID token not available") } claims, err := ParseIDTokenUnverified(idTokenStr) if err != nil { return nil, err } j.idTokenClaims = claims return claims, nil }Then update both methods to use
getIDTokenClaims()instead of parsing directly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
jwt/jwt.go(4 hunks)jwt/jwt_options.go(6 hunks)jwt/token.go(18 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
jwt/token.go (1)
jwt/jwt.go (2)
Token(29-34)ParseIDTokenUnverified(110-123)
🔇 Additional comments (7)
jwt/jwt_options.go (1)
15-310: LGTM! Excellent documentation and validator additions.The enhanced documentation for all options is comprehensive and provides clear examples. The three new validator options (WillValidateIssuer, WillValidateAudience, WillValidateClaims) follow consistent patterns and integrate well with the existing validation pipeline. The implementations correctly append validators to the token's processing configuration.
jwt/jwt.go (1)
14-239: LGTM! Comprehensive documentation and correct ParseIDTokenUnverified implementation.The enhanced documentation across all parse functions is excellent, providing clear guidance on when and how to use each function. The new
ParseIDTokenUnverifiedfunction correctly addresses the previous review comments by enabling ID token parsing without signature verification, which is appropriate since ID tokens are already validated during the OAuth2 flow. The implementation properly usesParseUnverifiedand includes appropriate safety warnings in the documentation.jwt/token.go (5)
238-271: Good Hasura fallback support for permissions.The implementation correctly tries the standard "permissions" claim first, then falls back to "x-hasura-permissions". This pattern provides good compatibility with both standard and Hasura token formats.
301-322: Consistent Hasura fallback pattern across claims.Good consistency in implementing Hasura fallback support for
GetOrganizationCodeandGetFeatureFlags. TheextractFeatureFlagshelper correctly handles the feature flag structure with type and value fields.Also applies to: 352-397
626-667: Good fix for ID token parsing.The implementation correctly uses
ParseIDTokenUnverifiedto parse ID tokens without requiring validation, which addresses the previous review comments. The required subject check and optional field extraction logic is sound.
692-736:GetUserOrganizationscorrectly addresses previous review concerns.Like
GetUserProfile, this method now correctly usesParseIDTokenUnverifiedto parse ID tokens without validation. The Hasura fallback support for org codes is properly implemented.
669-690: Useful generic claim accessors.
GetClaimandGetClaimsprovide helpful generic access to token claims, complementing the type-specific accessor methods. Good additions to the API surface.Also applies to: 785-802
…dling - Updated role extraction logic to only include roles with a Key, simplifying the condition for appending roles. - Enhanced the GetValidationErrors method to return nil when there are no validation errors, improving clarity in error handling. These changes improve code readability and maintainability while ensuring correct functionality in role management and validation error reporting.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
jwt/token.go (1)
568-570: Document the empty-input behavior ofHasRoles.When
HasRoles()is called with no role keys, it returnstrue(lines 568-570). While this could be a deliberate design choice (vacuous truth), it may be surprising to callers. The documentation states "Returns true if the user has at least one of the provided role keys," which doesn't explicitly cover the zero-input case.Consider either:
- Returning
falsefor empty input (no roles to match = no match)- Explicitly documenting this behavior in the function comment
The current behavior might hide bugs where
HasRoles()is accidentally called with no arguments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
jwt/token.go(18 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
jwt/token.go (1)
jwt/jwt.go (2)
Token(29-34)ParseIDTokenUnverified(110-123)
🔇 Additional comments (9)
jwt/token.go (9)
10-79: Token accessor methods look good.The methods for accessing the underlying OAuth2 token (GetRawToken, GetIdToken, GetAccessToken, GetRefreshToken, AsString) are well-documented and correctly implemented. Proper nil checks and consistent return patterns enhance safety.
81-236: Standard JWT claim accessors are well-implemented.The validation and standard claim methods (IsValid, GetSubject, GetIssuer, GetAudience, GetExpiration, GetIssuedAt, GetJWTID) correctly implement RFC 7519 specifications. Handling both single-string and array formats for audience, and multiple numeric types for timestamps, demonstrates good flexibility for JSON unmarshaling variations.
238-344: Permissions and organization accessors with Hasura fallback work correctly.The methods for retrieving permissions, scopes, organization code, and authorized party are well-structured. The fallback pattern (standard claim first, then Hasura format) is consistently applied and clearly documented.
346-488: Feature flag handling is comprehensive and type-safe.The feature flag methods provide both generic access (GetFeatureFlags, GetFeatureFlag) and type-specific accessors (GetFeatureFlagBool, GetFeatureFlagString, GetFeatureFlagInt). The fallback to Hasura format and handling of multiple numeric types demonstrate good design. The extractFeatureFlags helper correctly parses the Kinde feature flag structure.
595-667: User profile extraction is correctly implemented.The
GetUserProfilemethod now properly usesParseIDTokenUnverifiedto extract profile information without requiring validation (since the token was already validated in the OAuth flow). The method correctly requires the "sub" claim and treats other profile fields as optional, following OpenID Connect specifications.The past review concern about this method has been successfully addressed.
669-690: Generic claim accessor is well-designed.The
GetClaimmethod provides flexible access to any claim in the token, including both standard and custom claims. The implementation is straightforward with appropriate nil checks.
692-736: Organization extraction is correctly implemented.The
GetUserOrganizationsmethod now properly usesParseIDTokenUnverifiedand supports both standard ("org_codes") and Hasura ("x-hasura-org-codes") formats. This follows the same pattern asGetUserProfileand addresses the past review concern.
738-783: Helper functions are robust and well-documented.The internal helpers
extractStringArrayandtoStringhandle type conversions safely with proper nil checks. The behavior of silently skipping non-string elements and returning empty strings for invalid types is documented and appropriate for claim parsing.
785-827: Claims and validation error accessors are correctly implemented.The
GetClaimsmethod provides comprehensive access to all token claims. TheGetValidationErrorsmethod now correctly returnsnilwhen there are no validation errors (lines 823-825), which addresses the past review concern and matches the documented behavior.
1 participant
GetRolesandHasRolesmethods to retrieve user roles from both standard and Hasura claims.GetPermissions,GetOrganizationCode, andGetFeatureFlagsmethods to support fallback between standard and Hasura claim formats.This update improves the flexibility and robustness of token handling in the application.
Explain your changes
Suppose there is a related issue with enough detail for a reviewer to understand your changes fully. In that case, you can omit an explanation and instead include either “Fixes #XX” or “Updates #XX” where “XX” is the issue number.
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.