NEW Add functionality to limit MFA to specific user groups

If a user has already registered for MFA, enforce use of it even if they are not in an MFA group

Co-authored-by: Guy Marriott <guy.the.person@gmail.com>

src/Extension/SiteConfigExtension.php

@@ -6,9 +6,11 @@
 use SilverStripe\Forms\CompositeField;
 use SilverStripe\Forms\DateField;
 use SilverStripe\Forms\FieldList;
+use SilverStripe\Forms\ListboxField;
 use SilverStripe\Forms\OptionsetField;
 use SilverStripe\ORM\DataExtension;
 use SilverStripe\ORM\FieldType\DBField;
+use SilverStripe\Security\Group;
 use SilverStripe\View\Requirements;
 
 /**
@@ -34,6 +36,10 @@ class SiteConfigExtension extends DataExtension
         'MFAGracePeriodExpires' => 'Date',
     ];
 
+    private static $many_many = [
+      'MFAGroupRestrictions' => Group::class
+    ];
+
     private static $defaults = [
         'MFARequired' => false,
     ];
@@ -63,7 +69,29 @@ public function updateCMSFields(FieldList $fields)
         ));
         $mfaGraceEnd->addExtraClass('mfa-settings__grace-period');
 
-        $mfaOptions = CompositeField::create($mfaOptions, $mfaGraceEnd)
+        $groupsMap = [];
+        foreach (Group::get() as $group) {
+            // Listboxfield values are escaped, use ASCII char instead of »
+            $groupsMap[$group->ID] = $group->getBreadcrumbs(' > ');
+        }
+        asort($groupsMap);
+
+        $mfaGroupRestrict = ListboxField::create(
+            "MFAGroupRestrictions",
+            _t(__CLASS__ . '.MFA_GROUP_RESTRICTIONS', "MFA Groups")
+        )
+            ->setSource($groupsMap)
+            ->setAttribute(
+                'data-placeholder',
+                _t(__CLASS__ . '.MFA_GROUP_RESTRICTIONS_PLACEHOLDER', 'Click to select group')
+            )
+            ->setDescription(_t(
+                __CLASS__ . '.MFA_GROUP_RESTRICTIONS_DESCRIPTION',
+                'MFA will only be enabled for members of these selected groups. ' .
+                'If no groups are selected, MFA will be enabled for all users'
+            ));
+
+        $mfaOptions = CompositeField::create($mfaOptions, $mfaGraceEnd, $mfaGroupRestrict)
             ->setTitle(DBField::create_field(
                 'HTMLFragment',
                 _t(__CLASS__ . '.MULTI_FACTOR_AUTHENTICATION', 'Multi-factor authentication (MFA)')

src/Service/EnforcementManager.php

@@ -108,6 +108,10 @@ public function shouldRedirectToMFA(Member $member): bool
             return false;
         }
 
+        if (!$this->isUserInMFAEnabledGroup($member) && !$this->hasCompletedRegistration($member)) {
+            return false;
+        }
+
         if ($member->RegisteredMFAMethods()->exists()) {
             return true;
         }
@@ -266,4 +270,23 @@ protected function isEnabled(): bool
 
         return true;
     }
+
+    protected function isUserInMFAEnabledGroup(Member $member): bool
+    {
+        /** @var SiteConfig&SiteConfigExtension $siteConfig */
+        $siteConfig = SiteConfig::current_site_config();
+
+        $groups = $siteConfig->MFAGroupRestrictions();
+
+        // If no groups are set in the Site Config MFAGroupRestrictions field, MFA is enabled for all users
+        if ($groups->count() === 0) {
+            return true;
+        }
+        foreach ($groups as $group) {
+            if ($member->inGroup($group)) {
+                return true;
+            }
+        }
+        return false;
+    }
 }

tests/Behat/features/mfa-enabled.feature

@@ -14,4 +14,4 @@ Feature: MFA is enabled for the site
     Then I should see "Multi-factor authentication (MFA)"
     When I select "MFA is required for everyone" from the MFA settings
     And I press "Save"
-    Then I should see "Saved"
+    Then I should see a "Saved" success toast

tests/php/Authenticator/LoginHandlerTest.php

@@ -26,6 +26,7 @@
 use SilverStripe\MFA\Tests\Stub\Store\TestStore;
 use SilverStripe\MFA\Tests\Stub\BasicMath\Method;
 use SilverStripe\ORM\FieldType\DBDatetime;
