Merge branch '4.4' into 4

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/ORM/DataObjectTest.php

@@ -4,8 +4,6 @@ namespace SilverStripe\ORM\Tests;
 
 use InvalidArgumentException;
 use LogicException;
-use Page;
-use SilverStripe\CMS\Model\SiteTree;
 use SilverStripe\Core\Config\Config;
 use SilverStripe\Dev\SapphireTest;
 use SilverStripe\i18n\i18n;
@@ -20,11 +18,8 @@ use SilverStripe\ORM\FieldType\DBPolymorphicForeignKey;
 use SilverStripe\ORM\FieldType\DBVarchar;
 use SilverStripe\ORM\ManyManyList;
 use SilverStripe\ORM\Tests\DataObjectTest\Company;
-use SilverStripe\ORM\Tests\DataObjectTest\Fan;
-use SilverStripe\ORM\Tests\DataObjectTest\OtherSubclassWithSameField;
 use SilverStripe\ORM\Tests\DataObjectTest\Player;
-use SilverStripe\ORM\Tests\DataObjectTest\SubTeam;
+use SilverStripe\Security\Member;
-use SilverStripe\ORM\Tests\DataObjectTest\Team;
 use SilverStripe\View\ViewableData;
 use stdClass;
 
@@ -66,6 +61,19 @@ class DataObjectTest extends SapphireTest
         DataObjectTest\MockDynamicAssignmentDataObject::class
     );
 
+    protected function setUp()
+    {
+        parent::setUp();
+
+        $validator = Member::password_validator();
+        if ($validator) {
+            // Set low default password strength requirements so tests are not interfered with by user code
+            $validator
+                ->setMinTestScore(0)
+                ->setMinLength(6);
+        }
+    }
+
     public static function getExtraDataObjects()
     {
         return array_merge(

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

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;
+    }
+}