ENH Add field for toggling between everyone and groups

silverstripe · Oct 2, 2023 · 4c2396c · 4c2396c
1 parent 467ff28
commit 4c2396c
Show file tree

Hide file tree

Showing 8 changed files with 694 additions and 223 deletions.
diff --git a/src/Extension/SiteConfigExtension.php b/src/Extension/SiteConfigExtension.php
@@ -6,10 +6,12 @@
 use SilverStripe\Forms\CompositeField;
 use SilverStripe\Forms\DateField;
 use SilverStripe\Forms\FieldList;
-use SilverStripe\Forms\ListboxField;
 use SilverStripe\Forms\OptionsetField;
+use SilverStripe\Forms\TreeMultiselectField;
+use SilverStripe\MFA\Service\EnforcementManager;
 use SilverStripe\ORM\DataExtension;
 use SilverStripe\ORM\FieldType\DBField;
+use SilverStripe\ORM\ValidationResult;
 use SilverStripe\Security\Group;
 use SilverStripe\View\Requirements;
 
@@ -34,6 +36,9 @@ class SiteConfigExtension extends DataExtension
     private static $db = [
         'MFARequired' => 'Boolean',
         'MFAGracePeriodExpires' => 'Date',
+        'MFAAppliesTo' => 'Enum(["'
+            . EnforcementManager::APPLIES_TO_EVERYONE . '","'
+            . EnforcementManager::APPLIES_TO_GROUPS . '"])',
     ];
 
     private static $many_many = [
@@ -53,8 +58,8 @@ public function updateCMSFields(FieldList $fields)
             'MFARequired',
             '',
             [
-                false => _t(__CLASS__ . '.MFA_OPTIONAL', 'MFA is optional for everyone'),
-                true => _t(__CLASS__ . '.MFA_REQUIRED', 'MFA is required for everyone'),
+                false => _t(__CLASS__ . '.MFA_OPTIONAL2', 'MFA is optional'),
+                true => _t(__CLASS__ . '.MFA_REQUIRED2', 'MFA is required'),
             ]
         );
         $mfaOptions->addExtraClass('mfa-settings__required');
@@ -69,29 +74,25 @@ public function updateCMSFields(FieldList $fields)
         ));
         $mfaGraceEnd->addExtraClass('mfa-settings__grace-period');
 
-        $groupsMap = [];
-        foreach (Group::get() as $group) {
-            // Listboxfield values are escaped, use ASCII char instead of &raquo;
-            $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'
-            ));
+        $mfaAppliesToWho = OptionsetField::create(
+            'MFAAppliesTo',
+            _t(__CLASS__ . '.MFA_APPLIES_TO_TITLE', 'Who do these MFA settings apply to?'),
+            [
+                EnforcementManager::APPLIES_TO_EVERYONE => _t(__CLASS__ . '.EVERYONE', 'Everyone'),
+                EnforcementManager::APPLIES_TO_GROUPS => _t(__CLASS__ . '.ONLY_GROUPS', 'Only these groups (choose from list)'),
+            ]
+        );
 
