diff --git a/src/Security/Member.php b/src/Security/Member.php index d0e37e23c..8109fbb45 100644 --- a/src/Security/Member.php +++ b/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 - // recursion between changePassword and this method. - if (!$this->ID || ($this->isChanged('Password') && !$this->passwordChangesToWrite)) { - $this->changePassword($this->Password, false); + // cleartext passwords, as we can't rehash existing passwords. + if (!$this->ID || $this->isChanged('Password')) { + $this->encryptPassword(); } // save locale @@ -1703,11 +1702,11 @@ class Member extends DataObject } /** - * Change password. This will cause rehashing according to the `PasswordEncryption` property. This method will - * allow extensions to perform actions and augment the validation result if required before the password is written - * and can check it after the write also. + * Change password. This will cause rehashing according to the `PasswordEncryption` property via the + * `onBeforeWrite()` method. This method will allow extensions to perform actions and augment the validation + * 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; } /** diff --git a/tests/php/Security/MemberTest.php b/tests/php/Security/MemberTest.php index 5ae2c6128..16ba5bdc5 100644 --- a/tests/php/Security/MemberTest.php +++ b/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 testChangePasswordOnlyValidatesPlaintext() + { + // 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()); + } } diff --git a/tests/php/Security/MemberTest/TestPasswordValidator.php b/tests/php/Security/MemberTest/TestPasswordValidator.php index 8907b7830..834507f61 100644 --- a/tests/php/Security/MemberTest/TestPasswordValidator.php +++ b/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() { diff --git a/tests/php/Security/MemberTest/VerySpecificPasswordValidator.php b/tests/php/Security/MemberTest/VerySpecificPasswordValidator.php new file mode 100644 index 000000000..04f55cb58 --- /dev/null +++ b/tests/php/Security/MemberTest/VerySpecificPasswordValidator.php @@ -0,0 +1,19 @@ +addError('Password must be 17 characters long'); + } + return $result; + } +}