+use SilverStripe\Security\Group;
 use SilverStripe\Security\Member;
 use SilverStripe\Security\Security;
 use SilverStripe\Security\SecurityToken;
@@ -572,6 +573,44 @@ public function testGetBackURL()
         $this->assertSame('foobar', $handler->getBackURL());
     }
 
+
+    public function testMFAGroupRestriction()
+    {
+        $config = SiteConfig::current_site_config();
+
+        /** @var Group $group */
+        $group = $this->objFromFixture(Group::class, 'admingroup');
+        $config->MFAGroupRestrictions()->add($group);
+
+        // Test that MFA is required for a member of a group that has been set in SiteConfig
+        /** @var Member&MemberExtension $member */
+        $member = $this->objFromFixture(Member::class, 'guy');
+
+        $this->autoFollowRedirection = false;
+        $response = $this->doLogin($member, 'Password123');
+        $this->autoFollowRedirection = true;
+
+        $this->assertSame(302, $response->getStatusCode());
+        $this->assertStringEndsWith(
+            Controller::join_links(Security::login_url(), 'default/mfa'),
+            $response->getHeader('location')
+        );
+
+        // Test that MFA is not required for a member that does not belong to any of the selected groups
+        /** @var Member&MemberExtension $member */
+        $member = $this->objFromFixture(Member::class, 'colin');
+
+        $this->autoFollowRedirection = false;
+        $response = $this->doLogin($member, 'Password123');
+        $this->autoFollowRedirection = true;
+
+        $this->assertSame(302, $response->getStatusCode());
+        $this->assertStringEndsWith(
+            Security::login_url(),
+            $response->getHeader('location')
+        );
+    }
+
     public function methodlessMemberFixtureProvider()
     {
         return [['guy', 'carla']];

tests/php/Authenticator/LoginHandlerTest.yml

@@ -40,6 +40,8 @@ SilverStripe\Security\Member:
     Groups: =>SilverStripe\Security\Group.admingroup
   colin:
     Email: colin@example.com
+    Password: Password123
+    PasswordExpiry: 2030-01-01
     RegisteredMFAMethods: =>SilverStripe\MFA\Model\RegisteredMethod.colin-math
     DefaultRegisteredMethodID: =>SilverStripe\MFA\Model\RegisteredMethod.colin-math
     Groups: =>SilverStripe\Security\Group.contentgroup

tests/php/Service/EnforcementManagerTest.php

@@ -9,6 +9,7 @@
 use SilverStripe\MFA\Service\MethodRegistry;
 use SilverStripe\MFA\Tests\Stub\BasicMath\Method as BasicMathMethod;
 use SilverStripe\ORM\FieldType\DBDatetime;
+use SilverStripe\Security\Group;
 use SilverStripe\Security\Member;
 use SilverStripe\SiteConfig\SiteConfig;
 
@@ -253,6 +254,34 @@ public function testUserHasCompletedRegistrationWhenBackupMethodIsDisabled()
         $this->assertTrue(EnforcementManager::create()->hasCompletedRegistration($member));
     }
 
+    public function testShouldRedirectToMFAWhenUserIsInMFARestrictedGroup()
+    {
+        $this->setSiteConfig(['MFARequired' => true]);
+        $config = SiteConfig::current_site_config();
+        $group = $this->objFromFixture(Group::class, 'admingroup');
+        $config->MFAGroupRestrictions()->add($group);
+
+        /** @var Member $member */
+        $member = $this->objFromFixture(Member::class, 'sally_smith');
+        $this->logInAs($member);
+
+        $this->assertTrue(EnforcementManager::create()->shouldRedirectToMFA($member));
+    }
+
+    public function testShouldNotRedirectToMFAWhenUserIsNotInMFARestrictedGroup()
+    {
+        $this->setSiteConfig(['MFARequired' => true]);
+        $config = SiteConfig::current_site_config();
+        $group = $this->objFromFixture(Group::class, 'admingroup');
+        $config->MFAGroupRestrictions()->add($group);
+
+        /** @var Member $member */
+        $member = $this->objFromFixture(Member::class, 'sully_smith');
+        $this->logInAs($member);
+
+        $this->assertFalse(EnforcementManager::create()->shouldRedirectToMFA($member));
+    }
+
     /**
      * Helper method for changing the current SiteConfig values
      *