FIX Ensure canSkipMFA respects whether MFA is enabled

Previously the canSkipMFA method did not validate whether MFA was available, which resulted in a login loop if any other condition stopped the user from skipping, such as MFA being mandated via SiteConfig.
silverstripe · Apr 22, 2020 · 273cdd8 · 273cdd8
1 parent a06980b
commit 273cdd8
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 8 deletions.
diff --git a/src/Service/EnforcementManager.php b/src/Service/EnforcementManager.php
@@ -54,6 +54,7 @@ class EnforcementManager
      *
      * This is determined by a combination of:
      *
+     *  - Whether MFA is enabled and there are methods available for use
      *  - Whether the user has admin access (MFA is disabled by default for users that don't)
      *  - Whether MFA is required - @see EnforcementManager::isMFARequired()
      *  - Whether the user has registered MFA methods already
@@ -63,6 +64,10 @@ class EnforcementManager
      */
     public function canSkipMFA(Member $member): bool
     {
+        if (!$this->isEnabled()) {
+            return true;
+        }
+
         if ($this->config()->get('requires_admin_access') && !$this->hasAdminAccess($member)) {
             return true;
         }
@@ -99,13 +104,6 @@ public function shouldRedirectToMFA(Member $member): bool
             return false;
         }
 
-        $methodRegistry = MethodRegistry::singleton();
-        $methods = $methodRegistry->getMethods();
-        // If there are no methods available excluding backup codes, do not redirect
-        if (!count($methods) || (count($methods) === 1 && $methodRegistry->getBackupMethod() !== null)) {
-            return false;
-        }
-
         if ($this->config()->get('requires_admin_access') && !$this->hasAdminAccess($member)) {
             return false;
         }
@@ -246,10 +244,26 @@ protected function hasAdminAccess(Member $member): bool
     }
 
     /**
+     * MFA is enabled if:
+     *
+     * - The EnforcementManager::enabled configuration is set to true
+     * - There is at least one non-backup method available to register
+     *
      * @return bool
      */
     protected function isEnabled(): bool
     {
-        return (bool) $this->config()->get('enabled');
+        if (!$this->config()->get('enabled')) {
+            return false;
+        }
+
+        $methodRegistry = MethodRegistry::singleton();
+        $methods = $methodRegistry->getMethods();
+        // If there are no methods available excluding backup codes, do not redirect
+        if (!count($methods) || (count($methods) === 1 && $methodRegistry->getBackupMethod() !== null)) {
+            return false;
+        }
+
+        return true;
     }
 }
diff --git a/tests/php/Service/EnforcementManagerTest.php b/tests/php/Service/EnforcementManagerTest.php
@@ -27,6 +27,27 @@ protected function setUp()
         ]);
 
         EnforcementManager::config()->set('requires_admin_access', true);
+        EnforcementManager::config()->set('enabled', true);
+    }
+
+    public function testUserCanSkipWhenMFAIsDisabled()
+    {
+        $this->setSiteConfig(['MFARequired' => true]);
+        EnforcementManager::config()->set('enabled', false);
+
+        /** @var Member $member */
+        $member = $this->objFromFixture(Member::class, 'sally_smith');
+        $this->assertTrue(EnforcementManager::create()->canSkipMFA($member));
+    }
+
+    public function testUserCanSkipWhenNoMethodsAreAvailable()
+    {
+        $this->setSiteConfig(['MFARequired' => true]);
+        MethodRegistry::config()->set('methods', null);
+
+        /** @var Member $member */
+        $member = $this->objFromFixture(Member::class, 'sally_smith');
+        $this->assertTrue(EnforcementManager::create()->canSkipMFA($member));
     }
 
     public function testUserWithoutCMSAccessCanSkipWhenCMSAccessIsRequired()