diff --git a/core/core.services.yml b/core/core.services.yml index 6c2c78f..b3a69dc 100644 --- a/core/core.services.yml +++ b/core/core.services.yml @@ -7,6 +7,7 @@ parameters: default: keyvalue.database factory.keyvalue.expirable: default: keyvalue.expirable.database + password_hash_cost: 10 services: # Simple cache contexts, directly derived from the request context. cache_context.ip: @@ -763,15 +764,20 @@ services: class: Drupal\Core\Path\PathValidator arguments: ['@router', '@router.no_access_checks', '@current_user', '@path_processor_manager'] -# The argument to the hashing service defined in services.yml, to the -# constructor of PhpassHashedPassword is the log2 number of iterations for -# password stretching. -# @todo increase by 1 every Drupal version in order to counteract increases in -# the speed and power of computers available to crack the hashes. The current -# password hashing method was introduced in Drupal 7 with a log2 count of 15. + # The first argument of the hashing service (constructor of PhpPassword) is + # the 'cost' option of password_hash(). In Drupal 8 the 'cost' has the default + # value used by password_hash() which is 10. Future versions of Drupal may + # increase this value in order to counteract increases in the speed and power + # of computers available to crack the hashes. Note that an increase of 1 will + # double the time needed for password hashing. password: - class: Drupal\Core\Password\PhpassHashedPassword + class: Drupal\Core\Password\PhpPassword + arguments: ['%password_hash_cost%', '@drupal7_password'] + lazy: true + drupal7_password: + class: Drupal\Core\Password\Drupal7Password arguments: [16] + lazy: true request_format_route_filter: class: Drupal\Core\Routing\RequestFormatRouteFilter tags: diff --git a/core/lib/Drupal/Core/Password/PhpassHashedPassword.php b/core/lib/Drupal/Core/Password/Drupal7Password.php similarity index 81% rename from core/lib/Drupal/Core/Password/PhpassHashedPassword.php rename to core/lib/Drupal/Core/Password/Drupal7Password.php index 17cafdd..d4776a5 100644 --- a/core/lib/Drupal/Core/Password/PhpassHashedPassword.php +++ b/core/lib/Drupal/Core/Password/Drupal7Password.php @@ -2,21 +2,24 @@ /** * @file - * Definition of Drupal\Core\Password\PhpassHashedPassword + * Contains \Drupal\Core\Password\Drupal7Password. */ namespace Drupal\Core\Password; use Drupal\Component\Utility\Crypt; -use Drupal\user\UserInterface; /** * Secure password hashing functions based on the Portable PHP password * hashing framework. * + * This is a custom password hashing mechanism used in Drupal 7. We keep this + * password hasher in order to validate passwords migrated from Drupal 7 that + * are being rehashed on first user login. + * * @see http://www.openwall.com/phpass/ */ -class PhpassHashedPassword implements PasswordInterface { +class Drupal7Password implements PasswordInterface { /** * The minimum allowed log2 number of iterations for password stretching. */ @@ -45,7 +48,7 @@ class PhpassHashedPassword implements PasswordInterface { protected $countLog2; /** - * Constructs a new phpass password hashing instance. + * Constructs a new password hashing instance. * * @param int $countLog2 * Password stretching iteration count. Specifies the number of times the @@ -61,13 +64,13 @@ function __construct($countLog2) { /** * Encodes bytes into printable base 64 using the *nix standard from crypt(). * - * @param String $input + * @param string $input * The string containing bytes to encode. - * @param Integer $count + * @param int $count * The number of characters (bytes) to encode. * - * @return String - * Encoded string + * @return string + * Encoded string. */ protected function base64Encode($input, $count) { $output = ''; @@ -96,7 +99,8 @@ protected function base64Encode($input, $count) { } /** - * Generates a random base 64-encoded salt prefixed with settings for the hash. + * Generates a random base 64-encoded salt prefixed with settings for the + * hash. * * Proper use of salts may defeat a number of attacks, including: * - The ability to try candidate passwords against multiple hashes at once. @@ -104,7 +108,7 @@ protected function base64Encode($input, $count) { * - The ability to determine whether two users have the same (or different) * password without actually having to guess one of the passwords. * - * @return String + * @return string * A 12 character string containing the iteration count and a random salt. */ protected function generateSalt() { @@ -119,11 +123,11 @@ protected function generateSalt() { /** * Ensures that $count_log2 is within set bounds. * - * @param Integer $count_log2 + * @param int $count_log2 * Integer that determines the number of iterations used in the hashing * process. A larger value is more secure, but takes more time to complete. * - * @return Integer + * @return int * Integer within set bounds that is closest to $count_log2. */ protected function enforceLog2Boundaries($count_log2) { @@ -145,22 +149,22 @@ protected function enforceLog2Boundaries($count_log2) { * for an attacker to try to break the hash by brute-force computation of the * hashes of a large number of plain-text words or strings to find a match. * - * @param String $algo + * @param string $algo * The string name of a hashing algorithm usable by hash(), like 'sha256'. - * @param String $password + * @param string $password * Plain-text password up to 512 bytes (128 to 512 UTF-8 characters) to * hash. - * @param String $setting - * An existing hash or the output of $this->generateSalt(). Must be - * at least 12 characters (the settings and salt). + * @param string $setting + * An existing hash or the output of $this->generateSalt(). Must be at least + * 12 characters (the settings and salt). * - * @return String + * @return string * A string containing the hashed password (and salt) or FALSE on failure. * The return string will be truncated at HASH_LENGTH characters max. */ protected function crypt($algo, $password, $setting) { // Prevent DoS attacks by refusing to hash large passwords. - if (strlen($password) > 512) { + if (strlen($password) > PasswordInterface::PASSWORD_MAX_LENGTH) { return FALSE; } @@ -201,68 +205,72 @@ protected function crypt($algo, $password, $setting) { } /** - * Parse the log2 iteration count from a stored hash or setting string. + * Parses the log2 iteration count from a stored hash or setting string. + * + * @param string $setting + * An existing hash or the output of $this->generateSalt(). Must be at least + * 12 characters (the settings and salt). * - * @param String $setting - * An existing hash or the output of $this->generateSalt(). Must be - * at least 12 characters (the settings and salt). + * @return int + * The log2 iteration count. */ public function getCountLog2($setting) { return strpos(static::$ITOA64, $setting[3]); } /** - * Implements Drupal\Core\Password\PasswordInterface::hash(). + * {@inheritdoc} */ public function hash($password) { return $this->crypt('sha512', $password, $this->generateSalt()); } /** - * Implements Drupal\Core\Password\PasswordInterface::checkPassword(). + * {@inheritdoc} */ - public function check($password, UserInterface $account) { - if (substr($account->getPassword(), 0, 2) == 'U$') { + public function verify($password, $hash) { + if (substr($hash, 0, 2) == 'U$') { // This may be an updated password from user_update_7000(). Such hashes // have 'U' added as the first character and need an extra md5() (see the // Drupal 7 documentation). - $stored_hash = substr($account->getPassword(), 1); + $stored_hash = substr($hash, 1); $password = md5($password); } else { - $stored_hash = $account->getPassword(); + $stored_hash = $hash; } $type = substr($stored_hash, 0, 3); switch ($type) { case '$S$': // A normal Drupal 7 password using sha512. - $hash = $this->crypt('sha512', $password, $stored_hash); + $computed_hash = $this->crypt('sha512', $password, $stored_hash); break; case '$H$': // phpBB3 uses "$H$" for the same thing as "$P$". case '$P$': // A phpass password generated using md5. This is an // imported password or from an earlier Drupal version. - $hash = $this->crypt('md5', $password, $stored_hash); + $computed_hash = $this->crypt('md5', $password, $stored_hash); break; default: return FALSE; } - return ($hash && $stored_hash == $hash); + return ($computed_hash && $stored_hash === $computed_hash); } /** - * Implements Drupal\Core\Password\PasswordInterface::userNeedsNewHash(). + * {@inheritdoc} */ - public function userNeedsNewHash(UserInterface $account) { + public function needsRehash($hash) { // Check whether this was an updated password. - if ((substr($account->getPassword(), 0, 3) != '$S$') || (strlen($account->getPassword()) != static::HASH_LENGTH)) { + if ((substr($hash, 0, 3) != '$S$') || (strlen($hash) != static::HASH_LENGTH)) { return TRUE; } // Ensure that $count_log2 is within set bounds. $count_log2 = $this->enforceLog2Boundaries($this->countLog2); // Check whether the iteration count used differs from the standard number. - return ($this->getCountLog2($account->getPassword()) !== $count_log2); + return ($this->getCountLog2($hash) !== $count_log2); } + } diff --git a/core/lib/Drupal/Core/Password/PasswordInterface.php b/core/lib/Drupal/Core/Password/PasswordInterface.php index 9726f69..65f7639 100644 --- a/core/lib/Drupal/Core/Password/PasswordInterface.php +++ b/core/lib/Drupal/Core/Password/PasswordInterface.php @@ -2,48 +2,47 @@ /** * @file - * Definition of Drupal\Core\Password\PasswordInterface + * Contains \Drupal\Core\Password\PasswordInterface. */ namespace Drupal\Core\Password; -use Drupal\user\UserInterface; - /** * Secure password hashing functions for user authentication. */ interface PasswordInterface { /** + * Maximum password length. + */ + const PASSWORD_MAX_LENGTH = 512; + + /** * Hash a password using a secure hash. * * @param string $password * A plain-text password. * * @return string - * A string containing the hashed password (and a salt), or FALSE on failure. + * A string containing the hashed password, or FALSE on failure. */ public function hash($password); /** - * Check whether a plain text password matches a stored hashed password. - * - * Alternative implementations of this function may use other data in the - * $account object, for example the uid to look up the hash in a custom table - * or remote database. + * Verify whether a plain text password matches a hashed password. * * @param string $password * A plain-text password - * @param \Drupal\user\UserInterface $account - * A user entity. + * @param string $hash + * A hashed password. * * @return bool * TRUE if the password is valid, FALSE if not. */ - public function check($password, UserInterface $account); + public function verify($password, $hash); /** - * Check whether a user's hashed password needs to be replaced with a new hash. + * Check whether a hashed password needs to be replaced with a new hash. * * This is typically called during the login process when the plain text * password is available. A new hash is needed when the desired iteration @@ -52,15 +51,12 @@ public function check($password, UserInterface $account); * generated in an update like user_update_7000() (see the Drupal 7 * documentation). * - * Alternative implementations of this function might use other criteria based - * on the fields in $account. - * - * @param \Drupal\user\UserInterface $account - * A user entity. + * @param string $hash + * The existing hash to be checked. * - * @return boolean - * TRUE or FALSE. + * @return bool + * TRUE if the hash is outdated and needs rehash. */ - public function userNeedsNewHash(UserInterface $account); + public function needsRehash($hash); } diff --git a/core/lib/Drupal/Core/Password/PhpPassword.php b/core/lib/Drupal/Core/Password/PhpPassword.php new file mode 100644 index 0000000..fa2ef02 --- /dev/null +++ b/core/lib/Drupal/Core/Password/PhpPassword.php @@ -0,0 +1,97 @@ +=5.5.0) password hashing + * functions. + * + * @see http://php.net/manual/en/book.password.php + */ +class PhpPassword implements PasswordInterface { + + /** + * The algorithmic cost that should be used. This is the same 'cost' option as + * is used by the PHP (>= 5.5.0) password_hash() function. + * + * @var int + * + * @see password_hash(). + * @see http://php.net/manual/en/ref.password.php + */ + protected $cost; + + /** + * The Drupal 7 password hashing service. + * + * @var \Drupal\Core\Password\Drupal7Password + */ + protected $drupal7Password; + + /** + * Constructs a new password hashing instance. + * + * @param int $cost + * The algorithmic cost that should be used. + * @param \Drupal\Core\Password\PasswordInterface $drupal7_password + * The Drupal7 password hashing service. + */ + function __construct($cost, PasswordInterface $drupal7_password) { + $this->cost = $cost; + $this->drupal7Password = $drupal7_password; + } + + /** + * {@inheritdoc} + */ + public function hash($password) { + // Prevent DoS attacks by refusing to hash large passwords. + if (strlen($password) > static::PASSWORD_MAX_LENGTH) { + return FALSE; + } + + return password_hash($password, PASSWORD_BCRYPT, $this->getOptions()); + } + + /** + * {@inheritdoc} + */ + public function verify($password, $hash) { + // Try to verify an legacy hash. This may be an Drupal 6 or 7 hash. + if (substr($hash, 0, 4) != '$2y$') { + return $this->drupal7Password->verify($password, $hash); + } + + return password_verify($password, $hash); + } + + /** + * {@inheritdoc} + */ + public function needsRehash($hash) { + // The PHP 5.5 password_needs_rehash() will return TRUE in two cases: + // - The password is either a Drupal 6 MD5 hash rehashed during migration, + // either a Drupal 7 hashed password. In this case the legacy hash will + // not comply the expected password_needs_rehash() format (prefix '$2y$'). + // - The parameters of hashing engine were changed. For example the + // parameter 'password_hash_cost' (the hashing cost) has been increased in + // core.services.yml. + return password_needs_rehash($hash, PASSWORD_BCRYPT, $this->getOptions()); + } + + /** + * Returns password options. + * + * @return array + * Associative array with password options. + */ + protected function getOptions() { + return ['cost' => $this->cost]; + } + +} diff --git a/core/modules/migrate/migrate.services.yml b/core/modules/migrate/migrate.services.yml index 3f2d32d..d8ecc7f 100644 --- a/core/modules/migrate/migrate.services.yml +++ b/core/modules/migrate/migrate.services.yml @@ -20,3 +20,4 @@ services: password_migrate: class: Drupal\migrate\MigratePassword arguments: ['@password_original'] + lazy: true diff --git a/core/modules/migrate/src/MigratePassword.php b/core/modules/migrate/src/MigratePassword.php index ba42e8a..85a4882 100644 --- a/core/modules/migrate/src/MigratePassword.php +++ b/core/modules/migrate/src/MigratePassword.php @@ -8,7 +8,7 @@ namespace Drupal\migrate; use Drupal\Core\Password\PasswordInterface; -use Drupal\user\UserInterface; +use Drupal\migrate\Entity\MigrationInterface; /** * Replaces the original 'password' service in order to prefix the MD5 re-hashed @@ -26,6 +26,8 @@ class MigratePassword implements PasswordInterface { /** * Indicates if MD5 password prefixing is enabled. + * + * @var bool */ protected $enabled = FALSE; @@ -42,15 +44,15 @@ public function __construct(PasswordInterface $original_password) { /** * {@inheritdoc} */ - public function check($password, UserInterface $account) { - return $this->originalPassword->check($password, $account); + public function verify($password, $hash) { + return $this->originalPassword->verify($password, $hash); } /** * {@inheritdoc} */ - public function userNeedsNewHash(UserInterface $account) { - return $this->originalPassword->userNeedsNewHash($account); + public function needsRehash($hash) { + return $this->originalPassword->needsRehash($hash); } /** diff --git a/core/modules/migrate/src/MigrateServiceProvider.php b/core/modules/migrate/src/MigrateServiceProvider.php index 78a60bb..705f9d2 100644 --- a/core/modules/migrate/src/MigrateServiceProvider.php +++ b/core/modules/migrate/src/MigrateServiceProvider.php @@ -12,10 +12,10 @@ /** * Swaps the original 'password' service in order to handle password hashing for - * user migrations that have passwords hashed to MD5. + * user migrations that have passwords hashed with MD5 or Drupal 7 passwords. * * @see \Drupal\migrate\MigratePassword - * @see \Drupal\Core\Password\PhpassHashedPassword + * @see \Drupal\Core\Password\PhpPassword */ class MigrateServiceProvider implements ServiceModifierInterface { diff --git a/core/modules/migrate/src/Plugin/migrate/destination/EntityUser.php b/core/modules/migrate/src/Plugin/migrate/destination/EntityUser.php index 786988f..f9cbca7 100644 --- a/core/modules/migrate/src/Plugin/migrate/destination/EntityUser.php +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityUser.php @@ -13,7 +13,6 @@ use Drupal\migrate\Entity\MigrationInterface; use Drupal\migrate\MigrateException; use Drupal\migrate\MigratePassword; -use Drupal\migrate\Plugin\MigratePluginManager; use Drupal\migrate\Row; use Symfony\Component\DependencyInjection\ContainerInterface; @@ -79,6 +78,7 @@ public static function create(ContainerInterface $container, array $configuratio /** * {@inheritdoc} + * * @throws \Drupal\migrate\MigrateException */ public function import(Row $row, array $old_destination_id_values = array()) { diff --git a/core/modules/migrate_drupal/src/Tests/d6/MigrateUserTest.php b/core/modules/migrate_drupal/src/Tests/d6/MigrateUserTest.php index 0e781ce..997e383 100644 --- a/core/modules/migrate_drupal/src/Tests/d6/MigrateUserTest.php +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateUserTest.php @@ -156,6 +156,7 @@ public function testUser() { $roles[] = reset($role); } + /** @var \Drupal\user\UserInterface $user */ $user = User::load($source->uid); $this->assertIdentical($source->uid, $user->id()); $this->assertIdentical($source->name, $user->label()); @@ -181,9 +182,9 @@ public function testUser() { $this->assertIdentical(basename($source->picture), $file->getFilename()); } - // Use the API to check if the password has been salted and re-hashed to + // Use the API to verify if the password has been salted and re-hashed to // conform the Drupal >= 7. - $this->assertTrue(\Drupal::service('password')->check($source->pass_plain, $user)); + $this->assertTrue(\Drupal::service('password')->verify($source->pass_plain, $user->getPassword())); } } diff --git a/core/modules/simpletest/src/KernelTestBase.php b/core/modules/simpletest/src/KernelTestBase.php index 481b031..81170aa 100644 --- a/core/modules/simpletest/src/KernelTestBase.php +++ b/core/modules/simpletest/src/KernelTestBase.php @@ -345,8 +345,9 @@ public function containerBuild(ContainerBuilder $container) { $definition->clearTag('path_processor_inbound')->clearTag('path_processor_outbound'); } - if ($container->hasDefinition('password')) { - $container->getDefinition('password')->setArguments(array(1)); + if ($container->hasDefinition('password') && $container->hasDefinition('drupal7_password')) { + $container->getDefinition('drupal7_password')->setArguments([1]); + $container->getDefinition('password')->setArguments([4, $container->get('drupal7_password')]); } // Register the stream wrapper manager. diff --git a/core/modules/user/src/Entity/User.php b/core/modules/user/src/Entity/User.php index 05b642a..c0af59e 100644 --- a/core/modules/user/src/Entity/User.php +++ b/core/modules/user/src/Entity/User.php @@ -393,7 +393,7 @@ public function setExistingPassword($password) { * {@inheritdoc} */ public function checkExistingPassword(UserInterface $account_unchanged) { - return !empty($this->get('pass')->existing) && \Drupal::service('password')->check(trim($this->get('pass')->existing), $account_unchanged); + return !empty($this->get('pass')->existing) && \Drupal::service('password')->verify(trim($this->get('pass')->existing), $account_unchanged->getPassword()); } /** diff --git a/core/modules/user/src/Tests/UserLoginTest.php b/core/modules/user/src/Tests/UserLoginTest.php index 7981f26..5899802 100644 --- a/core/modules/user/src/Tests/UserLoginTest.php +++ b/core/modules/user/src/Tests/UserLoginTest.php @@ -2,13 +2,13 @@ /** * @file - * Definition of Drupal\user\Tests\UserLoginTest. + * Contains \Drupal\user\Tests\UserLoginTest. */ namespace Drupal\user\Tests; use Drupal\simpletest\WebTestBase; -use Drupal\Core\Password\PhpassHashedPassword; +use Drupal\user\UserInterface; use Drupal\user\Entity\User; /** @@ -19,6 +19,20 @@ class UserLoginTest extends WebTestBase { /** + * Drupal password hasher service. + * + * @var \Drupal\Core\Password\PasswordInterface + */ + private $passwordHasher; + + /** + * Drupal 7 password hasher service. + * + * @var \Drupal\Core\Password\PasswordInterface + */ + private $drupal7PasswordHasher; + + /** * Tests login with destination. */ function testLoginCacheTagsAndDestination() { @@ -32,6 +46,9 @@ function testLoginCacheTagsAndDestination() { $edit = array('name' => $user->getUserName(), 'pass' => $user->pass_raw); $this->drupalPostForm(NULL, $edit, t('Log in')); $this->assertUrl('foo', [], 'Redirected to the correct URL'); + + $this->passwordHasher = $this->container->get('password'); + $this->drupal7PasswordHasher = $this->container->get('drupal7_password'); } /** @@ -112,30 +129,24 @@ function testPerUserLoginFloodControl() { } /** - * Test that user password is re-hashed upon login after changing $count_log2. + * Test that user password is re-hashed upon login after changing the cost. */ function testPasswordRehashOnLogin() { - // Determine default log2 for phpass hashing algorithm - $default_count_log2 = 16; - - // Retrieve instance of password hashing algorithm - $password_hasher = $this->container->get('password'); - // Create a new user and authenticate. $account = $this->drupalCreateUser(array()); $password = $account->pass_raw; $this->drupalLogin($account); $this->drupalLogout(); - // Load the stored user. The password hash should reflect $default_count_log2. + // Load the stored user. The password hash should reflect $default_cost. $user_storage = $this->container->get('entity.manager')->getStorage('user'); + /** @var \Drupal\user\UserInterface $account */ $account = User::load($account->id()); - $this->assertIdentical($password_hasher->getCountLog2($account->getPassword()), $default_count_log2); + $this->assertTrue($this->passwordHasher->verify($password, $account->getPassword())); - // Change the required number of iterations by loading a test-module - // containing the necessary container builder code and then verify that the - // users password gets rehashed during the login. - $overridden_count_log2 = 19; - \Drupal::service('module_installer')->install(array('user_custom_phpass_params_test')); + // Change the required cost by loading a test-module containing the + // necessary container builder code and then verify that the users password + // gets rehashed during the login. + \Drupal::service('module_installer')->install(array('user_custom_pass_hash_params_test')); $this->resetAll(); $account->pass_raw = $password; @@ -143,7 +154,48 @@ function testPasswordRehashOnLogin() { // Load the stored user, which should have a different password hash now. $user_storage->resetCache(array($account->id())); $account = $user_storage->load($account->id()); - $this->assertIdentical($password_hasher->getCountLog2($account->getPassword()), $overridden_count_log2); + $this->assertTrue($this->passwordHasher->verify($password, $account->getPassword())); + } + + /** + * Test MD5 (Drupal 6) passwords rehashing. + */ + public function testDrupal6MigratedPasswordRehashing() { + /** @var \Drupal\user\UserInterface $account */ + $account = $this->drupalCreateUser(); + $plain = $account->pass_raw; + + // We pretend that the user was migrated from Drupal 6. + $md5_pass = md5($plain); + $migrated_pass = 'U' . $this->passwordHasher->hash($md5_pass); + $this->storeHashedPassword($account, $migrated_pass); + + // User first login after migration. + $this->drupalLogin($account); + $this->drupalLogout(); + + // After logging in the user password has been rehashed and is valid. + $this->assertTrue($this->passwordHasher->verify($plain, $account->getPassword())); + } + + /** + * Test Drupal 7 passwords rehashing. + */ + public function testDrupal7MigratedPasswordRehashing() { + /** @var \Drupal\user\UserInterface $account */ + $account = $this->drupalCreateUser(); + $plain = $account->pass_raw; + + // We pretend that the user was migrated from Drupal 7. + $d7_hash = $this->drupal7PasswordHasher->hash($plain); + $this->storeHashedPassword($account, $d7_hash); + + // User first login after migration. + $this->drupalLogin($account); + $this->drupalLogout(); + + // After logging in the user password has been rehashed and is valid. + $this->assertTrue($this->passwordHasher->verify($plain, $account->getPassword())); } /** @@ -179,4 +231,23 @@ function assertFailedLogin($account, $flood_trigger = NULL) { $this->assertText(t('Sorry, unrecognized username or password. Have you forgotten your password?')); } } + + /** + * Updates the hashed user password bypassing the API. + * + * We want to set an already hashed password. + * + * @param \Drupal\user\UserInterface $account + * The user account. + * @param string $hashed_password + * An already hashed password. + */ + protected function storeHashedPassword(UserInterface $account, $hashed_password) { + $account->setPassword($hashed_password); + db_update('users_field_data') + ->fields(['pass' => $hashed_password]) + ->condition('uid', $account->id()) + ->execute(); + } + } diff --git a/core/modules/user/src/UserAuth.php b/core/modules/user/src/UserAuth.php index 625b444..710e27e 100644 --- a/core/modules/user/src/UserAuth.php +++ b/core/modules/user/src/UserAuth.php @@ -8,7 +8,6 @@ namespace Drupal\user; use Drupal\Core\Entity\EntityManagerInterface; -use Drupal\Core\Entity\EntityStorageInterface; use Drupal\Core\Password\PasswordInterface; /** @@ -24,7 +23,7 @@ class UserAuth implements UserAuthInterface { protected $entityManager; /** - * The password service. + * The password hashing service. * * @var \Drupal\Core\Password\PasswordInterface */ @@ -33,8 +32,8 @@ class UserAuth implements UserAuthInterface { /** * Constructs a UserAuth object. * - * @param \Drupal\Core\Entity\EntityStorageInterface $storage - * The user storage. + * @param \Drupal\Core\Entity\EntityManagerInterface $entity_manager + * The entity manager. * @param \Drupal\Core\Password\PasswordInterface $password_checker * The password service. */ @@ -53,12 +52,12 @@ public function authenticate($username, $password) { $account_search = $this->entityManager->getStorage('user')->loadByProperties(array('name' => $username)); if ($account = reset($account_search)) { - if ($this->passwordChecker->check($password, $account)) { + if ($this->passwordChecker->verify($password, $account->getPassword())) { // Successful authentication. $uid = $account->id(); // Update user to new password scheme if needed. - if ($this->passwordChecker->userNeedsNewHash($account)) { + if ($this->passwordChecker->needsRehash($account->getPassword())) { $account->setPassword($password); $account->save(); } diff --git a/core/modules/user/tests/modules/user_custom_pass_hash_params_test/user_custom_pass_hash_params_test.info.yml b/core/modules/user/tests/modules/user_custom_pass_hash_params_test/user_custom_pass_hash_params_test.info.yml new file mode 100644 index 0000000..aca50c4 --- /dev/null +++ b/core/modules/user/tests/modules/user_custom_pass_hash_params_test/user_custom_pass_hash_params_test.info.yml @@ -0,0 +1,6 @@ +name: 'User custom password hash params test' +type: module +description: 'Support module for testing custom hashing password algorithm parameters.' +package: Testing +version: VERSION +core: 8.x diff --git a/core/modules/user/tests/modules/user_custom_pass_hash_params_test/user_custom_pass_hash_params_test.services.yml b/core/modules/user/tests/modules/user_custom_pass_hash_params_test/user_custom_pass_hash_params_test.services.yml new file mode 100644 index 0000000..56c1281 --- /dev/null +++ b/core/modules/user/tests/modules/user_custom_pass_hash_params_test/user_custom_pass_hash_params_test.services.yml @@ -0,0 +1,7 @@ +services: + password: + class: Drupal\Core\Password\PhpPassword + arguments: [11, '@drupal7_password'] + drupal7_password: + class: Drupal\Core\Password\Drupal7Password + arguments: [16] diff --git a/core/modules/user/tests/modules/user_custom_phpass_params_test/user_custom_phpass_params_test.info.yml b/core/modules/user/tests/modules/user_custom_phpass_params_test/user_custom_phpass_params_test.info.yml deleted file mode 100644 index 68b8ff9..0000000 --- a/core/modules/user/tests/modules/user_custom_phpass_params_test/user_custom_phpass_params_test.info.yml +++ /dev/null @@ -1,6 +0,0 @@ -name: 'User custom phpass params test' -type: module -description: 'Support module for testing custom phpass password algorithm parameters.' -package: Testing -version: VERSION -core: 8.x diff --git a/core/modules/user/tests/modules/user_custom_phpass_params_test/user_custom_phpass_params_test.services.yml b/core/modules/user/tests/modules/user_custom_phpass_params_test/user_custom_phpass_params_test.services.yml deleted file mode 100644 index 8950bfe..0000000 --- a/core/modules/user/tests/modules/user_custom_phpass_params_test/user_custom_phpass_params_test.services.yml +++ /dev/null @@ -1,4 +0,0 @@ -services: - password: - class: Drupal\Core\Password\PhpassHashedPassword - arguments: [19] diff --git a/core/modules/user/tests/src/Unit/UserAuthTest.php b/core/modules/user/tests/src/Unit/UserAuthTest.php index d70824d..48c0139 100644 --- a/core/modules/user/tests/src/Unit/UserAuthTest.php +++ b/core/modules/user/tests/src/Unit/UserAuthTest.php @@ -52,7 +52,7 @@ class UserAuthTest extends UnitTestCase { protected $username = 'test_user'; /** - * The test password + * The test password. * * @var string */ @@ -74,7 +74,7 @@ protected function setUp() { $this->testUser = $this->getMockBuilder('Drupal\user\Entity\User') ->disableOriginalConstructor() - ->setMethods(array('id', 'setPassword', 'save')) + ->setMethods(array('id', 'setPassword', 'save', 'getPassword')) ->getMock(); $this->userAuth = new UserAuth($entity_manager, $this->passwordService); @@ -134,8 +134,8 @@ public function testAuthenticateWithIncorrectPassword() { ->will($this->returnValue(array($this->testUser))); $this->passwordService->expects($this->once()) - ->method('check') - ->with($this->password, $this->testUser) + ->method('verify') + ->with($this->password, $this->testUser->getPassword()) ->will($this->returnValue(FALSE)); $this->assertFalse($this->userAuth->authenticate($this->username, $this->password)); @@ -157,8 +157,8 @@ public function testAuthenticateWithCorrectPassword() { ->will($this->returnValue(array($this->testUser))); $this->passwordService->expects($this->once()) - ->method('check') - ->with($this->password, $this->testUser) + ->method('verify') + ->with($this->password, $this->testUser->getPassword()) ->will($this->returnValue(TRUE)); $this->assertsame(1, $this->userAuth->authenticate($this->username, $this->password)); @@ -185,12 +185,12 @@ public function testAuthenticateWithCorrectPasswordAndNewPasswordHash() { ->will($this->returnValue(array($this->testUser))); $this->passwordService->expects($this->once()) - ->method('check') - ->with($this->password, $this->testUser) + ->method('verify') + ->with($this->password, $this->testUser->getPassword()) ->will($this->returnValue(TRUE)); $this->passwordService->expects($this->once()) - ->method('userNeedsNewHash') - ->with($this->testUser) + ->method('needsRehash') + ->with($this->testUser->getPassword()) ->will($this->returnValue(TRUE)); $this->assertsame(1, $this->userAuth->authenticate($this->username, $this->password)); diff --git a/core/tests/Drupal/Tests/Core/Password/Drupal7PasswordTest.php b/core/tests/Drupal/Tests/Core/Password/Drupal7PasswordTest.php new file mode 100644 index 0000000..466f86b --- /dev/null +++ b/core/tests/Drupal/Tests/Core/Password/Drupal7PasswordTest.php @@ -0,0 +1,188 @@ +password = $this->randomMachineName(); + $this->passwordHasher = new Drupal7Password(1); + $this->hashedPassword = $this->passwordHasher->hash($this->password); + $this->md5HashedPassword = 'U' . $this->passwordHasher->hash(md5($this->password)); + } + + /** + * Tests the hash count boundaries are enforced. + * + * @covers ::enforceLog2Boundaries + */ + public function testWithinBounds() { + $hasher = new FakeDrupal7Password(); + $this->assertEquals(Drupal7Password::MIN_HASH_COUNT, $hasher->enforceLog2Boundaries(1)); + $this->assertEquals(Drupal7Password::MAX_HASH_COUNT, $hasher->enforceLog2Boundaries(100)); + } + + /** + * Test a password needs update. + * + * @covers ::needsRehash + */ + public function testPasswordNeedsUpdate() { + // The md5 password should be flagged as needing an update. + $this->assertTrue($this->passwordHasher->needsRehash($this->md5HashedPassword)); + } + + /** + * Test password hashing. + * + * @covers ::hash + * @covers ::getCountLog2 + * @covers ::verify + * @covers ::needsRehash + */ + public function testPasswordHashing() { + $this->assertSame($this->passwordHasher->getCountLog2($this->hashedPassword), Drupal7Password::MIN_HASH_COUNT); + $this->assertNotEquals($this->hashedPassword, $this->md5HashedPassword); + $this->assertTrue($this->passwordHasher->verify($this->password, $this->md5HashedPassword)); + $this->assertTrue($this->passwordHasher->verify($this->password, $this->hashedPassword)); + // Since the log2 setting hasn't changed and the user has a valid password, + // userNeedsNewHash() should return FALSE. + $this->assertFalse($this->passwordHasher->needsRehash($this->hashedPassword)); + } + + /** + * Tests password rehashing. + * + * @covers ::hash + * @covers ::getCountLog2 + * @covers ::verify + * @covers ::needsRehash + */ + public function testPasswordRehashing() { + // Increment the log2 iteration to MIN + 1. + $password_hasher = new Drupal7Password(Drupal7Password::MIN_HASH_COUNT + 1); + $this->assertTrue($password_hasher->needsRehash($this->hashedPassword)); + // Re-hash the password. + $rehashed_password = $password_hasher->hash($this->password); + $this->assertSame($password_hasher->getCountLog2($rehashed_password), Drupal7Password::MIN_HASH_COUNT + 1); + $this->assertNotEquals($rehashed_password, $this->hashedPassword); + + // Now the hash should be OK. + $this->assertFalse($password_hasher->needsRehash($rehashed_password)); + $this->assertTrue($password_hasher->verify($this->password, $rehashed_password)); + $this->assertTrue($this->passwordHasher->verify($this->password, $rehashed_password)); + } + + /** + * Verifies that passwords longer than 512 bytes are not hashed. + * + * @covers ::crypt + * + * @dataProvider providerLongPasswords + */ + public function testLongPassword($password, $allowed) { + + $hashed_password = $this->passwordHasher->hash($password); + + if ($allowed) { + $this->assertNotFalse($hashed_password); + } + else { + $this->assertFalse($hashed_password); + } + } + + /** + * Provides the test matrix for testLongPassword(). + */ + public function providerLongPasswords() { + // '512 byte long password is allowed.' + $passwords['allowed'] = array(str_repeat('x', PasswordInterface::PASSWORD_MAX_LENGTH), TRUE); + // 513 byte long password is not allowed. + $passwords['too_long'] = array(str_repeat('x', PasswordInterface::PASSWORD_MAX_LENGTH + 1), FALSE); + + // Check a string of 3-byte UTF-8 characters, 510 byte long password is + // allowed. + $len = floor(PasswordInterface::PASSWORD_MAX_LENGTH / 3); + $diff = PasswordInterface::PASSWORD_MAX_LENGTH % 3; + $passwords['utf8'] = array(str_repeat('€', $len), TRUE); + // 512 byte long password is allowed. + $passwords['ut8_extended'] = array($passwords['utf8'][0] . str_repeat('x', $diff), TRUE); + + // Check a string of 3-byte UTF-8 characters, 513 byte long password is + // allowed. + $passwords['utf8_too_long'] = array(str_repeat('€', $len + 1), FALSE); + return $passwords; + } + +} + +/** + * A fake class for tests. + */ +class FakeDrupal7Password extends Drupal7Password { + + function __construct() { + // Noop. + } + + // Expose this method as public for tests. + public function enforceLog2Boundaries($count_log2) { + return parent::enforceLog2Boundaries($count_log2); + } + +} diff --git a/core/tests/Drupal/Tests/Core/Password/PasswordHashingTest.php b/core/tests/Drupal/Tests/Core/Password/PasswordHashingTest.php deleted file mode 100644 index f304be0..0000000 --- a/core/tests/Drupal/Tests/Core/Password/PasswordHashingTest.php +++ /dev/null @@ -1,196 +0,0 @@ -user = $this->getMockBuilder('Drupal\user\Entity\User') - ->disableOriginalConstructor() - ->getMock(); - $this->passwordHasher = new PhpassHashedPassword(1); - } - - /** - * Tests the hash count boundaries are enforced. - * - * @covers ::enforceLog2Boundaries - */ - public function testWithinBounds() { - $hasher = new FakePhpassHashedPassword(); - $this->assertEquals(PhpassHashedPassword::MIN_HASH_COUNT, $hasher->enforceLog2Boundaries(1), "Min hash count enforced"); - $this->assertEquals(PhpassHashedPassword::MAX_HASH_COUNT, $hasher->enforceLog2Boundaries(100), "Max hash count enforced"); - } - - - /** - * Test a password needs update. - * - * @covers ::userNeedsNewHash - */ - public function testPasswordNeedsUpdate() { - $this->user->expects($this->any()) - ->method('getPassword') - ->will($this->returnValue($this->md5Password)); - // The md5 password should be flagged as needing an update. - $this->assertTrue($this->passwordHasher->userNeedsNewHash($this->user), 'User with md5 password needs a new hash.'); - } - - /** - * Test password hashing. - * - * @covers ::hash - * @covers ::getCountLog2 - * @covers ::check - * @covers ::userNeedsNewHash - */ - public function testPasswordHashing() { - $this->hashedPassword = $this->passwordHasher->hash($this->password); - $this->user->expects($this->any()) - ->method('getPassword') - ->will($this->returnValue($this->hashedPassword)); - $this->assertSame($this->passwordHasher->getCountLog2($this->hashedPassword), PhpassHashedPassword::MIN_HASH_COUNT, 'Hashed password has the minimum number of log2 iterations.'); - $this->assertNotEquals($this->hashedPassword, $this->md5Password, 'Password hash changed.'); - $this->assertTrue($this->passwordHasher->check($this->password, $this->user), 'Password check succeeds.'); - // Since the log2 setting hasn't changed and the user has a valid password, - // userNeedsNewHash() should return FALSE. - $this->assertFalse($this->passwordHasher->userNeedsNewHash($this->user), 'User does not need a new hash.'); - } - - /** - * Tests password rehashing. - * - * @covers ::hash - * @covers ::getCountLog2 - * @covers ::check - * @covers ::userNeedsNewHash - */ - public function testPasswordRehashing() { - - // Increment the log2 iteration to MIN + 1. - $this->passwordHasher = new PhpassHashedPassword(PhpassHashedPassword::MIN_HASH_COUNT + 1); - $this->assertTrue($this->passwordHasher->userNeedsNewHash($this->user), 'User needs a new hash after incrementing the log2 count.'); - // Re-hash the password. - $rehashed_password = $this->passwordHasher->hash($this->password); - - $this->user->expects($this->any()) - ->method('getPassword') - ->will($this->returnValue($rehashed_password)); - $this->assertSame($this->passwordHasher->getCountLog2($rehashed_password), PhpassHashedPassword::MIN_HASH_COUNT + 1, 'Re-hashed password has the correct number of log2 iterations.'); - $this->assertNotEquals($rehashed_password, $this->hashedPassword, 'Password hash changed again.'); - - // Now the hash should be OK. - $this->assertFalse($this->passwordHasher->userNeedsNewHash($this->user), 'Re-hashed password does not need a new hash.'); - $this->assertTrue($this->passwordHasher->check($this->password, $this->user), 'Password check succeeds with re-hashed password.'); - } - - /** - * Verifies that passwords longer than 512 bytes are not hashed. - * - * @covers ::crypt - * - * @dataProvider providerLongPasswords - */ - public function testLongPassword($password, $allowed) { - - $hashed_password = $this->passwordHasher->hash($password); - - if ($allowed) { - $this->assertNotFalse($hashed_password); - } - else { - $this->assertFalse($hashed_password); - } - } - - /** - * Provides the test matrix for testLongPassword(). - */ - public function providerLongPasswords() { - // '512 byte long password is allowed.' - $passwords['allowed'] = array(str_repeat('x', 512), TRUE); - // 513 byte long password is not allowed. - $passwords['too_long'] = array(str_repeat('x', 513), FALSE); - - // Check a string of 3-byte UTF-8 characters, 510 byte long password is - // allowed. - $passwords['utf8'] = array(str_repeat('€', 170), TRUE); - // 512 byte long password is allowed. - $passwords['ut8_extended'] = array($passwords['utf8'][0] . 'xx', TRUE); - - // Check a string of 3-byte UTF-8 characters, 513 byte long password is - // allowed. - $passwords['utf8_too_long'] = array(str_repeat('€', 171), FALSE); - return $passwords; - } - -} - -/** - * A fake class for tests. - */ -class FakePhpassHashedPassword extends PhpassHashedPassword { - - function __construct() { - // Noop. - } - - // Expose this method as public for tests. - public function enforceLog2Boundaries($count_log2) { - return parent::enforceLog2Boundaries($count_log2); - } - -} diff --git a/core/tests/Drupal/Tests/Core/Password/PhpPhpPasswordTest.php b/core/tests/Drupal/Tests/Core/Password/PhpPhpPasswordTest.php new file mode 100644 index 0000000..7fb679c --- /dev/null +++ b/core/tests/Drupal/Tests/Core/Password/PhpPhpPasswordTest.php @@ -0,0 +1,162 @@ +passwordHasher = new PhpPassword(4, $d7_hashing_service); + $this->password = $this->randomMachineName(); + $this->md5Hash = md5($this->password); + $this->d7Hash = $d7_hashing_service->hash($this->password); + } + + /** + * Test a password needs update. + * + * @covers ::needsRehash + */ + public function testPasswordNeedsUpdate() { + // The md5 password should be flagged as needing an rehashing. + $this->assertTrue($this->passwordHasher->needsRehash($this->md5Hash)); + // The Drupal 7 password should be flagged as needing an rehashing. + $this->assertTrue($this->passwordHasher->needsRehash($this->d7Hash)); + } + + /** + * Test password hashing. + * + * @covers ::hash + * @covers ::verify + * @covers ::needsRehash + */ + public function testPasswordHashing() { + $this->hashedPassword = $this->passwordHasher->hash($this->password); + + $this->assertNotEquals($this->hashedPassword, $this->md5Hash); + $this->assertNotEquals($this->hashedPassword, $this->d7Hash); + + $this->assertTrue($this->passwordHasher->verify($this->password, $this->hashedPassword)); + $this->assertFalse($this->passwordHasher->needsRehash($this->hashedPassword)); + } + + /** + * Tests password rehashing. + * + * @covers ::hash + * @covers ::check + * @covers ::needsRehash + */ + public function testPasswordRehashing() { + // Increment the cost from 4 to 5. + $this->passwordHasher = new PhpPassword(5, new Drupal7Password(1)); + $this->assertTrue($this->passwordHasher->needsRehash($this->hashedPassword)); + + // Re-hash the password. + $rehashed_password = $this->passwordHasher->hash($this->password); + $this->assertNotEquals($rehashed_password, $this->hashedPassword); + + // Now the hash should be OK. + $this->assertFalse($this->passwordHasher->needsRehash($rehashed_password)); + $this->assertTrue($this->passwordHasher->verify($this->password, $rehashed_password)); + } + + /** + * Verifies that passwords longer than 512 bytes are not hashed. + * + * @covers ::hash + * + * @dataProvider providerLongPasswords + */ + public function testLongPassword($password, $allowed) { + + $hashed_password = $this->passwordHasher->hash($password); + + if ($allowed) { + $this->assertNotFalse($hashed_password); + } + else { + $this->assertFalse($hashed_password); + } + } + + /** + * Provides the test matrix for testLongPassword(). + */ + public function providerLongPasswords() { + // '512 byte long password is allowed.' + $passwords['allowed'] = array(str_repeat('x', PasswordInterface::PASSWORD_MAX_LENGTH), TRUE); + // 513 byte long password is not allowed. + $passwords['too_long'] = array(str_repeat('x', PasswordInterface::PASSWORD_MAX_LENGTH + 1), FALSE); + + // Check a string of 3-byte UTF-8 characters, 510 byte long password is + // allowed. + $len = floor(PasswordInterface::PASSWORD_MAX_LENGTH / 3); + $diff = PasswordInterface::PASSWORD_MAX_LENGTH % 3; + $passwords['utf8'] = array(str_repeat('€', $len), TRUE); + // 512 byte long password is allowed. + $passwords['ut8_extended'] = array($passwords['utf8'][0] . str_repeat('x', $diff), TRUE); + + // Check a string of 3-byte UTF-8 characters, 513 byte long password is + // allowed. + $passwords['utf8_too_long'] = array(str_repeat('€', $len + 1), FALSE); + return $passwords; + } + +}