FIX Member::changePassword() no longer applies password validation rules to the hashed value

src/Security/Member.php

@@ -922,10 +922,9 @@ class Member extends DataObject
         }
 
         // The test on $this->ID is used for when records are initially created. Note that this only works with
-        // cleartext passwords, as we can't rehash existing passwords. Checking passwordChangesToWrite prevents
+        // cleartext passwords, as we can't rehash existing passwords.
-        // recursion between changePassword and this method.
+        if (!$this->ID || $this->isChanged('Password')) {
-        if (!$this->ID || ($this->isChanged('Password') && !$this->passwordChangesToWrite)) {
+            $this->encryptPassword();
-            $this->changePassword($this->Password, false);
         }
 
         // save locale
@@ -1703,11 +1702,11 @@ class Member extends DataObject
     }
 
     /**
-     * Change password. This will cause rehashing according to the `PasswordEncryption` property. This method will
+     * Change password. This will cause rehashing according to the `PasswordEncryption` property via the
-     * allow extensions to perform actions and augment the validation result if required before the password is written
+     * `onBeforeWrite()` method. This method will allow extensions to perform actions and augment the validation
-     * and can check it after the write also.
+     * result if required before the password is written and can check it after the write also.
      *
-     * This method will encrypt the password prior to writing.
+     * `onBeforeWrite()` will encrypt the password prior to writing.
      *
      * @param string $password Cleartext password
      * @param bool $write Whether to write the member afterwards
@@ -1716,24 +1715,21 @@ class Member extends DataObject
     public function changePassword($password, $write = true)
     {
         $this->Password = $password;
-        $valid = $this->validate();
+        $result = $this->validate();
 
-        $this->extend('onBeforeChangePassword', $password, $valid);
+        $this->extend('onBeforeChangePassword', $password, $result);
 
-        if ($valid->isValid()) {
+        if ($result->isValid()) {
             $this->AutoLoginHash = null;
 
-            $this->encryptPassword();
-
             if ($write) {
-                $this->passwordChangesToWrite = true;
                 $this->write();
             }
         }
 
-        $this->extend('onAfterChangePassword', $password, $valid);
+        $this->extend('onAfterChangePassword', $password, $result);
 
-        return $valid;
+        return $result;
     }
 
     /**

tests/php/Security/MemberTest.php

@@ -279,7 +279,7 @@ class MemberTest extends FunctionalTest
     {
         /**
          * @var Member $member
-        */
+         */
         $member = $this->objFromFixture(Member::class, 'test');
         $this->assertNotNull($member);
 
@@ -1571,4 +1571,18 @@ class MemberTest extends FunctionalTest
 
         $this->assertSame('en_US', $newMember->Locale, 'New members receive the default locale');
     }
+
+    public function testChangePasswordHashesPasswordsOnce()
+    {
+        // This validator requires passwords to be 17 characters long
+        Member::set_password_validator(new MemberTest\VerySpecificPasswordValidator());
+
+        // This algorithm will never return a 17 character hash
+        Security::config()->set('password_encryption_algorithm', 'blowfish');
+
+        /** @var Member $member */
+        $member = $this->objFromFixture(Member::class, 'test');
+        $result = $member->changePassword('Password123456789'); // 17 characters long
+        $this->assertTrue($result->isValid());
+    }
 }

tests/php/Security/MemberTest/TestPasswordValidator.php

@@ -2,9 +2,10 @@
 
 namespace SilverStripe\Security\Tests\MemberTest;
 
+use SilverStripe\Dev\TestOnly;
 use SilverStripe\Security\PasswordValidator;
 
-class TestPasswordValidator extends PasswordValidator
+class TestPasswordValidator extends PasswordValidator implements TestOnly
 {
     public function __construct()
     {

tests/php/Security/MemberTest/VerySpecificPasswordValidator.php

@@ -0,0 +1,19 @@
+<?php
+
+namespace SilverStripe\Security\Tests\MemberTest;
+
+use SilverStripe\Dev\TestOnly;
+use SilverStripe\ORM\ValidationResult;
+use SilverStripe\Security\PasswordValidator;
+
+class VerySpecificPasswordValidator extends PasswordValidator implements TestOnly
+{
+    public function validate($password, $member)
+    {
+        $result = ValidationResult::create();
+        if (strlen($password) !== 17) {
+            $result->addError('Password must be 17 characters long');
+        }
+        return $result;
+    }
+}