From a00854c406215d787465422d7ea7def8c24180f5 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Fri, 20 Sep 2013 17:31:32 +0200 Subject: [feature/passwords] Do not pass phpbb_container to passwords manager PHPBB3-11610 --- phpBB/config/passwords.yml | 1 - phpBB/phpbb/passwords/driver/bcrypt.php | 8 --- phpBB/phpbb/passwords/driver/bcrypt_2y.php | 8 --- phpBB/phpbb/passwords/driver/interface.php | 7 -- phpBB/phpbb/passwords/driver/phpass.php | 8 --- phpBB/phpbb/passwords/driver/salted_md5.php | 8 --- phpBB/phpbb/passwords/helper.php | 21 +++--- phpBB/phpbb/passwords/manager.php | 102 ++++++++++++++++++---------- tests/passwords/manager_test.php | 10 ++- 9 files changed, 86 insertions(+), 87 deletions(-) diff --git a/phpBB/config/passwords.yml b/phpBB/config/passwords.yml index c9f9e238e8..26f604c2e2 100644 --- a/phpBB/config/passwords.yml +++ b/phpBB/config/passwords.yml @@ -49,6 +49,5 @@ services: class: phpbb_passwords_manager arguments: - @config - - @service_container - @passwords.driver_collection - %passwords.algorithm% diff --git a/phpBB/phpbb/passwords/driver/bcrypt.php b/phpBB/phpbb/passwords/driver/bcrypt.php index 2c2ab8e7b7..db41fe6b38 100644 --- a/phpBB/phpbb/passwords/driver/bcrypt.php +++ b/phpBB/phpbb/passwords/driver/bcrypt.php @@ -30,14 +30,6 @@ class phpbb_passwords_driver_bcrypt extends phpbb_passwords_driver_base return self::PREFIX; } - /** - * @inheritdoc - */ - public function get_type() - { - return get_class($this); - } - /** * @inheritdoc */ diff --git a/phpBB/phpbb/passwords/driver/bcrypt_2y.php b/phpBB/phpbb/passwords/driver/bcrypt_2y.php index 9277414d13..5b0dbdd311 100644 --- a/phpBB/phpbb/passwords/driver/bcrypt_2y.php +++ b/phpBB/phpbb/passwords/driver/bcrypt_2y.php @@ -30,14 +30,6 @@ class phpbb_passwords_driver_bcrypt_2y extends phpbb_passwords_driver_bcrypt return self::PREFIX; } - /** - * @inheritdoc - */ - public function get_type() - { - return get_class($this); - } - /** * @inheritdoc */ diff --git a/phpBB/phpbb/passwords/driver/interface.php b/phpBB/phpbb/passwords/driver/interface.php index a019830cb4..a2088db81c 100644 --- a/phpBB/phpbb/passwords/driver/interface.php +++ b/phpBB/phpbb/passwords/driver/interface.php @@ -33,13 +33,6 @@ interface phpbb_passwords_driver_interface */ public function get_prefix(); - /** - * Returns the name of the hash type - * - * @return string Hash type of driver - */ - public function get_type(); - /** * Hash the password * diff --git a/phpBB/phpbb/passwords/driver/phpass.php b/phpBB/phpbb/passwords/driver/phpass.php index 14ba60f279..d52ce96d11 100644 --- a/phpBB/phpbb/passwords/driver/phpass.php +++ b/phpBB/phpbb/passwords/driver/phpass.php @@ -29,12 +29,4 @@ class phpbb_passwords_driver_phpass extends phpbb_passwords_driver_salted_md5 { return self::PREFIX; } - - /** - * @inheritdoc - */ - public function get_type() - { - return get_class($this); - } } diff --git a/phpBB/phpbb/passwords/driver/salted_md5.php b/phpBB/phpbb/passwords/driver/salted_md5.php index b87daae4e3..6fa12948be 100644 --- a/phpBB/phpbb/passwords/driver/salted_md5.php +++ b/phpBB/phpbb/passwords/driver/salted_md5.php @@ -30,14 +30,6 @@ class phpbb_passwords_driver_salted_md5 extends phpbb_passwords_driver_base return self::PREFIX; } - /** - * @inheritdoc - */ - public function get_type() - { - return get_class($this); - } - /** * @inheritdoc */ diff --git a/phpBB/phpbb/passwords/helper.php b/phpBB/phpbb/passwords/helper.php index 952f491669..d91edb90a1 100644 --- a/phpBB/phpbb/passwords/helper.php +++ b/phpBB/phpbb/passwords/helper.php @@ -25,21 +25,14 @@ class phpbb_passwords_helper */ protected $manager; - /** - * @var phpbb_container - */ - protected $container; - /** * Construct a phpbb_passwords_helper object * * @param phpbb_passwords_manager $manager Crypto manager object - * @param phpbb_container $container phpBB container object */ - public function __construct($manager, $container) + public function __construct($manager) { $this->manager = $manager; - $this->container = $container; } /** @@ -85,14 +78,22 @@ class phpbb_passwords_helper $hash = $hash_settings[0]; // Put settings of current hash into data array - $stored_hash_type = $this->manager->get_hashing_algorithm($password_hash); + $stored_hash_type = $this->manager->detect_algorithm($password_hash); $this->combine_hash_output($data, 'prefix', $stored_hash_type->get_prefix()); $this->combine_hash_output($data, 'settings', $stored_hash_type->get_settings_only($password_hash)); // Hash current hash with the defined types foreach ($type as $cur_type) { - $new_hash_type = $this->container->get($cur_type); + if (isset($this->manager->algorithms[$cur_type])) + { + $new_hash_type = $this->manager->algorithms[$cur_type]; + } + else + { + return false; + } + $new_hash = $new_hash_type->hash(str_replace($stored_hash_type->get_settings_only($password_hash), '', $hash)); $this->combine_hash_output($data, 'prefix', $new_hash_type->get_prefix()); $this->combine_hash_output($data, 'settings', substr(str_replace('$', '\\', $new_hash_type->get_settings_only($new_hash, true)), 0)); diff --git a/phpBB/phpbb/passwords/manager.php b/phpBB/phpbb/passwords/manager.php index da6d65c487..cf6eddd135 100644 --- a/phpBB/phpbb/passwords/manager.php +++ b/phpBB/phpbb/passwords/manager.php @@ -26,12 +26,19 @@ class phpbb_passwords_manager protected $type = false; /** - * Hashing algorithm types + * Hashing algorithm type map + * Will be used to map hash prefix to type */ protected $type_map = false; /** - * Password convert flag. Password should be converted + * Service collection of hashing algorithms + * Needs to be public for passwords helper + */ + public $algorithms = false; + + /** + * Password convert flag. Signals that password should be converted */ public $convert_flag = false; @@ -47,21 +54,17 @@ class phpbb_passwords_manager */ protected $config; - /** - * phpBB compiled container - * @var service_container - */ - protected $container; - /** * Construct a passwords object * * @param phpbb_config $config phpBB configuration + * @param phpbb_di_service_collection $hashing_algorithms Hashing driver + * service collection + * @param string $default Default driver name */ - public function __construct($config, $container, $hashing_algorithms, $default) + public function __construct($config, $hashing_algorithms, $default) { $this->config = $config; - $this->container = $container; $this->type = $default; $this->fill_type_map($hashing_algorithms); @@ -77,9 +80,14 @@ class phpbb_passwords_manager { foreach ($hashing_algorithms as $algorithm) { + if (!isset($this->algorithms[$algorithm->get_name()])) + { + $this->algorithms[$algorithm->get_name()] = $algorithm; + } + if (!isset($this->type_map[$algorithm->get_prefix()])) { - $this->type_map[$algorithm->get_prefix()] = $algorithm; + $this->type_map[$algorithm->get_prefix()] = $algorithm->get_name(); } } } @@ -91,18 +99,38 @@ class phpbb_passwords_manager { if ($this->helper === null) { - $this->helper = new phpbb_passwords_helper($this, $this->container); + $this->helper = new phpbb_passwords_helper($this); } } /** - * Get the hash type from the supplied hash + * Get the algorithm specified by a specific prefix * - * @param string $hash Password hash that should be checked + * @param string $prefix Password hash prefix * * @return object The hash type object */ - public function get_hashing_algorithm($hash) + protected function get_algorithm($prefix) + { + if (isset($this->type_map[$prefix]) && isset($this->algorithms[$this->type_map[$prefix]])) + { + return $this->algorithms[$this->type_map[$prefix]]; + } + else + { + return false; + } + } + + /** + * Detect the hash type of the supplied hash + * + * @param string $hash Password hash that should be checked + * + * @return object|bool The hash type object or false if the specified + * type is not supported + */ + public function detect_algorithm($hash) { /* * preg_match() will also show hashing algos like $2a\H$, which @@ -112,7 +140,7 @@ class phpbb_passwords_manager */ if (!preg_match('#^\$([a-zA-Z0-9\\\]*?)\$#', $hash, $match)) { - return $this->type_map['$H$']; + return $this->get_algorithm('$H$'); } // Be on the lookout for multiple hashing algorithms @@ -123,32 +151,27 @@ class phpbb_passwords_manager $return_ary = array(); foreach ($hash_types as $type) { - if (isset($this->type_map["\${$type}\$"])) + // we do not support the same hashing + // algorithm more than once + if (isset($return_ary[$type])) { - // we do not support the same hashing - // algorithm more than once - if (isset($return_ary[$type])) - { - return false; - } - $return_ary[$type] = $this->type_map["\${$type}\$"]; + return false; } - else + + $return_ary[$type] = $this->get_algorithm("\${$type}\$"); + + if (empty($return_ary[$type])) { + return false; } } return $return_ary; } - if (isset($this->type_map[$match[0]])) - { - return $this->type_map[$match[0]]; - } - else - { - return false; - } + // get_algorithm() will automatically return false if prefix + // is not supported + return $this->get_algorithm($match[0]); } /** @@ -169,7 +192,14 @@ class phpbb_passwords_manager return $this->helper->combined_hash_password($password, $type); } - $hashing_algorithm = $this->container->get($type); + if (isset($this->algorithms[$type])) + { + $hashing_algorithm = $this->algorithms[$type]; + } + else + { + return false; + } // Do not support 8-bit characters with $2a$ bcrypt // Also see http://www.php.net/security/crypt_blowfish.php @@ -181,7 +211,7 @@ class phpbb_passwords_manager } } - return $this->container->get($type)->hash($password); + return $hashing_algorithm->hash($password); } /** @@ -195,7 +225,7 @@ class phpbb_passwords_manager public function check_hash($password, $hash) { // First find out what kind of hash we're dealing with - $stored_hash_type = $this->get_hashing_algorithm($hash); + $stored_hash_type = $this->detect_algorithm($hash); if ($stored_hash_type == false) { return false; diff --git a/tests/passwords/manager_test.php b/tests/passwords/manager_test.php index a069c9692e..aa05c8d641 100644 --- a/tests/passwords/manager_test.php +++ b/tests/passwords/manager_test.php @@ -46,7 +46,7 @@ class phpbb_passwords_manager_test extends PHPUnit_Framework_TestCase } // Set up avatar manager - $this->manager = new phpbb_passwords_manager($config, $this->phpbb_container, $this->passwords_drivers, 'passwords.driver.bcrypt_2y'); + $this->manager = new phpbb_passwords_manager($config, $this->passwords_drivers, 'passwords.driver.bcrypt_2y'); } public function hash_password_data() @@ -58,6 +58,7 @@ class phpbb_passwords_manager_test extends PHPUnit_Framework_TestCase array('passwords.driver.bcrypt_2y', '2a', 60), array('passwords.driver.bcrypt', '2a', 60), array('passwords.driver.salted_md5', 'H', 34), + array('passwords.driver.foobar', '', false), ); } else @@ -67,6 +68,7 @@ class phpbb_passwords_manager_test extends PHPUnit_Framework_TestCase array('passwords.driver.bcrypt_2y', '2y', 60), array('passwords.driver.bcrypt', '2a', 60), array('passwords.driver.salted_md5', 'H', 34), + array('passwords.driver.foobar', '', false), ); } } @@ -77,6 +79,12 @@ class phpbb_passwords_manager_test extends PHPUnit_Framework_TestCase public function test_hash_password($type, $prefix, $length) { $password = $this->default_pw; + + if (!$length) + { + $this->assertEquals(false, $hash = $this->manager->hash_password($password, $type)); + return; + } $time = microtime(true); // Limit each test to 1 second -- cgit v1.2.1