From 467ff2837679a6947ac4cfdac6928803f7642956 Mon Sep 17 00:00:00 2001 From: Scott Sutherland Date: Fri, 30 Oct 2020 12:25:51 +1300 Subject: [PATCH] NEW Add functionality to limit MFA to specific user groups If a user has already registered for MFA, enforce use of it even if they are not in an MFA group Co-authored-by: Guy Marriott --- src/Extension/SiteConfigExtension.php | 30 ++++++++++++++- src/Service/EnforcementManager.php | 23 ++++++++++++ tests/Behat/features/mfa-enabled.feature | 2 +- tests/php/Authenticator/LoginHandlerTest.php | 39 ++++++++++++++++++++ tests/php/Authenticator/LoginHandlerTest.yml | 2 + tests/php/Service/EnforcementManagerTest.php | 29 +++++++++++++++ 6 files changed, 123 insertions(+), 2 deletions(-) diff --git a/src/Extension/SiteConfigExtension.php b/src/Extension/SiteConfigExtension.php index 186cd38f..bfa3ff31 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,29 @@ public function updateCMSFields(FieldList $fields) )); $mfaGraceEnd->addExtraClass('mfa-settings__grace-period'); - $mfaOptions = CompositeField::create($mfaOptions, $mfaGraceEnd) + $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' + )); + + $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 fb94be29..3b6f6d4a 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) && !$this->hasCompletedRegistration($member)) { + return false; + } + if ($member->RegisteredMFAMethods()->exists()) { return true; } @@ -266,4 +270,23 @@ protected function isEnabled(): bool return true; } + + protected function isUserInMFAEnabledGroup(Member $member): bool + { + /** @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/Behat/features/mfa-enabled.feature b/tests/Behat/features/mfa-enabled.feature index 1e18da60..3db7b49d 100644 --- a/tests/Behat/features/mfa-enabled.feature +++ b/tests/Behat/features/mfa-enabled.feature @@ -14,4 +14,4 @@ Feature: MFA is enabled for the site Then I should see "Multi-factor authentication (MFA)" When I select "MFA is required for everyone" from the MFA settings And I press "Save" - Then I should see "Saved" + Then I should see a "Saved" success toast diff --git a/tests/php/Authenticator/LoginHandlerTest.php b/tests/php/Authenticator/LoginHandlerTest.php index 9a851080..8173b492 100644 --- 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\Security\Group; use SilverStripe\Security\Member; use SilverStripe\Security\Security; use SilverStripe\Security\SecurityToken; @@ -572,6 +573,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 diff --git a/tests/php/Service/EnforcementManagerTest.php b/tests/php/Service/EnforcementManagerTest.php index 56c7f1d0..fa7cd5e7 100644 --- a/tests/php/Service/EnforcementManagerTest.php +++ b/tests/php/Service/EnforcementManagerTest.php @@ -9,6 +9,7 @@ use SilverStripe\MFA\Service\MethodRegistry; use SilverStripe\MFA\Tests\Stub\BasicMath\Method as BasicMathMethod; use SilverStripe\ORM\FieldType\DBDatetime; +use SilverStripe\Security\Group; use SilverStripe\Security\Member; use SilverStripe\SiteConfig\SiteConfig; @@ -253,6 +254,34 @@ public function testUserHasCompletedRegistrationWhenBackupMethodIsDisabled() $this->assertTrue(EnforcementManager::create()->hasCompletedRegistration($member)); } + public function testShouldRedirectToMFAWhenUserIsInMFARestrictedGroup() + { + $this->setSiteConfig(['MFARequired' => true]); + $config = SiteConfig::current_site_config(); + $group = $this->objFromFixture(Group::class, 'admingroup'); + $config->MFAGroupRestrictions()->add($group); + + /** @var Member $member */ + $member = $this->objFromFixture(Member::class, 'sally_smith'); + $this->logInAs($member); + + $this->assertTrue(EnforcementManager::create()->shouldRedirectToMFA($member)); + } + + public function testShouldNotRedirectToMFAWhenUserIsNotInMFARestrictedGroup() + { + $this->setSiteConfig(['MFARequired' => true]); + $config = SiteConfig::current_site_config(); + $group = $this->objFromFixture(Group::class, 'admingroup'); + $config->MFAGroupRestrictions()->add($group); + + /** @var Member $member */ + $member = $this->objFromFixture(Member::class, 'sully_smith'); + $this->logInAs($member); + + $this->assertFalse(EnforcementManager::create()->shouldRedirectToMFA($member)); + } + /** * Helper method for changing the current SiteConfig values *