API CHANGE Use Config for registering default password encryptors

Using the config system for registering password encryptors Remove the eval on password encryptor construction by using reflection Throws deprecation messages when using static register / unregister
silverstripe · Apr 7, 2012 · 0d031a5 · 0d031a5
1 parent aebbb10
commit 0d031a5
Show file tree

Hide file tree

Showing 5 changed files with 61 additions and 29 deletions.
diff --git a/_config.php b/_config.php
@@ -61,12 +61,6 @@
 	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

diff --git a/_config/PasswordEncryptor.yml b/_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
diff --git a/security/PasswordEncryptor.php b/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,36 +36,45 @@ static function get_encryptors() {
 	 * @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;
 	}
 
 	/**
 	 * @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 = (($p = strpos($classWithArgs, '(')) !== false) ? substr($classWithArgs, 0, $p) : $classWithArgs;
+		$class=key($encryptors[$algorithm]);
 		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));
 	}
 
 	/**

diff --git a/tests/security/MemberAuthenticatorTest.php b/tests/security/MemberAuthenticatorTest.php
@@ -30,8 +30,7 @@ function testLegacyPasswordHashMigrationUponLogin() {
 	}
 
 	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();

diff --git a/tests/security/PasswordEncryptorTest.php b/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(
-			'PasswordEncryptorTest_TestEncryptor',
-			$e
-		);
+		$this->assertType('PasswordEncryptorTest_TestEncryptor', $e );
 	}
 
 	/**
@@ -17,25 +31,27 @@ function testCreateForCodeNotFound() {
 	}
 
 	function testRegister() {
-		PasswordEncryptor::register('test', 'PasswordEncryptorTest_TestEncryptor');
-		$this->assertContains('test', array_keys(PasswordEncryptor::get_encryptors()));
-		$this->assertContains('PasswordEncryptorTest_TestEncryptor', array_values(PasswordEncryptor::get_encryptors()));
+		Config::inst()->update('PasswordEncryptor', 'encryptors', array('test'=>array('PasswordEncryptorTest_TestEncryptor'=>null)));
+		$encryptors = 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');
-		PasswordEncryptor::unregister('test');
+		Config::inst()->update('PasswordEncryptor', 'encryptors', array('test'=>array('PasswordEncryptorTest_TestEncryptor'=>null)));
+		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 @@ function testEncrytorPHPHash() {
 	}
 
 	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 @@ function testEncrytorPHPHashCompare() {
 	 * 	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';