fix(consent): use GetLoginSession in logout handlers to handle remember=false sessions - #4119
fix(consent): use GetLoginSession in logout handlers to handle remember=false sessions#4119waterWang wants to merge 3 commits into
Conversation
|
|
📝 WalkthroughWalkthroughThe login manager and SQL persister now support lookup of any login session by ID. RP-initiated and headless logout use this lookup instead of requiring a remembered session. ChangesLogout session lookup
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
persistence/sql/persister_consent.go (1)
243-255: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd regression coverage for non-remembered sessions.
Line [248] intentionally removes the
remember = TRUEpredicate. Add coverage that persists aremember = falsesession, verifiesGetLoginSessionreturns it, and confirms the RP-initiated and headless logout paths use it.🤖 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 `@persistence/sql/persister_consent.go` around lines 243 - 255, Add regression tests around Persister.GetLoginSession that persist a session with remember set to false and verify it is returned successfully. Extend coverage for both RP-initiated and headless logout flows to confirm they retrieve and use this non-remembered session, while preserving existing remembered-session behavior.
🤖 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.
Nitpick comments:
In `@persistence/sql/persister_consent.go`:
- Around line 243-255: Add regression tests around Persister.GetLoginSession
that persist a session with remember set to false and verify it is returned
successfully. Extend coverage for both RP-initiated and headless logout flows to
confirm they retrieve and use this non-remembered session, while preserving
existing remembered-session behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e975ad0b-60c7-495b-8766-5176734b67c5
📒 Files selected for processing (3)
consent/manager.goconsent/strategy_default.gopersistence/sql/persister_consent.go
Fixes #3132
Problem
When a user logs in with
remember: false, the login session is persisted inhydra_oauth2_authentication_session(withremember = FALSE), but the logout handler usesGetRememberedLoginSessionwhich has aWHERE remember = TRUEfilter. This causes two issues:RP-initiated logout with
id_token_hint:issueLogoutVerifierat line 942 callsGetRememberedLoginSession(hintSid), getsErrNotFound, and silently redirects topost_logout_redirect_uriwithout any front-/back-channel logout — even though the ID token hint contains a validsidthat identifies the session.Admin headless logout:
HandleHeadlessLogoutat line 1099 callsGetRememberedLoginSession(sid), getsErrNotFound, and returns a silent 204 no-op — even though the session row exists.Solution
Add a new
GetLoginSessionmethod to theLoginManagerinterface and its SQL persistence implementation — identical toGetRememberedLoginSessionbut without theWHERE remember = TRUEfilter. Use it at the two logout call sites above.The filtered variant (
GetRememberedLoginSession) is kept for the cookie-based SSO lookup at line 103, whereremember=falsesessions should indeed not enable SSO skip.Changes
consent/manager.go: AddGetLoginSession(ctx, id)to theLoginManagerinterfacepersistence/sql/persister_consent.go: ImplementGetLoginSession(same asGetRememberedLoginSessionminus theWHERE remember = TRUEfilter)consent/strategy_default.go:issueLogoutVerifier(line 942): useGetLoginSessioninstead ofGetRememberedLoginSessionHandleHeadlessLogout(line 1099): useGetLoginSessioninstead ofGetRememberedLoginSessionGetRememberedLoginSessionRoot cause analysis credit
Detailed analysis by @fkammer in the issue comments confirmed the fix is minimal — no schema changes, no API changes needed.
Summary by CodeRabbit