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

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."