Make id_token_hint optional for RP-initiated logout - #4299
Conversation
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughAdds RP-initiated logout confirmation when ChangesLogout confirmation flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RP
participant LogoutService
participant SessionSignOutExecutor
participant ConfirmationPrompt
participant SSO
RP->>LogoutService: Submit logout request without id_token_hint
LogoutService->>SessionSignOutExecutor: Start flow with prompt required
SessionSignOutExecutor->>ConfirmationPrompt: Request user confirmation
ConfirmationPrompt->>SessionSignOutExecutor: Confirm sign-out
SessionSignOutExecutor->>SSO: Terminate session
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
madurangasiriwardena
added
Type/Improvement
trigger-pr-builder
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
madurangasiriwardena
removed
the
trigger-pr-builder
1de09e8 to
633c296
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/internal/oauth/oauth2/logout/service.go (1)
187-236: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win🔴 Documentation Required
This PR introduces user-facing changes that are not covered by documentation updates underdocs/.
Please update the relevant documentation before merging.Missing documentation:
backend/internal/oauth/oauth2/logout/service.go#L187-L236: Document optionalid_token_hint,client_id-based logout resolution, confirmation behavior, and redirect validation indocs/content/apis.mdx.frontend/apps/console/src/features/login-flow/data/executors.json#L948-L950: DocumentpromptOnSignOutand the conditional sign-out flow/template in an appropriatedocs/content/guides/page.🤖 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 `@backend/internal/oauth/oauth2/logout/service.go` around lines 187 - 236, Document the new sign-out behavior in docs/content/apis.mdx for logoutService.Resolve: explain that id_token_hint is optional, client_id can resolve the client, missing hints require user confirmation, and post_logout_redirect_uri must match the client’s registered redirects. Also document promptOnSignOut and its conditional sign-out flow/template in an appropriate docs/content/guides/ page, covering the executor configuration at frontend/apps/console/src/features/login-flow/data/executors.json lines 948-950.Source: Path instructions
🤖 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.
Outside diff comments:
In `@backend/internal/oauth/oauth2/logout/service.go`:
- Around line 187-236: Document the new sign-out behavior in
docs/content/apis.mdx for logoutService.Resolve: explain that id_token_hint is
optional, client_id can resolve the client, missing hints require user
confirmation, and post_logout_redirect_uri must match the client’s registered
redirects. Also document promptOnSignOut and its conditional sign-out
flow/template in an appropriate docs/content/guides/ page, covering the executor
configuration at
frontend/apps/console/src/features/login-flow/data/executors.json lines 948-950.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a8438114-bf6d-4084-85f9-5adcfa0b930f
📒 Files selected for processing (15)
backend/internal/flow/common/constants.gobackend/internal/flow/executor/constants.gobackend/internal/flow/executor/session_signout_executor.gobackend/internal/flow/executor/session_signout_executor_test.gobackend/internal/oauth/oauth2/logout/service.gobackend/internal/oauth/oauth2/logout/service_test.gofrontend/apps/console/src/features/flows/data/templates.jsonfrontend/apps/console/src/features/flows/models/steps.tsfrontend/apps/console/src/features/flows/utils/__tests__/resolveStepMetadata.test.tsfrontend/apps/console/src/features/login-flow/components/resource-property-panel/extended-properties/ExecutionExtendedProperties.tsxfrontend/apps/console/src/features/login-flow/components/resource-property-panel/extended-properties/__tests__/ExecutionExtendedProperties.test.tsxfrontend/apps/console/src/features/login-flow/components/resource-property-panel/extended-properties/execution-properties/SessionSignOutProperties.tsxfrontend/apps/console/src/features/login-flow/components/resource-property-panel/extended-properties/execution-properties/constants.tsfrontend/apps/console/src/features/login-flow/data/executors.jsonfrontend/packages/i18n/src/locales/en-US.ts
| // RuntimeKeyLogoutPromptShown is the session sign-out node's own guard, set when it routes to the | ||
| // confirmation prompt so that on re-run (after the user confirms) it terminates instead of | ||
| // prompting again. | ||
| RuntimeKeyLogoutPromptShown = "logoutPromptShown" |
There was a problem hiding this comment.
We have a type field in the prompt node action that can be used to indicate a metadata about the action edge. We should be able to improve this logic and get rid of this runtime data key by utilizing that.
Let's do it as a followup
ThaminduDilshan
added
the
trigger-pr-builder
633c296 to
4c26526
Compare
Drop the requirement that id_token_hint accompany post_logout_redirect_uri. A logout carrying only client_id (with or without a registered post_logout_redirect_uri) now resolves; the redirect target is still confirmed against the client's registered allow-list, satisfying the OIDC RP-Initiated Logout requirement for confirming the redirection target. When no valid id_token_hint is supplied, the OAuth layer marks the sign-out flow with logoutPromptRequired. SessionSignOutExecutor reads it and, when its node opts in via the promptOnSignOut property, routes to the node's onIncomplete confirmation prompt before terminating the session, prompting once per flow run. Nodes that do not opt in terminate as before, leaving the always-prompt default sign-out flow unchanged. Add a Confirm Sign Out Without a Hint flow-builder template that wires this behavior with builder-native constructs (an onIncomplete edge and the promptOnSignOut property) so it renders and round-trips in the flow editor. Expose promptOnSignOut as an editable checkbox on the End Session executor in the Console flow editor, mirroring how other executors surface node properties. The property is a boolean, consistent with the provisioning executor's options.
4c26526 to
35dfe14
Compare
3 participants
Purpose
Makes id_token_hint optional for OIDC RP-Initiated Logout. Previously the logout endpoint rejected any request that supplied post_logout_redirect_uri without an id_token_hint.
Per the RP-Initiated Logout spec, the OP may perform post-logout redirection without an id_token_hint as long as it confirms the legitimacy of the redirection target by other means, and per RPLogout it MUST ask the End-User to confirm the logout when no id_token_hint was provided. This PR implements both: a logout carrying only client_id now resolves (the redirect target is still validated against the client's registered allow-list), and when no valid hint is present the sign-out flow can prompt the user to confirm before the session is terminated.
A new "Confirm Sign Out Without a Hint" flow-builder template wires up this conditional-confirmation behavior.
Approach
Prompt logout consent always
Prompt logout consent when id_token_hint is not provided
Available flow templates
Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit