Merge pull request #7067 from open-sausages/pulls/4.0/security-merge

[SS-2017-002] FIX Lock out users who dont exist in the DB

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";

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();
         }
     }

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;

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());
+    }
 }

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'
         );