Skip to content

Commit

Permalink
FIX Backport EnforcementManager fixes to 3.0 (#382)
Browse files Browse the repository at this point in the history 
* FIX Members minimal CMS access were previously stuck in a login loop, now redirects to MFA

* FIX Check whether user is eligible for MFA in canSkipMFA method

Users with no CMS access are not eligible to use MFA by default. This
was respected in the shouldRedirectToMFA method, but not in canSkipMFA,
which resulted in these users being booted back to the login screen
whenever they attempted to log in.

This commit also tidies some of the docblocks in EnforcementManager, and
adjusts the hasAdminAccess method to always act as the provided Member.

* FIX Increase code coverage, correct logic in grace period tests

* FIX Add code coverage for EnforcementManager::hasCompletedRegistration

* FIX Remove redundant Config nesting in EnforcementManagerTest

* FIX Ensure canSkipMFA respects whether MFA is enabled

Previously the canSkipMFA method did not validate whether MFA was
available, which resulted in a login loop if any other condition stopped
the user from skipping, such as MFA being mandated via SiteConfig.

* DOCS Add inline explainers to requires_admin_access checks

Co-authored-by: Robbie Averill <[email protected]>
  • Loading branch information
@robbieaverill
Garion Herman and robbieaverill authored Jul 15, 2020
1 parent b0210dc commit 8563416
Show file tree
Hide file tree
Showing 3 changed files with 170 additions and 57 deletions.
128 changes: 78 additions & 50 deletions src/Service/EnforcementManager.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@

use LeftAndMain;
use Config;
use Security;
use Session;
use SilverStripe\MFA\Authenticator\MemberAuthenticator;
use SilverStripe\MFA\Extension\MemberExtension;
use SilverStripe\MFA\Extension\SiteConfigExtension;
use Date as DBDate;
Expand Down Expand Up @@ -46,44 +49,51 @@ class EnforcementManager extends SS_Object
private static $enabled = true;

/**
* Whether the current member can skip the multi-factor authentication registration process.
* Whether the provided member can skip the MFA registration process.
*
* This is determined by a combination of:
* - Whether MFA is required or optional
* - If MFA is required, whether there is a grace period
* - If MFA is required and there is a grace period, whether we're currently within that timeframe
*
* - Whether MFA is enabled and there are methods available for use
* - Whether the user has admin access (MFA is disabled by default for users that don't)
* - Whether MFA is required - @see EnforcementManager::isMFARequired()
* - Whether the user has registered MFA methods already
*
* @param Member&MemberExtension $member
* @return bool
*/
public function canSkipMFA(Member $member): bool
{
if (!$this->isEnabled()) {
return true;
}

// Users without admin access must skip MFA when it's limited to admin users
if ($this->config()->get('requires_admin_access') && !$this->hasAdminAccess($member)) {
return true;
}

if ($this->isMFARequired()) {
return false;
}

// If they've already registered MFA methods we will not allow them to skip the authentication process
$registeredMethods = $member->RegisteredMFAMethods();
if ($registeredMethods->exists()) {
if ($member->RegisteredMFAMethods()->exists()) {
return false;
}

// MFA is optional, or is required but might be within a grace period (see isMFARequired)
return true;
}

/**
* Whether the authentication process should redirect the user to multi-factor authentication registration or
* login.
* Whether the authentication process should redirect the provided user to MFA registration or login.
*
* This is determined by a combination of:
* - Whether MFA is enabled
* - Whether MFA is required or optional
* - Whether the user has registered MFA methods already
* - If the user doesn't have any registered MFA methods already, and MFA is optional, whether the user has opted
* to skip the registration process
*
* Note that in determining this, we ignore whether or not MFA is enabled for the site in general.
* - Whether MFA is enabled and there are methods available for use
* - Whether the user has admin access (MFA is disabled by default for users that don't)
* - Whether the user has existing MFA methods registered
* - Whether a grace period is in effect (we always redirect eligible users in this case)
* - Whether MFA is mandatory (without a grace period or after it has expired)
* - Whether the user has previously opted to skip the registration process
*
* @param Member&MemberExtension $member
* @return bool
Expand All @@ -94,26 +104,20 @@ public function shouldRedirectToMFA(Member $member): bool
return false;
}

// Users without admin access must not be redirected to MFA when it's limited to admin users
if ($this->config()->get('requires_admin_access') && !$this->hasAdminAccess($member)) {
return false;
}

$methodRegistry = MethodRegistry::singleton();
$methods = $methodRegistry->getMethods();
// If there are no methods available excluding backup codes, do not redirect
if (!count($methods) || (count($methods) === 1 && $methodRegistry->getBackupMethod() !== null)) {
return false;
}

if ($member->RegisteredMFAMethods()->exists()) {
return true;
}

if ($this->isMFARequired()) {
if ($this->isGracePeriodInEffect()) {
return true;
}

if ($this->isGracePeriodInEffect()) {
if ($this->isMFARequired()) {
return true;
}

Expand All @@ -125,9 +129,8 @@ public function shouldRedirectToMFA(Member $member): bool
}

/**
* Check if the provided member has registered the required MFA methods. This includes a "back-up" method set in
* configuration plus at least one other method.
* Note that this method returns true if there is no backup method registered (and they have one other method
* Check if the provided member has registered the required MFA methods. This includes the default backup method
* if configured, and at least one other method.
*
* @param Member&MemberExtension $member
* @return bool
Expand All @@ -147,8 +150,8 @@ public function hasCompletedRegistration(Member $member): bool
}

/**
* Whether multi-factor authentication is required for site members. This also takes into account whether a
* grace period is set and whether we're currently inside the window for it.
* 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.
*
Expand Down Expand Up @@ -209,42 +212,67 @@ public function isGracePeriodInEffect(): bool
}

/**
* Decides whether the current user has access to any LeftAndMain controller, which indicates some level
* Decides whether the provided user has access to any LeftAndMain controller, which indicates some level
* of access to the CMS.
*
* See LeftAndMain::init().
*
* @see LeftAndMain::init()
* @param Member $member
* @return bool
*/
protected function hasAdminAccess(Member $member): bool
{
$leftAndMain = LeftAndMain::singleton();
if ($leftAndMain->canView($member)) {
return true;
}

// Look through all LeftAndMain subclasses to find if one permits the member to view
$menu = $leftAndMain->MainMenu();
foreach ($menu as $candidate) {
if (
$candidate->Link
&& $candidate->Link !== $leftAndMain->Link()
&& $candidate->MenuItem->controller
&& singleton($candidate->MenuItem->controller)->canView($member)
) {
// DANGER ZONE: CMS 3 has no 'Member::actAs' functionality, so we have to replicate it as closely as
// possible. This comes in the form of marking the member as logged in via the session for the duration
// of the permission checking operation.
$currentMemberID = Session::get("loggedInAs");
try {
Session::set('loggedInAs', $member->ID);

$leftAndMain = LeftAndMain::singleton();
if ($leftAndMain->canView($member)) {
return true;
}
}

return false;
// Look through all LeftAndMain subclasses to find if one permits the member to view
$menu = $leftAndMain->MainMenu(false);
foreach ($menu as $candidate) {
if (
$candidate->Link
&& $candidate->Link !== $leftAndMain->Link()
&& $candidate->MenuItem->controller
&& singleton($candidate->MenuItem->controller)->canView($member)
) {
return true;
}
}

return false;
} finally {
Session::set('loggedInAs', $currentMemberID);
}
}

/**
* MFA is enabled if:
*
* - The EnforcementManager::enabled configuration is set to true
* - There is at least one non-backup method available to register
*
* @return bool
*/
protected function isEnabled(): bool
{
return (bool) $this->config()->get('enabled');
if (!$this->config()->get('enabled')) {
return false;
}

$methodRegistry = MethodRegistry::singleton();
$methods = $methodRegistry->getMethods();
// If there are no methods available excluding backup codes, do not redirect
if (!count($methods) || (count($methods) === 1 && $methodRegistry->getBackupMethod() !== null)) {
return false;
}

return true;
}
}
93 changes: 86 additions & 7 deletions tests/php/Service/EnforcementManagerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,29 +28,72 @@ public function setUp()
]);

