Skip to content

Commit

Permalink
Merge pull request #218 from creative-commoners/pulls/4.0/sudo-mode-t…
Browse files Browse the repository at this point in the history 
…imeout-handling

FIX Redirect user back when accessing MFA with a stale session
  • Loading branch information
Garion Herman authored Jun 25, 2019
2 parents 6880ff9 + 8dc0740 commit 284e533
Show file tree
Hide file tree
Showing 7 changed files with 63 additions and 31 deletions.
2 changes: 1 addition & 1 deletion client/dist/js/bundle-cms.js
View file

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion client/dist/js/bundle.js
View file

Large diffs are not rendered by default.

7 changes: 6 additions & 1 deletion client/src/components/BasicMath/Login.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,12 @@ class Login extends Component {
const { ss: { i18n } } = window;

if (!numbers) {
return <LoadingIndicator block />;
return (
<div>
<LoadingIndicator block />
{ moreOptionsControl }
</div>
);
}

return (
Expand Down
42 changes: 24 additions & 18 deletions src/Authenticator/LoginHandler.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,10 +125,10 @@ public function doLogin($data, MemberLoginForm $form, HTTPRequest $request)
*
* @return HTTPResponse|array
*/
public function mfa()
public function mfa(HTTPRequest $request)
{
$store = $this->getStore();
if (!$store || !$store->getMember()) {
if (!$store || !$store->getMember() || !$this->getSudoModeService()->check($request->getSession())) {
return $this->redirectBack();
}

Expand Down Expand Up @@ -178,11 +178,16 @@ public function startRegistration(HTTPRequest $request): HTTPResponse
$sessionMember = $store ? $store->getMember() : null;
$loggedInMember = Security::getCurrentUser();

if ((is_null($loggedInMember) && is_null($sessionMember))
if (($loggedInMember === null && $sessionMember === null)
|| !$this->getSudoModeService()->check($request->getSession())
) {
return $this->jsonResponse(
['errors' => [_t(__CLASS__ . '.NOT_AUTHENTICATING', 'You must be logged or logging in')]],
['errors' => [
_t(
__CLASS__ . '.NOT_AUTHENTICATING',
'You must be logged in or logging in. Please refresh the page and try again.'
)
]],
403
);
}
Expand Down Expand Up @@ -236,11 +241,16 @@ public function finishRegistration(HTTPRequest $request): HTTPResponse
$sessionMember = $store ? $store->getMember() : null;
$loggedInMember = Security::getCurrentUser();

if ((is_null($loggedInMember) && is_null($sessionMember))
if (($loggedInMember === null && $sessionMember === null)
|| !$this->getSudoModeService()->check($request->getSession())
) {
return $this->jsonResponse(
['errors' => [_t(__CLASS__ . '.NOT_AUTHENTICATING', 'You must be logged or logging in')]],
['errors' => [
_t(
__CLASS__ . '.NOT_AUTHENTICATING',
'You must be logged in or logging in. Please refresh the page and try again.'
)
]],
403
);
}
Expand Down Expand Up @@ -320,10 +330,8 @@ public function skipRegistration(HTTPRequest $request): HTTPResponse
public function startVerification(HTTPRequest $request): HTTPResponse
{
$store = $this->getStore();
$member = $store->getMember();

// If we don't have a valid member we shouldn't be here, or if sudo mode is not active yet.
if (!$member || !$this->getSudoModeService()->check($request->getSession())) {
if (!$store || !$store->getMember() || !$this->getSudoModeService()->check($request->getSession())) {
return $this->jsonResponse(['message' => 'Forbidden'], 403);
}

Expand All @@ -349,23 +357,21 @@ public function startVerification(HTTPRequest $request): HTTPResponse
public function finishVerification(HTTPRequest $request): HTTPResponse
{
$store = $this->getStore();
$member = $store->getMember();

if ($member->isLockedOut()) {
// Enforce sudo mode
if (!$this->getSudoModeService()->check($request->getSession())) {
return $this->jsonResponse([
'message' => _t(
__CLASS__ . '.LOCKED_OUT',
'Your account is temporarily locked. Please try again later.'
__CLASS__ . '.SUDO_MODE_REQUIRED',
'You need to re-verify your account before continuing. Please reload and try again.'
),
], 403);
}

// Enforce sudo mode
if (!$this->getSudoModeService()->check($request->getSession())) {
if ($store && ($member = $store->getMember()) && $member->isLockedOut()) {
return $this->jsonResponse([
'message' => _t(
__CLASS__ . '.SUDO_MODE_REQUIRED',
'You need to re-verify your account before continuing. Please reload and try again.'
__CLASS__ . '.LOCKED_OUT',
'Your account is temporarily locked. Please try again later.'
),
], 403);
}
Expand Down
2 changes: 1 addition & 1 deletion src/Controller/AdminRegistrationController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ public function finishRegistration(HTTPRequest $request): HTTPResponse

if (!$store || !$this->getSudoModeService()->check($request->getSession())) {
return $this->jsonResponse(
['errors' => [_t(__CLASS__ . '.INVALID_SESSION', 'Invalid session. Please try again')]],
['errors' => [_t(__CLASS__ . '.INVALID_SESSION', 'Invalid session. Please refresh and try again.')]],
400
);
}
Expand Down
37 changes: 29 additions & 8 deletions tests/php/Authenticator/LoginHandlerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace SilverStripe\MFA\Tests\Authenticator;

use PHPUnit_Framework_MockObject_MockObject;
use SilverStripe\Control\Controller;
use SilverStripe\Control\HTTPRequest;
use SilverStripe\Control\HTTPResponse;
use SilverStripe\Control\Session;
Expand All @@ -24,8 +25,8 @@
use SilverStripe\ORM\FieldType\DBDatetime;
use SilverStripe\Security\Member;
use SilverStripe\Security\Security;
use SilverStripe\SecurityExtensions\Service\SudoModeServiceInterface;
use SilverStripe\Security\SecurityToken;
use SilverStripe\SecurityExtensions\Service\SudoModeServiceInterface;
use SilverStripe\SiteConfig\SiteConfig;

class LoginHandlerTest extends FunctionalTest
Expand Down Expand Up @@ -63,7 +64,27 @@ public function testMFAStepIsAdded()
$this->autoFollowRedirection = true;

$this->assertSame(302, $response->getStatusCode());
$this->assertStringEndsWith('/Security/login/default/mfa', $response->getHeader('location'));
$this->assertStringEndsWith(
Controller::join_links(Security::login_url(), 'default/mfa'),
$response->getHeader('location')
);
}

public function testMFARedirectsBackWhenSudoModeIsInactive()
{
/** @var SudoModeServiceInterface&PHPUnit_Framework_MockObject_MockObject $sudoModeService */
$sudoModeService = $this->createMock(SudoModeServiceInterface::class);
$sudoModeService->expects($this->once())->method('check')->willReturn(false);
Injector::inst()->registerService($sudoModeService, SudoModeServiceInterface::class);

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

$this->autoFollowRedirection = false;
$response = $this->get(Controller::join_links(Security::login_url(), 'default/mfa'));

$this->assertSame(302, $response->getStatusCode());
}

public function testMethodsNotBeingAvailableWillLogin()
Expand All @@ -88,7 +109,7 @@ public function testMFASchemaEndpointIsNotAccessibleByDefault()
{
// Assert that this endpoint is not available if you haven't started the login process
$this->autoFollowRedirection = false;
$response = $this->get('Security/login/default/mfa/schema');
$response = $this->get(Controller::join_links(Security::login_url(), 'default/mfa/schema'));
$this->autoFollowRedirection = true;

$this->assertSame(302, $response->getStatusCode());
Expand All @@ -101,7 +122,7 @@ public function testMFASchemaEndpointReturnsMethodDetails()
$member = $this->objFromFixture(Member::class, 'guy');
$this->scaffoldPartialLogin($member);

$result = $this->get('Security/login/default/mfa/schema');
$result = $this->get(Controller::join_links(Security::login_url(), 'default/mfa/schema'));

$response = json_decode($result->getBody(), true);

Expand Down Expand Up @@ -136,7 +157,7 @@ public function testMFASchemaEndpointShowsRegisteredMethodsIfSetUp()
$member = $this->objFromFixture(Member::class, 'simon');
$this->scaffoldPartialLogin($member);

$result = $this->get('Security/login/default/mfa/schema');
$result = $this->get(Controller::join_links(Security::login_url(), 'default/mfa/schema'));

$response = json_decode($result->getBody(), true);

Expand Down Expand Up @@ -167,7 +188,7 @@ public function testMFASchemaEndpointProvidesDefaultMethodIfSet()
$member = $this->objFromFixture(Member::class, 'robbie');
$this->scaffoldPartialLogin($member);

$result = $this->get('Security/login/default/mfa/schema');
$result = $this->get(Controller::join_links(Security::login_url(), 'default/mfa/schema'));

$response = json_decode($result->getBody(), true);

Expand Down Expand Up @@ -196,7 +217,7 @@ public function testCannotSkipMFA($mfaRequired, $member = 'robbie')
$this->scaffoldPartialLogin($this->objFromFixture(Member::class, $member));
}

$response = $this->get('Security/login/default/mfa/skip');
$response = $this->get(Controller::join_links(Security::login_url(), 'default/mfa/skip'));
$this->assertContains('You cannot skip MFA registration', $response->getBody());
}

Expand All @@ -222,7 +243,7 @@ public function testSkipRegistration()
$memberId = $member->write();
$this->logInAs($member);

$response = $this->get('Security/login/default/mfa/skip');
$response = $this->get(Controller::join_links(Security::login_url(), 'default/mfa/skip'));

$this->assertSame(200, $response->getStatusCode());

Expand Down
2 changes: 1 addition & 1 deletion tests/php/Authenticator/RegisterHandlerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ public function testEnforcesSudoMode()

$response = $this->post(self::URL, ['dummy' => 'data'], null, $this->session(), json_encode(['number' => 7]));
$this->assertSame(403, $response->getStatusCode());
$this->assertContains('You must be logged or logging in', (string) $response->getBody());
$this->assertContains('You must be logged in or logging in', (string) $response->getBody());
}

/**
Expand Down

0 comments on commit 284e533

Please sign in to comment.