Merge pull request #212 from creative-commoners/pulls/4.0/redirect-loop

FIX Users without CMS access are now able to log in correctly

src/Authenticator/LoginHandler.php

@@ -420,7 +420,7 @@ public function redirectAfterSuccessfulLogin(): HTTPResponse
                 _t(__CLASS__ . '.MFA_LOGIN_INCOMPLETE', 'You must provide MFA login details'),
                 ValidationResult::TYPE_ERROR
             );
-            return $this->redirect($loginUrl);
+            return $this->redirect($this->getBackURL() ?: $loginUrl);
         }
 
         $request = $this->getRequest();
@@ -431,7 +431,9 @@ 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->canSkipMFA($member) && !$enforcementManager->hasCompletedRegistration($member)) {
+        if (!$enforcementManager->hasCompletedRegistration($member)
+            && $enforcementManager->shouldRedirectToMFA($member)
+        ) {
             // Log them out again
             /** @var IdentityStore $identityStore */
             $identityStore = Injector::inst()->get(IdentityStore::class);
@@ -441,18 +443,12 @@ public function redirectAfterSuccessfulLogin(): HTTPResponse
                 _t(__CLASS__ . '.INVALID_REGISTRATION', 'You must complete MFA registration'),
                 ValidationResult::TYPE_ERROR
             );
-            return $this->redirect($loginUrl);
+            return $this->redirect($this->getBackURL() ?: $loginUrl);
         }
 
         // Clear the "additional data"
-        $data = $request->getSession()->get(static::SESSION_KEY . '.additionalData') ?: [];
         $request->getSession()->clear(static::SESSION_KEY . '.additionalData');
 
-        // Redirecting after successful login expects a getVar to be set
-        if (!empty($data['BackURL'])) {
-            $request['BackURL'] = $data['BackURL'];
-        }
-
         // Ensure any left over session state is cleaned up
         $store = $this->getStore();
         if ($store) {
@@ -504,6 +500,25 @@ public function getLogger(): ?LoggerInterface
         return $this->logger;
     }
 
+    /**
+     * Adds another option for the back URL to be returned from a current MFA session store
+     *
+     * @return string|null
+     */
+    public function getBackURL(): ?string
+    {
+        $backURL = parent::getBackURL();
+
+        if (!$backURL && $this->getRequest()) {
+            $data = $this->getRequest()->getSession()->get(static::SESSION_KEY . '.additionalData');
+            if (isset($data['BackURL'])) {
+                $backURL = $data['BackURL'];
+            }
+        }
+
+        return $backURL;
+    }
+
     /**
      * Respond with the given array as a JSON response
      *

src/Service/EnforcementManager.php

@@ -2,6 +2,7 @@
 
 namespace SilverStripe\MFA\Service;
 
+use SilverStripe\Admin\LeftAndMain;
 use SilverStripe\Core\Config\Config;
 use SilverStripe\Core\Config\Configurable;
 use SilverStripe\Core\Injector\Injectable;
@@ -10,7 +11,6 @@
 use SilverStripe\ORM\FieldType\DBDate;
 use SilverStripe\Security\Member;
 use SilverStripe\SiteConfig\SiteConfig;
-use SilverStripe\SiteConfig\SiteConfigLeftAndMain;
 
 /**
  * The EnforcementManager class is responsible for making decisions regarding multi factor authentication app flow,
@@ -218,9 +218,7 @@ public function isGracePeriodInEffect(): bool
      */
     protected function hasAdminAccess(Member $member): bool
     {
-        // We need to use an actual LeftAndMain implementation, otherwise LeftAndMain::canView() returns true
-        // because no required permission codes are declared
-        $leftAndMain = SiteConfigLeftAndMain::singleton();
+        $leftAndMain = LeftAndMain::singleton();
         if ($leftAndMain->canView($member)) {
             return true;
         }
@@ -229,7 +227,7 @@ protected function hasAdminAccess(Member $member): bool
         $menu = $leftAndMain->MainMenu();
         foreach ($menu as $candidate) {
             if ($candidate->Link
-                && $candidate->Link != $leftAndMain->Link()
+                && $candidate->Link !== $leftAndMain->Link()
                 && $candidate->MenuItem->controller
                 && singleton($candidate->MenuItem->controller)->canView($member)
             ) {

tests/php/Authenticator/LoginHandlerTest.php

@@ -411,6 +411,21 @@ public function testFinishVerificationPassesExceptionMessagesThroughFromMethodsW
         $this->assertSame($failedLogins + 1, $member->FailedLoginCount, 'Failed login is registered');
     }
 
+    public function testGetBackURL()
+    {
+        $handler = new LoginHandler('foo', $this->createMock(MemberAuthenticator::class));
+
+        $request = new HTTPRequest('GET', '/');
+        $handler->setRequest($request);
+
+        $session = new Session([]);
+        $request->setSession($session);
+
+        $session->set(LoginHandler::SESSION_KEY . '.additionalData', ['BackURL' => 'foobar']);
+
+        $this->assertSame('foobar', $handler->getBackURL());
+    }
+
     /**
      * Mark the given user as partially logged in - ie. they've entered their email/password and are currently going
      * through the MFA process

tests/php/Service/EnforcementManagerTest.php

@@ -100,6 +100,14 @@ public function testShouldRedirectToMFAWhenUserHasAccessToReportsOnly()
         $this->assertTrue(EnforcementManager::create()->shouldRedirectToMFA($member));
     }
 
+    public function testShouldRedirectToMFAForContentAuthors()
+    {
+        $memberID = $this->logInWithPermission('CMS_ACCESS_CMSMain');
+        /** @var Member $member */
+        $member = Member::get()->byID($memberID);
+        $this->assertTrue(EnforcementManager::create()->shouldRedirectToMFA($member));
+    }
+
     public function testShouldRedirectToMFAWhenUserHasRegisteredMFAMethod()
     {
         /** @var Member $member */