feat: support for oidc max age#949
Open
steveiliop56 wants to merge 3 commits into
Open
Conversation
dosubot
Bot
added
the
size:L
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds OIDC ChangesOIDC prompt enforcement and auth_time propagation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
internal/service/oidc_service.go (1)
938-949:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
max_ageis not extracted from JWT claims.
DecodeAuthorizeJWTextractspromptbut notmax_age. If a client sends the authorize request as a JWT (request object), themax_ageclaim will be ignored whilereq.MaxAgeremains empty.🔧 Proposed fix
return &AuthorizeRequest{ Scope: get("scope"), ResponseType: get("response_type"), ClientID: get("client_id"), RedirectURI: get("redirect_uri"), State: get("state"), Nonce: get("nonce"), CodeChallenge: get("code_challenge"), CodeChallengeMethod: get("code_challenge_method"), Prompt: get("prompt"), + MaxAge: get("max_age"), }, nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/oidc_service.go` around lines 938 - 949, The DecodeAuthorizeJWT function is missing extraction of the max_age claim from the JWT when constructing the AuthorizeRequest struct. Add a new field MaxAge to the AuthorizeRequest return statement and populate it using the get() method with the parameter "max_age", following the same pattern used for other JWT claims like prompt, state, and nonce.
🧹 Nitpick comments (1)
internal/service/oidc_service.go (1)
676-678: ⚡ Quick winConsider OIDC spec compliance for
auth_timeduring refresh.Setting
auth_timeto0emits"auth_time": 0in the ID token (Unix epoch). Per OIDC Core spec,auth_timeshould reflect the original authentication time. Consider either:
- Persisting
auth_timein the OIDC session DB and retrieving it during refresh- Omitting
auth_timeentirely from refreshed tokens (useomitemptybehavior)The current approach could confuse relying parties that validate
auth_time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/oidc_service.go` around lines 676 - 678, The generateIDToken call in the refresh token flow is passing a hardcoded auth_time value of 0, which violates OIDC spec compliance. Either persist the original auth_time value when the OIDC session is first created and stored in the entry object, then retrieve and pass that value during refresh instead of 0, OR modify the generateIDToken function and the underlying token generation logic to support omitting auth_time entirely from the JWT when it's not available (using omitempty struct tags). This ensures that refreshed ID tokens either contain the original authentication time or exclude the field entirely rather than emitting a zero Unix timestamp.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/oidc_controller.go`:
- Around line 222-242: The code accesses userContext.Authenticated and
userContext.AuthTime in the max_age validation block without checking if
userContext is nil or in a valid state. If NewFromGin returns an error other
than ErrUserContextNotFound, userContext could be nil or partially initialized,
leading to a nil dereference. Add a nil check for userContext before the
condition that checks userContext.Authenticated to ensure the userContext is
properly initialized before accessing its fields.
---
Outside diff comments:
In `@internal/service/oidc_service.go`:
- Around line 938-949: The DecodeAuthorizeJWT function is missing extraction of
the max_age claim from the JWT when constructing the AuthorizeRequest struct.
Add a new field MaxAge to the AuthorizeRequest return statement and populate it
using the get() method with the parameter "max_age", following the same pattern
used for other JWT claims like prompt, state, and nonce.
---
Nitpick comments:
In `@internal/service/oidc_service.go`:
- Around line 676-678: The generateIDToken call in the refresh token flow is
passing a hardcoded auth_time value of 0, which violates OIDC spec compliance.
Either persist the original auth_time value when the OIDC session is first
created and stored in the entry object, then retrieve and pass that value during
refresh instead of 0, OR modify the generateIDToken function and the underlying
token generation logic to support omitting auth_time entirely from the JWT when
it's not available (using omitempty struct tags). This ensures that refreshed ID
tokens either contain the original authentication time or exclude the field
entirely rather than emitting a zero Unix timestamp.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 30182146-d821-4934-bb60-ff56143d5be2
📒 Files selected for processing (6)
frontend/src/lib/hooks/screen-params.tsfrontend/src/pages/authorize-page.tsxfrontend/src/pages/login-page.tsxinternal/controller/oidc_controller.gointernal/model/context.gointernal/service/oidc_service.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on #948
Summary by CodeRabbit
promptparameter (none/login) to control authentication flow, including silent auth when requested.