FIX Redirect user back when accessing MFA with a stale session

client/src/components/BasicMath/Login.js

@@ -35,7 +35,12 @@ class Login extends Component {
     const { ss: { i18n } } = window;
 
     if (!numbers) {
-      return <LoadingIndicator block />;
+      return (
+        <div>
+          <LoadingIndicator block />
+          { moreOptionsControl }
+        </div>
+      );
     }
 
     return (

src/Authenticator/LoginHandler.php

@@ -125,10 +125,10 @@ public function doLogin($data, MemberLoginForm $form, HTTPRequest $request)
      *
      * @return HTTPResponse|array
      */
-    public function mfa()
+    public function mfa(HTTPRequest $request)
     {
         $store = $this->getStore();
-        if (!$store || !$store->getMember()) {
+        if (!$store || !$store->getMember() || !$this->getSudoModeService()->check($request->getSession())) {
             return $this->redirectBack();
         }
 
@@ -178,11 +178,16 @@ public function startRegistration(HTTPRequest $request): HTTPResponse
         $sessionMember = $store ? $store->getMember() : null;
         $loggedInMember = Security::getCurrentUser();
 
-        if ((is_null($loggedInMember) && is_null($sessionMember))
+        if (($loggedInMember === null && $sessionMember === null)
             || !$this->getSudoModeService()->check($request->getSession())
         ) {
             return $this->jsonResponse(
-                ['errors' => [_t(__CLASS__ . '.NOT_AUTHENTICATING', 'You must be logged or logging in')]],
+                ['errors' => [
+                    _t(
+                        __CLASS__ . '.NOT_AUTHENTICATING',
+                        'You must be logged in or logging in. Please refresh the page and try again.'
+                    )
+                ]],
                 403
             );
         }
@@ -236,11 +241,16 @@ public function finishRegistration(HTTPRequest $request): HTTPResponse
         $sessionMember = $store ? $store->getMember() : null;
         $loggedInMember = Security::getCurrentUser();
 
-        if ((is_null($loggedInMember) && is_null($sessionMember))
+        if (($loggedInMember === null && $sessionMember === null)
             || !$this->getSudoModeService()->check($request->getSession())
         ) {
             return $this->jsonResponse(
-                ['errors' => [_t(__CLASS__ . '.NOT_AUTHENTICATING', 'You must be logged or logging in')]],
+                ['errors' => [
+                    _t(
+                        __CLASS__ . '.NOT_AUTHENTICATING',
+                        'You must be logged in or logging in. Please refresh the page and try again.'
+                    )
+                ]],
                 403
             );
         }
@@ -320,10 +330,8 @@ public function skipRegistration(HTTPRequest $request): HTTPResponse
     public function startVerification(HTTPRequest $request): HTTPResponse
     {
         $store = $this->getStore();
-        $member = $store->getMember();
-
         // If we don't have a valid member we shouldn't be here, or if sudo mode is not active yet.
-        if (!$member || !$this->getSudoModeService()->check($request->getSession())) {
+        if (!$store || !$store->getMember() || !$this->getSudoModeService()->check($request->getSession())) {
             return $this->jsonResponse(['message' => 'Forbidden'], 403);
         }
 
@@ -349,23 +357,21 @@ public function startVerification(HTTPRequest $request): HTTPResponse
     public function finishVerification(HTTPRequest $request): HTTPResponse
     {
         $store = $this->getStore();
-        $member = $store->getMember();
-
-        if ($member->isLockedOut()) {
+        // Enforce sudo mode
+        if (!$this->getSudoModeService()->check($request->getSession())) {
             return $this->jsonResponse([
                 'message' => _t(
-                    __CLASS__ . '.LOCKED_OUT',
-                    'Your account is temporarily locked. Please try again later.'
+                    __CLASS__ . '.SUDO_MODE_REQUIRED',
+                    'You need to re-verify your account before continuing. Please reload and try again.'
                 ),
             ], 403);
         }
 
-        // Enforce sudo mode
-        if (!$this->getSudoModeService()->check($request->getSession())) {
+        if ($store && ($member = $store->getMember()) && $member->isLockedOut()) {
             return $this->jsonResponse([
                 'message' => _t(
-                    __CLASS__ . '.SUDO_MODE_REQUIRED',
-                    'You need to re-verify your account before continuing. Please reload and try again.'
+                    __CLASS__ . '.LOCKED_OUT',
+                    'Your account is temporarily locked. Please try again later.'
                 ),
             ], 403);
         }

src/Controller/AdminRegistrationController.php

@@ -103,7 +103,7 @@ public function finishRegistration(HTTPRequest $request): HTTPResponse
 
         if (!$store || !$this->getSudoModeService()->check($request->getSession())) {
             return $this->jsonResponse(
-                ['errors' => [_t(__CLASS__ . '.INVALID_SESSION', 'Invalid session. Please try again')]],
+                ['errors' => [_t(__CLASS__ . '.INVALID_SESSION', 'Invalid session. Please refresh and try again.')]],
                 400
             );
         }

tests/php/Authenticator/LoginHandlerTest.php

@@ -3,6 +3,7 @@
 namespace SilverStripe\MFA\Tests\Authenticator;
 
 use PHPUnit_Framework_MockObject_MockObject;
+use SilverStripe\Control\Controller;
 use SilverStripe\Control\HTTPRequest;
 use SilverStripe\Control\HTTPResponse;
 use SilverStripe\Control\Session;
@@ -24,8 +25,8 @@
 use SilverStripe\ORM\FieldType\DBDatetime;
 use SilverStripe\Security\Member;
 use SilverStripe\Security\Security;
-use SilverStripe\SecurityExtensions\Service\SudoModeServiceInterface;
 use SilverStripe\Security\SecurityToken;
+use SilverStripe\SecurityExtensions\Service\SudoModeServiceInterface;
 use SilverStripe\SiteConfig\SiteConfig;
 
 class LoginHandlerTest extends FunctionalTest
@@ -63,7 +64,27 @@ public function testMFAStepIsAdded()
         $this->autoFollowRedirection = true;
 
         $this->assertSame(302, $response->getStatusCode());
-        $this->assertStringEndsWith('/Security/login/default/mfa', $response->getHeader('location'));
+        $this->assertStringEndsWith(
+            Controller::join_links(Security::login_url(), 'default/mfa'),
+            $response->getHeader('location')
+        );
+    }
+
+    public function testMFARedirectsBackWhenSudoModeIsInactive()
+    {
+        /** @var SudoModeServiceInterface&PHPUnit_Framework_MockObject_MockObject $sudoModeService */
+        $sudoModeService = $this->createMock(SudoModeServiceInterface::class);
+        $sudoModeService->expects($this->once())->method('check')->willReturn(false);
+        Injector::inst()->registerService($sudoModeService, SudoModeServiceInterface::class);
+
+        /** @var Member&MemberExtension $member */
+        $member = $this->objFromFixture(Member::class, 'guy');
+        $this->scaffoldPartialLogin($member);
+
+        $this->autoFollowRedirection = false;
+        $response = $this->get(Controller::join_links(Security::login_url(), 'default/mfa'));
+
+        $this->assertSame(302, $response->getStatusCode());
     }
 
     public function testMethodsNotBeingAvailableWillLogin()
@@ -88,7 +109,7 @@ public function testMFASchemaEndpointIsNotAccessibleByDefault()
     {
         // Assert that this endpoint is not available if you haven't started the login process
         $this->autoFollowRedirection = false;
-        $response = $this->get('Security/login/default/mfa/schema');
+        $response = $this->get(Controller::join_links(Security::login_url(), 'default/mfa/schema'));
         $this->autoFollowRedirection = true;
 
         $this->assertSame(302, $response->getStatusCode());
@@ -101,7 +122,7 @@ public function testMFASchemaEndpointReturnsMethodDetails()
         $member = $this->objFromFixture(Member::class, 'guy');
         $this->scaffoldPartialLogin($member);
 
-        $result = $this->get('Security/login/default/mfa/schema');
+        $result = $this->get(Controller::join_links(Security::login_url(), 'default/mfa/schema'));
 
         $response = json_decode($result->getBody(), true);
 
@@ -136,7 +157,7 @@ public function testMFASchemaEndpointShowsRegisteredMethodsIfSetUp()
         $member = $this->objFromFixture(Member::class, 'simon');
         $this->scaffoldPartialLogin($member);
 
-        $result = $this->get('Security/login/default/mfa/schema');
+        $result = $this->get(Controller::join_links(Security::login_url(), 'default/mfa/schema'));
 
         $response = json_decode($result->getBody(), true);
 
@@ -167,7 +188,7 @@ public function testMFASchemaEndpointProvidesDefaultMethodIfSet()
         $member = $this->objFromFixture(Member::class, 'robbie');
         $this->scaffoldPartialLogin($member);
 
-        $result = $this->get('Security/login/default/mfa/schema');
+        $result = $this->get(Controller::join_links(Security::login_url(), 'default/mfa/schema'));
 
         $response = json_decode($result->getBody(), true);
 
@@ -196,7 +217,7 @@ public function testCannotSkipMFA($mfaRequired, $member = 'robbie')
             $this->scaffoldPartialLogin($this->objFromFixture(Member::class, $member));
         }
 
-        $response = $this->get('Security/login/default/mfa/skip');
+        $response = $this->get(Controller::join_links(Security::login_url(), 'default/mfa/skip'));
         $this->assertContains('You cannot skip MFA registration', $response->getBody());
     }
 
@@ -222,7 +243,7 @@ public function testSkipRegistration()
         $memberId = $member->write();
         $this->logInAs($member);
 
-        $response = $this->get('Security/login/default/mfa/skip');
+        $response = $this->get(Controller::join_links(Security::login_url(), 'default/mfa/skip'));
 
         $this->assertSame(200, $response->getStatusCode());
 

tests/php/Authenticator/RegisterHandlerTest.php

@@ -262,7 +262,7 @@ public function testEnforcesSudoMode()
 
         $response = $this->post(self::URL, ['dummy' => 'data'], null, $this->session(), json_encode(['number' => 7]));
         $this->assertSame(403, $response->getStatusCode());
-        $this->assertContains('You must be logged or logging in', (string) $response->getBody());
+        $this->assertContains('You must be logged in or logging in', (string) $response->getBody());
     }
 
     /**