feat(auth): Inject organizationID claim into request header#539
Conversation
WalkthroughIntroduces a new constant for an organization ID HTTP header and propagates organization ID from JWT claims into request headers within the authentication middleware. Includes a test case validating that the organization ID from claims is correctly set as a request header. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #539 +/- ##
==========================================
- Coverage 29.14% 28.51% -0.64%
==========================================
Files 166 166
Lines 6714 7943 +1229
==========================================
+ Hits 1957 2265 +308
- Misses 4640 5561 +921
Partials 117 117 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (3)
auth/additional_checks.go (2)
42-44: Simplify redundant condition.The check
expectedOrgID != ""is redundant because we already return early on line 33 whenexpectedOrgID == "". At this point in the code, we knowexpectedOrgIDis not empty.♻️ Simplify the condition
- if expectedOrgID != "" && orgID != expectedOrgID { + if orgID != expectedOrgID { return oidc.ErrOrgIDInvalid }
36-44: Consider setting header after validation succeeds.Currently, the header is set on line 40 before the validation check on line 42. This means if the
orgIDdoesn't matchexpectedOrgID, the header is still set on the request even though an error is returned. While this may not cause issues in practice (since the request will be rejected), it's cleaner to only modify the request after all validation passes.♻️ Set header after validation
orgID := claims.GetOrganizationID() if orgID == "" { return oidc.ErrOrgIDNotPresent } - r.Header.Set(FormanceHeaderOrganizationID, orgID) if expectedOrgID != "" && orgID != expectedOrgID { return oidc.ErrOrgIDInvalid } + r.Header.Set(FormanceHeaderOrganizationID, orgID) return nilauth/middleware_test.go (1)
65-94: Good test coverage for the happy path, but consider edge cases.The test correctly verifies that the organization ID from the token is set as a request header when validation succeeds. The test structure is clear and well-organized.
Consider adding test cases for these scenarios:
When
expectedOrgID == ""(no validation required) - Verify whether the header should be set when the token contains an org ID but the endpoint doesn't require validation (relates to the verification request inadditional_checks.go)When validation fails - Verify behavior when
orgIDin token doesn't matchexpectedOrgIDWhen
orgIDis missing from token - Verify the error case when token lacks organization claims📝 Example test cases
t.Run("orgID not set when validation not required", func(t *testing.T) { t.Parallel() keySet, privateKey, issuer := setupTestKeySet(t) // Provider returns empty string (no validation required) provider := func(*http.Request) (orgID string, err error) { return "", nil } additionalChecks := []AdditionalCheck{CheckOrganizationIDClaim(provider)} authenticator := NewJWTAuth(keySet, issuer, "test-service", false, additionalChecks) token := createAccessTokenWithOrgClaims(t, privateKey, issuer, []string{}, "test-user", "some-org-id") handler := Middleware(authenticator)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte("OK")) })) req := httptest.NewRequest("GET", "/test", nil) req.Header.Set("Authorization", "Bearer "+token) req = req.WithContext(logging.TestingContext()) rr := httptest.NewRecorder() handler.ServeHTTP(rr, req) require.Equal(t, http.StatusOK, rr.Code) // Verify expected behavior - should header be set or not? // assert.Equal(t, "", req.Header.Get(FormanceHeaderOrganizationID)) }) t.Run("validation fails when orgID mismatch", func(t *testing.T) { t.Parallel() keySet, privateKey, issuer := setupTestKeySet(t) provider := func(*http.Request) (orgID string, err error) { return "expected-org", nil } additionalChecks := []AdditionalCheck{CheckOrganizationIDClaim(provider)} authenticator := NewJWTAuth(keySet, issuer, "test-service", false, additionalChecks) token := createAccessTokenWithOrgClaims(t, privateKey, issuer, []string{}, "test-user", "different-org") handler := Middleware(authenticator)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) })) req := httptest.NewRequest("GET", "/test", nil) req.Header.Set("Authorization", "Bearer "+token) req = req.WithContext(logging.TestingContext()) rr := httptest.NewRecorder() handler.ServeHTTP(rr, req) require.Equal(t, http.StatusForbidden, rr.Code) })
📜 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/additional_checks.goauth/middleware_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 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/middleware_test.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.go
🔇 Additional comments (3)
auth/additional_checks.go (2)
10-11: LGTM! Well-defined header constant.The constant follows Go naming conventions and uses the standard
X-prefix for custom HTTP headers.
31-40: Verify: Should header be set when validation is not required?The PR objective states to "propagate organization ID from JWT claims into request headers." However, when
expectedOrgID == ""(no validation required), the function returns early on line 33 without setting the header. This means the header is only set when org ID validation is explicitly required.Consider whether the header should be propagated whenever an
orgIDis present in the token claims, regardless of whether validation is required for that endpoint. This could be useful for downstream services, logging, or auditing even when the endpoint doesn't enforce org ID checks.If the header should be set whenever
orgIDis present (regardless of validation requirement), consider this approach:+ orgID := claims.GetOrganizationID() + if orgID != "" { + r.Header.Set(FormanceHeaderOrganizationID, orgID) + } + expectedOrgID, err := fn(r) if err != nil { return err } // if the endpoint doesn't require a particular orgID we consider it valid if expectedOrgID == "" { return nil } - orgID := claims.GetOrganizationID() if orgID == "" { return oidc.ErrOrgIDNotPresent } - r.Header.Set(FormanceHeaderOrganizationID, orgID) if expectedOrgID != "" && orgID != expectedOrgID { return oidc.ErrOrgIDInvalid } return nilauth/middleware_test.go (1)
9-9: LGTM! Appropriate test library import.The
assertpackage is correctly imported for the new assertion on line 93 and is consistent with existing test dependencies.
1 participant
Relates to EN-518