Refactor JWT token handling and enhance tests - #29
Conversation
- Introduce new methods to retrieve claims such as issuer, audience, expiration, issued at, JWT ID, permissions, scopes, organization code, authorized party, and feature flags. - Update existing methods for better clarity and functionality. - Add comprehensive tests for new and existing methods to ensure correct behavior and handling of edge cases. - Improve documentation for methods related to token claims.
WalkthroughAdds numerous JWT Token accessor methods, a FeatureFlag model, AsString and validation-error accessors in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant T as Token
participant P as JWT Parser
participant CMs as Claims (Map)
participant FF as Feature Flags
C->>T: New Token(raw)
T->>P: Parse raw JWT
P-->>T: Parsed token or error
T-->>C: Token (with validation errors if any)
rect rgba(230,245,255,0.5)
note over C,T: Access standard claims
C->>T: GetIssuer / GetSubject / GetAudience / GetExpiration / GetIssuedAt / GetJWTID
T->>CMs: Read claim(s)
CMs-->>T: Values (string/number/array)
T-->>C: Normalized values
end
rect rgba(240,255,230,0.5)
note over C,T: Feature flags access
C->>T: GetFeatureFlags / GetFeatureFlagXxx(name)
T->>CMs: Read feature_flags
CMs-->>T: map[string]{t,v}
T->>FF: Normalize FeatureFlag
FF-->>T: Typed value (bool/string/int)
T-->>C: Result (+ok)
end
rect rgba(255,245,230,0.5)
note over C,T: Serialization and errors
C->>T: AsString()
T-->>C: JSON string or error
C->>T: GetValidationErrors()
T-->>C: error (if any)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
jwt/token.go (1)
339-341: Ensure GetValidationErrors returns nil when there are no errorsThe current implementation of GetValidationErrors always calls newError, which in turn invokes fmt.Errorf with a non-empty format string and the nil error (and no additional errors). Because Go’s fmt.Errorf treats
%wexactly like%v—and%vprints<nil>rather than returning a nil error—newError will always produce a non-nil error value even when j.validationErrors is empty (go.dev, exchangetuts.com).To avoid “manufacturing” an error when there truly are no validation errors, add an explicit nil-check:
• File jwt/token.go, lines 339–341:
func (j *Token) GetValidationErrors() error { - return newError("token validation errors", nil, j.validationErrors...) + if len(j.validationErrors) == 0 { + return nil + } + return newError("token validation errors", nil, j.validationErrors...) }This change ensures that consumers of GetValidationErrors can simply write:
if err := token.GetValidationErrors(); err != nil { // handle real validation error(s) }without being misled by a spurious, empty-error wrapper.
🧹 Nitpick comments (15)
jwt/token.go (10)
42-49: Document and safeguard JSON serialization of oauth2.Token (PII exposure and missing extras).AsString marshals the oauth2.Token directly. Two caveats:
- Sensitive fields (access/refresh tokens) will be serialized as-is; easy to leak in logs.
- oauth2.Token.WithExtra data (e.g., id_token) lives in unexported fields and won’t appear in the JSON, which may surprise callers.
Recommend clarifying the docstring and optionally providing a redacted variant (e.g., AsRedactedString) to avoid accidental leakage.
Apply this doc tweak:
-// AsString returns the token as a JSON string. +// AsString returns the oauth2.Token as a JSON string. +// Note: +// - Sensitive fields like AccessToken and RefreshToken are included verbatim. +// - Values added via oauth2.Token.WithExtra (e.g., "id_token") are stored in unexported fields +// and will not be present in the JSON output. func (j *Token) AsString() (string, error) { marshalledToken, err := json.Marshal(j.rawToken) if err != nil { return "", err } return string(marshalledToken), nil }Optionally add a safe alternative (outside this hunk):
// AsRedactedString returns a JSON string with sensitive fields masked. func (j *Token) AsRedactedString() (string, error) { if j.rawToken == nil { return "null", nil } redacted := *j.rawToken if redacted.AccessToken != "" { redacted.AccessToken = "****" } if redacted.RefreshToken != "" { redacted.RefreshToken = "****" } b, err := json.Marshal(redacted) if err != nil { return "", err } return string(b), nil }
65-79: Prefer jwt/v5 helper over manual MapClaims cast for iss.Use Claims.GetIssuer() to support both MapClaims and RegisteredClaims and avoid map-typing boilerplate.
-// GetIssuer returns the iss claim of the token. -func (j *Token) GetIssuer() string { - if j.processing.parsed == nil || j.processing.parsed.Claims == nil { - return "" - } - if claims, ok := j.processing.parsed.Claims.(golangjwt.MapClaims); ok { - if issuer, exists := claims["iss"]; exists { - if issuerStr, ok := issuer.(string); ok { - return issuerStr - } - } - } - return "" -} +// GetIssuer returns the iss claim of the token. +func (j *Token) GetIssuer() string { + if j.processing.parsed == nil || j.processing.parsed.Claims == nil { + return "" + } + iss, _ := j.processing.parsed.Claims.GetIssuer() + return iss +}
80-103: Simplify aud parsing with jwt/v5 API and cover more shapes.Use Claims.GetAudience() which normalizes string/array forms into ClaimStrings.
-// GetAudience returns the aud claim of the token. -func (j *Token) GetAudience() []string { - if j.processing.parsed == nil || j.processing.parsed.Claims == nil { - return nil - } - if claims, ok := j.processing.parsed.Claims.(golangjwt.MapClaims); ok { - if audience, exists := claims["aud"]; exists { - switch aud := audience.(type) { - case string: - return []string{aud} - case []interface{}: - result := make([]string, 0, len(aud)) - for _, a := range aud { - if aStr, ok := a.(string); ok { - result = append(result, aStr) - } - } - return result - } - } - } - return nil -} +// GetAudience returns the aud claim as a slice of strings (nil if absent). +func (j *Token) GetAudience() []string { + if j.processing.parsed == nil || j.processing.parsed.Claims == nil { + return nil + } + aud, _ := j.processing.parsed.Claims.GetAudience() + if len(aud) == 0 { + return nil + } + return []string(aud) +}
104-123: Normalize exp using jwt/v5 NumericDate (ms vs s tolerance stays intact).Leverage Claims.GetExpirationTime() for correctness across claim encodings and avoid manual type switches. This also keeps your current behavior (returns whatever numeric units the token uses) but via the official helper.
-// GetExpiration returns the exp claim of the token. -func (j *Token) GetExpiration() (int64, bool) { - if j.processing.parsed == nil || j.processing.parsed.Claims == nil { - return 0, false - } - if claims, ok := j.processing.parsed.Claims.(golangjwt.MapClaims); ok { - if exp, exists := claims["exp"]; exists { - switch expVal := exp.(type) { - case float64: - return int64(expVal), true - case int64: - return expVal, true - case int: - return int64(expVal), true - } - } - } - return 0, false -} +// GetExpiration returns the exp claim (Unix timestamp) if present. +func (j *Token) GetExpiration() (int64, bool) { + if j.processing.parsed == nil || j.processing.parsed.Claims == nil { + return 0, false + } + nd, _ := j.processing.parsed.Claims.GetExpirationTime() + if nd == nil { + return 0, false + } + return nd.Time.Unix(), true +}Note: The JWT spec defines NumericDate in seconds. Some IdPs emit ms; if you need auto-conversion, we can add a heuristic (e.g., treat values > 253402300799 as ms and divide by 1000). Happy to wire that if desired.
124-143: Use jwt/v5 helper for iat to reduce type branching.-// GetIssuedAt returns the iat claim of the token. -func (j *Token) GetIssuedAt() (int64, bool) { - if j.processing.parsed == nil || j.processing.parsed.Claims == nil { - return 0, false - } - if claims, ok := j.processing.parsed.Claims.(golangjwt.MapClaims); ok { - if iat, exists := claims["iat"]; exists { - switch iatVal := iat.(type) { - case float64: - return int64(iatVal), true - case int64: - return iatVal, true - case int: - return int64(iatVal), true - } - } - } - return 0, false -} +// GetIssuedAt returns the iat claim (Unix timestamp) if present. +func (j *Token) GetIssuedAt() (int64, bool) { + if j.processing.parsed == nil || j.processing.parsed.Claims == nil { + return 0, false + } + nd, _ := j.processing.parsed.Claims.GetIssuedAt() + if nd == nil { + return 0, false + } + return nd.Time.Unix(), true +}
144-157: Use jwt/v5 helper for jti.-// GetJWTID returns the jti claim of the token. -func (j *Token) GetJWTID() string { - if j.processing.parsed == nil || j.processing.parsed.Claims == nil { - return "" - } - if claims, ok := j.processing.parsed.Claims.(golangjwt.MapClaims); ok { - if jti, exists := claims["jti"]; exists { - if jtiStr, ok := jti.(string); ok { - return jtiStr - } - } - } - return "" -} +// GetJWTID returns the jti claim of the token. +func (j *Token) GetJWTID() string { + if j.processing.parsed == nil || j.processing.parsed.Claims == nil { + return "" + } + jti, _ := j.processing.parsed.Claims.GetID() + return jti +} ``] --- `159-178`: **Broaden permissions parsing to handle common shapes.** Many providers emit permissions as []string or a single space-/comma-delimited string. Current code only handles []interface{}. ```diff -// GetPermissions returns the permissions claim of the token. -func (j *Token) GetPermissions() []string { +// GetPermissions returns the permissions claim of the token. +func (j *Token) GetPermissions() []string { if j.processing.parsed == nil || j.processing.parsed.Claims == nil { return nil } - if claims, ok := j.processing.parsed.Claims.(golangjwt.MapClaims); ok { - if permissions, exists := claims["permissions"]; exists { - if perms, ok := permissions.([]interface{}); ok { - result := make([]string, 0, len(perms)) - for _, p := range perms { - if pStr, ok := p.(string); ok { - result = append(result, pStr) - } - } - return result - } - } - } + if claims, ok := j.processing.parsed.Claims.(golangjwt.MapClaims); ok { + if raw, exists := claims["permissions"]; exists { + switch v := raw.(type) { + case []interface{}: + out := make([]string, 0, len(v)) + for _, p := range v { + if s, ok := p.(string); ok { + out = append(out, s) + } + } + if len(out) > 0 { + return out + } + case []string: + if len(v) > 0 { + return append([]string(nil), v...) + } + case string: + if v != "" { + // support both space and comma delimiters + sep := "," + if bytesIndex := func() int { for i := range v { if v[i] == ' ' { return i } }; return -1 }(); bytesIndex != -1 { + sep = " " + } + parts := strings.Split(v, sep) + out := make([]string, 0, len(parts)) + for _, p := range parts { + if ps := strings.TrimSpace(p); ps != "" { + out = append(out, ps) + } + } + if len(out) > 0 { + return out + } + } + } + } + } return nil }Note: add imports if you adopt the string parsing:
import "strings"
180-199: Handle scp as both array and string.scp often arrives as a space-delimited string. Extend parsing to cover both.
-// GetScopes returns the scp claim of the token. -func (j *Token) GetScopes() []string { +// GetScopes returns the scp claim of the token. +func (j *Token) GetScopes() []string { if j.processing.parsed == nil || j.processing.parsed.Claims == nil { return nil } if claims, ok := j.processing.parsed.Claims.(golangjwt.MapClaims); ok { - if scopes, exists := claims["scp"]; exists { - if scps, ok := scopes.([]interface{}); ok { - result := make([]string, 0, len(scps)) - for _, s := range scps { - if sStr, ok := s.(string); ok { - result = append(result, sStr) - } - } - return result - } - } + if raw, exists := claims["scp"]; exists { + switch v := raw.(type) { + case []interface{}: + out := make([]string, 0, len(v)) + for _, s := range v { + if ss, ok := s.(string); ok { + out = append(out, ss) + } + } + if len(out) > 0 { + return out + } + case []string: + if len(v) > 0 { + return append([]string(nil), v...) + } + case string: + if v != "" { + parts := strings.Fields(v) // split on whitespace + if len(parts) > 0 { + return parts + } + } + } + } } return nil }Note: add
import "strings"if not already added.
237-265: Consider returning an empty map instead of nil for feature flags.Returning an empty map simplifies callers (no nil-checks before range). Tests currently expect nil for “missing,” so this is optional and would require test alignment.
If you decide to switch, update:
- return nil + return map[string]FeatureFlag{} ... - return nil + return map[string]FeatureFlag{}
328-337: Avoid exposing internal claims map; return a shallow copy.Returning the underlying MapClaims allows external mutation of parsed token state. Prefer returning a copy.
-// GetClaims returns the claims of the token. -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 - } - return make(map[string]any) -} +// GetClaims returns a shallow copy of the token claims. +func (j *Token) GetClaims() map[string]any { + if j.processing.parsed == nil || j.processing.parsed.Claims == nil { + return map[string]any{} + } + if mc, ok := j.processing.parsed.Claims.(golangjwt.MapClaims); ok { + out := make(map[string]any, len(mc)) + for k, v := range mc { + out[k] = v + } + return out + } + return map[string]any{} +}jwt/jwt_test.go (5)
299-335: Add coverage for permissions as string and []string.Some providers encode permissions as "perm1 perm2" or []string. Extend tests to lock the desired behavior.
Add subtests:
t.Run("returns permissions when permissions claim is space-delimited string", func(t *testing.T) { token := &Token{ processing: tokenProcessing{ parsed: &golangjwt.Token{ Claims: golangjwt.MapClaims{ "permissions": "read:users read:competitions", }, }, }, } perms := token.GetPermissions() assert.Equal(t, []string{"read:users", "read:competitions"}, perms) }) t.Run("returns permissions when permissions claim is []string", func(t *testing.T) { token := &Token{ processing: tokenProcessing{ parsed: &golangjwt.Token{ Claims: golangjwt.MapClaims{ "permissions": []string{"read:users", "read:competitions"}, }, }, }, } perms := token.GetPermissions() assert.Equal(t, []string{"read:users", "read:competitions"}, perms) })
337-373: Add coverage for scp as a space-delimited string.scp frequently appears as a single string (“openid profile email”). Locking this in tests will prevent regressions.
Add a subtest:
t.Run("returns scopes when scp claim is space-delimited string", func(t *testing.T) { token := &Token{ processing: tokenProcessing{ parsed: &golangjwt.Token{ Claims: golangjwt.MapClaims{ "scp": "openid profile email offline", }, }, }, } scopes := token.GetScopes() assert.Equal(t, []string{"openid", "profile", "email", "offline"}, scopes) })
576-596: Clarify AsString expectations and add nil-token test.Currently we only test the happy path. Add a test to document behavior when rawToken is nil and to note that WithExtra fields (e.g., id_token) aren’t included in JSON.
Add:
t.Run("returns 'null' when raw token is nil", func(t *testing.T) { token := &Token{} s, err := token.AsString() assert.NoError(t, err) assert.Equal(t, "null", s) // json.Marshal(nil) -> "null" }) // Optional: assert extras are not present in JSON output t.Run("extras are not serialized by AsString", func(t *testing.T) { raw := (&oauth2.Token{}).WithExtra(map[string]any{"id_token": "test_id"}) token := &Token{rawToken: raw} s, err := token.AsString() assert.NoError(t, err) assert.NotContains(t, s, "test_id") // extras unexported; not in JSON })If you adopt an AsRedactedString helper, mirror tests to assert masking of AccessToken/RefreshToken.
598-614: Add tests for GetValidationErrors empty vs non-empty.Guard against accidental non-nil error when no validation errors exist.
Add:
t.Run("GetValidationErrors returns nil when empty", func(t *testing.T) { token := &Token{validationErrors: nil} assert.Nil(t, token.GetValidationErrors()) }) t.Run("GetValidationErrors returns aggregated error when non-empty", func(t *testing.T) { errs := []error{assert.AnError} token := &Token{validationErrors: errs} err := token.GetValidationErrors() assert.Error(t, err) })
74-110: Overall tests look solid; consider a small helper to reduce repetition.There’s repeated construction of Token{processing: tokenProcessing{parsed: &jwt.Token{Claims: MapClaims{...}}}}. Introduce a small helper to create tokens from claims to tighten the tests.
func tokenWithClaims(mc golangjwt.MapClaims) *Token { return &Token{processing: tokenProcessing{parsed: &golangjwt.Token{Claims: mc}}} }Then replace repetitive blocks with:
token := tokenWithClaims(golangjwt.MapClaims{ "scp": []interface{}{"openid"} })Also applies to: 112-163, 165-218, 220-259, 261-297, 375-449, 451-470, 472-505, 506-540, 541-575, 616-648, 650-758, 760-804, 805-877, 878-950, 951-1051, 1053-1070
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
jwt/jwt_test.go(2 hunks)jwt/token.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
jwt/jwt_test.go (2)
jwt/jwt.go (1)
Token(22-27)jwt/token.go (1)
FeatureFlag(232-235)
jwt/token.go (1)
jwt/jwt.go (1)
Token(22-27)
🔇 Additional comments (2)
jwt/token.go (2)
56-63: LGTM: Subject retrieval uses jwt/v5 helpers.Using Claims.GetSubject() is the right move for both MapClaims and RegisteredClaims.
317-326: LGTM: Minimal, safe coercion helper.toString is intentionally strict; appropriate for typing the feature-flag “t” field.
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 (1)
README.md (1)
18-18: Fix typos and a likely package name mistake in docsThese are user-facing docs; typos and a probable incorrect import/package reference can confuse readers.
Recommended fixes:
- Line 18: “Autorization” → “Authorization”.
- Lines 22–23: “slient secret” → “client secret”; “not exposes tokens” → “does not expose tokens”; consider “session cookie” → “a session cookie”.
- Lines 54–55: Device flow is shown under the authorization_code package. If this is actually a distinct device authorization flow, the import likely should be github.com/kinde-oss/kinde-go/oauth2/device_authorization (or whatever the package is named). Please confirm and correct.
- Line 56: “separatees” → “separates”; also “environment with the limited input capabilities” → “environments with limited input capabilities”.
- Lines 61–65 (comments in snippet): “applicaiton” → “application”; “retreiving” → “retrieving”.
- Line 71: “will provide following methods” → “provides the following methods”.
- Lines 95–99: “optioanlly” → “optionally”; “aquired” → “acquired”.
- Line 116: “willl” → “will”.
I can submit a follow-up PR with these corrections if you prefer.
Also applies to: 22-23, 54-56, 61-65, 71-71, 95-99, 116-116
🧹 Nitpick comments (3)
README.md (3)
165-171: Tighten phrasing, fix “JWT tokens” tautology, and align OAuth2 namingMinor wording and style nits to improve clarity and consistency. Also expands examples of validation options and token info.
Apply this diff:
-**Quick Overview:** - -- Parse JWT tokens from HTTP headers, strings, session storage, or OAuth2 tokens -- Flexible validation options (algorithm, audience, issuer, claims, etc.) -- JWKS support for token signature validation -- Comprehensive token information access -- Seamless integration with OAuth2 flows +**Quick Overview:** + +- Parse JWTs from HTTP headers, raw strings, session storage, or OAuth 2.0 token responses. +- Flexible validation options (algorithm, audience, issuer, required claims, clock skew, custom time function). +- JWKS‑based signature verification. +- Access to comprehensive token information (subject, issuer, audience, expiry, claims, feature flags). +- Seamless integration with OAuth 2.0 flows.
161-171: Optional: include a one‑line import hint to reduce frictionEven though details moved to jwt/README.md, adding a tiny import hint improves DX without bloating this page.
For example, after the Quick Overview:
import "github.com/kinde-oss/kinde-go/jwt"
163-163: Verify jwt/README.md link and API surface
- ✅ Confirmed
jwt/README.mdexists and the relative link fromREADME.md:163resolves correctly on GitHub.- ✅ Documented parsing methods in
jwt/README.md:
•ParseFromString(lines 46–54)
•ParseFromAuthorizationHeader(lines 57–64)
•ParseFromSessionStorage(lines 69–77)⚠️ NoParseOAuth2Token(orParseFromOAuth2Token) helper is defined in the README. If that method exists in code, please add its documentation; otherwise, remove or correct this reference.- ✅ Verified getters and helpers are all present:
•IsValid,GetSubject,GetIssuer,GetAudience,GetClaims,AsString,GetValidationErrors(e.g. lines 36–40, 223–246)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
README.md(1 hunks)jwt/README.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- jwt/README.md
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~168-~168: There might be a mistake here.
Context: ...gorithm, audience, issuer, claims, etc.) - JWKS support for token signature validat...
(QB_NEW_EN)
[grammar] ~169-~169: There might be a mistake here.
Context: ...S support for token signature validation - Comprehensive token information access -...
(QB_NEW_EN)
[grammar] ~170-~170: There might be a mistake here.
Context: ...- Comprehensive token information access - Seamless integration with OAuth2 flows ...
(QB_NEW_EN)
🔇 Additional comments (1)
README.md (1)
161-164: Good reorg: root README stays high-level and defers details to jwt/README.mdThe new “JWT Package” section is concise and points to the dedicated docs. This reduces duplication and keeps the root README focused. Nice.
1 participant
Refactor methods for retrieving JWT claims to improve clarity and functionality. Add comprehensive tests to ensure correct behavior and edge case handling. Update documentation for better understanding of token claims.