ENH Add field for toggling between everyone and groups

client/src/legacy/SiteConfig.js

@@ -1,9 +1,9 @@
 window.jQuery.entwine('ss', ($) => {
   $('[name="MFARequired"]').entwine({
     /**
-     * Enable or disable the associated "grace period" date field
+     * Set grace field enabled state based on whether it's enabled or not
      */
-    onchange() {
+    setGraceFieldState() {
       const isRequired = parseInt(this.val(), 10);
       if (isRequired) {
         $('.mfa-settings__grace-period').removeAttr('disabled');
@@ -13,10 +13,51 @@ window.jQuery.entwine('ss', ($) => {
     },
 
     /**
-     * Ensure the "grace period" hide/show handlers wake up when the page does
+     * Set correct grace field enabled state when value changes
+     */
+    onchange() {
+      this.setGraceFieldState();
+    },
+
+    /**
+     * Set correct grace field enabled state when form loads
+     */
+    onmatch() {
+      this.setGraceFieldState();
+    },
+  });
+
+  $('[name="MFAAppliesTo"]').entwine({
+    /**
+     * Set group field visibility based on which option is selected
+     */
+    setGroupFieldVisibility() {
+      // Ignore radio buttons that aren't selected.
+      if (!this.is(':checked')) {
+        return;
+      }
+
+      // Toggle display
+      const groupField = $('.js-mfa-group-restrictions');
+      if (this.val() === 'everyone') {
+        groupField.hide();
+      } else {
+        groupField.show();
+      }
+    },
+
+    /**
+     * Set group field visibility when selection changes
+     */
+    onchange() {
+      this.setGroupFieldVisibility();
+    },
+
+    /**
+     * Set group field visibility when form loads
      */
     onmatch() {
-      this.onchange();
+      this.setGroupFieldVisibility();
     },
   });
 });

docs/en/userguide/02_Administrator_manual/01_Configuring_MFA_site_settings.md

@@ -12,14 +12,14 @@ and select the **Access** tab.
 
 Here, you can select one of two modes of operation for MFA on your site:
 
-## MFA is optional for everyone (default)
+## MFA is optional (default)
 
 This is the default setting when MFA is installed. Everyone will be prompted to
 set up multi-factor authentication upon their first login, but they can skip the
 setup process and continue to log in as they did before. They will be able to
 set up MFA later via their Profile page in Silverstripe CMS.
 
-## MFA is required for everyone
+## MFA is required
 
 Everyone without MFA setup will be prompted with a message requiring them to
 setup MFA when they attempt to log in. If they choose not to proceed with setup,
@@ -37,3 +37,16 @@ be prompted with the option to set it up on every login, until MFA is set up.
 
 ![A screenshot of the site-wide MFA settings UI with the 'MFA is required for everyone' option selected and a date entered in the 'MFA will be required from' field](../_images/02-01-2-grace-period.png)
 </div>
+
+## Limit MFA to specific groups
+
+While it is generally recommended to apply the above settings to _all_ users globally,
+you can choose to limit MFA registration to specific groups by setting "Who do these
+MFA settings apply to?" to "Only these groups" and then selecting the relevant groups
+from the dropdown field.
+
+Note that any users who have already registered MFA for their account prior to changing
+this setting will still need to use MFA to log in, even if they are not in one of the
+groups you have selected.
+
+![A screenshot of the site-wide MFA settings UI with the 'Only these groups' option selected and a group entered in the 'MFA Groups' field](../_images/02-01-3-only-these-groups.png)

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,28 @@ 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 »
-            $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.'
+        ))->addExtraClass('js-mfa-group-restrictions');
+
+        $mfaOptions = CompositeField::create($mfaOptions, $mfaGraceEnd, $mfaAppliesToWho, $mfaGroupRestrict)
             ->setTitle(DBField::create_field(
                 'HTMLFragment',
                 _t(__CLASS__ . '.MULTI_FACTOR_AUTHENTICATION', 'Multi-factor authentication (MFA)')
@@ -101,6 +105,22 @@ 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
      *

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,17 +281,19 @@ 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;
         }
-        foreach ($groups as $group) {
-            if ($member->inGroup($group)) {
-                return true;
-            }
-        }
-        return false;
+        return $member->inGroups($groups);
     }
 }

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);
     }
 }

tests/Behat/features/mfa-enabled.feature

@@ -8,10 +8,46 @@ 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
+
+  # This scenario must be before any other "select a group" scenario, since the saved settings
+  # aren't reset between scenarios.
+  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."
+
+  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
+    Then I should not see "MFA Groups"
+    When I select "Only these groups (choose from list)" from "Who do these MFA settings apply to?" input group
+    Then I should see "MFA Groups"
+    When 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."