From 638b420ada1c03f964935e5d6d30c530d4f682bc Mon Sep 17 00:00:00 2001 From: Scott Sutherland Date: Fri, 30 Oct 2020 12:25:51 +1300 Subject: [PATCH] Add functionality to limit MFA to specific user groups --- src/Extension/SiteConfigExtension.php | 33 ++++++++++++++++- src/Service/EnforcementManager.php | 23 ++++++++++++ tests/php/Authenticator/LoginHandlerTest.php | 39 ++++++++++++++++++++ tests/php/Authenticator/LoginHandlerTest.yml | 2 + 4 files changed, 96 insertions(+), 1 deletion(-) diff --git a/src/Extension/SiteConfigExtension.php b/src/Extension/SiteConfigExtension.php index be142d8c..71812275 100644 --- a/src/Extension/SiteConfigExtension.php +++ b/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,32 @@ public function updateCMSFields(FieldList $fields) )); $mfaGraceEnd->addExtraClass('mfa-settings__grace-period'); - $mfaOptions = CompositeField::create($mfaOptions, $mfaGraceEnd) + $mapFn = function ($groups = []) { + $map = []; + foreach ($groups as $group) { + // Listboxfield values are escaped, use ASCII char instead of » + $map[$group->ID] = $group->getBreadcrumbs(' > '); + } + asort($map); + return $map; + }; + $groupsMap = $mapFn(Group::get()); + + $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)') diff --git a/src/Service/EnforcementManager.php b/src/Service/EnforcementManager.php index 4221aad9..a78ca9ea 100644 --- a/src/Service/EnforcementManager.php +++ b/src/Service/EnforcementManager.php @@ -108,6 +108,10 @@ public function shouldRedirectToMFA(Member $member): bool return false; } + if (!$this->isUserInMFAEnabledGroup($member)) { + return false; + } + if ($member->RegisteredMFAMethods()->exists()) { return true; } @@ -266,4 +270,23 @@ protected function isEnabled(): bool return true; } + + protected function isUserInMFAEnabledGroup(Member $member) + { + /** @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; + } } diff --git a/tests/php/Authenticator/LoginHandlerTest.php b/tests/php/Authenticator/LoginHandlerTest.php index fd908236..36572ed3 100644 --- a/tests/php/Authenticator/LoginHandlerTest.php +++ b/tests/php/Authenticator/LoginHandlerTest.php @@ -23,6 +23,7 @@ use SilverStripe\MFA\Store\StoreInterface; 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; @@ -537,6 +538,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']]; diff --git a/tests/php/Authenticator/LoginHandlerTest.yml b/tests/php/Authenticator/LoginHandlerTest.yml index 180bcb3c..c85be77f 100644 --- a/tests/php/Authenticator/LoginHandlerTest.yml +++ b/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