From 89fc8e5fdd03c20922b8529549cfc072f3decaf6 Mon Sep 17 00:00:00 2001
From: Andrew O'Neil <andrew@silverstripe.com>
Date: Mon, 7 May 2012 15:03:53 +1200
Subject: [PATCH] APICHANGE: PasswordEncryptor::check() allows for more
 powerful password checking, deprecating PasswordEncryptor::compare()

---
 security/Member.php                      | 11 ++---------
 security/MemberPassword.php              |  9 ++-------
 security/PasswordEncryptor.php           | 21 +++++++++++++++++++++
 tests/security/PasswordEncryptorTest.php | 13 +++++++------
 4 files changed, 32 insertions(+), 22 deletions(-)

diff --git a/security/Member.php b/security/Member.php
index 0558c28d6..7a8193984 100644
--- a/security/Member.php
+++ b/security/Member.php
@@ -190,15 +190,8 @@ class Member extends DataObject implements TemplateGlobalProvider {
 	public function checkPassword($password) {
 		$result = $this->canLogIn();
 
-		$spec = Security::encrypt_password(
-			$password, 
-			$this->Salt, 
-			$this->PasswordEncryption,
-			$this
-		);
-		$e = $spec['encryptor'];
-
-		if(!$e->compare($this->Password, $spec['password'])) {
+		$e = PasswordEncryptor::create_for_algorithm($this->PasswordEncryption);
+		if(!$e->check($this->Password, $password, $this->Salt, $this)) {
 			$result->error(_t (
 				'Member.ERRORWRONGCRED',
 				'That doesn\'t seem to be the right e-mail address or password. Please try again.'
diff --git a/security/MemberPassword.php b/security/MemberPassword.php
index 02d18c940..df28ae4e3 100644
--- a/security/MemberPassword.php
+++ b/security/MemberPassword.php
@@ -42,13 +42,8 @@ class MemberPassword extends DataObject {
 	 * @return Boolean
 	 */	
 	function checkPassword($password) {
-		$spec = Security::encrypt_password(
-			$password, 
-			$this->Salt, 
-			$this->PasswordEncryption
-		);
-		$e = $spec['encryptor'];
-		return $e->compare($this->Password, $spec['password']);
+		$e = PasswordEncryptor::create_for_algorithm($this->PasswordEncryption);
+		return $e->check($this->Password, $password, $this->Salt, $this->Member());
 	}
 	
 	
diff --git a/security/PasswordEncryptor.php b/security/PasswordEncryptor.php
index 98624d5cb..9b5b80f69 100644
--- a/security/PasswordEncryptor.php
+++ b/security/PasswordEncryptor.php
@@ -109,10 +109,23 @@ abstract class PasswordEncryptor {
 	 * @param String $hash1
 	 * @param String $hash2
 	 * @return boolean
+	 *
+	 * @deprecated 3.0 - Use PasswordEncryptor::check() instead.
 	 */
 	function compare($hash1, $hash2) {
+		Deprecation::notice('3.0.0', 'PasswordEncryptor::compare() is deprecated, replaced by PasswordEncryptor::check().');
 		return ($hash1 === $hash2);
 	}
+
+	/**
+	 * This usually just returns a strict string comparison,
+	 * but is necessary for retain compatibility with password hashed
+	 * with flawed algorithms - see {@link PasswordEncryptor_LegacyPHPHash} and
+	 * {@link PasswordEncryptor_Blowfish}
+	 */
+	function check($hash, $password, $salt = null, $member = null) {
+		return $hash === $this->encrypt($password, $salt, $member);
+	}
 }
 
 /**
@@ -217,10 +230,18 @@ class PasswordEncryptor_LegacyPHPHash extends PasswordEncryptor_PHPHash {
 	}
 	
 	function compare($hash1, $hash2) {
+		Deprecation::notice('3.0.0', 'PasswordEncryptor::compare() is deprecated, replaced by PasswordEncryptor::check().');
+
 		// Due to flawed base_convert() floating poing precision, 
 		// only the first 10 characters are consistently useful for comparisons.
 		return (substr($hash1, 0, 10) === substr($hash2, 0, 10));
 	}
+
+	function check($hash, $password, $salt = null, $member = null) {
+		// Due to flawed base_convert() floating poing precision, 
+		// only the first 10 characters are consistently useful for comparisons.
+		return (substr($hash, 0, 10) === substr($this->encrypt($password, $salt, $member), 0, 10));
+	}
 }
 
 /**
diff --git a/tests/security/PasswordEncryptorTest.php b/tests/security/PasswordEncryptorTest.php
index 0a5096b80..d7a2a9634 100644
--- a/tests/security/PasswordEncryptorTest.php
+++ b/tests/security/PasswordEncryptorTest.php
@@ -72,11 +72,11 @@ class PasswordEncryptorTest extends SapphireTest {
 		);
 	}
 	
-	function testEncryptorPHPHashCompare() {
+	function testEncryptorPHPHashCheck() {
 		Config::inst()->update('PasswordEncryptor', 'encryptors', array('test_sha1'=>array('PasswordEncryptor_PHPHash'=>'sha1')));
 		$e = PasswordEncryptor::create_for_algorithm('test_sha1');
-		$this->assertTrue($e->compare(sha1('mypassword'), sha1('mypassword')));
-		$this->assertFalse($e->compare(sha1('mypassword'), sha1('mywrongpassword')));
+		$this->assertTrue($e->check(sha1('mypassword'), 'mypassword'));
+		$this->assertFalse($e->check(sha1('mypassword'), 'mywrongpassword'));
 	}
 	
 	/**
@@ -85,15 +85,16 @@ class PasswordEncryptorTest extends SapphireTest {
 	 * Handy command for reproducing via CLI on different architectures:
 	 * 	php -r "echo(base_convert(sha1('mypassword'), 16, 36));"
 	 */
-	function testEncryptorLegacyPHPHashCompare() {
+	function testEncryptorLegacyPHPHashCheck() {
 		Config::inst()->update('PasswordEncryptor', 'encryptors', array('test_sha1legacy'=>array('PasswordEncryptor_LegacyPHPHash'=>'sha1')));
 		$e = PasswordEncryptor::create_for_algorithm('test_sha1legacy');
 		// precomputed hashes for 'mypassword' from different architectures
 		$amdHash = 'h1fj0a6m4o6k0sosks88oo08ko4gc4s';
 		$intelHash = 'h1fj0a6m4o0g04ocg00o4kwoc4wowws';
 		$wrongHash = 'h1fjxxxxxxxxxxxxxxxxxxxxxxxxxxx';
-		$this->assertTrue($e->compare($amdHash, $intelHash));
-		$this->assertFalse($e->compare($amdHash, $wrongHash));
+		$this->assertTrue($e->check($amdHash, "mypassword"));
+		$this->assertTrue($e->check($intelHash, "mypassword"));
+		$this->assertFalse($e->check($wrongHash, "mypassword"));
 	}
 }