Issue #2503083 by pwolanin, neclimdul: Simplify PasswordInterface so it's not...

Issue #2503083 by pwolanin, neclimdul: Simplify PasswordInterface so it's not coupled to UserInterface

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 @@ 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);
 
 }

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 @@ protected function enforceLog2Boundaries($count_log2) {
    */
   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 @@ public function getCountLog2($setting) {
   }
 
   /**
-   * 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);
   }
+
 }

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 @@ public function __construct(PasswordInterface $original_password) {
   /**
    * {@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);
   }
 
   /**

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());
@@ -183,7 +184,7 @@ public function testUser() {
 
       // 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()));
     }
   }
 

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')->check(trim($this->get('pass')->existing), $account_unchanged->getPassword());
   }
 
   /**

core/modules/user/src/Tests/UserLoginTest.php

@@ -144,6 +144,7 @@ function testPasswordRehashOnLogin() {
     $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()));
   }
 
   /**

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->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();
           }

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);
@@ -135,7 +135,7 @@ public function testAuthenticateWithIncorrectPassword() {
 
     $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 @@ public function testAuthenticateWithCorrectPassword() {
 
     $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 @@ public function testAuthenticateWithCorrectPasswordAndNewPasswordHash() {
 
     $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));

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 @@ public function testWithinBounds() {
   /**
    * 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 @@ public function testPasswordNeedsUpdate() {
    * @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 @@ public function testPasswordHashing() {
    * @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 @@ public function testLongPassword($password, $allowed) {
    */
   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;
   }