-        $mfaOptions = CompositeField::create($mfaOptions, $mfaGraceEnd, $mfaGroupRestrict)
+        $mfaGroupRestrict = TreeMultiselectField::create(
+            'MFAGroupRestrictions',
+            _t(__CLASS__ . '.MFA_GROUP_RESTRICTIONS', 'MFA Groups'),
+            Group::class
+        )->setDescription(_t(
+            __CLASS__ . '.MFA_GROUP_RESTRICTIONS_DESCRIPTION',
+            'MFA will only be enabled for members of these selected groups.'
+        ));
+
+        $mfaOptions = CompositeField::create($mfaOptions, $mfaGraceEnd, $mfaAppliesToWho, $mfaGroupRestrict)
             ->setTitle(DBField::create_field(
                 'HTMLFragment',
                 _t(__CLASS__ . '.MULTI_FACTOR_AUTHENTICATION', 'Multi-factor authentication (MFA)')
@@ -101,6 +102,19 @@ public function updateCMSFields(FieldList $fields)
         $fields->addFieldToTab('Root.Access', $mfaOptions);
     }
 
+    public function validate(ValidationResult $validationResult)
+    {
+        if ($this->owner->MFAAppliesTo == EnforcementManager::APPLIES_TO_GROUPS && !$this->owner->MFAGroupRestrictions()->exists()) {
+            $validationResult->addFieldError(
+                'MFAGroupRestrictions',
+                _t(
+                    __CLASS__ . '.MFA_GROUP_RESTRICTIONS_VALIDATION',
+                    'At least one group must be selected, or the MFA settings should apply to everyone.'
+                )
+            );
+        }
+    }
+
     /**
      * Gets an anchor tag for CMS users to click to find out more about MFA in the SilverStripe CMS
      *

diff --git a/src/Service/EnforcementManager.php b/src/Service/EnforcementManager.php
@@ -23,6 +23,9 @@ class EnforcementManager
     use Configurable;
     use Injectable;
 
+    public const APPLIES_TO_EVERYONE = 'everyone';
+    public const APPLIES_TO_GROUPS = 'groups';
+
     /**
      * Indicate how many MFA methods the user must authenticate with before they are considered logged in
      *
@@ -72,7 +75,7 @@ public function canSkipMFA(Member $member): bool
             return true;
         }
 
-        if ($this->isMFARequired()) {
+        if ($this->isMFARequired() && $this->isUserInMFAEnabledGroup($member)) {
             return false;
         }
 
@@ -108,14 +111,14 @@ public function shouldRedirectToMFA(Member $member): bool
             return false;
         }
 
-        if (!$this->isUserInMFAEnabledGroup($member) && !$this->hasCompletedRegistration($member)) {
-            return false;
-        }
-
         if ($member->RegisteredMFAMethods()->exists()) {
             return true;
         }
 
+        if (!$this->isUserInMFAEnabledGroup($member) && !$this->hasCompletedRegistration($member)) {
+            return false;
+        }
+
         if ($this->isGracePeriodInEffect()) {
             return true;
         }
@@ -156,7 +159,9 @@ public function hasCompletedRegistration(Member $member): bool
      * Whether MFA is required for eligible users. This takes into account whether a grace period is set and whether
      * we're currently inside the window for it.
      *
-     * Note that in determining this, we ignore whether or not MFA is enabled for the site in general.
+     * Note that in determining this, we ignore whether or not MFA is enabled for the site in general. We also ignore
+     * whether any given member is in the list of groups for which MFA has been enabled - for that, call
+     * {@see isUserInMFAEnabledGroup()}
      *
      * @return bool
      */
@@ -276,9 +281,16 @@ protected function isUserInMFAEnabledGroup(Member $member): bool
         /** @var SiteConfig&SiteConfigExtension $siteConfig */
         $siteConfig = SiteConfig::current_site_config();
 
+        // If we aren't restricting by groups, then we pass this check and move on
+        // to any other applicable checks.
+        if ($siteConfig->MFAAppliesTo !== self::APPLIES_TO_GROUPS) {
+            return true;
+        }
+
         $groups = $siteConfig->MFAGroupRestrictions();
 
-        // If no groups are set in the Site Config MFAGroupRestrictions field, MFA is enabled for all users
+        // If no groups are set in the Site Config MFAGroupRestrictions field, MFA is enabled for all users.
+        // This should generally not be possible, but can happen if a group is deleted after being set in that field.
         if ($groups->count() === 0) {
             return true;
         }

diff --git a/tests/Behat/Context/LoginContext.php b/tests/Behat/Context/LoginContext.php
@@ -44,7 +44,7 @@ public function multiFactorAuthenticationIsOptional()
      */
     public function iSelectFromTheMfaSettings($option)
     {
-        $value = $option === 'MFA is required for everyone' ? 1 : 0;
+        $value = $option === 'MFA is required' ? 1 : 0;
         $this->getMainContext()->selectOption('MFARequired', $value);
     }
 }
diff --git a/tests/Behat/features/mfa-enabled.feature b/tests/Behat/features/mfa-enabled.feature
@@ -8,10 +8,42 @@ Feature: MFA is enabled for the site
     And I go to "/admin"
     Then I should see the CMS
 
-  Scenario: I can set MFA to be required
+  Scenario: I can set MFA to be required for all users
     Given I go to "/admin/settings"
     And I click the "Access" CMS tab
     Then I should see "Multi-factor authentication (MFA)"
-    When I select "MFA is required for everyone" from the MFA settings
+    When I select "MFA is required" from the MFA settings
     And I press "Save"
     Then I should see a "Saved" success toast
+
+  Scenario: I can set MFA to be required for a given group
+    Given I go to "/admin/settings"
+    And I click the "Access" CMS tab
+    Then I should see "Multi-factor authentication (MFA)"
+    When I select "MFA is required" from the MFA settings
+    And I select "Only these groups (choose from list)" from "Who do these MFA settings apply to?" input group
+    And I select "ADMIN group" in the "#Form_EditForm_MFAGroupRestrictions_Holder" tree dropdown
+    And I press "Save"
+    Then I should see a "Saved" success toast
+    Then I should not see "At least one group must be selected, or the MFA settings should apply to everyone."
+
+  Scenario: I can set MFA to be optional for a given group
+    Given I go to "/admin/settings"
+    And I click the "Access" CMS tab
+    Then I should see "Multi-factor authentication (MFA)"
+    When I select "MFA is optional" from the MFA settings
+    And I select "Only these groups (choose from list)" from "Who do these MFA settings apply to?" input group
+    And I select "ADMIN group" in the "#Form_EditForm_MFAGroupRestrictions_Holder" tree dropdown
+    And I press "Save"
+    Then I should see a "Saved" success toast
+    Then I should not see "At least one group must be selected, or the MFA settings should apply to everyone."
+
+  Scenario: I must add at least one group if requiring MFA for groups
+    Given I go to "/admin/settings"
+    And I click the "Access" CMS tab
+    Then I should see "Multi-factor authentication (MFA)"
+    When I select "MFA is required" from the MFA settings
+    And I select "Only these groups (choose from list)" from "Who do these MFA settings apply to?" input group
+    And I press "Save"
+    Then I should not see a "Saved" success toast
+    Then I should see "At least one group must be selected, or the MFA settings should apply to everyone."
diff --git a/tests/php/Authenticator/LoginHandlerTest.php b/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\ORM\ValidationException;
 use SilverStripe\Security\Group;
 use SilverStripe\Security\Member;
 use SilverStripe\Security\Security;
