From d22ca62c6f324548b7c494c84b39a4197e7a5f35 Mon Sep 17 00:00:00 2001
From: Thomas Speak <thomas.speak@gmail.com>
Date: Thu, 17 Oct 2013 18:42:50 +0100
Subject: [PATCH] BUG FailedLoginCount reset

If you fail your maximum login attempts and are locked out, further failed login attempts add to your already existing FailedLoginCount as it is only reset if you log in successfully. This means that if you're locked out, then try again, one failure will automatically lock you out again, regardless of what you set your max limit to.

Example:

lock_out_after_incorrect_logins: 3
FailedLoginCount: 0

The user fails three login attempts.

lock_out_after_incorrect_logins: 3
FailedLoginCount: 3

The user is now locked out.

Lockout time passes.

The user fails their 4th login.

lock_out_after_incorrect_logins: 3
FailedLoginCount: 4

This will continue to happen until the user successfully logs in, without giving them the pre-defined amount of login attempts again due to this condition being met after every incorrect login:
```php
if($this->FailedLoginCount >= self::config()->lock_out_after_incorrect_logins) {
```

FailedLoginTestCount Test Added
---
 security/Member.php           |  1 +
 tests/security/MemberTest.php | 48 ++++++++++++++++++++++++++++++++++-
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/security/Member.php b/security/Member.php
index e373a07d3..88f6d61dd 100644
--- a/security/Member.php
+++ b/security/Member.php
@@ -1412,6 +1412,7 @@ class Member extends DataObject implements TemplateGlobalProvider {
 			if($this->FailedLoginCount >= self::config()->lock_out_after_incorrect_logins) {
 				$lockoutMins = self::config()->lock_out_delay_mins;
 				$this->LockedOutUntil = date('Y-m-d H:i:s', time() + $lockoutMins*60);
+				$this->FailedLoginCount = 0;
 				$this->write();
 			}
 		}
diff --git a/tests/security/MemberTest.php b/tests/security/MemberTest.php
index 279a16848..990e8b02d 100644
--- a/tests/security/MemberTest.php
+++ b/tests/security/MemberTest.php
@@ -7,7 +7,7 @@ class MemberTest extends FunctionalTest {
 	protected static $fixture_file = 'MemberTest.yml';
 	
 	protected $orig = array();
-	protected $local = null; 
+	protected $local = null;
 	
 	protected $illegalExtensions = array(
 		'Member' => array(
@@ -698,6 +698,52 @@ class MemberTest extends FunctionalTest {
 		);
 	}
 
+	public function testFailedLoginCount() {
+		$maxFailedLoginsAllowed = 3;
+		//set up the config variables to enable login lockouts
+		Config::nest();
+		Config::inst()->update('Member', 'lock_out_after_incorrect_logins', $maxFailedLoginsAllowed);
+
+		$member = $this->objFromFixture('Member', 'test');
+		$failedLoginCount = $member->FailedLoginCount;
+
+		for ($i = 1; $i < $maxFailedLoginsAllowed; ++$i) {
+			$member->registerFailedLogin();
+
+			$this->assertEquals(
+				++$failedLoginCount,
+				$member->FailedLoginCount,
+				'Failed to increment $member->FailedLoginCount'
+			);
+
+			$this->assertFalse(
+				$member->isLockedOut(),
+				"Member has been locked out too early"
+			);
+		}
+
+		//fail login until max login attempts is reached
+		$member->FailedLoginCount = 0;
+		for ($i = 0; $i < $maxFailedLoginsAllowed; ++$i) {
+			$member->registerFailedLogin();
+		}
+		//check to see if they've been locked out
+		$this->assertTrue(
+			$member->isLockedOut(),
+			'Member was not locked out when max logins met'
+		);
+
+		//after they're locked out, need to check FailedLoginCount was reset to 0
+		$this->assertEquals(
+			$member->FailedLoginCount,
+			0,
+			'Failed login count was not reset after lockout'
+		);
+
+		//test all done, unnest config
+		Config::unnest();
+	}
+
 }
 class MemberTest_ViewingAllowedExtension extends DataExtension implements TestOnly {