FIX correctly redirect after successful log in

src/Authenticator/LoginHandler.php

@@ -442,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 */
@@ -457,6 +457,10 @@ public function redirectAfterSuccessfulLogin(): HTTPResponse
             return $this->redirect($this->getBackURL() ?: $loginUrl);
         }
 
+        // 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"
         $request->getSession()->clear(static::SESSION_KEY . '.additionalData');
 
@@ -512,7 +516,7 @@ 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
      *
      * @return string|null
      */

tests/php/Authenticator/LoginHandlerTest.php

@@ -232,24 +232,78 @@ public function cannotSkipMFAProvider()
         ];
     }
 
-    public function testSkipRegistration()
+    /**
+     * @param string $memberFixture
+     * @param bool $mfaRequiredInGrace
+     * @param string|null $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 string $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 string $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,19 +559,25 @@ 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',
             null,
-            array(
+            [
                 'Email' => $member->Email,
                 'Password' => $password,
                 'AuthenticationMethod' => MemberAuthenticator::class,
                 'action_doLogin' => 1,
-            )
+            ]
         );
     }
 

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: [email protected]
+    RegisteredMFAMethods: =>SilverStripe\MFA\Model\RegisteredMethod.colin-math
+    DefaultRegisteredMethodID: =>SilverStripe\MFA\Model\RegisteredMethod.colin-math
+    Groups: =>SilverStripe\Security\Group.contentgroup
+  carla:
+    Email: [email protected]
+    Groups: =>SilverStripe\Security\Group.contentgroup
+  pete:
+    Email: [email protected]
+    PasswordExpiry: 1990-01-01
+    Groups: =>SilverStripe\Security\Group.contentgroup