Merge pull request #290 from stojg/pull/passwordencryptor-config

API CHANGE Use Config for registering default password encryptors

_config.php

@@ -61,12 +61,6 @@ if(!defined('EMAIL_BOUNCEHANDLER_KEY')) {
 	define('EMAIL_BOUNCEHANDLER_KEY', '1aaaf8fb60ea253dbf6efa71baaacbb3');
 }
 
-PasswordEncryptor::register('none', 'PasswordEncryptor_None');
-PasswordEncryptor::register('md5', 'PasswordEncryptor_LegacyPHPHash("md5")');
-PasswordEncryptor::register('sha1','PasswordEncryptor_LegacyPHPHash("sha1")');
-PasswordEncryptor::register('md5_v2.4', 'PasswordEncryptor_PHPHash("md5")');
-PasswordEncryptor::register('sha1_v2.4','PasswordEncryptor_PHPHash("sha1")');
-
 // Zend_Cache temp directory setting
 $_ENV['TMPDIR'] = TEMP_FOLDER; // for *nix
 $_ENV['TMP'] = TEMP_FOLDER; // for Windows

_config/PasswordEncryptor.yml

@@ -0,0 +1,14 @@
+name: PasswordEncryptor
+---
+PasswordEncryptor:
+  encryptors:
+    none:
+      PasswordEncryptor_None: 
+    md5:
+      PasswordEncryptor_LegacyPHPHash: md5
+    sha1:
+      PasswordEncryptor_LegacyPHPHash: sha1
+    md5_v2.4:
+      PasswordEncryptor_PHPHash: md5
+    sha1_v2.4:
+      PasswordEncryptor_PHPHash: sha1

security/PasswordEncryptor.php