Config::inst()->update(EnforcementManager::class, 'requires_admin_access', true);
Config::inst()->update(EnforcementManager::class, 'enabled', true);
}

public function testUserCanSkipWhenMFAIsDisabled()
{
$this->setSiteConfig(['MFARequired' => true]);
Config::inst()->update(EnforcementManager::class, 'enabled', false);

/** @var Member $member */
$member = $this->objFromFixture(Member::class, 'sally_smith');
$this->assertTrue(EnforcementManager::create()->canSkipMFA($member));
}

public function testUserCanSkipWhenNoMethodsAreAvailable()
{
$this->setSiteConfig(['MFARequired' => true]);
Config::inst()->update(MethodRegistry::class, 'methods', null);

/** @var Member $member */
$member = $this->objFromFixture(Member::class, 'sally_smith');
$this->assertTrue(EnforcementManager::create()->canSkipMFA($member));
}

public function testUserWithoutCMSAccessCanSkipWhenCMSAccessIsRequired()
{
$this->setSiteConfig(['MFARequired' => true]);

/** @var Member $member */
$member = $this->objFromFixture(Member::class, 'sammy_smith');
$this->assertTrue(EnforcementManager::create()->canSkipMFA($member));
}

public function testUserWithoutCMSAccessCannotSkipWhenCMSAccessIsNotRequired()
{
$this->setSiteConfig(['MFARequired' => true]);
Config::inst()->update(EnforcementManager::class, 'requires_admin_access', false);

/** @var Member $member */
$member = $this->objFromFixture(Member::class, 'sammy_smith');
$this->assertFalse(EnforcementManager::create()->canSkipMFA($member));
}

