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

security/Member.php

@@ -398,7 +398,35 @@ public function canLogIn() {
 	 * Returns true if this user is locked out
 	 */
 	public function isLockedOut() {
-		return $this->LockedOutUntil && SS_Datetime::now()->Format('U') < strtotime($this->LockedOutUntil);
+		global $debug;
+		if ($this->LockedOutUntil && $this->dbObject('LockedOutUntil')->InFuture()) {
+			return true;
+		}
+
+		if ($this->config()->lock_out_after_incorrect_logins <= 0) {
+			return false;
+		}
+
+		$attempts = LoginAttempt::get()->filter($filter = array(
+				'Email' => $this->{static::config()->unique_identifier_field},
+		))->sort('Created', 'DESC')->limit($this->config()->lock_out_after_incorrect_logins);
+
+		if ($attempts->count() < $this->config()->lock_out_after_incorrect_logins) {
+			return false;
+		}
+
+		foreach ($attempts as $attempt) {
+			if ($attempt->Status === 'Success') {
+				return false;
+			}
+		}
+
+		$lockedOutUntil = $attempts->first()->dbObject('Created')->Format('U') + ($this->config()->lock_out_delay_mins * 60);
+		if (SS_Datetime::now()->Format('U') < $lockedOutUntil) {
+			return true;
+		}
+
+		return false;
 	}
 
 	/**
@@ -1660,7 +1688,7 @@ public function changePassword($password) {
 	public function registerFailedLogin() {
 		if(self::config()->lock_out_after_incorrect_logins) {
 			// 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 >= self::config()->lock_out_after_incorrect_logins) {
 				$lockoutMins = self::config()->lock_out_delay_mins;
@@ -1679,6 +1707,7 @@ public function registerSuccessfulLogin() {
 		if(self::config()->lock_out_after_incorrect_logins) {
 			// Forgive all past login failures
 			$this->FailedLoginCount = 0;
+			$this->LockedOutUntil = null;
 			$this->write();
 		}
 	}

security/MemberAuthenticator.php

@@ -70,6 +70,14 @@ protected static function authenticate_member($data, $form, &$success) {
 		if($member && !$asDefaultAdmin) {
 			$result = $member->checkPassword($data['Password']);
 			$success = $result->valid();
+		} elseif (!$asDefaultAdmin) {
+			// spoof a login attempt
+			$member = Member::create();
+			$member->Email = $email;
+			$member->{Member::config()->unique_identifier_field} = $data['Password'] . '-wrong';
+			$member->PasswordEncryption = 'none';
+			$result = $member->checkPassword($data['Password']);
+			$member = null;
 		} else {
 			$result = new ValidationResult(false, _t('Member.ERRORWRONGCRED'));
 		}
@@ -94,7 +102,7 @@ protected static function authenticate_member($data, $form, &$success) {
 	 * @param bool $success
 	 */
 	protected static function record_login_attempt($data, $member, $success) {
-		if(!Security::config()->login_recording) return;
+		if(!Security::config()->login_recording && !Member::config()->lock_out_after_incorrect_logins) return;
 
 		// Check email is valid
 		$email = isset($data['Email']) ? $data['Email'] : null;

tests/security/MemberAuthenticatorTest.php

@@ -180,6 +180,45 @@ public function testDefaultAdminLockOut()
 		), $form);
 
 		$this->assertTrue(Member::default_admin()->isLockedOut());
-		$this->assertEquals(Member::default_admin()->LockedOutUntil, '2016-04-18 00:10:00');
+		$this->assertEquals('2016-04-18 00:10:00', Member::default_admin()->LockedOutUntil);
+	}
+
+	public function testNonExistantMemberGetsLoginAttemptRecorded()
+	{
+		Config::inst()->update('Member', 'lock_out_after_incorrect_logins', 1);
+		$email = 'notreal@example.com';
+		$this->assertFalse(Member::get()->filter(array('Email' => $email))->exists());
+		$this->assertCount(0, LoginAttempt::get());
+		$response = MemberAuthenticator::authenticate(array(
+			'Email' => $email,
+			'Password' => 'password',
+		));
+		$this->assertNull($response);
+		$this->assertCount(1, LoginAttempt::get());
+		$attempt = LoginAttempt::get()->first();
+		$this->assertEquals($email, $attempt->Email);
+		$this->assertEquals('Failure', $attempt->Status);
+
+	}
+
+	public function testNonExistantMemberGetsLockedOut()
+	{
+		Config::inst()->update('Member', 'lock_out_after_incorrect_logins', 1);
+		Config::inst()->update('Member', 'lock_out_delay_mins', 10);
+		$email = 'notreal@example.com';
+
+		$this->assertFalse(Member::get()->filter(array('Email' => $email))->exists());
+
+		$response = MemberAuthenticator::authenticate(array(
+			'Email' => $email,
+			'Password' => 'password'
+		));
+
+		$this->assertNull($response);
+		$member = new Member();
+		$member->Email = $email;
+
+		$this->assertTrue($member->isLockedOut());
+		$this->assertFalse($member->canLogIn()->valid());
 	}
 }

tests/security/SecurityTest.php

@@ -402,6 +402,7 @@ public function testChangePasswordFromLostPassword() {
 	public function testRepeatedLoginAttemptsLockingPeopleOut() {
 		$local = i18n::get_locale();
 		i18n::set_locale('en_US');
+		SS_Datetime::set_mock_now(DBField::create_field('SS_Datetime', '2017-05-22 00:00:00'));
 
 		Member::config()->lock_out_after_incorrect_logins = 5;
 		Member::config()->lock_out_delay_mins = 15;
@@ -418,10 +419,9 @@ public function testRepeatedLoginAttemptsLockingPeopleOut() {
 				);
 				$this->assertContains($this->loginErrorMessage(), Convert::raw2xml(_t('Member.ERRORWRONGCRED')));
 			} else {
-				// Fuzzy matching for time to avoid side effects from slow running tests
-				$this->assertGreaterThan(
-					time() + 14*60,
-					strtotime($member->LockedOutUntil),
+				$this->assertEquals(
+					SS_Datetime::now()->Format('U') + (15 * 60),
+					$member->dbObject('LockedOutUntil')->Format('U'),
 					'User has a lockout time set after too many failed attempts'
 				);
 			}
@@ -444,14 +444,12 @@ public function testRepeatedLoginAttemptsLockingPeopleOut() {
 			'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", $this->idFromFixture('Member', 'test'));
-		$member->LockedOutUntil = date('Y-m-d H:i:s', time() - 30);
-		$member->write();
+		// Move into the future so we can login again
+		SS_Datetime::set_mock_now(DBField::create_field('SS_Datetime', '2017-06-22 00:00:00'));
 		$this->doTestLoginForm('testuser@example.com' , '1nitialPassword');
 		$this->assertEquals(
-			$this->session()->inst_get('loggedInAs'),
 			$member->ID,
+			$this->session()->inst_get('loggedInAs'),
 			'After lockout expires, the user can login again'
 		);
 
@@ -471,8 +469,8 @@ public function testRepeatedLoginAttemptsLockingPeopleOut() {
 
 		$this->doTestLoginForm('testuser@example.com' , '1nitialPassword');
 		$this->assertEquals(
-			$this->session()->inst_get('loggedInAs'),
 			$member->ID,
+			$this->session()->inst_get('loggedInAs'),
 			'The user can login successfully after lockout expires, if staying below the threshold'
 		);