@@ -22,7 +22,7 @@ abstract class PasswordEncryptor {
 	 * @return Array Map of encryptor code to the used class.
 	 */
 	static function get_encryptors() {
-		return self::$encryptors;
+		return Config::inst()->get('PasswordEncryptor', 'encryptors');
 	}
 	
 	/**
@@ -36,6 +36,7 @@ abstract class PasswordEncryptor {
 	 * @param String $class Classname of a {@link PasswordEncryptor} subclass
 	 */
 	static function register($code, $class) {
+		Deprecation::notice('3.0', 'Use the Config system to register Password encryptors');
 		self::$encryptors[$code] = $class;
 	}
 	
@@ -43,29 +44,37 @@ abstract class PasswordEncryptor {
 	 * @param String $code Unique lookup.
 	 */
 	static function unregister($code) {
+		Deprecation::notice('3.0', 'Use the Config system to unregister Password encryptors');
 		if(isset(self::$encryptors[$code])) unset(self::$encryptors[$code]);
 	}
 	
 	/**
 	 * @param String $algorithm
-	 * @return PasswordEncryptor|Boolean Returns FALSE if class was not found
+	 * @return PasswordEncryptor
+	 * @throws PasswordEncryptor_NotFoundException
 	 */
 	static function create_for_algorithm($algorithm) {
-		if(!isset(self::$encryptors[$algorithm])) {
+		$encryptors = self::get_encryptors();
+		if(!isset($encryptors[$algorithm])) {
 			throw new PasswordEncryptor_NotFoundException(
 				sprintf('No implementation found for "%s"', $algorithm)
 			);
 		}
 		
-		$classWithArgs = self::$encryptors[$algorithm];
+		$class=key($encryptors[$algorithm]);
-		$class = (($p = strpos($classWithArgs, '(')) !== false) ? substr($classWithArgs, 0, $p) : $classWithArgs;
 		if(!class_exists($class)) {
 			throw new PasswordEncryptor_NotFoundException(
 				sprintf('No class found for "%s"', $class)
 			);
-		}
 
-		return eval("return new $classWithArgs;");
+		}
+		$refClass = new ReflectionClass($class);
+		if(!$refClass->getConstructor()) {
+			return new $class;
+		}
+		
+		$arguments = $encryptors[$algorithm];
+		return($refClass->newInstanceArgs($arguments));
 	}
 		
 	/**

tests/security/MemberAuthenticatorTest.php

@@ -30,8 +30,7 @@ class MemberAuthenticatorTest extends SapphireTest {
 	}
 	
 	function testNoLegacyPasswordHashMigrationOnIncompatibleAlgorithm() {
-		PasswordEncryptor::register('crc32', 'PasswordEncryptor_PHPHash("crc32")');
+		Config::inst()->update('PasswordEncryptor', 'encryptors', array('crc32'=>array('PasswordEncryptor_PHPHash'=>'crc32')));
-		
 		$field=Member::get_unique_identifier_field();
 		
 		$member = new Member();

tests/security/PasswordEncryptorTest.php

@@ -1,12 +1,26 @@
 <?php
 class PasswordEncryptorTest extends SapphireTest {
+
+	/**
+	 *
+	 * @var Config
+	 */
+	private $config = null;
+
+	public function setUp() {
+		parent::setUp();
+		$this->config = clone(Config::inst());
+	}
+
+	public function tearDown() {
+		parent::tearDown();
+		Config::set_instance($this->config);
+	}
+
 	function testCreateForCode() {
-		PasswordEncryptor::register('test', 'PasswordEncryptorTest_TestEncryptor');
+		Config::inst()->update('PasswordEncryptor', 'encryptors', array('test'=>array('PasswordEncryptorTest_TestEncryptor'=>null)));
 		$e = PasswordEncryptor::create_for_algorithm('test');
-		$this->assertType(
+		$this->assertType('PasswordEncryptorTest_TestEncryptor', $e );
-			'PasswordEncryptorTest_TestEncryptor',
-			$e
-		);
 	}
 	
 	/**
@@ -17,25 +31,27 @@ class PasswordEncryptorTest extends SapphireTest {
 	}
 	
 	function testRegister() {
-		PasswordEncryptor::register('test', 'PasswordEncryptorTest_TestEncryptor');
+		Config::inst()->update('PasswordEncryptor', 'encryptors', array('test'=>array('PasswordEncryptorTest_TestEncryptor'=>null)));
-		$this->assertContains('test', array_keys(PasswordEncryptor::get_encryptors()));
+		$encryptors = PasswordEncryptor::get_encryptors();
-		$this->assertContains('PasswordEncryptorTest_TestEncryptor', array_values(PasswordEncryptor::get_encryptors()));
+		$this->assertContains('test', array_keys($encryptors));
+		$encryptor = $encryptors['test'];
+		$this->assertContains('PasswordEncryptorTest_TestEncryptor', key($encryptor));
 	}
 	
 	function testUnregister() {
-		PasswordEncryptor::register('test', 'PasswordEncryptorTest_TestEncryptor');
+		Config::inst()->update('PasswordEncryptor', 'encryptors', array('test'=>array('PasswordEncryptorTest_TestEncryptor'=>null)));
-		PasswordEncryptor::unregister('test');
+		Config::inst()->remove('PasswordEncryptor', 'encryptors', 'test');
 		$this->assertNotContains('test', array_keys(PasswordEncryptor::get_encryptors()));
 	}
 	
 	function testEncrytorPHPHashWithArguments() {
-		PasswordEncryptor::register('test_md5', 'PasswordEncryptor_PHPHash("md5")');
+		Config::inst()->update('PasswordEncryptor', 'encryptors', array('test_md5'=>array('PasswordEncryptor_PHPHash'=>'md5')));
 		$e = PasswordEncryptor::create_for_algorithm('test_md5');
 		$this->assertEquals('md5', $e->getAlgorithm());
 	}
 	
 	function testEncrytorPHPHash() {
-		PasswordEncryptor::register('test_sha1', 'PasswordEncryptor_PHPHash("sha1")');
+		Config::inst()->update('PasswordEncryptor', 'encryptors', array('test_sha1'=>array('PasswordEncryptor_PHPHash'=>'sha1')));
 		$e = PasswordEncryptor::create_for_algorithm('test_sha1');
 		$password = 'mypassword';
 		$salt = 'mysalt';
@@ -46,7 +62,7 @@ class PasswordEncryptorTest extends SapphireTest {
 	}
 	
 	function testEncrytorPHPHashCompare() {
-		PasswordEncryptor::register('test_sha1', 'PasswordEncryptor_PHPHash("sha1")');
+		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')));
@@ -59,7 +75,7 @@ class PasswordEncryptorTest extends SapphireTest {
 	 * 	php -r "echo(base_convert(sha1('mypassword'), 16, 36));"
 	 */
 	function testEncrytorLegacyPHPHashCompare() {
-		PasswordEncryptor::register('test_sha1legacy', 'PasswordEncryptor_LegacyPHPHash("sha1")');
+		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';