Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH Refactor password reset to support SameSite=Strict #11566

Draft
wants to merge 1 commit into
base: 5
Choose a base branch
from
Draft

ENH Refactor password reset to support SameSite=Strict #11566

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/Security/MemberAuthenticator/ChangePasswordForm.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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): ChangePasswordForm
{
$this->fields->push(HiddenField::create('AutoLoginHash', 'AutoLoginHash', $hash));

return $this;
}
}
62 changes: 21 additions & 41 deletions src/Security/MemberAuthenticator/ChangePasswordHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
'<p>' . _t(
Expand All @@ -91,10 +83,12 @@ public function changepassword()
) . '</p>'
);

// 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,
];
}

Expand All @@ -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
Expand Down Expand Up @@ -144,23 +138,6 @@ public function changepassword()
);
}


/**
* @param Member $member
* @param string $token
*/
protected function setSessionToken($member, $token)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't remove protected functions in a minor release, as they count as part of our "Public API" - see https://docs.silverstripe.org/en/5/project_governance/public_api/

{
// 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
Expand Down Expand Up @@ -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')));
}
}
Expand All @@ -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
Expand All @@ -254,15 +228,15 @@ 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
$validationResult = $member->changePassword($data['NewPassword1']);
if (!$validationResult->isValid()) {
$form->setSessionValidationResult($validationResult);

return $this->redirectBackToForm();
return $this->redirectBackToForm($member->ID, $data['AutoLoginHash']);
}

// Clear locked out status
Expand Down Expand Up @@ -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
Expand All @@ -319,10 +291,18 @@ public function doChangePassword(array $data, $form)
*
* @return HTTPResponse
*/
public function redirectBackToForm()
public function redirectBackToForm(?int $withMemberID = null, ?string $withToken = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't amend public method signatures in a minor release, as they count as part of our "Public API" - see https://docs.silverstripe.org/en/5/project_governance/public_api/

{
// 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);
}
Expand Down
49 changes: 48 additions & 1 deletion tests/php/Security/MemberAuthenticator/ChangePasswordHandlerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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&#039;re logged in',
$result->getBody(),
'User is logged in'
);
}
}
1 change: 0 additions & 1 deletion tests/php/Security/MemberAuthenticator/ChangePasswordHandlerTest.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,3 @@ SilverStripe\Security\Member:
sarah:
FirstName: Sarah
Surname: Smith
AutoLoginToken: foobar
8 changes: 1 addition & 7 deletions tests/php/Security/SecurityTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,13 +510,7 @@ public function testChangePasswordFromLostPassword()

// Check.
$response = $this->get('Security/changepassword/?m=' . $admin->ID . '&t=' . $token);
$this->assertEquals(302, $response->getStatusCode());
$this->assertEquals(
Director::absoluteURL('Security/changepassword'),
Director::absoluteURL((string) $response->getHeader('Location'))
);
// Follow redirection to form without hash in GET parameter
$this->get('Security/changepassword');
$this->assertEquals(200, $response->getStatusCode());
$this->doTestChangepasswordForm('1nitialPassword', 'changedPassword#123');
$this->assertEquals($this->idFromFixture(Member::class, 'test'), $this->session()->get('loggedInAs'));

Expand Down
Loading