feat: add configuration to skip copying assertion audience in JWT bearer grant type#4076
feat: add configuration to skip copying assertion audience in JWT bearer grant type#4076elatt wants to merge 14 commits intoory:masterfrom
Conversation
…ving pointer Agent-Logs-Url: https://github.com/datarobot-forks/hydra/sessions/b002d7b7-a0e5-4e55-b97d-4ab7d3450d04 Co-authored-by: elatt <2617872+elatt@users.noreply.github.com>
…plain bool GrantTypeJWTBearerOmitAssertionAudience Agent-Logs-Url: https://github.com/datarobot-forks/hydra/sessions/b002d7b7-a0e5-4e55-b97d-4ab7d3450d04 Co-authored-by: elatt <2617872+elatt@users.noreply.github.com>
Agent-Logs-Url: https://github.com/datarobot-forks/hydra/sessions/2f217798-4d49-45a8-b852-44b3bf5e678c Co-authored-by: elatt <2617872+elatt@users.noreply.github.com>
…g-logic refactor: replace *bool GrantTypeJWTBearerCopyAssertionAudience with plain bool GrantTypeJWTBearerOmitAssertionAudience
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis change introduces a new configuration flag Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fosite/fosite.go (1)
101-112:⚠️ Potential issue | 🟠 MajorAvoid widening
Configurator; this is a public breaking change.Adding
GrantTypeJWTBearerOmitAssertionAudienceProviderhere will make any downstream customfosite.Configuratorimplementation fail to compile on upgrade. Prefer checking for this capability via an optional interface inrfc7523.Handlerso existing configurators remain source-compatible.Compatibility-preserving sketch
--- a/fosite/fosite.go +++ b/fosite/fosite.go type Configurator interface { IDTokenIssuerProvider IDTokenLifespanProvider AllowedPromptsProvider EnforcePKCEProvider EnforcePKCEForPublicClientsProvider EnablePKCEPlainChallengeMethodProvider GrantTypeJWTBearerCanSkipClientAuthProvider GrantTypeJWTBearerIDOptionalProvider GrantTypeJWTBearerIssuedDateOptionalProvider - GrantTypeJWTBearerOmitAssertionAudienceProvider GetJWTMaxDurationProvider AudienceStrategyProvider ScopeStrategyProvider ... }// fosite/handler/rfc7523/handler.go type omitAssertionAudienceProvider interface { GetGrantTypeJWTBearerOmitAssertionAudience(ctx context.Context) bool } omitAudience := false if p, ok := c.Config.(omitAssertionAudienceProvider); ok { omitAudience = p.GetGrantTypeJWTBearerOmitAssertionAudience(ctx) } if !omitAudience { for _, audience := range claims.Audience { request.GrantAudience(audience) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fosite/fosite.go` around lines 101 - 112, The Configurator interface was widened by adding GrantTypeJWTBearerOmitAssertionAudienceProvider which breaks downstream implementers; remove that addition and instead perform a capability check inside the rfc7523.Handler: define a local optional interface (e.g., omitAssertionAudienceProvider with GetGrantTypeJWTBearerOmitAssertionAudience(ctx context.Context) bool), detect it via a type assertion against c.Config, set omitAudience accordingly, and only call request.GrantAudience for each entry in claims.Audience when omitAudience is false (use the existing symbols Configurator, rfc7523.Handler, omitAssertionAudienceProvider, GetGrantTypeJWTBearerOmitAssertionAudience, claims.Audience, request.GrantAudience to locate and implement the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fosite/handler/rfc7523/handler_test.go`:
- Line 783: Update the failing test's description string to reflect the renamed
config: replace the text "CopyAssertionAudience" with
"GrantTypeJWTBearerOmitAssertionAudience" in the test case message so it reads
e.g. "audience from assertion JWT should NOT be copied when
GrantTypeJWTBearerOmitAssertionAudience is false"; locate the string in the test
in handler_test.go (the test that references the
GrantTypeJWTBearerOmitAssertionAudience config) and change only the message to
match the current config name.
---
Outside diff comments:
In `@fosite/fosite.go`:
- Around line 101-112: The Configurator interface was widened by adding
GrantTypeJWTBearerOmitAssertionAudienceProvider which breaks downstream
implementers; remove that addition and instead perform a capability check inside
the rfc7523.Handler: define a local optional interface (e.g.,
omitAssertionAudienceProvider with
GetGrantTypeJWTBearerOmitAssertionAudience(ctx context.Context) bool), detect it
via a type assertion against c.Config, set omitAudience accordingly, and only
call request.GrantAudience for each entry in claims.Audience when omitAudience
is false (use the existing symbols Configurator, rfc7523.Handler,
omitAssertionAudienceProvider, GetGrantTypeJWTBearerOmitAssertionAudience,
claims.Audience, request.GrantAudience to locate and implement the change).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b57bef9-9d95-4ade-b014-074e28e79f76
📒 Files selected for processing (9)
.schema/config.schema.jsondriver/config/provider.godriver/config/provider_test.gofosite/config.gofosite/config_default.gofosite/fosite.gofosite/handler/rfc7523/handler.gofosite/handler/rfc7523/handler_test.gospec/config.json
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
fosite/handler/rfc7523/handler_test.go (1)
737-755: Make this audience fixture explicit in the test to reduce coupling.This test currently depends on
createStandardClaim()’s shared defaultAudience. Consider settingcl.Audienceexplicitly here so this behavior test doesn’t break if that helper changes for unrelated cases.♻️ Proposed refactor
cl := s.createStandardClaim() + cl.Audience = jwt.Audience{"https://www.example.com/token", "leela", "fry"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fosite/handler/rfc7523/handler_test.go` around lines 737 - 755, The test relies on a shared default Audience from createStandardClaim(); make the fixture explicit by setting cl.Audience directly after cl := s.createStandardClaim() (e.g., cl.Audience = []string{"https://www.example.com/token", "leela", "fry"}) before calling s.createTestAssertion and HandleTokenEndpointRequest, so the assertion JWT audience is deterministic and the assertion of s.accessRequest.GetGrantedAudience() remains stable even if createStandardClaim() changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@fosite/handler/rfc7523/handler_test.go`:
- Around line 737-755: The test relies on a shared default Audience from
createStandardClaim(); make the fixture explicit by setting cl.Audience directly
after cl := s.createStandardClaim() (e.g., cl.Audience =
[]string{"https://www.example.com/token", "leela", "fry"}) before calling
s.createTestAssertion and HandleTokenEndpointRequest, so the assertion JWT
audience is deterministic and the assertion of
s.accessRequest.GetGrantedAudience() remains stable even if
createStandardClaim() changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f23ea16f-3b5b-456b-b012-2bfd6a5ec2df
📒 Files selected for processing (1)
fosite/handler/rfc7523/handler_test.go
…tor; use optional capability check in rfc7523 handler Agent-Logs-Url: https://github.com/datarobot-forks/hydra/sessions/a6005422-a68d-416f-ac57-9227f69af433 Co-authored-by: elatt <2617872+elatt@users.noreply.github.com>
…tor; use optional capability check in rfc7523 handler Agent-Logs-Url: https://github.com/datarobot-forks/hydra/sessions/a6005422-a68d-416f-ac57-9227f69af433 Co-authored-by: elatt <2617872+elatt@users.noreply.github.com>
…on-grant-type-jwt-bearer-omit-au Remove GrantTypeJWTBearerOmitAssertionAudienceProvider from Configurator; use optional capability check in rfc7523 handler
Agent-Logs-Url: https://github.com/datarobot-forks/hydra/sessions/feb1a66f-5a5f-4c1a-83ef-7caa142abd6b Co-authored-by: elatt <2617872+elatt@users.noreply.github.com>
…anges-go-mod Revert spurious go.mod and go.sum changes from merge commit
Introduce a configuration option to control whether the audience from the assertion JWT is copied into the resulting access token in the JWT bearer grant type. By default, this behavior remains enabled. The implementation includes updates to the configuration interface, handler logic, and tests to ensure expected functionality.
Summary by CodeRabbit
New Features
Configuration
oauth2.grant.jwt.omit_assertion_audienceboolean setting available for OAuth2 JWT-bearer grant configuration.Tests