Repository navigation
fix: preserve next redirect across OAuth session cleanup - #470
djoseph-apphelix wants to merge 2 commits into
Conversation
b5b9498 to
f069208
Compare
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues were identified for the change preserving the post-login redirect across logout.
No open issues 🎉
What changed in this PR
Preserves the intended post-login destination when EdXOAuth2.start() logs out an already-authenticated user, preventing session cleanup from losing social-auth’s stored next redirect.
Changes:
- Saves
nextbefore logout and restores it afterward without preserving other session data. - Adds a regression test that shares the strategy session with
request.session. - Documents the redirect fix in the changelog.
| File | Description |
|---|---|
| src/auth_backends/tests/test_backends.py | Tests that session cleanup preserves next while logging out the user. |
| src/auth_backends/backends.py | Restores the stored redirect after logout flushes the session. |
| CHANGELOG.rst | Records the fix under Unreleased. |
Review effort: Balanced
There was a problem hiding this comment.
I was wondering if there were other query parameters that we might also need to retain. I worked with Claude on that for a bit, and here is an AI comment based on my question. Note that openedx-platform uses this setting.
[UPDATED] I moved my comment to #471 so this could potentially land faster. I am going to add an approval to the PR as-is.
There was a problem hiding this comment.
@feanil: See my comment #470 (review). You'll need to decide if you want to approve and land this as-is, or wait until someone is able to do the larger fix. Thank you.
[UPDATE]: @djoseph-apphelix already fixed the new issue as part of this PR, so there is no longer a decision to be made around 1 vs 2 PRs.
|
FYI: @openedx/committers-auth-backends |
|
@openedx/committers-auth-backends: I presume (but am uncertain) that @djoseph-apphelix had support from AI. I passed on the AI policy and asked that that be made transparent (if needed), but hope that the bug fix doesn't get held up for that. This is a fix that probably should be backported, but not sure if that requires updating all the services? |
EdXOAuth2.start() logs out an already-authenticated user before starting a new OAuth flow. Django's logout() flushes the session, which also drops the sanitized `next` value social-auth stored one step earlier, so the user lands on LOGIN_REDIRECT_URL instead of the page they requested. Read `next` before the logout and write it back afterwards. The cleanup itself is unchanged. The existing test did not catch this because social-core's TestStrategy keeps its session in a private dict rather than request.session, so the flush never reached it. The new test points the strategy at request.session, matching DjangoStrategy. Coding assistant: Claude Code
social-auth's do_auth() stores every field named in FIELDS_STORED_IN_SESSION right before start(), at the same point it stores the sanitized next. The forced logout() flushed those too. Carry them across alongside next. No current EdXOAuth2 consumer sets FIELDS_STORED_IN_SESSION, so this is hardening for deployments that add fields later. Closes openedx#471 Coding assistant: Claude Code
764465b to
9a97775
Compare
Problem
Since 4.6.2,
EdXOAuth2.start()callslogout(request)when a user is already authenticated.logout()flushes the session, and social-auth had just stored the sanitizednextredirect in that session. The value is lost, anddo_complete()falls back toLOGIN_REDIRECT_URL.On Credentials,
LOGIN_REDIRECT_URLis the staff-only/api/v2/credentials/, so learners with an existing Credentials session who follow/login/?next=/credentials/<uuid>/get a 403 instead of their certificate. Learners with no session are unaffected, which made it hard to reproduce.Fix
Read
nextand every field named inFIELDS_STORED_IN_SESSIONfrom the strategy session beforelogout(), and set them again after. The cleanup itself is unchanged. Nothing else social-auth writes beforestart()is touched.The
FIELDS_STORED_IN_SESSIONpart covers #471.do_auth()stores those fields at the same point it storesnext, so the flush lost them too. No currentEdXOAuth2consumer sets that list (the LMS does, but it does not log in through this backend), so that half is hardening rather than a known production bug.nextis handled explicitly because social-auth stores it through a separate, sanitized path, and listing it inFIELDS_STORED_IN_SESSIONas well is harmless.Why the existing test passed
OAuth2Testuses social-core'sTestStrategy, whose session is a private dict, notrequest.session, sologout()cannot reach it. The new test points the strategy's session atrequest.session, mirroringDjangoStrategy. Without the fix it fails withNone != '/credentials/abc/', and without the second commit it fails withNone != 'login'on theauth_entryfield.Verification
pytest src/auth_backends/tests: 22 passedCloses #471
AI Disclosure
Claude (Claude Code) assisted with the root-cause investigation, the code and test changes, and drafting this description. I reproduced the bug and verified the fix on a Credentials local development environment (devstack) and reviewed every change.