Conversation
WalkthroughAdds an extensible additional-checks framework to JWT authentication, introduces organization-aware access token claims and errors, wires organization checks into module/CLI, enforces scope logic, updates middleware to return 403 for org validation failures, and expands tests and mocks accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware
participant JWTAuth
participant TokenParser
participant OIDC as OIDCVerifier
participant AdditionalChecks
Client->>Middleware: HTTP Request (Authorization header)
Middleware->>JWTAuth: Authenticate(request)
JWTAuth->>TokenParser: claimsFromRequest(r, &claims, keySet)
TokenParser->>TokenParser: validate header, decrypt/parse, verify signature
TokenParser->>OIDC: signature verification / key lookup
OIDC-->>TokenParser: verified claims
TokenParser-->>JWTAuth: populated claims
rect rgb(200,230,200)
Note over JWTAuth,AdditionalChecks: Execute additional checks (loop)
JWTAuth->>AdditionalChecks: check(request, claims)
alt Provider returns orgID and match
AdditionalChecks-->>JWTAuth: nil
else Provider returns orgID and mismatch
AdditionalChecks-->>JWTAuth: ErrOrgIDInvalid
else Provider returns error / org missing
AdditionalChecks-->>JWTAuth: ErrOrgIDNotPresent / error
end
end
JWTAuth-->>Middleware: (allowed bool, err)
alt err is ErrOrgIDInvalid or ErrOrgIDNotPresent
Middleware-->>Client: 403 Forbidden
else err != nil
Middleware-->>Client: 401 Unauthorized
else allowed
Middleware-->>Client: 200 OK (pass to handler)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (5)
auth/scopes.go (1)
12-26: Minor: Error message could be more precise for write-only methods.The error message always states
need <service>:read|write, but for non-read methods (POST, PUT, DELETE, PATCH), only:writescope is actually required. This could be slightly misleading during debugging.Consider adjusting the error message based on the method:
🔎 Proposed refinement
func checkScopes(service string, method string, scopes oidc.SpaceDelimitedArray) (bool, error) { - allowed := true //nolint:ineffassign + var allowed bool + var requiredScope string switch method { case http.MethodOptions, http.MethodGet, http.MethodHead, http.MethodTrace: allowed = collectionutils.Contains(scopes, service+":read") || collectionutils.Contains(scopes, service+":write") + requiredScope = service + ":read or " + service + ":write" default: allowed = collectionutils.Contains(scopes, service+":write") + requiredScope = service + ":write" } if !allowed { - return false, fmt.Errorf("missing access, found scopes: '%s' need %s:read|write", strings.Join(scopes, ", "), service) + return false, fmt.Errorf("missing access, found scopes: '%s' need %s", strings.Join(scopes, ", "), requiredScope) } return true, nil }auth/organization_auth.go (1)
77-79: Redundant condition check.The condition
expectedOrgID != ""on line 77 is redundant since we already return early on lines 68-70 whenexpectedOrgID == "". At this point,expectedOrgIDis guaranteed to be non-empty.🔎 Proposed simplification
- if expectedOrgID != "" && orgID != expectedOrgID { + if orgID != expectedOrgID { return false, oidc.ErrOrgIDInvalid }auth/organization_auth_test.go (1)
69-156: Consider adding test coverage for scope checking.All current tests have
checkScopes: false. Consider adding subtests that exercise thecheckScopes: truepath to ensure the scope validation logic integrates correctly with the organization auth flow.Example scenarios:
- Success with valid token, correct orgID, and valid scopes
- Failure with valid token, correct orgID, but missing required scopes
auth/auth_test.go (2)
85-102: Missing test coverage forJWTOrganizationAuthin success case.Unlike the other test cases which now use table-driven tests to cover both authenticators, this success test only validates
JWTAuth. For consistency and full coverage, consider refactoring to includeJWTOrganizationAuth.🔎 Suggested refactor
t.Run("success with valid token", func(t *testing.T) { t.Parallel() keySet, privateKey, issuer := setupTestKeySet(t) + tests := []struct { + name string + auth Authenticator + }{ + { + name: "JWTAuth", + auth: NewJWTAuth(keySet, issuer, "test-service", false), + }, + { + name: "JWTOrganizationAuth", + auth: NewJWTOrganizationAuth(keySet, issuer, "test-service", false, noOrgGetterFn), + }, + } - auth := NewJWTAuth(keySet, issuer, "test-service", false) - - // Create access token - token := createAccessToken(t, privateKey, issuer, []string{}, "test-user") - - // Create request with valid token - req := httptest.NewRequest("GET", "/test", nil) - req.Header.Set("Authorization", "Bearer "+token) - req = req.WithContext(logging.TestingContext()) - - authenticated, err := auth.Authenticate(nil, req) - require.NoError(t, err) - require.True(t, authenticated) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create access token + token := createAccessToken(t, privateKey, issuer, []string{}, "test-user") + + // Create request with valid token + req := httptest.NewRequest("GET", "/test", nil) + req.Header.Set("Authorization", "Bearer "+token) + req = req.WithContext(logging.TestingContext()) + + authenticated, err := tt.auth.Authenticate(nil, req) + require.NoError(t, err) + require.True(t, authenticated) + }) + } })
121-132: Consider addingt.Parallel()to table-driven subtests.The inner subtests don't call
t.Parallel(), which means they run sequentially within each outer parallel test. Since each subtest is independent (creates its own request), addingt.Parallel()can improve test execution time.🔎 Suggested change (applies to all similar subtests)
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() req := httptest.NewRequest("GET", "/test", nil)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
auth/auth.goauth/auth_test.goauth/organization_auth.goauth/organization_auth_test.goauth/scopes.gooidc/organization_aware_access_token_claims.gooidc/verifier.go
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-18T08:02:30.517Z
Learnt from: gfyrag
Repo: formancehq/go-libs PR: 532
File: auth/auth.go:92-110
Timestamp: 2025-11-18T08:02:30.517Z
Learning: In formancehq/go-libs auth package, the ClaimsFromRequest function intentionally returns claims alongside errors when issuer, signature, or expiration validation fails. This allows callers to access claim data even when validation doesn't pass completely.
Applied to files:
oidc/verifier.gooidc/organization_aware_access_token_claims.goauth/auth_test.goauth/scopes.goauth/auth.go
📚 Learning: 2025-11-03T11:37:07.289Z
Learnt from: gfyrag
Repo: formancehq/go-libs PR: 521
File: oidc/client/signing.go:15-24
Timestamp: 2025-11-03T11:37:07.289Z
Learning: In the formancehq/go-libs repository, the custom time package (github.com/formancehq/go-libs/v3/time) defines a Time struct that embeds the standard library's time.Time. Because of Go's embedding promotion, values of the custom Time type can be passed directly to functions expecting a standard time.Time, such as oidc.FromTime(). No explicit accessor like .Time is needed.
Applied to files:
oidc/organization_aware_access_token_claims.go
📚 Learning: 2025-11-05T15:43:35.659Z
Learnt from: gfyrag
Repo: formancehq/go-libs PR: 524
File: auth/module.go:28-43
Timestamp: 2025-11-05T15:43:35.659Z
Learning: In the formancehq/go-libs auth module, retry logic via retryablehttp is only needed for OIDC discovery at startup (e.g., during service reboots). The RemoteKeySet used for runtime JWT verification has its own HTTP request handling and does not require the retryable HTTP client.
Applied to files:
auth/auth_test.goauth/auth.go
📚 Learning: 2025-11-03T12:12:17.346Z
Learnt from: gfyrag
Repo: formancehq/go-libs PR: 521
File: oidc/client/device.go:19-22
Timestamp: 2025-11-03T12:12:17.346Z
Learning: In the formancehq/go-libs oidc package, direct assignment of []string to oidc.SpaceDelimitedArray fields (such as ClientCredentialsRequest.Scope) is acceptable and compiles successfully, even though SpaceDelimitedArray is a named type rather than a type alias. Explicit conversion is not required.
Applied to files:
auth/scopes.go
📚 Learning: 2025-11-03T11:34:31.815Z
Learnt from: gfyrag
Repo: formancehq/go-libs PR: 521
File: oidc/client/device.go:94-94
Timestamp: 2025-11-03T11:34:31.815Z
Learning: In the go-libs oidc package, interface methods that return *http.Client (such as HttpClient() on DeviceAuthorizationCaller) should not have nil-checks added. The interface contract requires callers to provide a valid HTTP client, and defensive nil-checking is not desired.
Applied to files:
auth/scopes.goauth/auth.go
🧬 Code graph analysis (4)
oidc/organization_aware_access_token_claims.go (2)
oidc/token.go (2)
AccessTokenClaims(87-91)NewAccessTokenClaims(93-110)oidc/verifier.go (1)
Claims(18-28)
auth/organization_auth.go (3)
oidc/keyset.go (1)
KeySet(27-30)oidc/organization_aware_access_token_claims.go (1)
OrganizationAwareAccessTokenClaims(8-10)oidc/verifier.go (4)
CheckIssuer(115-120)CheckExpiration(190-196)ErrOrgIDNotPresent(58-58)ErrOrgIDInvalid(59-59)
auth/scopes.go (2)
oidc/types.go (1)
SpaceDelimitedArray(172-172)collectionutils/slice.go (1)
Contains(70-77)
auth/auth.go (3)
oidc/token.go (1)
AccessTokenClaims(87-91)oidc/verifier.go (4)
CheckIssuer(115-120)CheckExpiration(190-196)DecryptToken(91-93)ParseToken(95-106)oidc/keyset.go (1)
KeySet(27-30)
🪛 GitHub Actions: Default
auth/organization_auth_test.go
[error] 1-1: Unstaged changes detected: 'Changes not staged for commit: modified: auth/organization_auth_test.go'. The pipeline exited with code 1 due to uncommitted changes.
🔇 Additional comments (7)
oidc/verifier.go (1)
57-59: LGTM!The new organization ID error variables follow the established pattern in this file and provide clear, semantic error signaling for the organization-aware authentication flow.
auth/organization_auth_test.go (1)
18-67: LGTM!The helper function is well-structured and correctly creates signed JWTs with optional organization_id claims for testing. The use of go-jose for signing is consistent with the library's approach.
oidc/organization_aware_access_token_claims.go (1)
17-27: No action needed. The code correctly handles nil or missing organization_id claims by returning an empty string, which is the expected pattern in this codebase. Reading from a nil map returns the zero value without error, making the current implementation safe and idiomatic. Defensive nil-checking for initialization is not expected in this codebase's design patterns.auth/organization_auth.go (1)
38-81: Signature verification is correctly performed byclaimsFromRequest.The
claimsFromRequestfunction (line 41) performs JWT signature verification using the providedkeySetparameter viaoidc.CheckSignature(). If signature validation fails, an error is returned immediately, preventing further authentication checks from proceeding.auth/auth_test.go (1)
395-430: Good addition of issuer validation test.The new test case properly validates that both authenticators reject tokens with mismatched issuers and correctly returns
oidc.ErrIssuerInvalid. This aligns with the enhanced issuer verification introduced in this PR.auth/auth.go (2)
33-52: LGTM!The
Authenticatemethod correctly orders the validation steps: parse claims first, then validate issuer, expiration, and finally scopes. The early return whencheckScopesis disabled is a good optimization.
75-108: LGTM!The generic
claimsFromRequestfunction cleanly handles the token extraction and validation flow. The use of generics allows flexibility for different claim types while maintaining type safety.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #537 +/- ##
==========================================
+ Coverage 28.68% 29.11% +0.42%
==========================================
Files 162 166 +4
Lines 6644 6711 +67
==========================================
+ Hits 1906 1954 +48
- Misses 4622 4640 +18
- Partials 116 117 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
auth/cli.go (1)
25-55: Consider extracting shared flag parsing to reduce duplication.Both
FXModuleFromFlagsandOrganizationAwareFXModuleFromFlagsduplicate the flag parsing logic. You could extract this to a helper:🔎 Proposed refactor
+func moduleConfigFromFlags(cmd *cobra.Command) ModuleConfig { + authEnabled, _ := cmd.Flags().GetBool(AuthEnabledFlag) + authIssuer, _ := cmd.Flags().GetString(AuthIssuerFlag) + authReadKeySetMaxRetries, _ := cmd.Flags().GetInt(AuthReadKeySetMaxRetriesFlag) + authCheckScopes, _ := cmd.Flags().GetBool(AuthCheckScopesFlag) + authService, _ := cmd.Flags().GetString(AuthServiceFlag) + + return ModuleConfig{ + Enabled: authEnabled, + Issuer: authIssuer, + ReadKeySetMaxRetries: authReadKeySetMaxRetries, + CheckScopes: authCheckScopes, + Service: authService, + } +} + func FXModuleFromFlags(cmd *cobra.Command) fx.Option { - authEnabled, _ := cmd.Flags().GetBool(AuthEnabledFlag) - authIssuer, _ := cmd.Flags().GetString(AuthIssuerFlag) - authReadKeySetMaxRetries, _ := cmd.Flags().GetInt(AuthReadKeySetMaxRetriesFlag) - authCheckScopes, _ := cmd.Flags().GetBool(AuthCheckScopesFlag) - authService, _ := cmd.Flags().GetString(AuthServiceFlag) - - return Module(ModuleConfig{ - Enabled: authEnabled, - Issuer: authIssuer, - ReadKeySetMaxRetries: authReadKeySetMaxRetries, - CheckScopes: authCheckScopes, - Service: authService, - }, nil) + return Module(moduleConfigFromFlags(cmd), nil) } func OrganizationAwareFXModuleFromFlags(cmd *cobra.Command, fn OrganizationIDGetterFn) fx.Option { - authEnabled, _ := cmd.Flags().GetBool(AuthEnabledFlag) - authIssuer, _ := cmd.Flags().GetString(AuthIssuerFlag) - authReadKeySetMaxRetries, _ := cmd.Flags().GetInt(AuthReadKeySetMaxRetriesFlag) - authCheckScopes, _ := cmd.Flags().GetBool(AuthCheckScopesFlag) - authService, _ := cmd.Flags().GetString(AuthServiceFlag) - - return Module(ModuleConfig{ - Enabled: authEnabled, - Issuer: authIssuer, - ReadKeySetMaxRetries: authReadKeySetMaxRetries, - CheckScopes: authCheckScopes, - Service: authService, - }, fn) + return Module(moduleConfigFromFlags(cmd), fn) }auth/auth_test.go (1)
121-132: Consider addingt.Parallel()to inner subtests for faster execution.The inner subtests (e.g., line 122) don't call
t.Parallel(), while the outer test does. Adding parallelism to inner subtests could speed up test execution, though it's optional since the test cases are lightweight.🔎 Proposed change
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() req := httptest.NewRequest("GET", "/test", nil)This pattern applies to all the table-driven test loops in this file.
auth/organization_auth.go (1)
77-79: Simplify redundant condition.At this point,
expectedOrgID != ""is already guaranteed to be true (we returned early on line 68-70 if it was empty). The condition can be simplified.🔎 Proposed fix
- if expectedOrgID != "" && orgID != expectedOrgID { + if orgID != expectedOrgID { return false, oidc.ErrOrgIDInvalid }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
auth/auth.goauth/auth_test.goauth/cli.goauth/module.goauth/module_test.goauth/organization_auth.goauth/organization_auth_test.gooidc/organization_aware_access_token_claims.gooidc/verifier.go
🚧 Files skipped from review as they are similar to previous changes (1)
- auth/organization_auth_test.go
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-18T08:02:30.517Z
Learnt from: gfyrag
Repo: formancehq/go-libs PR: 532
File: auth/auth.go:92-110
Timestamp: 2025-11-18T08:02:30.517Z
Learning: In formancehq/go-libs auth package, the ClaimsFromRequest function intentionally returns claims alongside errors when issuer, signature, or expiration validation fails. This allows callers to access claim data even when validation doesn't pass completely.
Applied to files:
oidc/organization_aware_access_token_claims.gooidc/verifier.goauth/auth.goauth/auth_test.go
📚 Learning: 2025-11-03T11:37:07.289Z
Learnt from: gfyrag
Repo: formancehq/go-libs PR: 521
File: oidc/client/signing.go:15-24
Timestamp: 2025-11-03T11:37:07.289Z
Learning: In the formancehq/go-libs repository, the custom time package (github.com/formancehq/go-libs/v3/time) defines a Time struct that embeds the standard library's time.Time. Because of Go's embedding promotion, values of the custom Time type can be passed directly to functions expecting a standard time.Time, such as oidc.FromTime(). No explicit accessor like .Time is needed.
Applied to files:
oidc/organization_aware_access_token_claims.go
📚 Learning: 2025-11-03T11:34:31.815Z
Learnt from: gfyrag
Repo: formancehq/go-libs PR: 521
File: oidc/client/device.go:94-94
Timestamp: 2025-11-03T11:34:31.815Z
Learning: In the go-libs oidc package, interface methods that return *http.Client (such as HttpClient() on DeviceAuthorizationCaller) should not have nil-checks added. The interface contract requires callers to provide a valid HTTP client, and defensive nil-checking is not desired.
Applied to files:
auth/auth.go
📚 Learning: 2025-11-03T12:12:17.346Z
Learnt from: gfyrag
Repo: formancehq/go-libs PR: 521
File: oidc/client/device.go:19-22
Timestamp: 2025-11-03T12:12:17.346Z
Learning: In the formancehq/go-libs oidc package, direct assignment of []string to oidc.SpaceDelimitedArray fields (such as ClientCredentialsRequest.Scope) is acceptable and compiles successfully, even though SpaceDelimitedArray is a named type rather than a type alias. Explicit conversion is not required.
Applied to files:
auth/auth.go
📚 Learning: 2025-11-05T15:43:35.659Z
Learnt from: gfyrag
Repo: formancehq/go-libs PR: 524
File: auth/module.go:28-43
Timestamp: 2025-11-05T15:43:35.659Z
Learning: In the formancehq/go-libs auth module, retry logic via retryablehttp is only needed for OIDC discovery at startup (e.g., during service reboots). The RemoteKeySet used for runtime JWT verification has its own HTTP request handling and does not require the retryable HTTP client.
Applied to files:
auth/auth.goauth/auth_test.go
🧬 Code graph analysis (6)
oidc/organization_aware_access_token_claims.go (2)
oidc/token.go (2)
AccessTokenClaims(87-91)NewAccessTokenClaims(93-110)oidc/verifier.go (1)
Claims(18-28)
auth/module_test.go (3)
logging/adapter_logrus.go (1)
Testing(75-90)auth/middleware.go (1)
Authenticator(7-9)auth/module.go (2)
Module(25-82)ModuleConfig(14-20)
auth/organization_auth.go (3)
oidc/keyset.go (1)
KeySet(27-30)oidc/organization_aware_access_token_claims.go (1)
OrganizationAwareAccessTokenClaims(8-10)oidc/verifier.go (4)
CheckIssuer(115-120)CheckExpiration(190-196)ErrOrgIDNotPresent(58-58)ErrOrgIDInvalid(59-59)
auth/auth.go (3)
oidc/token.go (1)
AccessTokenClaims(87-91)oidc/verifier.go (4)
CheckIssuer(115-120)CheckExpiration(190-196)DecryptToken(91-93)ParseToken(95-106)oidc/keyset.go (1)
KeySet(27-30)
auth/auth_test.go (6)
auth/middleware.go (1)
Authenticator(7-9)auth/auth.go (1)
NewJWTAuth(18-30)auth/organization_auth.go (1)
NewJWTOrganizationAuth(22-36)logging/context.go (1)
TestingContext(36-38)oidc/token.go (1)
NewAccessTokenClaims(93-110)oidc/verifier.go (1)
ErrIssuerInvalid(37-37)
auth/cli.go (3)
auth/organization_auth.go (1)
OrganizationIDGetterFn(11-11)auth/module.go (2)
Module(25-82)ModuleConfig(14-20)testing/testservice/service.go (1)
Service(16-23)
🔇 Additional comments (10)
oidc/verifier.go (1)
57-59: LGTM!The new organization-related errors follow the established pattern in this file (e.g.,
ErrAuthTimeNotPresent,ErrAcrInvalid) and provide clear semantic meaning for organization_id claim validation failures.auth/auth_test.go (1)
395-430: LGTM!Good addition of issuer mismatch testing for both authenticators with proper
assert.ErrorIsusage to verify the specific error type.auth/module.go (2)
22-35: LGTM!The updated signature with clear documentation and the early return for disabled auth simplifies the control flow.
57-80: LGTM!Clean conditional wiring based on whether
orgIdGetterFnis provided. The branching logic correctly selects betweenNewJWTOrganizationAuthandNewJWTAuth.auth/organization_auth.go (1)
38-59: LGTM!The authentication flow correctly validates the JWT in proper order: parse claims, check issuer, check expiration, then validate scopes. This matches the pattern used in
JWTAuth.Authenticate.auth/module_test.go (1)
102-141: LGTM!Good addition of the organization-aware module test that verifies discovery is still called when
orgIdGetterFnis provided. The test properly validates that the module initializes correctly with the organization ID getter.auth/auth.go (3)
50-64: LGTM - Previous issue resolved!The order is now correct:
claimsFromRequestis called first to populate the claims struct, thenCheckIssuerandCheckExpirationare called on the populated claims. This addresses the issue from the previous review.
66-100: LGTM!The generic
claimsFromRequesthelper cleanly encapsulates the common token extraction and validation logic. This enables reuse by both standard and organization-aware authentication flows.One minor note: the empty slice
[]string{}at line 93 defaults toRS256, ES256, PS256pertoJoseSignatureAlgorithmsin verifier.go, which is a sensible default.
39-42: LGTM!Clean simplification - early return when scopes aren't checked, otherwise delegate to the centralized
checkScopeshelper.oidc/organization_aware_access_token_claims.go (1)
17-27: No action required—theClaimsmap is properly populated during token parsing.The
AccessTokenClaimsstruct has a customUnmarshalJSONmethod (oidc/token.go:118) that callsunmarshalJSONMulti(data, (*atcAlias)(a), &a.Claims). This utility unmarshals the raw JWT token data into theClaimsmap by callingjson.Unmarshal(data, &a.Claims), ensuring all claim fields—includingorganization_id—are captured in the map. TheGetOrganizationID()method will work correctly for any organization ID present in the token.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
auth/middleware_test.go (1)
81-99: Consider addingt.Parallel()to inner subtests for consistency.The outer "forbidden" test and other test cases in this file use
t.Parallel(), but the inner table-driven subtests don't. Adding it would maintain consistency and allow parallel execution.🔎 Suggested fix
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() ctrl := gomock.NewController(t) authenticator := NewMockAuthenticator(ctrl)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
auth/authenticator_generated.goauth/middleware.goauth/middleware_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-03T11:34:31.815Z
Learnt from: gfyrag
Repo: formancehq/go-libs PR: 521
File: oidc/client/device.go:94-94
Timestamp: 2025-11-03T11:34:31.815Z
Learning: In the go-libs oidc package, interface methods that return *http.Client (such as HttpClient() on DeviceAuthorizationCaller) should not have nil-checks added. The interface contract requires callers to provide a valid HTTP client, and defensive nil-checking is not desired.
Applied to files:
auth/middleware.goauth/middleware_test.go
🧬 Code graph analysis (2)
auth/middleware.go (1)
oidc/verifier.go (2)
ErrOrgIDNotPresent(58-58)ErrOrgIDInvalid(59-59)
auth/middleware_test.go (4)
oidc/verifier.go (2)
ErrOrgIDInvalid(59-59)ErrOrgIDNotPresent(58-58)auth/authenticator_generated.go (1)
NewMockAuthenticator(32-36)auth/middleware.go (1)
Middleware(15-38)logging/context.go (1)
TestingContext(36-38)
⏰ 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). (2)
- GitHub Check: Ci
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
auth/authenticator_generated.go (1)
1-56: LGTM!Standard MockGen-generated code for the
Authenticatorinterface. The generated mock follows proper gomock patterns and correctly implements the interface method signature.auth/middleware.go (1)
20-27: LGTM!The error handling correctly distinguishes between authentication failures (401) and authorization failures (403). Using
errors.Is()properly handles wrapped errors, which aligns with the test cases that wrap these sentinel errors withfmt.Errorf.auth/middleware_test.go (1)
67-79: Good test design: wrapped errors validateerrors.Is()behavior.The test cases correctly wrap the sentinel errors with
fmt.Errorf("err: %w", ...), which validates that the middleware'serrors.Is()check properly handles wrapped errors. This is a robust testing pattern.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @auth/auth_test.go:
- Line 486: Update the t.Run invocation string to fix the typo: change
t.Run("failure due to additonal check", func(t *testing.T) { ... }) to use the
correct spelling "additional" so it reads t.Run("failure due to additional
check", func(t *testing.T) { ... }).
🧹 Nitpick comments (1)
auth/additional_checks.go (1)
39-41: Redundant condition check.The
expectedOrgID != ""check on line 39 is redundant since line 30 already returns early whenexpectedOrgID == "". The condition will always be true at this point.🔎 Proposed simplification
- if expectedOrgID != "" && orgID != expectedOrgID { + if orgID != expectedOrgID { return oidc.ErrOrgIDInvalid }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
auth/additional_checks.goauth/auth.goauth/auth_test.goauth/cli.goauth/middleware_test.goauth/module.goauth/module_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- auth/cli.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-18T08:02:30.517Z
Learnt from: gfyrag
Repo: formancehq/go-libs PR: 532
File: auth/auth.go:92-110
Timestamp: 2025-11-18T08:02:30.517Z
Learning: In formancehq/go-libs auth package, the ClaimsFromRequest function intentionally returns claims alongside errors when issuer, signature, or expiration validation fails. This allows callers to access claim data even when validation doesn't pass completely.
Applied to files:
auth/additional_checks.goauth/auth_test.goauth/auth.go
📚 Learning: 2025-11-03T11:34:31.815Z
Learnt from: gfyrag
Repo: formancehq/go-libs PR: 521
File: oidc/client/device.go:94-94
Timestamp: 2025-11-03T11:34:31.815Z
Learning: In the go-libs oidc package, interface methods that return *http.Client (such as HttpClient() on DeviceAuthorizationCaller) should not have nil-checks added. The interface contract requires callers to provide a valid HTTP client, and defensive nil-checking is not desired.
Applied to files:
auth/additional_checks.goauth/middleware_test.goauth/auth.go
📚 Learning: 2025-11-05T15:43:35.659Z
Learnt from: gfyrag
Repo: formancehq/go-libs PR: 524
File: auth/module.go:28-43
Timestamp: 2025-11-05T15:43:35.659Z
Learning: In the formancehq/go-libs auth module, retry logic via retryablehttp is only needed for OIDC discovery at startup (e.g., during service reboots). The RemoteKeySet used for runtime JWT verification has its own HTTP request handling and does not require the retryable HTTP client.
Applied to files:
auth/middleware_test.goauth/auth.go
🧬 Code graph analysis (5)
auth/module_test.go (6)
auth/middleware.go (1)
Authenticator(11-13)auth/additional_checks.go (2)
AdditionalCheck(10-10)CheckOrganizationIDClaim(17-44)auth/module.go (2)
Module(24-68)ModuleConfig(14-22)testing/testservice/service.go (1)
Service(16-23)query/expression.go (1)
Context(13-15)logging/adapter_logrus.go (1)
Testing(75-90)
auth/additional_checks.go (3)
oidc/token.go (1)
AccessTokenClaims(87-91)oidc/organization_aware_access_token_claims.go (1)
OrganizationAwareAccessTokenClaims(8-10)oidc/verifier.go (2)
ErrOrgIDNotPresent(58-58)ErrOrgIDInvalid(59-59)
auth/module.go (2)
auth/additional_checks.go (1)
AdditionalCheck(10-10)auth/auth.go (1)
NewJWTAuth(19-33)
auth/middleware_test.go (3)
auth/auth.go (1)
NewJWTAuth(19-33)oidc/verifier.go (2)
ErrOrgIDInvalid(59-59)ErrOrgIDNotPresent(58-58)auth/middleware.go (1)
Middleware(15-38)
auth/auth.go (4)
oidc/keyset.go (1)
KeySet(27-30)auth/additional_checks.go (1)
AdditionalCheck(10-10)oidc/token.go (1)
AccessTokenClaims(87-91)oidc/verifier.go (4)
CheckIssuer(115-120)CheckExpiration(190-196)DecryptToken(91-93)ParseToken(95-106)
🔇 Additional comments (10)
auth/module_test.go (1)
102-147: LGTM!The new test case properly validates that the module with
AdditionalCheckscorrectly invokes the OIDC discovery endpoint. The test structure is consistent with existing patterns and appropriately uses a dummy provider for wiring verification.auth/additional_checks.go (1)
10-32: Well-designed extensibility pattern.The
AdditionalCheckfunction type andOrganizationIDProviderabstraction provide a clean, composable way to inject authorization logic. The early return for emptyexpectedOrgIDcorrectly handles endpoints that don't require organization scoping.auth/middleware_test.go (1)
64-101: Good coverage for 403 Forbidden scenarios.The test correctly validates that organization ID-related errors result in HTTP 403 Forbidden. Using wrapped errors (
fmt.Errorf("err: %w", ...)) realistically tests theerrors.Is()check in middleware.Consider adding
t.Parallel()to the inner subtests (line 82) for consistency with other tests in this file, though this is optional given the test's simplicity.auth/module.go (1)
20-21: Clean integration of AdditionalChecks into the module.The
AdditionalChecksfield is properly added toModuleConfigand correctly passed toNewJWTAuth. The early return pattern for disabled auth improves code clarity.Also applies to: 56-65
auth/auth.go (3)
42-47: Correct short-circuit evaluation for additional checks.The loop correctly stops on the first failing check and returns the error immediately, preventing unnecessary computation and ensuring proper error propagation.
60-75: Claim validation order is correct.The implementation properly sequences operations: first extract and parse claims via
claimsFromRequest, then validate issuer, then check expiration. This addresses the ordering concern from previous reviews.
77-110: Generic helper provides good abstraction.The
claimsFromRequest[CLAIMS any]helper cleanly separates token extraction/parsing from validation logic. The authorization header parsing correctly handles both "bearer" and "Bearer" prefixes.auth/auth_test.go (3)
81-130: Well-structured test helper for organization-aware tokens.The
createAccessTokenWithOrgClaimshelper correctly handles the optionalorganizationIDby conditionally adding it to the private claims map. UsingaccessTokenClaims.MarshalJSON()ensures proper serialization of the organization-aware claims structure.
486-523: Good validation of short-circuit behavior.The test correctly verifies that additional checks stop executing after the first failure (line 522 asserts exactly 2 checks were performed out of 3). This confirms proper error propagation in the authentication flow.
525-621: Comprehensive coverage for CheckOrganizationIDClaim scenarios.The four test cases thoroughly validate the organization ID claim check:
- Success with matching org ID
- Success when no org ID is expected (empty provider return)
- Failure on org ID mismatch (
ErrOrgIDInvalid)- Failure when token lacks org ID (
ErrOrgIDNotPresent)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
auth/auth_test.go (1)
143-143: Consider usingnilfor consistency.Line 143 passes
[]AdditionalCheck{}while most other tests passnilfor the additionalChecks parameter. Both work, but usingnilconsistently would improve uniformity across the test suite.🔎 Proposed change
- auth := NewJWTAuth(keySet, issuer, "test-service", false, []AdditionalCheck{}) + auth := NewJWTAuth(keySet, issuer, "test-service", false, nil)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
auth/auth_test.goauth/cli.go
🚧 Files skipped from review as they are similar to previous changes (1)
- auth/cli.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-18T08:02:30.517Z
Learnt from: gfyrag
Repo: formancehq/go-libs PR: 532
File: auth/auth.go:92-110
Timestamp: 2025-11-18T08:02:30.517Z
Learning: In formancehq/go-libs auth package, the ClaimsFromRequest function intentionally returns claims alongside errors when issuer, signature, or expiration validation fails. This allows callers to access claim data even when validation doesn't pass completely.
Applied to files:
auth/auth_test.go
🧬 Code graph analysis (1)
auth/auth_test.go (5)
oidc/organization_aware_access_token_claims.go (2)
NewOrganizationAwareAccessTokenClaims(12-15)ClaimOrganizationID(5-5)oidc/verifier.go (4)
Claims(18-28)ErrIssuerInvalid(37-37)ErrOrgIDInvalid(59-59)ErrOrgIDNotPresent(58-58)auth/additional_checks.go (2)
AdditionalCheck(10-10)CheckOrganizationIDClaim(17-44)oidc/token.go (2)
AccessTokenClaims(87-91)NewAccessTokenClaims(93-110)auth/auth.go (1)
NewJWTAuth(19-33)
⏰ 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). (2)
- GitHub Check: Analyze (go)
- GitHub Check: Ci
🔇 Additional comments (4)
auth/auth_test.go (4)
81-130: LGTM! Well-structured helper for organization-aware tokens.The helper correctly creates tokens with optional organization_id claims using
NewOrganizationAwareAccessTokenClaimsand properly usesMarshalJSON()for custom serialization.
525-621: LGTM! Comprehensive coverage of organization ID validation.The test cases thoroughly exercise all paths of
CheckOrganizationIDClaim: successful match, no requirement, mismatch, and missing organization_id. Error assertions correctly useErrorIsfor validation errors.
158-484: LGTM! Excellent use of test matrices for coverage.The test matrix pattern effectively validates that additional checks don't interfere with core authentication functionality. The consistent structure across multiple test cases improves maintainability while ensuring comprehensive coverage of both execution paths.
486-486: Typo in test name."additonal" should be "additional".
🔎 Proposed fix
- t.Run("failure due to additonal check", func(t *testing.T) { + t.Run("failure due to additional check", func(t *testing.T) {Likely an incorrect or invalid review comment.
2 participants
Fixes: EN-518