Add role and permission handling in JWT token - #48
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.
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.
- 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.
- 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.
…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.
WalkthroughThis PR introduces JWT token parsing helpers with pluggable validation options, a new Account API client infrastructure for authenticated paginated API requests, token claim extraction methods supporting both standard and Hasura formats, OAuth2 Account API integration, and comprehensive test coverage across JWT, Account API, OAuth2 flows, and a new Gin framework integration. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Token as JWT Token
participant ACF as AuthorizationCodeFlow
participant Client as Account API Client
participant API as Account API
App->>Token: GetPermissionsWithAPI(ctx, apiClient, forceAPI=true)
Token->>ACF: Extract issuer & access token
alt Token has data
ACF-->>Token: Return from token claims
else Force API
Token->>Client: CallAccountAPIPaginated("/permissions")
Client->>Client: Prepare authenticated request
Client->>API: GET /permissions + Bearer token
API-->>Client: Page 1 (BaseAccountResponse with permissions)
Client->>Client: Check if more pages (cursor)
Client->>API: GET /permissions?starting_after=cursor
API-->>Client: Page 2 (additional permissions)
Client->>Client: Merge & deduplicate pages
Client-->>Token: PermissionsWithOrg (merged results)
end
Token-->>App: PermissionsWithOrg
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
jwt/jwt.go (1)
148-157: Silent error handling inParseFromSessionStoragemay hide issues.Both
json.Unmarshalcalls ignore errors. If the JSON is malformed, the function proceeds with partially initialized data, which could lead to confusing downstream errors.func ParseFromSessionStorage(rawToken string, options ...func(*Token)) (*Token, error) { token := oauth2.Token{} - json.Unmarshal([]byte(rawToken), &token) + if err := json.Unmarshal([]byte(rawToken), &token); err != nil { + return nil, fmt.Errorf("failed to unmarshal token from session storage: %w", err) + } var extra map[string]interface{} - json.Unmarshal([]byte(rawToken), &extra) + if err := json.Unmarshal([]byte(rawToken), &extra); err != nil { + return nil, fmt.Errorf("failed to unmarshal extra fields from session storage: %w", err) + } tokenExtra := token.WithExtra(extra) return ParseOAuth2Token(tokenExtra, options...) }jwt/jwt_options.go (1)
73-98: Returningnilonkeyfunc.Newerror can cause nil pointer dereference.If
keyfunc.Newfails (line 90-93), the function returnsnil. When this nil option is applied inParseOAuth2Token, it will cause a panic. The first error case (lines 80-84) correctly returns an option that adds a validation error, but the second case does not.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)) + } }
🧹 Nitpick comments (6)
frameworks/gin_kinde/gin_kinde_test.go (1)
15-214: Consider extracting test setup into a helper function.The test subtests have repetitive setup code (creating store, router, session). Extracting this into a helper function would reduce duplication and improve maintainability.
Example helper:
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 }jwt/jwt.go (1)
58-66: Consider more specific error message for malformed Authorization header.The error "invalid token" doesn't distinguish between missing header and malformed format. Consider a more specific message.
func ParseFromAuthorizationHeader(r *http.Request, options ...func(*Token)) (*Token, error) { requestedToken := r.Header.Get("Authorization") + if requestedToken == "" { + return nil, fmt.Errorf("missing Authorization header") + } splitToken := strings.Split(requestedToken, "Bearer") if len(splitToken) != 2 { - return nil, fmt.Errorf("invalid token") + return nil, fmt.Errorf("invalid Authorization header format: expected 'Bearer <token>'") } requestedToken = strings.TrimSpace(splitToken[1]) return ParseOAuth2Token(&oauth2.Token{AccessToken: requestedToken}, options...) }oauth2/authorization_code/token_source_test.go (1)
62-88: Consider adding a comment explaining why the test expects failure.The test name says "successfully validates token" but expects an error. While the comment explains this, it could be confusing. Consider renaming or restructuring.
- t.Run("successfully validates token", func(t *testing.T) { + t.Run("validates token structure but fails without real JWKS", func(t *testing.T) {jwt/jwt_options.go (1)
320-338: Error formatting innewErrorproduces unusual error chain.The format string
"%w: %s"puts the wrapped error before the message, which is unconventional. Additionally, appending": %w"for each additional error creates a long chain that may be hard to read.Consider a more conventional error message format:
func newError(message string, err error, more ...error) error { - 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 + if message == "" && err == nil && len(more) == 0 { + return nil + } + + allErrors := make([]error, 0, 1+len(more)) + if err != nil { + allErrors = append(allErrors, err) + } + allErrors = append(allErrors, more...) + + if message != "" { + return fmt.Errorf("%s: %w", message, errors.Join(allErrors...)) + } + return errors.Join(allErrors...) }This uses
errors.Joinfor cleaner multi-error aggregation (requires importingerrorspackage).kinde/account_api/client.go (1)
347-370: Consider JSON-key-based deduplication for robustness.Using raw JSON bytes for deduplication may fail if the API returns objects with different key ordering across pages. If objects have a unique identifier (like
id), consider extracting that for deduplication instead.This is acceptable if the API guarantees consistent JSON serialization order.
jwt/token.go (1)
794-802: Consider returning a copy to prevent mutation.The method returns the internal claims map directly. If callers modify this map, it could affect subsequent calls to other Token methods. Consider returning a shallow copy for defensive programming.
func (j *Token) GetClaims() map[string]any { if j.processing.parsed == nil { return make(map[string]any) } if claims, ok := j.processing.parsed.Claims.(golangjwt.MapClaims); ok { - return claims + result := make(map[string]any, len(claims)) + for k, v := range claims { + result[k] = v + } + return result } return make(map[string]any) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
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(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 (9)
oauth2/authorization_code/authorization_code.go (1)
jwt/jwt.go (1)
Token(29-34)
oauth2/client_credentials/options_test.go (4)
oauth2/client_credentials/client_credentials.go (1)
ClientCredentialsFlow(30-35)oauth2/client_credentials/options.go (1)
WithKindeManagementAPI(41-60)jwt/jwt.go (1)
Token(29-34)jwt/jwt_options.go (1)
WillValidateAlgorithm(201-209)
oauth2/authorization_code/options_test.go (2)
oauth2/authorization_code/authorization_code.go (1)
AuthorizationCodeFlow(78-90)oauth2/authorization_code/options.go (9)
WithPrompt(35-39)WithOffline(42-46)WithCustomStateGenerator(49-53)WithClientID(63-67)WithClientSecret(70-74)WithScopes(77-81)WithAdditionalScope(84-88)WithPKCE(104-117)WithPKCEChallengeMethod(119-144)
oauth2/client_credentials/token_source_test.go (1)
oauth2/authorization_code/token_source_test.go (4)
MockSessionHooksForTokenSource(15-17)TestSessionTokenSource_validateToken(62-88)TestSessionTokenSource_Token(90-118)TestSessionTokenSource_getValidatedToken(120-140)
oauth2/authorization_code/token_source_test.go (3)
oauth2/client_credentials/token_source_test.go (2)
MockSessionHooksForTokenSource(17-19)TestSessionTokenSource_validateToken(34-58)oauth2/authorization_code/authorization_code.go (1)
AuthorizationCodeFlow(78-90)jwt/jwt_options.go (1)
WillValidateWithJWKSUrl(73-98)
frameworks/gin_kinde/gin_kinde_test.go (1)
frameworks/gin_kinde/gin_kinde.go (1)
UseKindeAuth(99-167)
jwt/jwt_test.go (1)
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)
jwt/token.go (1)
jwt/jwt.go (2)
Token(29-34)ParseIDTokenUnverified(110-123)
🔇 Additional comments (56)
frameworks/gin_kinde/gin_kinde_test.go (2)
310-336: LGTM!Good edge case coverage for type validation. The test properly verifies that invalid types stored in the session are caught and reported with a clear error message.
338-364: LGTM!Consistent edge case coverage for token type validation, mirroring the code_verifier test pattern.
oauth2/authorization_code/account_api_test.go (1)
7-14: LGTM!The skipped test serves as documentation for integration testing requirements. This is a reasonable approach for tests that require full OAuth flow setup.
oauth2/authorization_code/account_api.go (1)
12-38: LGTM!The implementation correctly:
- Extracts the issuer from the token to use as the base URL
- Validates that both the issuer and access token exist
- Creates a closure that provides fresh tokens on each call
- Returns a properly configured Account API client
oauth2/client_credentials/options_test.go (2)
12-145: LGTM!Comprehensive test coverage for option helpers, including edge cases like invalid URLs, empty inputs, and URL parsing variations. Tests properly validate both happy and error paths.
148-212: LGTM!Good edge case coverage for URL parsing, including empty strings and URLs with ports. The tests appropriately verify graceful handling of edge cases.
oauth2/client_credentials/token_source_test.go (2)
60-109: LGTM!Tests appropriately cover both cached token usage and error handling scenarios. The comments acknowledge the limitations of unit testing without full OAuth2 setup, which is reasonable.
34-58: This test helper function exists in the test package.The
testclientCredentialsToken()helper function is defined at line 83 inoauth2/client_credentials/client_credentials_test.go. Since both test files are in the same package, the function is accessible totoken_source_test.go.Likely an incorrect or invalid review comment.
kinde/account_api/client_test.go (4)
14-46: LGTM!Good test coverage for client construction, including trailing slash handling and option application.
48-111: LGTM!Comprehensive tests for API calls, covering successful requests, error responses, and empty token validation. The tests properly validate headers and response parsing.
113-239: LGTM!Excellent pagination test coverage, including:
- Single page responses
- Multi-page array responses with cursor advancement
- Multi-page object responses with merging
- Proper assertion of call counts and cursor parameters
241-317: LGTM!Thorough tests for merge utilities, covering array deduplication, object merging, nested objects, and arrays within objects.
jwt/account_api_helpers_test.go (5)
17-64: LGTM!Good test coverage for permissions fetching, validating both token-based and API-based retrieval paths with proper ForceAPI flag handling.
66-114: LGTM!Consistent test pattern for roles, covering both token and API fetch paths with appropriate assertions.
116-167: LGTM!Well-structured tests for feature flags, validating both data sources and type/value handling.
169-245: LGTM!Excellent comprehensive testing of entitlements API with detailed field validation. The test covers all fields including ID, FixedCharge, PriceName, UnitAmount, FeatureKey, FeatureName, and limit values. Error path is also properly tested.
247-263: LGTM!Clean helper function for creating test tokens with custom claims. The use of
t.Helper()is appropriate for test utilities.oauth2/authorization_code/authorization_code.go (1)
57-58: TheGetTokenmethod is part of theIAuthorizationCodeFlowinterface's stable API. Since this interface is used as a return type from the factory functionNewAuthorizationCodeFlow()and not intended for external implementation, adding this method is not a breaking change. Consider removing or updating this comment if external implementations are not a supported use case.Likely an incorrect or invalid review comment.
oauth2/authorization_code/options_test.go (5)
1-10: LGTM!Imports are appropriate for the test file. The use of
testify/assertfor assertions and theoauth2package for test fixtures is correct.
11-44: LGTM!Test cases for
WithAuthParameterare thorough, covering:
- Adding new parameters
- Appending to existing parameters
- Deduplication of values
The test logic correctly validates the expected behavior.
197-219: LGTM!PKCE tests properly validate:
usePKCEis set totrue- Challenge method defaults to
S256- Code challenge is generated
Good coverage of the PKCE initialization logic.
221-253: LGTM!Tests for
WithPKCEChallengeMethodcorrectly verify:
- S256 method setting
- Plain method setting
- Default fallback to S256 for invalid methods
This aligns with the implementation in
options.gothat accepts only "S256" or "plain".
102-112: No action required. ThenewTestSessionHooks()helper function is defined in the same test package atoauth2/authorization_code/authorization_code_test.go:228and is accessible to all test files within that package.Likely an incorrect or invalid review comment.
jwt/jwt.go (2)
14-26: LGTM!The enhanced documentation for
tokenProcessingstruct fields clearly explains the purpose of each field. This improves code maintainability.
195-239: LGTM!The
ParseOAuth2Tokenfunction correctly:
- Initializes token with empty slices to avoid nil pointer issues
- Applies all options before parsing
- Collects validation errors while continuing to process
- Returns both token and aggregated errors, allowing callers to inspect partial results
The design choice to return the token even on validation failure is well-documented and enables useful inspection of invalid tokens.
oauth2/authorization_code/token_source_test.go (4)
14-60: LGTM!The mock implementation is comprehensive and follows the testify/mock pattern correctly. All
ISessionHooksinterface methods are properly implemented with mock expectations.
90-118: LGTM!The test correctly validates error propagation when session hooks fail. The skipped test is appropriately documented as requiring full OAuth2 setup.
120-140: LGTM!Good test for
getValidatedTokenerror path. The assertions verify both that an error is returned and that it contains the expected message.
75-78: No action required.testJwtToken()is defined locally within theoauth2/authorization_codepackage test files and is properly accessible by other test files in the same package following Go's standard testing conventions.jwt/jwt_test.go (6)
1130-1251: LGTM!Comprehensive test coverage for
GetRolesincluding:
- Nil parsed token handling
- Standard roles claim with map structure
- Hasura x-hasura-roles claim fallback
- String-based roles handling
- Missing claims handling
- Precedence of standard roles over Hasura roles
Tests correctly validate the expected behavior for role extraction.
1253-1329: LGTM!
HasRolestests properly cover:
- Empty role check returns true
- Missing roles in token
- Partial role matching (has one of requested)
- No matching roles
- String-based roles compatibility
Good edge case coverage.
1331-1425: LGTM!
GetUserProfiletests are thorough:
- Missing ID token handling
- Full profile extraction from ID token
- Missing required
subclaim handling- Minimal profile with only
subclaimUsing unsigned JWTs with
golangjwt.UnsafeAllowNoneSignatureTypeis appropriate for unit testing profile extraction logic.
1427-1471: LGTM!
GetClaimtests correctly validate:
- Nil parsed token handling
- Existing claim retrieval
- Missing claim handling
Simple and effective test coverage.
1473-1559: LGTM!
GetUserOrganizationstests properly cover:
- Missing ID token
- Standard
org_codesclaim- Hasura
x-hasura-org-codesfallback- Precedence of standard over Hasura format
Good coverage of the claim precedence logic.
1561-1688: LGTM!Tests for Hasura claim handling across permissions, organization code, and feature flags correctly validate:
- Fallback to Hasura-prefixed claims when standard claims are missing
- Precedence of standard claims over Hasura claims when both exist
This ensures proper support for both standard and Hasura JWT formats.
jwt/jwt_options.go (3)
15-19: LGTM!Good documentation for the
Optiontype explaining its purpose in the parsing and validation workflow.
201-209: LGTM!
WillValidateAlgorithmcorrectly defaults to RS256 when no algorithms are specified, which aligns with Kinde's standard algorithm.
254-268: LGTM!
WillValidateAudiencecorrectly validates that the expected audience is present in the token's audience claim, supporting both single string and array formats.kinde/account_api/client.go (5)
86-125: LGTM!The
callAPImethod correctly handles token retrieval, request construction, and error scenarios. The use ofdefer resp.Body.Close()ensures proper resource cleanup.
197-228: LGTM!The pagination detection logic correctly distinguishes between array and object responses, delegating to the appropriate handler. The early return optimization for single-page responses is efficient.
237-286: LGTM!The array pagination correctly accumulates items across pages and handles deduplication via
mergeArrays. The cursor-based pagination usingstarting_afterfollows standard patterns.
448-469: Acknowledged limitation with%vdeduplication.As noted in the doc comment, using
fmt.Sprintf("%v", item)for deduplication may not work reliably for complex nested objects due to non-deterministic map iteration order. This is acceptable for the current use case with simple structures.
404-439: LGTM!The deep merge implementation correctly handles recursive map merging, array merging, and type mismatches. The fallback to
obj2when types don't match is a reasonable default.jwt/token.go (8)
29-65: LGTM!Token accessor methods correctly handle nil checks and follow the
(value, exists)return pattern consistently. The type assertion forid_tokenfrom extras is appropriate.
171-215: LGTM!The timestamp extraction correctly handles the common JSON number types (
float64,int64,int). The conversion fromfloat64toint64is safe for Unix timestamps.
238-271: LGTM!The dual-format support (standard and Hasura) is correctly implemented with appropriate fallback logic.
526-563: LGTM!The
extractRolesfunction correctly handles both string-based roles and structured role objects. The decision to filter out roles without aKeyis well-documented and aligns withHasRolesrequirements.
565-593: LGTM!The
HasRolesimplementation efficiently uses a map for O(1) role lookup. The "any" (OR) semantics is clearly documented. Returningtruefor emptyroleKeysfollows vacuous truth convention.
626-667: LGTM!The
GetUserProfileimplementation correctly extracts OpenID Connect claims from the ID token. The defensive nil checks and required/optional field handling are appropriate.
681-690: LGTM!The generic
GetClaimmethod provides flexible access to any claim in the token, supporting custom claims beyond the standard accessors.
822-827: LGTM!The validation error aggregation provides useful debugging information when token validation fails.
jwt/account_api_helpers.go (5)
46-81: LGTM!The dual-path approach (token vs API) is cleanly implemented. The API response is correctly mapped to permission key strings.
99-130: LGTM!The roles API integration follows the same consistent pattern as permissions. The mapping to the
Rolestruct preserves all relevant fields (ID, Name, Key).
182-220: LGTM!The entitlement and plan types are well-documented with clear field descriptions. The structure appropriately models billing-related data.
236-287: LGTM!The implementation correctly fetches entitlements exclusively from the API, as noted in the documentation. The field mapping is comprehensive and the response structure is well-defined.
149-180: The feature flag type formats are already consistent between token and API sources. Both use short type codes ("b", "i", "s") as confirmed by the test cases inaccount_api_helpers_test.go. The API response returns"type": "b"and"type": "s"(lines 147-148 of the test), matching the token format. The proposed type normalization is unnecessary sinceGetFeatureFlagsWithAPIcorrectly passes through the API response types that already align with the token format used byGetFeatureFlagBool,GetFeatureFlagString, andGetFeatureFlagInttype-checking methods.Likely an incorrect or invalid review comment.
| 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) | ||
| }) |
There was a problem hiding this comment.
Weak assertion doesn't validate middleware behavior.
The assertion assert.NotNil(t, w) only checks that the ResponseRecorder exists, but doesn't validate that the kinde_client was actually set in the context or that the middleware executed correctly. Consider checking the response code or using a test route that accesses the context value.
Example improvement:
privateGroup.GET("/test", func(c *gin.Context) {
client, exists := c.Get("kinde_client")
assert.True(t, exists)
assert.NotNil(t, client)
c.String(200, "ok")
})
w := httptest.NewRecorder()
req := httptest.NewRequest("GET", "/private/test", nil)
router.ServeHTTP(w, req)🤖 Prompt for AI Agents
In frameworks/gin_kinde/gin_kinde_test.go around lines 236 to 243, the test only
asserts that the ResponseRecorder exists which doesn't verify the middleware set
"kinde_client" in the Gin context; modify the test to register a handler on the
private route that reads c.Get("kinde_client") and asserts the value exists and
is non-nil, return a 200 response from that handler, then perform the same
httptest request and assert the response code is 200 and the assertions inside
the handler passed.
| func NewClient(baseURL string, getToken func(ctx context.Context) (string, error), opts ...ClientOption) (*Client, error) { | ||
| // Remove trailing slash | ||
| baseURL = strings.TrimSuffix(baseURL, "/") | ||
|
|
||
| client := &Client{ | ||
| httpClient: http.DefaultClient, | ||
| baseURL: baseURL, | ||
| getToken: getToken, | ||
| } | ||
|
|
||
| for _, opt := range opts { | ||
| opt(client) | ||
| } | ||
|
|
||
| return client, nil | ||
| } |
There was a problem hiding this comment.
Missing validation contradicts documentation.
The doc comment states "Returns an error if the baseURL is empty" but the function never validates baseURL or getToken. An empty baseURL will cause malformed API URLs, and a nil getToken will panic when callAPI is invoked.
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, "/")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func NewClient(baseURL string, getToken func(ctx context.Context) (string, error), opts ...ClientOption) (*Client, error) { | |
| // Remove trailing slash | |
| baseURL = strings.TrimSuffix(baseURL, "/") | |
| client := &Client{ | |
| httpClient: http.DefaultClient, | |
| baseURL: baseURL, | |
| getToken: getToken, | |
| } | |
| for _, opt := range opts { | |
| opt(client) | |
| } | |
| return client, nil | |
| } | |
| 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, "/") | |
| client := &Client{ | |
| httpClient: http.DefaultClient, | |
| baseURL: baseURL, | |
| getToken: getToken, | |
| } | |
| for _, opt := range opts { | |
| opt(client) | |
| } | |
| return client, nil | |
| } |
🤖 Prompt for AI Agents
kinde/account_api/client.go lines 62-77: The constructor claims to return an
error when baseURL is empty but currently does no validation and also does not
guard against a nil getToken (which will panic later). Add input validation at
the start of NewClient: if baseURL == "" return a descriptive error; if getToken
== nil return a descriptive error; perform strings.TrimSuffix after validating
non-empty baseURL; keep existing option application and return the constructed
client on success.
1 participant
Explain your changes
Add role and permission handling in JWT token
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.