public function testCannotSkipWhenMFAIsRequiredWithNoGracePeriod()
{
$this->setSiteConfig(['MFARequired' => true]);

$member = new Member();
/** @var Member $member */
$member = $this->objFromFixture(Member::class, 'reports_user');
$this->assertFalse(EnforcementManager::create()->canSkipMFA($member));
}

public function testCanSkipWhenMFAIsRequiredWithGracePeriodExpiringInFuture()
{
$this->setSiteConfig(['MFARequired' => true, 'MFAGracePeriodExpires' => '2019-01-30']);

$member = new Member();
/** @var Member $member */
$member = $this->objFromFixture(Member::class, 'reports_user');
$this->assertTrue(EnforcementManager::create()->canSkipMFA($member));
}

public function testCannotSkipWhenMFAIsRequiredWithGracePeriodExpiringInPast()
{
$this->setSiteConfig(['MFARequired' => true, 'MFAGracePeriodExpires' => '2018-12-25']);

$member = new Member();
/** @var Member $member */
$member = $this->objFromFixture(Member::class, 'reports_user');
$this->assertFalse(EnforcementManager::create()->canSkipMFA($member));
}

Expand Down Expand Up @@ -122,7 +165,29 @@ public function testShouldRedirectToMFAWhenMFAIsRequired()
{
$this->setSiteConfig(['MFARequired' => true]);
/** @var Member $member */
$member = $this->objFromFixture(Member::class, 'sally_smith');
$member = $this->objFromFixture(Member::class, 'sully_smith');
$member->logIn();

$this->assertTrue(EnforcementManager::create()->shouldRedirectToMFA($member));
}

public function testShouldRedirectToMFAWhenMFAIsRequiredWithGracePeriodExpiringInFuture()
{
$this->setSiteConfig(['MFARequired' => true, 'MFAGracePeriodExpires' => '2019-01-30']);

/** @var Member&MemberExtension $member */
$member = $this->objFromFixture(Member::class, 'sully_smith');
$member->logIn();

$this->assertTrue(EnforcementManager::create()->shouldRedirectToMFA($member));
}

public function testShouldRedirectToMFAWhenMFAIsRequiredWithGracePeriodExpiringInPast()
{
$this->setSiteConfig(['MFARequired' => true, 'MFAGracePeriodExpires' => '2018-12-25']);

/** @var Member&MemberExtension $member */
$member = $this->objFromFixture(Member::class, 'sully_smith');
$member->logIn();

$this->assertTrue(EnforcementManager::create()->shouldRedirectToMFA($member));
Expand All @@ -146,9 +211,7 @@ public function testShouldNotRedirectToMFAWhenMFAIsOptionalAndHasBeenSkipped()
$this->setSiteConfig(['MFARequired' => false]);

/** @var Member&MemberExtension $member */
$member = $this->objFromFixture(Member::class, 'sammy_smith');
$member->HasSkippedMFARegistration = true;
$member->write();
$member = $this->objFromFixture(Member::class, 'sully_smith');
$member->logIn();

$this->assertFalse(EnforcementManager::create()->shouldRedirectToMFA($member));
Expand Down Expand Up @@ -177,6 +240,22 @@ public function testShouldNotRedirectToMFAWhenNoMethodsAreRegisteredInTheSystem(
$this->assertFalse(EnforcementManager::create()->shouldRedirectToMFA($member));
}

public function testGracePeriodIsNotInEffectWhenMFAIsRequiredButNoGracePeriodIsSet()
{
$this->setSiteConfig(['MFARequired' => true]);
$this->assertFalse(EnforcementManager::create()->isGracePeriodInEffect());
}

public function testUserHasCompletedRegistrationWhenBackupMethodIsDisabled()
{
Config::inst()->update(MethodRegistry::class, 'default_backup_method', null);

/** @var Member $member */
$member = $this->objFromFixture(Member::class, 'sally_smith');

$this->assertTrue(EnforcementManager::create()->hasCompletedRegistration($member));
}

/**
* Helper method for changing the current SiteConfig values
*
Expand Down
6 changes: 6 additions & 0 deletions tests/php/Service/EnforcementManagerTest.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ Member:
FirstName: Sammy
Surname: Smith
Email: [email protected]
sully_smith:
FirstName: Sully
Surname: Smith
Email: [email protected]
Groups: =>Group.reportsgroup
HasSkippedMFARegistration: true
reports_user:
FirstName: Reports
Surname: User
Expand Down

0 comments on commit 8563416

Please sign in to comment.