From 91e10891c3bed2ddc0fc88d949794a4a8bccd9e2 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Mon, 2 Oct 2023 13:56:44 +1300 Subject: [PATCH] ENH Add field for toggling between everyone and groups --- src/Extension/SiteConfigExtension.php | 36 +++++++++++++++++++++++---- src/Service/EnforcementManager.php | 26 +++++++++++++------ 2 files changed, 50 insertions(+), 12 deletions(-) diff --git a/src/Extension/SiteConfigExtension.php b/src/Extension/SiteConfigExtension.php index bfa3ff31..3c82014f 100644 --- a/src/Extension/SiteConfigExtension.php +++ b/src/Extension/SiteConfigExtension.php @@ -8,8 +8,10 @@ use SilverStripe\Forms\FieldList; use SilverStripe\Forms\ListboxField; use SilverStripe\Forms\OptionsetField; +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'); @@ -76,6 +81,15 @@ public function updateCMSFields(FieldList $fields) } asort($groupsMap); + $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)'), + ] + ); + $mfaGroupRestrict = ListboxField::create( "MFAGroupRestrictions", _t(__CLASS__ . '.MFA_GROUP_RESTRICTIONS', "MFA Groups") @@ -87,11 +101,10 @@ public function updateCMSFields(FieldList $fields) ) ->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' + 'MFA will only be enabled for members of these selected groups.' )); - $mfaOptions = CompositeField::create($mfaOptions, $mfaGraceEnd, $mfaGroupRestrict) + $mfaOptions = CompositeField::create($mfaOptions, $mfaGraceEnd, $mfaAppliesToWho, $mfaGroupRestrict) ->setTitle(DBField::create_field( 'HTMLFragment', _t(__CLASS__ . '.MULTI_FACTOR_AUTHENTICATION', 'Multi-factor authentication (MFA)') @@ -101,6 +114,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 index 3b6f6d4a..74eddbf5 100644 --- 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; }