@@ -573,42 +574,69 @@ public function testGetBackURL()
         $this->assertSame('foobar', $handler->getBackURL());
     }
 
+    public function testMFAGroupRestrictionValidation()
+    {
+        $config = SiteConfig::current_site_config();
+        $config->MFAAppliesTo = EnforcementManager::APPLIES_TO_GROUPS;
+        $this->expectException(ValidationException::class);
+        $config->write();
+    }
+
+    public function provideMFAGroupRestriction()
+    {
+        return [
+            'member in group, has registered method' => [
+                'memberFixture' => 'simon',
+                'hasMFA' => true,
+            ],
+            'member in group, with no registered method' => [
+                'memberFixture' => 'guy',
+                'hasMFA' => true,
+            ],
+            'member not in group, has registered method' => [
+                'memberFixture' => 'colin',
+                'hasMFA' => true,
+            ],
+            'member not in group, with no registered method' => [
+                'memberFixture' => 'carla',
+                'hasMFA' => false,
+            ],
+        ];
+    }
 
-    public function testMFAGroupRestriction()
+    /**
+     * @dataProvider provideMFAGroupRestriction
+     */
+    public function testMFAGroupRestriction(string $memberFixture, bool $hasMFA)
     {
         $config = SiteConfig::current_site_config();
 
         /** @var Group $group */
         $group = $this->objFromFixture(Group::class, 'admingroup');
         $config->MFAGroupRestrictions()->add($group);
+        $config->MFAAppliesTo = EnforcementManager::APPLIES_TO_GROUPS;
+        $config->write();
 
-        // 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');
+        $member = $this->objFromFixture(Member::class, $memberFixture);
 
         $this->autoFollowRedirection = false;
         $response = $this->doLogin($member, 'Password123');
         $this->autoFollowRedirection = true;
 
+        // Validate that MFA is only enabled for the relevant group, and for users who already registered an MFA method
+        $path = $hasMFA ? 'default/mfa' : 'default';
         $this->assertSame(302, $response->getStatusCode());
         $this->assertStringEndsWith(
-            Controller::join_links(Security::login_url(), 'default/mfa'),
+            Controller::join_links(Security::login_url(), $path),
             $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')
-        );
+        // Validate that the member logged in successfully if MFA was not available.
+        if (!$hasMFA) {
+            $response = $this->get('/Security/login');
+            $this->assertExactMatchBySelector('#MemberLoginForm_LoginForm_error', "You're logged in as {$member->FirstName}.");
+        }
     }
 
     public function methodlessMemberFixtureProvider()

diff --git a/tests/php/Authenticator/LoginHandlerTest.yml b/tests/php/Authenticator/LoginHandlerTest.yml
@@ -25,30 +25,39 @@ SilverStripe\Security\Group:
 
 SilverStripe\Security\Member:
   guy:
+    FirstName: Guy
     Email: [email protected]
     Password: Password123
     PasswordExpiry: 2030-01-01
     Groups: =>SilverStripe\Security\Group.admingroup
   simon:
+    FirstName: Simon
     Email: [email protected]
+    Password: Password123
     RegisteredMFAMethods: =>SilverStripe\MFA\Model\RegisteredMethod.simon-math
     Groups: =>SilverStripe\Security\Group.admingroup
   robbie:
+    FirstName: Robbie
     Email: [email protected]
     RegisteredMFAMethods: =>SilverStripe\MFA\Model\RegisteredMethod.robbie-math
     DefaultRegisteredMethodID: =>SilverStripe\MFA\Model\RegisteredMethod.robbie-math
     Groups: =>SilverStripe\Security\Group.admingroup
   colin:
+    FirstName: Colin
     Email: [email protected]
     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
   carla:
+    FirstName: Carla
     Email: [email protected]
+    Password: Password123
     Groups: =>SilverStripe\Security\Group.contentgroup
   pete:
+    FirstName: Pete
     Email: [email protected]
+    Password: Password123
     PasswordExpiry: 1990-01-01
     Groups: =>SilverStripe\Security\Group.contentgroup