From fd2bc62e43468c987d89e4b6267944db0a79db72 Mon Sep 17 00:00:00 2001 From: Alex Pott Date: Mon, 22 Jun 2015 15:54:10 -0500 Subject: [PATCH] Issue #2503083 by pwolanin, neclimdul: Simplify PasswordInterface so it's not coupled to UserInterface --- .../Core/Password/PasswordInterface.php | 38 ++++++------ .../Core/Password/PhpassHashedPassword.php | 32 +++++----- core/modules/migrate/src/MigratePassword.php | 9 ++- .../src/Tests/d6/MigrateUserTest.php | 3 +- core/modules/user/src/Entity/User.php | 2 +- core/modules/user/src/Tests/UserLoginTest.php | 1 + core/modules/user/src/UserAuth.php | 11 ++-- .../user/tests/src/Unit/UserAuthTest.php | 14 ++--- .../Core/Password/PasswordHashingTest.php | 61 ++++++++----------- 9 files changed, 80 insertions(+), 91 deletions(-) diff --git a/core/lib/Drupal/Core/Password/PasswordInterface.php b/core/lib/Drupal/Core/Password/PasswordInterface.php index 9726f69f91c2..17aa26ddd567 100644 --- a/core/lib/Drupal/Core/Password/PasswordInterface.php +++ b/core/lib/Drupal/Core/Password/PasswordInterface.php @@ -2,18 +2,21 @@ /** * @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. * @@ -21,29 +24,25 @@ interface PasswordInterface { * 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. + * Check 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 check($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 @@ interface PasswordInterface { * 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 string $hash + * The existing hash to be checked. * - * @param \Drupal\user\UserInterface $account - * A user entity. - * - * @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/PhpassHashedPassword.php b/core/lib/Drupal/Core/Password/PhpassHashedPassword.php index 17cafddc1070..6e9257a1dc7b 100644 --- a/core/lib/Drupal/Core/Password/PhpassHashedPassword.php +++ b/core/lib/Drupal/Core/Password/PhpassHashedPassword.php @@ -2,13 +2,12 @@ /** * @file - * Definition of Drupal\Core\Password\PhpassHashedPassword + * Contains \Drupal\Core\Password\PhpassHashedPassword. */ namespace Drupal\Core\Password; use Drupal\Component\Utility\Crypt; -use Drupal\user\UserInterface; /** * Secure password hashing functions based on the Portable PHP password @@ -160,7 +159,7 @@ class PhpassHashedPassword implements PasswordInterface { */ 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; } @@ -212,57 +211,58 @@ class PhpassHashedPassword implements PasswordInterface { } /** - * 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 check($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/modules/migrate/src/MigratePassword.php b/core/modules/migrate/src/MigratePassword.php index ba42e8a69be1..479aac8a129e 100644 --- a/core/modules/migrate/src/MigratePassword.php +++ b/core/modules/migrate/src/MigratePassword.php @@ -8,7 +8,6 @@ namespace Drupal\migrate; use Drupal\Core\Password\PasswordInterface; -use Drupal\user\UserInterface; /** * Replaces the original 'password' service in order to prefix the MD5 re-hashed @@ -42,15 +41,15 @@ class MigratePassword implements PasswordInterface { /** * {@inheritdoc} */ - public function check($password, UserInterface $account) { - return $this->originalPassword->check($password, $account); + public function check($password, $hash) { + return $this->originalPassword->check($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_drupal/src/Tests/d6/MigrateUserTest.php b/core/modules/migrate_drupal/src/Tests/d6/MigrateUserTest.php index 0e781cec9cdb..d7ebec08de9f 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 @@ class MigrateUserTest extends MigrateDrupal6TestBase { $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()); @@ -183,7 +184,7 @@ class MigrateUserTest extends MigrateDrupal6TestBase { // Use the API to check 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')->check($source->pass_plain, $user->getPassword())); } } diff --git a/core/modules/user/src/Entity/User.php b/core/modules/user/src/Entity/User.php index 05b642a96429..53155a019613 100644 --- a/core/modules/user/src/Entity/User.php +++ b/core/modules/user/src/Entity/User.php @@ -393,7 +393,7 @@ class User extends ContentEntityBase implements UserInterface { * {@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')->check(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 7981f2615686..4462f440e61e 100644 --- a/core/modules/user/src/Tests/UserLoginTest.php +++ b/core/modules/user/src/Tests/UserLoginTest.php @@ -144,6 +144,7 @@ class UserLoginTest extends WebTestBase { $user_storage->resetCache(array($account->id())); $account = $user_storage->load($account->id()); $this->assertIdentical($password_hasher->getCountLog2($account->getPassword()), $overridden_count_log2); + $this->assertTrue($password_hasher->check($password, $account->getPassword())); } /** diff --git a/core/modules/user/src/UserAuth.php b/core/modules/user/src/UserAuth.php index 625b4440fdc3..0d0451205db8 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 @@ class UserAuth implements UserAuthInterface { $account_search = $this->entityManager->getStorage('user')->loadByProperties(array('name' => $username)); if ($account = reset($account_search)) { - if ($this->passwordChecker->check($password, $account)) { + if ($this->passwordChecker->check($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/src/Unit/UserAuthTest.php b/core/modules/user/tests/src/Unit/UserAuthTest.php index d70824d75435..3699f1501f3b 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 @@ class UserAuthTest extends UnitTestCase { $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); @@ -135,7 +135,7 @@ class UserAuthTest extends UnitTestCase { $this->passwordService->expects($this->once()) ->method('check') - ->with($this->password, $this->testUser) + ->with($this->password, $this->testUser->getPassword()) ->will($this->returnValue(FALSE)); $this->assertFalse($this->userAuth->authenticate($this->username, $this->password)); @@ -158,7 +158,7 @@ class UserAuthTest extends UnitTestCase { $this->passwordService->expects($this->once()) ->method('check') - ->with($this->password, $this->testUser) + ->with($this->password, $this->testUser->getPassword()) ->will($this->returnValue(TRUE)); $this->assertsame(1, $this->userAuth->authenticate($this->username, $this->password)); @@ -186,11 +186,11 @@ class UserAuthTest extends UnitTestCase { $this->passwordService->expects($this->once()) ->method('check') - ->with($this->password, $this->testUser) + ->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/PasswordHashingTest.php b/core/tests/Drupal/Tests/Core/Password/PasswordHashingTest.php index f304be0ec9fb..8371f2b1f292 100644 --- a/core/tests/Drupal/Tests/Core/Password/PasswordHashingTest.php +++ b/core/tests/Drupal/Tests/Core/Password/PasswordHashingTest.php @@ -8,6 +8,7 @@ namespace Drupal\Tests\Core\Password; use Drupal\Core\Password\PhpassHashedPassword; +use Drupal\Core\Password\PasswordInterface; use Drupal\Tests\UnitTestCase; /** @@ -37,7 +38,7 @@ class PasswordHashingTest extends UnitTestCase { * * @var string */ - protected $md5Password; + protected $md5HashedPassword; /** * The hashed password. @@ -58,10 +59,10 @@ class PasswordHashingTest extends UnitTestCase { */ protected function setUp() { parent::setUp(); - $this->user = $this->getMockBuilder('Drupal\user\Entity\User') - ->disableOriginalConstructor() - ->getMock(); + $this->password = $this->randomMachineName(); $this->passwordHasher = new PhpassHashedPassword(1); + $this->hashedPassword = $this->passwordHasher->hash($this->password); + $this->md5HashedPassword = 'U' . $this->passwordHasher->hash(md5($this->password)); } /** @@ -79,14 +80,11 @@ class PasswordHashingTest extends UnitTestCase { /** * Test a password needs update. * - * @covers ::userNeedsNewHash + * @covers ::needsRehash */ 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.'); + $this->assertTrue($this->passwordHasher->needsRehash($this->md5HashedPassword), 'Upgraded md5 password hash needs a new hash.'); } /** @@ -95,19 +93,16 @@ class PasswordHashingTest extends UnitTestCase { * @covers ::hash * @covers ::getCountLog2 * @covers ::check - * @covers ::userNeedsNewHash + * @covers ::needsRehash */ 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.'); + $this->assertNotEquals($this->hashedPassword, $this->md5HashedPassword, 'Password hashes not the same.'); + $this->assertTrue($this->passwordHasher->check($this->password, $this->md5HashedPassword), 'Password check succeeds.'); + $this->assertTrue($this->passwordHasher->check($this->password, $this->hashedPassword), '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.'); + $this->assertFalse($this->passwordHasher->needsRehash($this->hashedPassword), 'Does not need a new hash.'); } /** @@ -116,25 +111,21 @@ class PasswordHashingTest extends UnitTestCase { * @covers ::hash * @covers ::getCountLog2 * @covers ::check - * @covers ::userNeedsNewHash + * @covers ::needsRehash */ 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.'); + $password_hasher = new PhpassHashedPassword(PhpassHashedPassword::MIN_HASH_COUNT + 1); + $this->assertTrue($password_hasher->needsRehash($this->hashedPassword), '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.'); + $rehashed_password = $password_hasher->hash($this->password); + $this->assertSame($password_hasher->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.'); + $this->assertFalse($password_hasher->needsRehash($rehashed_password), 'Re-hashed password does not need a new hash.'); + $this->assertTrue($password_hasher->check($this->password, $rehashed_password), 'Password check succeeds with re-hashed password.'); + $this->assertTrue($this->passwordHasher->check($this->password, $rehashed_password), 'Password check succeeds with re-hashed password with original hasher.'); } /** @@ -161,19 +152,21 @@ class PasswordHashingTest extends UnitTestCase { */ public function providerLongPasswords() { // '512 byte long password is allowed.' - $passwords['allowed'] = array(str_repeat('x', 512), TRUE); + $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', 513), FALSE); + $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. - $passwords['utf8'] = array(str_repeat('€', 170), TRUE); + $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] . 'xx', TRUE); + $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('€', 171), FALSE); + $passwords['utf8_too_long'] = array(str_repeat('€', $len + 1), FALSE); return $passwords; }