Redirect authenticated users away from the password restore form - #1557
Open
LukeTowers wants to merge 1 commit into
Open
LukeTowers wants to merge 1 commit into
LukeTowers wants to merge 1 commit into
Conversation
The backend's password restore form is for signed-out users: it takes a login, sends that account a reset link, and reports the same thing either way. It has no notion of an acting user, while the code it calls does — `Backend\Models\User::getResetPasswordCode()` applies an authorization check when a user is signed in — so the form's behaviour could depend on whether a session was present, which it should not. Both `restore()` and `restore_onSubmit()` now redirect a signed-in user to the backend instead. The guard is in both methods because `Backend\Classes\Controller::run()` dispatches AJAX handlers before the page action, so `restore_onSubmit` is reachable as the `onSubmit` handler on the `restore` action without `restore()` running at all. Behaviour changes: - A signed-in backend user who opens `backend/auth/restore`, or posts to it, is redirected to the backend rather than served the form. Signing out first still reaches it, which is the flow the form is for, and a user who wants to change their own password uses My Settings. Tests: `Backend\Tests\Controllers\AuthRestoreOracleTest` — `testAuthenticatedRestoreDoesNotRevealAccountExistence` and `testAuthenticatedRestoreAjaxHandlerDoesNotRevealAccountExistence` fail without the change; `testGuestRestoreResponseIsIdenticalForKnownAndUnknownLogins`, `testGuestRestoreStillIssuesACodeForAKnownAccount` and `testResetEndpointIsIdenticalForKnownAndUnknownUserIds` are controls that pass either way. `modules/backend` is green at 301 tests / 718 assertions and `phpcs` is clean.
|
Warning Review limit reachedNext included review available in 12 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
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 |
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.
The backend's password restore form is for signed-out users: it takes a login, sends that account a reset link, and reports the same thing either way. It has no notion of an acting user, while the code it calls does —
Backend\Models\User::getResetPasswordCode()applies an authorization check when a user is signed in — so the form's behaviour could depend on whether a session was present, which it should not. Bothrestore()andrestore_onSubmit()now redirect a signed-in user to the backend instead.The guard is in both methods because
Backend\Classes\Controller::run()dispatches AJAX handlers before the page action, sorestore_onSubmitis reachable as theonSubmithandler on therestoreaction withoutrestore()running at all.Behaviour changes
backend/auth/restore, or posts to it, is redirected to the backend rather than served the form. Signing out first still reaches it, which is the flow the form is for, and a user who wants to change their own password uses My Settings.Tests
Backend\Tests\Controllers\AuthRestoreOracleTest.testAuthenticatedRestoreDoesNotRevealAccountExistenceandtestAuthenticatedRestoreAjaxHandlerDoesNotRevealAccountExistencefail without the change.testGuestRestoreResponseIsIdenticalForKnownAndUnknownLogins,testGuestRestoreStillIssuesACodeForAKnownAccountandtestResetEndpointIsIdenticalForKnownAndUnknownUserIdsare controls and pass either way.modules/backendis green at 301 tests / 718 assertions;phpcsis clean on both files.