diff --git a/src/Security/MemberAuthenticator/ChangePasswordForm.php b/src/Security/MemberAuthenticator/ChangePasswordForm.php index 4a2efa6cfab..9d7082e1baa 100644 --- a/src/Security/MemberAuthenticator/ChangePasswordForm.php +++ b/src/Security/MemberAuthenticator/ChangePasswordForm.php @@ -79,4 +79,14 @@ protected function getFormActions() return $actions; } + + /** + * Adds a hidden field to the form with the given autologin hash, to support the "forgot password" workflow. + */ + public function addAutoLoginHash(string $hash): self + { + $this->fields->push(HiddenField::create('AutoLoginHash', 'AutoLoginHash', $hash)); + + return $this; + } } diff --git a/src/Security/MemberAuthenticator/ChangePasswordHandler.php b/src/Security/MemberAuthenticator/ChangePasswordHandler.php index bd027ba8de5..bbffe25b2b9 100644 --- a/src/Security/MemberAuthenticator/ChangePasswordHandler.php +++ b/src/Security/MemberAuthenticator/ChangePasswordHandler.php @@ -75,14 +75,6 @@ public function changepassword() // Check whether we are merely changing password, or resetting. if ($token !== null && $member && $member->validateAutoLoginToken($token)) { - $this->setSessionToken($member, $token); - - // Redirect to myself, but without the hash in the URL - return $this->redirect($this->link); - } - - $session = $this->getRequest()->getSession(); - if ($session->get('AutoLoginHash')) { $message = DBField::create_field( 'HTMLFragment', '

' . _t( @@ -91,10 +83,12 @@ public function changepassword() ) . '

