From 30986b4ea3d6a111e182f1970267696b991ea660 Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Tue, 9 May 2017 21:24:15 +0100 Subject: [PATCH] [SS-2017-002] FIX Lock out users who dont exist in the DB --- src/Security/LoginAttempt.php | 11 +++- src/Security/Member.php | 42 +++++++++++-- .../MemberAuthenticator.php | 6 +- .../php/Security/MemberAuthenticatorTest.php | 59 +++++++++++++++++++ tests/php/Security/SecurityTest.php | 41 ++++++------- 5 files changed, 130 insertions(+), 29 deletions(-) diff --git a/src/Security/LoginAttempt.php b/src/Security/LoginAttempt.php index 0ea551391..d6e84fff8 100644 --- a/src/Security/LoginAttempt.php +++ b/src/Security/LoginAttempt.php @@ -24,6 +24,15 @@ use SilverStripe\ORM\DataObject; */ class LoginAttempt extends DataObject { + /** + * Success status + */ + const SUCCESS = 'Success'; + + /** + * Failure status + */ + const FAILURE = 'Failure'; private static $db = array( 'Email' => 'Varchar(255)', @@ -32,7 +41,7 @@ class LoginAttempt extends DataObject ); private static $has_one = array( - 'Member' => 'SilverStripe\\Security\\Member', // only linked if the member actually exists + 'Member' => Member::class, // only linked if the member actually exists ); private static $table_name = "LoginAttempt"; diff --git a/src/Security/Member.php b/src/Security/Member.php index 30cac32d6..4fc4b633a 100644 --- a/src/Security/Member.php +++ b/src/Security/Member.php @@ -350,13 +350,44 @@ class Member extends DataObject */ public function isLockedOut() { - if (!$this->LockedOutUntil) { + /** @var DBDatetime $lockedOutUntilObj */ + $lockedOutUntilObj = $this->dbObject('LockedOutUntil'); + if ($lockedOutUntilObj->InFuture()) { + return true; + } + + $maxAttempts = $this->config()->get('lock_out_after_incorrect_logins'); + if ($maxAttempts <= 0) { return false; } - /** @var DBDatetime $lockedOutUntil */ - $lockedOutUntil = $this->dbObject('LockedOutUntil'); - return DBDatetime::now()->getTimestamp() < $lockedOutUntil->getTimestamp(); + $idField = static::config()->get('unique_identifier_field'); + $attempts = LoginAttempt::get() + ->filter('Email', $this->{$idField}) + ->sort('Created', 'DESC') + ->limit($maxAttempts); + + if ($attempts->count() < $maxAttempts) { + return false; + } + + foreach ($attempts as $attempt) { + if ($attempt->Status === 'Success') { + return false; + } + } + + // Calculate effective LockedOutUntil + /** @var DBDatetime $firstFailureDate */ + $firstFailureDate = $attempts->first()->dbObject('Created'); + $maxAgeSeconds = $this->config()->get('lock_out_delay_mins') * 60; + $lockedOutUntil = $firstFailureDate->getTimestamp() + $maxAgeSeconds; + $now = DBDatetime::now()->getTimestamp(); + if ($now < $lockedOutUntil) { + return true; + } + + return false; } /** @@ -1652,7 +1683,7 @@ class Member extends DataObject $lockOutAfterCount = self::config()->get('lock_out_after_incorrect_logins'); if ($lockOutAfterCount) { // Keep a tally of the number of failed log-ins so that we can lock people out - $this->FailedLoginCount = $this->FailedLoginCount + 1; + ++$this->FailedLoginCount; if ($this->FailedLoginCount >= $lockOutAfterCount) { $lockoutMins = self::config()->get('lock_out_delay_mins'); @@ -1672,6 +1703,7 @@ class Member extends DataObject if (self::config()->get('lock_out_after_incorrect_logins')) { // Forgive all past login failures $this->FailedLoginCount = 0; + $this->LockedOutUntil = null; $this->write(); } } diff --git a/src/Security/MemberAuthenticator/MemberAuthenticator.php b/src/Security/MemberAuthenticator/MemberAuthenticator.php index c854ca629..b60401a50 100644 --- a/src/Security/MemberAuthenticator/MemberAuthenticator.php +++ b/src/Security/MemberAuthenticator/MemberAuthenticator.php @@ -99,7 +99,7 @@ class MemberAuthenticator implements Authenticator $member->registerFailedLogin(); } } elseif ($member) { - $member->registerSuccessfulLogin(); + $member->registerSuccessfulLogin(); } else { // A non-existing member occurred. This will make the result "valid" so let's invalidate $result->addError(_t( @@ -179,13 +179,13 @@ class MemberAuthenticator implements Authenticator if ($success && $member) { // successful login (member is existing with matching password) $attempt->MemberID = $member->ID; - $attempt->Status = 'Success'; + $attempt->Status = LoginAttempt::SUCCESS; // Audit logging hook $member->extend('authenticationSucceeded'); } else { // Failed login - we're trying to see if a user exists with this email (disregarding wrong passwords) - $attempt->Status = 'Failure'; + $attempt->Status = LoginAttempt::FAILURE; if ($member) { // Audit logging hook $attempt->MemberID = $member->ID; diff --git a/tests/php/Security/MemberAuthenticatorTest.php b/tests/php/Security/MemberAuthenticatorTest.php index 475138ded..3b638f169 100644 --- a/tests/php/Security/MemberAuthenticatorTest.php +++ b/tests/php/Security/MemberAuthenticatorTest.php @@ -11,6 +11,7 @@ use SilverStripe\ORM\ValidationResult; use SilverStripe\Security\Authenticator; use SilverStripe\Security\DefaultAdminService; use SilverStripe\Security\IdentityStore; +use SilverStripe\Security\LoginAttempt; use SilverStripe\Security\Member; use SilverStripe\Security\MemberAuthenticator\CMSMemberAuthenticator; use SilverStripe\Security\MemberAuthenticator\CMSMemberLoginForm; @@ -205,4 +206,62 @@ class MemberAuthenticatorTest extends SapphireTest $this->assertFalse($defaultAdmin->canLogin()); $this->assertEquals('2016-04-18 00:10:00', $defaultAdmin->LockedOutUntil); } + + public function testNonExistantMemberGetsLoginAttemptRecorded() + { + Security::config()->set('login_recording', true); + Member::config() + ->set('lock_out_after_incorrect_logins', 1) + ->set('lock_out_delay_mins', 10); + + $email = 'notreal@example.com'; + $this->assertFalse(Member::get()->filter(array('Email' => $email))->exists()); + $this->assertCount(0, LoginAttempt::get()); + $authenticator = new MemberAuthenticator(); + $result = new ValidationResult(); + $member = $authenticator->authenticate( + [ + 'Email' => $email, + 'Password' => 'password', + ], + Controller::curr()->getRequest(), + $result + ); + $this->assertFalse($result->isValid()); + $this->assertNull($member); + $this->assertCount(1, LoginAttempt::get()); + $attempt = LoginAttempt::get()->first(); + $this->assertEquals($email, $attempt->Email); + $this->assertEquals(LoginAttempt::FAILURE, $attempt->Status); + } + + public function testNonExistantMemberGetsLockedOut() + { + Security::config()->set('login_recording', true); + Member::config() + ->set('lock_out_after_incorrect_logins', 1) + ->set('lock_out_delay_mins', 10); + + $email = 'notreal@example.com'; + $this->assertFalse(Member::get()->filter(array('Email' => $email))->exists()); + + $authenticator = new MemberAuthenticator(); + $result = new ValidationResult(); + $member = $authenticator->authenticate( + [ + 'Email' => $email, + 'Password' => 'password', + ], + Controller::curr()->getRequest(), + $result + ); + + $this->assertNull($member); + $this->assertFalse($result->isValid()); + $member = new Member(); + $member->Email = $email; + + $this->assertTrue($member->isLockedOut()); + $this->assertFalse($member->canLogIn()); + } } diff --git a/tests/php/Security/SecurityTest.php b/tests/php/Security/SecurityTest.php index f9e71b913..ea1f77b83 100644 --- a/tests/php/Security/SecurityTest.php +++ b/tests/php/Security/SecurityTest.php @@ -282,7 +282,7 @@ class SecurityTest extends FunctionalTest Security::setCurrentUser($member); /* Visit the Security/logout page with a test referer, but without a security token */ - $response = $this->get( + $this->get( Config::inst()->get(Security::class, 'logout_url'), null, ['Referer' => Director::absoluteBaseURL() . 'testpage'] @@ -494,8 +494,11 @@ class SecurityTest extends FunctionalTest i18n::set_locale('en_US'); Member::config()->set('lock_out_after_incorrect_logins', 5); Member::config()->set('lock_out_delay_mins', 15); + DBDatetime::set_mock_now('2017-05-22 00:00:00'); // Login with a wrong password for more than the defined threshold + /** @var Member $member */ + $member = null; for ($i = 1; $i <= 6; $i++) { $this->doTestLoginForm('testuser@example.com', 'incorrectpassword'); /** @var Member $member */ @@ -513,38 +516,36 @@ class SecurityTest extends FunctionalTest ) ); } else { - // Fuzzy matching for time to avoid side effects from slow running tests - $this->assertGreaterThan( - time() + 14*60, - strtotime($member->LockedOutUntil), + // Lockout should be exactly 15 minutes from now + /** @var DBDatetime $lockedOutUntilObj */ + $lockedOutUntilObj = $member->dbObject('LockedOutUntil'); + $this->assertEquals( + DBDatetime::now()->getTimestamp() + (15 * 60), + $lockedOutUntilObj->getTimestamp(), 'User has a lockout time set after too many failed attempts' ); } } - $msg = _t( - 'SilverStripe\\Security\\Member.ERRORLOCKEDOUT2', - 'Your account has been temporarily disabled because of too many failed attempts at ' . - 'logging in. Please try again in {count} minutes.', - null, - array('count' => 15) - ); - $this->assertHasMessage($msg); - - + $msg = _t( + 'SilverStripe\\Security\\Member.ERRORLOCKEDOUT2', + 'Your account has been temporarily disabled because of too many failed attempts at ' . + 'logging in. Please try again in {count} minutes.', + null, + array('count' => 15) + ); + $this->assertHasMessage($msg); $this->doTestLoginForm('testuser@example.com', '1nitialPassword'); $this->assertNull( $this->session()->get('loggedInAs'), 'The user can\'t log in after being locked out, even with the right password' ); - // (We fake this by re-setting LockedOutUntil) - $member = DataObject::get_by_id(Member::class, $this->idFromFixture(Member::class, 'test')); - $member->LockedOutUntil = date('Y-m-d H:i:s', time() - 30); - $member->write(); + // Move into the future so we can login again + DBDatetime::set_mock_now('2017-06-22 00:00:00'); $this->doTestLoginForm('testuser@example.com', '1nitialPassword'); $this->assertEquals( - $this->session()->get('loggedInAs'), $member->ID, + $this->session()->get('loggedInAs'), 'After lockout expires, the user can login again' );