Fix OIDC auth fallback when primary authenticator errors#635
Conversation
The CompositeAuthenticator was short-circuiting on primary errors without trying the OIDC authenticator. When miren.cloud's JWT validator received a GitHub Actions OIDC token, it failed to find the token's key ID in cloud's JWKS and returned an error, which prevented the OIDC authenticator from ever being tried. Now the composite always tries OIDC regardless of whether the primary errored. If OIDC succeeds, its identity is returned. If both fail, the primary error is returned.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCompositeAuthenticator was updated: its Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/oidcauth/composite_test.go`:
- Around line 72-109: The test
TestCompositeAuthenticator_PrimaryErrorOIDCSucceeds is misnamed and currently
asserts the error path; update it to actually verify OIDC fallback by injecting
a stub OIDC authenticator that implements Authenticate and returns a successful
rpc.Identity (e.g., Subject "repo:acme/app:ref:refs/heads/main" and Method
rpc.AuthMethodOIDC) so that when primary (mockAuthenticator with err "key with
ID xyz not found") fails, CompositeAuthenticator returns the OIDC identity and
no error; replace the noisy comments and the unused oidcMock with this stub,
construct comp with the stub as the oidc field (use CompositeAuthenticator or
NewCompositeAuthenticator as appropriate), call comp.Authenticate and assert
err==nil and the returned identity matches OIDC values.
In `@pkg/oidcauth/composite.go`:
- Around line 68-78: Guard access to c.oidc before calling its Authenticate
method: in the block where oidcIdentity, oidcErr := c.oidc.Authenticate(ctx, r)
is invoked, first check if c.oidc != nil and only call c.oidc.Authenticate when
non-nil; if c.oidc is nil set oidcIdentity = nil and oidcErr = nil (or a clear
sentinel like ErrNoOIDCConfigured if you prefer) so the existing return logic
that prefers the primary err still works and no panic occurs; reference c.oidc,
Authenticate, oidcIdentity, and oidcErr to locate the change.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
pkg/oidcauth/composite.gopkg/oidcauth/composite_test.go
- Change CompositeAuthenticator.oidc from *OIDCAuthenticator to rpc.Authenticator so OIDC fallback can be tested with a stub - Guard c.oidc.Authenticate() with a nil check to prevent panic when oidc is unset - Fix TestCompositeAuthenticator_PrimaryErrorOIDCSucceeds to actually verify OIDC fallback returns a successful identity when primary errors
Summary
CompositeAuthenticatorto try the OIDC authenticator even when the primary authenticator returns an errorTest plan