' ); - // Subsequent request after the "first load with hash" (see previous if clause). + $form = $this->changePasswordForm(); + $form->addAutoLoginHash($member->encryptWithUserSettings($token)); + return [ 'Content' => $message, - 'Form' => $this->changePasswordForm() + 'Form' => $form, ]; } @@ -110,7 +104,7 @@ public function changepassword() return [ 'Content' => $message, - 'Form' => $this->changePasswordForm() + 'Form' => $this->changePasswordForm(), ]; } // Show a friendly message saying the login token has expired @@ -144,23 +138,6 @@ public function changepassword() ); } - - /** - * @param Member $member - * @param string $token - */ - protected function setSessionToken($member, $token) - { - // if there is a current member, they should be logged out - if ($curMember = Security::getCurrentUser()) { - Injector::inst()->get(IdentityStore::class)->logOut(); - } - - $this->getRequest()->getSession()->regenerateSessionId(); - // Store the hash for the change password form. Will be unset after reload within the ChangePasswordForm. - $this->getRequest()->getSession()->set('AutoLoginHash', $member->encryptWithUserSettings($token)); - } - /** * Return a link to this request handler. * The link returned is supplied in the constructor @@ -215,16 +192,13 @@ public function doChangePassword(array $data, $form) return $this->redirectBackToForm(); } - $session = $this->getRequest()->getSession(); if (!$member) { - if ($session->get('AutoLoginHash')) { - $member = Member::member_from_autologinhash($session->get('AutoLoginHash')); + if (isset($data['AutoLoginHash'])) { + $member = Member::member_from_autologinhash($data['AutoLoginHash']); } - // The user is not logged in and no valid auto login hash is available + // The user is not logged in and no valid token was provided if (!$member) { - $session->clear('AutoLoginHash'); - return $this->redirect($this->addBackURLParam(Security::singleton()->Link('login'))); } } @@ -240,7 +214,7 @@ public function doChangePassword(array $data, $form) ); // redirect back to the form, instead of using redirectBack() which could send the user elsewhere. - return $this->redirectBackToForm(); + return $this->redirectBackToForm($member->ID, $data['AutoLoginHash']); } // Fail if passwords do not match @@ -254,7 +228,7 @@ public function doChangePassword(array $data, $form) ); // redirect back to the form, instead of using redirectBack() which could send the user elsewhere. - return $this->redirectBackToForm(); + return $this->redirectBackToForm($member->ID, $data['AutoLoginHash']); } // Check if the new password is accepted @@ -262,7 +236,7 @@ public function doChangePassword(array $data, $form) if (!$validationResult->isValid()) { $form->setSessionValidationResult($validationResult); - return $this->redirectBackToForm(); + return $this->redirectBackToForm($member->ID, $data['AutoLoginHash']); } // Clear locked out status @@ -293,8 +267,6 @@ public function doChangePassword(array $data, $form) $identityStore->logIn($member, false, $this->getRequest()); } - $session->clear('AutoLoginHash'); - // Redirect to backurl $backURL = $this->getBackURL(); if ($backURL @@ -319,10 +291,18 @@ public function doChangePassword(array $data, $form) * * @return HTTPResponse */ - public function redirectBackToForm() + public function redirectBackToForm(?int $withMemberID = null, ?string $withToken = null) { // Redirect back to form - $url = $this->addBackURLParam(Security::singleton()->Link('changepassword')); + $url = Security::singleton()->Link('changepassword'); + + // Include token data if performing an unauthenticated password reset + if ($withMemberID && $withToken) { + $url = Controller::join_links($url, "?m={$withMemberID}&t={$withToken}"); + } + + // Add Back URL if available + $url = $this->addBackURLParam($url); return $this->redirect($url); } diff --git a/tests/php/Security/MemberAuthenticator/ChangePasswordHandlerTest.php b/tests/php/Security/MemberAuthenticator/ChangePasswordHandlerTest.php index f318d01f3d5..fd040fb5758 100644 --- a/tests/php/Security/MemberAuthenticator/ChangePasswordHandlerTest.php +++ b/tests/php/Security/MemberAuthenticator/ChangePasswordHandlerTest.php @@ -10,8 +10,9 @@ use SilverStripe\Security\MemberAuthenticator\ChangePasswordHandler; use SilverStripe\Security\MemberAuthenticator\MemberAuthenticator; use SilverStripe\Security\Security; +use SilverStripe\Dev\FunctionalTest; -class ChangePasswordHandlerTest extends SapphireTest +class ChangePasswordHandlerTest extends FunctionalTest { protected static $fixture_file = 'ChangePasswordHandlerTest.yml'; @@ -46,4 +47,50 @@ public function testExpiredOrInvalidTokenProvidesLostPasswordAndLoginLink() $this->assertStringContainsString('Security/lostpassword', $result['Content'], 'Lost password URL is included'); $this->assertStringContainsString('Security/login', $result['Content'], 'Login URL is included'); } + + public function testLegitimateTokenLoadsChangePasswordForm() + { + $member = $this->objFromFixture(Member::class, 'sarah'); + $token = $member->generateAutologinTokenAndStoreHash(); + $hash = $member->AutoLoginHash; + + $request = new HTTPRequest('GET', '/Security/changepassword', [ + 'm' => $member->ID, + 't' => $token, + ]); + $request->setSession(new Session([])); + + /** @var ChangePasswordHandler $handler */ + $handler = $this->getMockBuilder(ChangePasswordHandler::class) + ->disableOriginalConstructor() + ->setMethods(null) + ->getMock(); + + $result = $handler->setRequest($request)->changepassword(); + + $this->assertIsArray($result, 'An array is returned'); + $this->assertArrayHasKey('Form', $result, 'Form is included'); + + $hashField = $result['Form']->HiddenFields()->dataFieldByName('AutoLoginHash') ?? null; + $this->assertIsObject($hashField, 'AutoLoginHash field is included'); + $this->assertEquals($hashField->value, $hash, 'AutoLoginHash field value is correct'); + } + + public function testSubmittingChangePasswordFormSucceedsWithValidToken() + { + $member = $this->objFromFixture(Member::class, 'sarah'); + $hash = $member->generateAutologinTokenAndStoreHash(); + + $this->get("/Security/changepassword?m={$member->ID}&t={$hash}"); + $result = $this->submitForm('ChangePasswordForm_ChangePasswordForm', null, [ + 'NewPassword1' => 'newpassword', + 'NewPassword2' => 'newpassword', + ]); + + $this->assertStringContainsString( + 'You're logged in', + $result->getBody(), + 'User is logged in' + ); + } } diff --git a/tests/php/Security/MemberAuthenticator/ChangePasswordHandlerTest.yml b/tests/php/Security/MemberAuthenticator/ChangePasswordHandlerTest.yml index 8eb8967ca3a..ff36d285860 100644 --- a/tests/php/Security/MemberAuthenticator/ChangePasswordHandlerTest.yml +++ b/tests/php/Security/MemberAuthenticator/ChangePasswordHandlerTest.yml @@ -2,4 +2,3 @@ SilverStripe\Security\Member: sarah: FirstName: Sarah Surname: Smith - AutoLoginToken: foobar