Conversation
WalkthroughAdds audience claim validation, a ControlPlaneAgent abstraction and AuthenticateOnControlPlane flow, ControlPlane middleware that injects org/client IDs into context, a NoAgent implementation, and support for annotated FX modules to create multiple distinct authenticator instances. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Middleware as ControlPlaneMiddleware
participant Auth as JWTAuth
participant OIDC as oidc.AccessTokenClaims
participant Agent as ControlPlaneAgent
Client->>Middleware: HTTP Request
Middleware->>Auth: AuthenticateOnControlPlane(request)
Auth->>OIDC: Extract & validate token claims
alt Claims Valid
Auth->>Auth: Run additional checks (audience, scopes)
alt All checks pass
Auth->>Agent: NewDefaultControlPlaneAgent(claims)
Agent-->>Auth: ControlPlaneAgent
Auth-->>Middleware: (agent, nil)
else Check fails
Auth-->>Middleware: (agent, error)
Middleware-->>Client: 403 Forbidden
end
else Claims invalid
Auth-->>Middleware: (agent, error)
Middleware-->>Client: 401/403
end
alt Success
Middleware->>Middleware: Inject org/client IDs into context
Middleware->>Client: Call next handler with updated context
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #540 +/- ##
==========================================
+ Coverage 29.14% 29.22% +0.07%
==========================================
Files 166 174 +8
Lines 6714 6943 +229
==========================================
+ Hits 1957 2029 +72
- Misses 4640 4797 +157
Partials 117 117 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@auth/additional_checks.go`:
- Around line 48-69: The CheckAudienceClaim function should guard against an
expectedAudience with an empty Host (caused by a scheme-less URL); normalize or
validate expectedAudience at the start of CheckAudienceClaim by ensuring it has
a scheme (e.g., if expectedAudience.Scheme == "" prepend "http://" and re-parse
or construct a url.URL) and return an error if normalization fails, or document
the requirement—update the function to use the normalized expectedAudience.Host
for comparisons so host checks never compare against an empty string.
🧹 Nitpick comments (4)
auth/agent_test.go (1)
22-30: Consider adding edge case tests forGetOrganizationID.The test covers the happy path. Consider adding cases for:
nilClaims map (should return empty string)- Non-string
organization_idvalue (should return empty string)Example edge case tests
func TestDefaultAgent_GetOrganizationID_NilClaims(t *testing.T) { t.Parallel() claims := oidc.AccessTokenClaims{ Claims: nil, } agent := auth.NewDefaultAgent(claims) assert.Equal(t, "", agent.GetOrganizationID()) } func TestDefaultAgent_GetOrganizationID_NonStringValue(t *testing.T) { t.Parallel() claims := oidc.AccessTokenClaims{ Claims: map[string]interface{}{"organization_id": 123}, } agent := auth.NewDefaultAgent(claims) assert.Equal(t, "", agent.GetOrganizationID()) }auth/additional_checks.go (1)
56-58: Consider handlinghttps://explicitly in the comment.The check
!strings.HasPrefix(audienceStr, "http")correctly handles bothhttp://andhttps://schemes. The inline comment could be slightly clearer.Minor comment clarification
- if !strings.HasPrefix(audienceStr, "http") { // urlParse requires a scheme + if !strings.HasPrefix(audienceStr, "http") { // url.Parse requires a scheme (handles both http:// and https://)auth/middleware.go (1)
11-14: Consider using typed context keys to prevent collisions.String-based context keys can collide with keys from other packages. Using unexported typed keys is more idiomatic in Go.
Example using typed context keys
+type contextKey string + const ( - ContextKeyAuthClaimOrganizationID = "AuthClaim-OrganizationID" - ContextKeyAuthClaimClientID = "AuthClaim-ClientID" + ContextKeyAuthClaimOrganizationID contextKey = "AuthClaim-OrganizationID" + ContextKeyAuthClaimClientID contextKey = "AuthClaim-ClientID" )auth/additional_checks_test.go (1)
81-90: Consider enabling parallel test execution and adding edge cases.The test is well-structured. A few optional improvements:
- Add
t.Parallel()for faster test execution- Consider adding a test case for empty audience list
Suggested improvements
for name, tt := range tests { t.Run(name, func(t *testing.T) { + t.Parallel() expectedAudience, err := url.Parse(tt.expectedAudienceStr)Additional test case for empty audience:
"EmptyAudienceList": { expectedAudienceStr: "http://example.com", claims: &oidc.AccessTokenClaims{ TokenClaims: oidc.TokenClaims{ Audience: []string{}, }, }, expectedError: oidc.ErrAudience, },
gfyrag
left a comment
There was a problem hiding this comment.
Starting to think we should have a "go-libs-controlplane" repository...
Feels like we are mixing different stack in the same bag.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@auth/auth.go`:
- Around line 36-40: JWTAuth.authenticate currently returns nil on any
ClaimsFromRequest error, discarding claims that may accompany
issuer/signature/expiration validation failures; modify authenticate to capture
the claims returned by ClaimsFromRequest, build the ControlPlaneAgent (using
those claims) even when ClaimsFromRequest returns an error, and return that
agent together with the error so callers can access claims for context/logging
(only return nil agent for unrecoverable header/format errors—detect those
separately and keep existing nil behavior). Ensure changes are made in the
JWTAuth.authenticate function and use the returned claims from ClaimsFromRequest
when constructing the agent.
🧹 Nitpick comments (3)
auth/middleware.go (1)
11-14: Prefer typed context keys to avoid collisions.
Using plain strings risks collisions with other context values. A small typed key keeps it safer and idiomatic.♻️ Suggested change
+type contextKey string + const ( - ContextKeyAuthClaimOrganizationID = "AuthClaim-OrganizationID" - ContextKeyAuthClaimClientID = "AuthClaim-ClientID" + ContextKeyAuthClaimOrganizationID contextKey = "AuthClaim-OrganizationID" + ContextKeyAuthClaimClientID contextKey = "AuthClaim-ClientID" )auth/control_plane_agent.go (1)
22-27: Align org ID extraction with existing helper.
Reusingoidc.OrganizationAwareAccessTokenClaimskeeps semantics consistent withCheckOrganizationIDClaim.♻️ Suggested change
func (a DefaultControlPlaneAgent) GetOrganizationID() string { - organizationID := a.claims.Claims["organization_id"] - if organizationIDStr, ok := organizationID.(string); ok { - return organizationIDStr - } - return "" + claims := &oidc.OrganizationAwareAccessTokenClaims{AccessTokenClaims: a.claims} + return claims.GetOrganizationID() }auth/module.go (1)
32-40: Guard empty annotationTag to avoid name:"" collisions.
If callers pass an empty tag, fall back to the unannotated module to prevent ambiguous naming.♻️ Suggested change
func AnnotatedModule(cfg ModuleConfig, annotationTag string) fx.Option { + if annotationTag == "" { + return Module(cfg) + } nameAnnotation := `name:"` + annotationTag + `"` options := ModuleOptions(nameAnnotation)
Relates to EN-118