From 9d78eb7fe6f63eb1b630a032b2fba03eaa07b569 Mon Sep 17 00:00:00 2001
From: Damian Mooyman <damian@silverstripe.com>
Date: Fri, 24 Oct 2014 13:43:39 +1300
Subject: [PATCH] BUG Fix BasicAuth not resetting failed login counts on
 authentication

---
 security/Member.php              | 15 ++++++++++++---
 security/MemberAuthenticator.php |  2 ++
 tests/security/BasicAuthTest.php | 33 +++++++++++++++++++++++++++-----
 tests/security/BasicAuthTest.yml | 28 +++++++++++++++------------
 4 files changed, 58 insertions(+), 20 deletions(-)

diff --git a/security/Member.php b/security/Member.php
index 19f9c4292..8e7262f1d 100644
--- a/security/Member.php
+++ b/security/Member.php
@@ -463,9 +463,7 @@ class Member extends DataObject implements TemplateGlobalProvider {
 		}
 		
 		// Clear the incorrect log-in count
-		if(self::config()->lock_out_after_incorrect_logins) {
-			$this->FailedLoginCount = 0;
-		}
+		$this->registerSuccessfulLogin();
 		
 		// Don't set column if its not built yet (the login might be precursor to a /dev/build...)
 		if(array_key_exists('LockedOutUntil', DB::fieldList('Member'))) {
@@ -1560,6 +1558,17 @@ class Member extends DataObject implements TemplateGlobalProvider {
 			}
 		}
 	}
+
+	/**
+	 * Tell this member that a successful login has been made
+	 */
+	public function registerSuccessfulLogin() {
+		if(self::config()->lock_out_after_incorrect_logins) {
+			// Forgive all past login failures
+			$this->FailedLoginCount = 0;
+			$this->write();
+		}
+	}
 	
 	/**
 	 * Get the HtmlEditorConfig for this user to be used in the CMS.
diff --git a/security/MemberAuthenticator.php b/security/MemberAuthenticator.php
index cf96e5e25..4fa72b8cd 100644
--- a/security/MemberAuthenticator.php
+++ b/security/MemberAuthenticator.php
@@ -73,6 +73,8 @@ class MemberAuthenticator extends Authenticator {
 		if(!$success) {
 			if($member) $member->registerFailedLogin();
 			if($form) $form->sessionMessage($result->message(), 'bad');
+		} else {
+			if($member) $member->registerSuccessfulLogin();
 		}
 
 		return $member;
diff --git a/tests/security/BasicAuthTest.php b/tests/security/BasicAuthTest.php
index 686db0b50..8be7ce73e 100644
--- a/tests/security/BasicAuthTest.php
+++ b/tests/security/BasicAuthTest.php
@@ -14,15 +14,14 @@ class BasicAuthTest extends FunctionalTest {
 		parent::setUp();
 
 		// Fixtures assume Email is the field used to identify the log in identity
-		self::$original_unique_identifier_field = Member::config()->unique_identifier_field;
+		Config::nest();
 		Member::config()->unique_identifier_field = 'Email';
+		Member::config()->lock_out_after_incorrect_logins = 10;
 	}
 
 	public function tearDown() {
+		Config::unnest();
 		parent::tearDown();
-		
-		BasicAuth::protect_entire_site(false);
-		Member::config()->unique_identifier_field = self::$original_unique_identifier_field;
 	}
 
 	public function testBasicAuthEnabledWithoutLogin() {
@@ -104,7 +103,31 @@ class BasicAuthTest extends FunctionalTest {
 		$_SERVER['PHP_AUTH_USER'] = $origUser;
 		$_SERVER['PHP_AUTH_PW'] = $origPw;
 	}
-	
+
+	public function testBasicAuthFailureIncreasesFailedLoginCount() {
+		// Prior to login
+		$check = Member::get()->filter('Email', 'failedlogin@test.com')->first();
+		$this->assertEquals(0, $check->FailedLoginCount);
+
+		// First failed attempt
+		$_SERVER['PHP_AUTH_USER'] = 'failedlogin@test.com';
+		$_SERVER['PHP_AUTH_PW'] = 'test';
+		$response = Director::test('BasicAuthTest_ControllerSecuredWithoutPermission');
+		$check = Member::get()->filter('Email', 'failedlogin@test.com')->first();
+		$this->assertEquals(1, $check->FailedLoginCount);
+
+		// Second failed attempt
+		$_SERVER['PHP_AUTH_PW'] = 'testwrong';
+		$response = Director::test('BasicAuthTest_ControllerSecuredWithoutPermission');
+		$check = Member::get()->filter('Email', 'failedlogin@test.com')->first();
+		$this->assertEquals(2, $check->FailedLoginCount);
+
+		// successful basic auth should reset failed login count
+		$_SERVER['PHP_AUTH_PW'] = 'Password';
+		$response = Director::test('BasicAuthTest_ControllerSecuredWithoutPermission');
+		$check = Member::get()->filter('Email', 'failedlogin@test.com')->first();
+		$this->assertEquals(0, $check->FailedLoginCount);
+	}
 }
 
 class BasicAuthTest_ControllerSecuredWithPermission extends Controller implements TestOnly {
diff --git a/tests/security/BasicAuthTest.yml b/tests/security/BasicAuthTest.yml
index 4ab8b9441..fe8bd02d4 100644
--- a/tests/security/BasicAuthTest.yml
+++ b/tests/security/BasicAuthTest.yml
@@ -1,15 +1,19 @@
 Group:
-   mygroup:
-      Code: mygroup
+  mygroup:
+    Code: mygroup
 Member:
-   user-in-mygroup:
-      Email: user-in-mygroup@test.com
-      Password: test
-      Groups: =>Group.mygroup
-   user-without-groups:
-      Email: user-without-groups@test.com
-      Password: test
+  user-in-mygroup:
+    Email: user-in-mygroup@test.com
+    Password: test
+    Groups: =>Group.mygroup
+  user-without-groups:
+    Email: user-without-groups@test.com
+    Password: test
+  failed-login:
+    Email: failedlogin@test.com
+    Password: Password
+    FailedLoginCount: 0
 Permission:
-   mycode:
-      Code: MYCODE
-      Group: =>Group.mygroup
\ No newline at end of file
+  mycode:
+    Code: MYCODE
+    Group: =>Group.mygroup