From a0d2e191757879884f3d39ce782852dc31ff71ec Mon Sep 17 00:00:00 2001 From: Dylan Wagstaff Date: Tue, 9 Jul 2019 16:00:49 +1200 Subject: [PATCH 1/5] FIX correctly redirect after successful log in The usage of the BackURL key in the request does not work with MFA as it operates over many reqeusts, interrupting the 'normal' operation (default member authenticator logic). MFA gets around this by storing the BackURL value in the session tied to the request - but this was being (correctly) cleared before the redirect is performed after a successful log in. This commit fixes this by making a short lived store in memory for this value, and is still able to be utilised by the redirect logic from the 'normal' operation. --- src/Authenticator/LoginHandler.php | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/Authenticator/LoginHandler.php b/src/Authenticator/LoginHandler.php index 6eeb23e8..595489e2 100644 --- a/src/Authenticator/LoginHandler.php +++ b/src/Authenticator/LoginHandler.php @@ -74,6 +74,11 @@ class LoginHandler extends BaseLoginHandler */ protected $logger; + /** + * @var string hold the back url for the duration of the request (being in memory) {@see getBackURL} + */ + protected $backURL = null; + /** * Override the parent "doLogin" to insert extra steps into the flow * @@ -457,6 +462,9 @@ public function redirectAfterSuccessfulLogin(): HTTPResponse return $this->redirect($this->getBackURL() ?: $loginUrl); } + // Store the back URL before clearing the session data + $this->backURL = $this->getBackURL(); + // Clear the "additional data" $request->getSession()->clear(static::SESSION_KEY . '.additionalData'); @@ -512,7 +520,9 @@ public function getLogger(): ?LoggerInterface } /** - * Adds another option for the back URL to be returned from a current MFA session store + * Adds more options for the back URL - to be returned from a current MFA session store or from memory during this + * request. The latter is required as session store is cleared before the 'BackURL' key is utilised + * {@see redirectAfterSuccessfulLogin} * * @return string|null */ @@ -527,7 +537,7 @@ public function getBackURL(): ?string } } - return $backURL; + return $backURL ?: $this->backURL; } /** From 2f55fe401f312868c0006837b77480eaf5b71338 Mon Sep 17 00:00:00 2001 From: Guy Marriott Date: Wed, 10 Jul 2019 14:33:44 +1200 Subject: [PATCH 2/5] FIX Partially revert #212 to restore correct redirection logic --- src/Authenticator/LoginHandler.php | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/Authenticator/LoginHandler.php b/src/Authenticator/LoginHandler.php index 595489e2..212c7766 100644 --- a/src/Authenticator/LoginHandler.php +++ b/src/Authenticator/LoginHandler.php @@ -74,11 +74,6 @@ class LoginHandler extends BaseLoginHandler */ protected $logger; - /** - * @var string hold the back url for the duration of the request (being in memory) {@see getBackURL} - */ - protected $backURL = null; - /** * Override the parent "doLogin" to insert extra steps into the flow * @@ -447,8 +442,8 @@ public function redirectAfterSuccessfulLogin(): HTTPResponse // This is potentially redundant logic as the member should only be logged in if they've fully registered. // They're allowed to login if they can skip - so only do assertions if they're not allowed to skip // We'll also check that they've registered the required MFA details - if (!$enforcementManager->hasCompletedRegistration($member) - && $enforcementManager->shouldRedirectToMFA($member) + if (!$enforcementManager->canSkipMFA($member) + && !$enforcementManager->hasCompletedRegistration($member) ) { // Log them out again /** @var IdentityStore $identityStore */ @@ -462,8 +457,8 @@ public function redirectAfterSuccessfulLogin(): HTTPResponse return $this->redirect($this->getBackURL() ?: $loginUrl); } - // Store the back URL before clearing the session data - $this->backURL = $this->getBackURL(); + // Redirecting after successful login expects a getVar to be set, store it before clearing the session data + $request['BackURL'] = $this->getBackURL(); // Clear the "additional data" $request->getSession()->clear(static::SESSION_KEY . '.additionalData'); @@ -537,7 +532,7 @@ public function getBackURL(): ?string } } - return $backURL ?: $this->backURL; + return $backURL; } /** From b9f783e4b932f2486dbc4d414c0fb5eaf500b615 Mon Sep 17 00:00:00 2001 From: Guy Marriott Date: Thu, 11 Jul 2019 15:26:16 +1200 Subject: [PATCH 3/5] Add tests for back url redirects --- tests/php/Authenticator/LoginHandlerTest.php | 124 +++++++++++++++++-- tests/php/Authenticator/LoginHandlerTest.yml | 22 +++- 2 files changed, 134 insertions(+), 12 deletions(-) diff --git a/tests/php/Authenticator/LoginHandlerTest.php b/tests/php/Authenticator/LoginHandlerTest.php index d3d70f95..21990171 100644 --- a/tests/php/Authenticator/LoginHandlerTest.php +++ b/tests/php/Authenticator/LoginHandlerTest.php @@ -2,6 +2,7 @@ namespace SilverStripe\MFA\Tests\Authenticator; +use Page; use PHPUnit_Framework_MockObject_MockObject; use SilverStripe\Control\Controller; use SilverStripe\Control\HTTPRequest; @@ -232,24 +233,77 @@ public function cannotSkipMFAProvider() ]; } - public function testSkipRegistration() + /** + * @param $memberFixture + * @param $expectedRedirect + * @dataProvider skipRegistrationProvider + */ + public function testSkipRegistration($memberFixture, $mfaRequiredInGrace = false, $expectedRedirect = null) { - $this->setSiteConfig(['MFARequired' => false]); + if ($mfaRequiredInGrace) { + $this->setSiteConfig([ + 'MFARequired' => true, + 'MFAGracePeriodExpires' => DBDatetime::now() + ->setValue(strtotime('+1 day', DBDatetime::now()->getTimestamp()))->Rfc2822() + ]); + } else { + $this->setSiteConfig(['MFARequired' => false]); + } + + if (!$expectedRedirect) { + $expectedRedirect = Controller::join_links(Security::login_url(), 'default'); + } - $member = new Member(); - $member->FirstName = 'Some new'; - $member->Surname = 'member'; - $memberId = $member->write(); - $this->logInAs($member); + /** @var Member $member */ + $member = $this->objFromFixture(Member::class, $memberFixture); + $this->scaffoldPartialLogin($member); + $this->autoFollowRedirection = false; $response = $this->get(Controller::join_links(Security::login_url(), 'default/mfa/skip')); - $this->assertSame(200, $response->getStatusCode()); + // Assert a redirect is given + $this->assertSame(302, $response->getStatusCode()); + + // Assert the redirect is to the expected location + $this->assertStringEndsWith($expectedRedirect, $response->getHeader('location')); - $member = Member::get()->byID($memberId); + // Assert the user is now logged in + $this->assertSame($member->ID, Security::getCurrentUser()->ID, 'User is successfully logged in'); + + // Assert that the member is tracked as having skipped registration + $member = Member::get()->byID($member->ID); $this->assertTrue((bool)$member->HasSkippedMFARegistration); } + public function skipRegistrationProvider() + { + return [ + ['guy'], + ['guy', true], + ['pete', false, 'Security/changepassword'], + ['pete', true, 'Security/changepassword'], + ]; + } + + /** + * @param $memberFixture + * @dataProvider methodlessMemberFixtureProvider + */ + public function testBackURLIsPreservedWhenSkipping($memberFixture) + { + /** @var Member $member */ + $member = $this->objFromFixture(Member::class, $memberFixture); + $this->scaffoldPartialLogin($member); + + $this->doLogin($member, 'Password123', 'admin/pages'); + + $this->autoFollowRedirection = false; + $response = $this->get(Controller::join_links(Security::login_url(), 'default/mfa/skip')); + + $this->assertSame(302, $response->getStatusCode()); + $this->assertStringEndsWith('admin/pages', $response->getHeader('location')); + } + /** * @expectedException \SilverStripe\MFA\Exception\MemberNotFoundException */ @@ -431,6 +485,43 @@ public function testFinishVerificationPassesExceptionMessagesThroughFromMethodsW $this->assertSame($failedLogins + 1, $member->FailedLoginCount, 'Failed login is registered'); } + /** + * @param $memberFixture + * @dataProvider methodlessMemberFixtureProvider + */ + public function testFinishVerificationWillRedirectToTheBackURLSetAsLoginIsStarted($memberFixture) + { + /** @var Member $member */ + $member = $this->objFromFixture(Member::class, $memberFixture); + $this->scaffoldPartialLogin($member); + + $this->doLogin($member, 'Password123', 'admin/pages'); + + /** @var LoginHandler|PHPUnit_Framework_MockObject_MockObject $handler */ + $handler = $this->getMockBuilder(LoginHandler::class) + ->setMethods(['completeVerificationRequest']) + ->disableOriginalConstructor() + ->getMock(); + + $handler->expects($this->once())->method('completeVerificationRequest')->willReturn(Result::create()); + + $request = new HTTPRequest('GET', '/'); + $request->setSession(new Session([])); + $store = new SessionStore($member); + $store->setMethod('basic-math'); + $handler->setStore($store); + + $response = $handler->finishVerification($request); + + // Assert "Accepted" response + $this->assertSame(202, $response->getStatusCode()); + + $this->autoFollowRedirection = false; + $response = $this->get(Controller::join_links(Security::login_url(), 'default/mfa/complete')); + $this->assertSame(302, $response->getStatusCode()); + $this->assertStringEndsWith('admin/pages', $response->getHeader('location')); + } + public function testGetBackURL() { $handler = new LoginHandler('foo', $this->createMock(MemberAuthenticator::class)); @@ -446,6 +537,11 @@ public function testGetBackURL() $this->assertSame('foobar', $handler->getBackURL()); } + public function methodlessMemberFixtureProvider() + { + return [['guy', 'carla']]; + } + /** * Mark the given user as partially logged in - ie. they've entered their email/password and are currently going * through the MFA process @@ -463,9 +559,15 @@ protected function scaffoldPartialLogin(Member $member) * @param string $password * @return HTTPResponse */ - protected function doLogin(Member $member, $password) + protected function doLogin(Member $member, $password, $backUrl = null) { - $this->get(Config::inst()->get(Security::class, 'login_url')); + $url = Config::inst()->get(Security::class, 'login_url'); + + if ($backUrl) { + $url .= '?BackURL=' . $backUrl; + } + + $this->get($url); return $this->submitForm( 'MemberLoginForm_LoginForm', diff --git a/tests/php/Authenticator/LoginHandlerTest.yml b/tests/php/Authenticator/LoginHandlerTest.yml index 25b3fd09..180bcb3c 100644 --- a/tests/php/Authenticator/LoginHandlerTest.yml +++ b/tests/php/Authenticator/LoginHandlerTest.yml @@ -3,16 +3,25 @@ SilverStripe\MFA\Model\RegisteredMethod: MethodClassName: "SilverStripe\\MFA\\Tests\\Stub\\BasicMath\\Method" robbie-math: MethodClassName: "SilverStripe\\MFA\\Tests\\Stub\\BasicMath\\Method" + colin-math: + MethodClassName: "SilverStripe\\MFA\\Tests\\Stub\\BasicMath\\Method" + SilverStripe\Security\Permission: admin: Code: ADMIN + content: + Code: CMS_ACCESS_CMSMain SilverStripe\Security\Group: admingroup: Title: Create, edit and delete pages Code: admingroup Permissions: =>SilverStripe\Security\Permission.admin + contentgroup: + Title: Content author + Code: contentgroup + Permissions: =>SilverStripe\Security\Permission.content SilverStripe\Security\Member: guy: @@ -29,4 +38,15 @@ SilverStripe\Security\Member: RegisteredMFAMethods: =>SilverStripe\MFA\Model\RegisteredMethod.robbie-math DefaultRegisteredMethodID: =>SilverStripe\MFA\Model\RegisteredMethod.robbie-math Groups: =>SilverStripe\Security\Group.admingroup - + colin: + Email: colin@example.com + RegisteredMFAMethods: =>SilverStripe\MFA\Model\RegisteredMethod.colin-math + DefaultRegisteredMethodID: =>SilverStripe\MFA\Model\RegisteredMethod.colin-math + Groups: =>SilverStripe\Security\Group.contentgroup + carla: + Email: carla@example.com + Groups: =>SilverStripe\Security\Group.contentgroup + pete: + Email: pete@example.com + PasswordExpiry: 1990-01-01 + Groups: =>SilverStripe\Security\Group.contentgroup From b05cddd01e7bb538d69a486be904f16991b470a5 Mon Sep 17 00:00:00 2001 From: Guy Marriott Date: Thu, 11 Jul 2019 17:03:42 +1200 Subject: [PATCH 4/5] DOCS Update docblocks --- src/Authenticator/LoginHandler.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Authenticator/LoginHandler.php b/src/Authenticator/LoginHandler.php index 212c7766..8288f401 100644 --- a/src/Authenticator/LoginHandler.php +++ b/src/Authenticator/LoginHandler.php @@ -458,6 +458,7 @@ public function redirectAfterSuccessfulLogin(): HTTPResponse } // Redirecting after successful login expects a getVar to be set, store it before clearing the session data + /** @see HTTPRequest::offsetSet */ $request['BackURL'] = $this->getBackURL(); // Clear the "additional data" @@ -515,9 +516,7 @@ public function getLogger(): ?LoggerInterface } /** - * Adds more options for the back URL - to be returned from a current MFA session store or from memory during this - * request. The latter is required as session store is cleared before the 'BackURL' key is utilised - * {@see redirectAfterSuccessfulLogin} + * Adds more options for the back URL - to be returned from a current MFA session store * * @return string|null */ From 0454110276ea4fa2f1f043422f4394467c3aefbc Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Mon, 15 Jul 2019 14:29:06 +0100 Subject: [PATCH 5/5] Update doc blocks, optimise imports, prefer short array syntax --- tests/php/Authenticator/LoginHandlerTest.php | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/php/Authenticator/LoginHandlerTest.php b/tests/php/Authenticator/LoginHandlerTest.php index 21990171..fd908236 100644 --- a/tests/php/Authenticator/LoginHandlerTest.php +++ b/tests/php/Authenticator/LoginHandlerTest.php @@ -2,7 +2,6 @@ namespace SilverStripe\MFA\Tests\Authenticator; -use Page; use PHPUnit_Framework_MockObject_MockObject; use SilverStripe\Control\Controller; use SilverStripe\Control\HTTPRequest; @@ -234,8 +233,9 @@ public function cannotSkipMFAProvider() } /** - * @param $memberFixture - * @param $expectedRedirect + * @param string $memberFixture + * @param bool $mfaRequiredInGrace + * @param string|null $expectedRedirect * @dataProvider skipRegistrationProvider */ public function testSkipRegistration($memberFixture, $mfaRequiredInGrace = false, $expectedRedirect = null) @@ -286,7 +286,7 @@ public function skipRegistrationProvider() } /** - * @param $memberFixture + * @param string $memberFixture * @dataProvider methodlessMemberFixtureProvider */ public function testBackURLIsPreservedWhenSkipping($memberFixture) @@ -486,7 +486,7 @@ public function testFinishVerificationPassesExceptionMessagesThroughFromMethodsW } /** - * @param $memberFixture + * @param string $memberFixture * @dataProvider methodlessMemberFixtureProvider */ public function testFinishVerificationWillRedirectToTheBackURLSetAsLoginIsStarted($memberFixture) @@ -572,12 +572,12 @@ protected function doLogin(Member $member, $password, $backUrl = null) return $this->submitForm( 'MemberLoginForm_LoginForm', null, - array( + [ 'Email' => $member->Email, 'Password' => $password, 'AuthenticationMethod' => MemberAuthenticator::class, 'action_doLogin' => 1, - ) + ] ); }