From 0d031a5045243a7d0392d0d324df1d22072da1e7 Mon Sep 17 00:00:00 2001 From: Stig Lindqvist Date: Sat, 7 Apr 2012 14:59:55 +1200 Subject: [PATCH] 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 --- _config.php | 6 --- _config/PasswordEncryptor.yml | 14 +++++++ security/PasswordEncryptor.php | 23 +++++++---- tests/security/MemberAuthenticatorTest.php | 3 +- tests/security/PasswordEncryptorTest.php | 44 +++++++++++++++------- 5 files changed, 61 insertions(+), 29 deletions(-) create mode 100644 _config/PasswordEncryptor.yml diff --git a/_config.php b/_config.php index abecc340e..ccd191568 100644 --- a/_config.php +++ b/_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 diff --git a/_config/PasswordEncryptor.yml b/_config/PasswordEncryptor.yml new file mode 100644 index 000000000..662d22732 --- /dev/null +++ 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 \ No newline at end of file diff --git a/security/PasswordEncryptor.php b/security/PasswordEncryptor.php index 2574d7876..c229a7c16 100644 --- 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,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 = (($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 index 4816aa112..78bf2a735 100644 --- a/tests/security/MemberAuthenticatorTest.php +++ b/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(); diff --git a/tests/security/PasswordEncryptorTest.php b/tests/security/PasswordEncryptorTest.php index c514b3ab2..3b4f36683 100644 --- a/tests/security/PasswordEncryptorTest.php +++ b/tests/security/PasswordEncryptorTest.php @@ -1,12 +1,26 @@ 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 @@ class PasswordEncryptorTest extends SapphireTest { } 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 @@ 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';