Merge pull request #7436 from creative-commoners/pulls/4.0/consistent-change-password-api

NEW Ensure changePassword is called by onBeforeWrite for a consistent API

docs/en/04_Changelogs/4.0.0.md

@@ -2507,6 +2507,7 @@ New `TimeField` methods replace `getConfig()` / `setConfig()`
 
  * `LoginForm` now has an abstract method `getAuthenticatorName()`. If you have made subclasses of this,
    you will need to define this method and return a short name describing the login method.
- * `MemberLoginForm` has a new constructor argument for the authenticator class, athough tis is usually
+ * `MemberLoginForm` has a new constructor argument for the authenticator class, although this is usually
    constructed by `MemberAuthenticator` and won't affect normal use.
- * `Authenticator` methods `register` and `unregister` are deprecated in favor of using `Config`
+ * `Authenticator` methods `register` and `unregister` are deprecated in favour of using `Config`
+ * Unused `SetPassword` property removed from `Member`. Use `Member::changePassword` or set `Password` directly.

src/Security/Member.php

@@ -56,7 +56,6 @@ use SilverStripe\ORM\ValidationResult;
  * @property int $FailedLoginCount
  * @property string $DateFormat
  * @property string $TimeFormat
- * @property string $SetPassword Pseudo-DB field for temp storage. Not emitted to DB
  */
 class Member extends DataObject
 {
@@ -254,6 +253,14 @@ class Member extends DataObject
      */
     private static $auto_login_token_lifetime = 172800;
 
+    /**
+     * Used to track whether {@link Member::changePassword} has made changed that need to be written. Used to prevent
+     * the write from calling changePassword again.
+     *
+     * @var bool
+     */
+    protected $passwordChangesToWrite = false;
+
     /**
      * Ensure the locale is set to something sensible by default.
      */
@@ -850,10 +857,6 @@ class Member extends DataObject
      */
     public function onBeforeWrite()
     {
-        if ($this->SetPassword) {
-            $this->Password = $this->SetPassword;
-        }
-
         // If a member with the same "unique identifier" already exists with a different ID, don't allow merging.
         // Note: This does not a full replacement for safeguards in the controller layer (e.g. in a registration form),
         // but rather a last line of defense against data inconsistencies.
@@ -898,36 +901,11 @@ class Member extends DataObject
                 ->send();
         }
 
-        // The test on $this->ID is used for when records are initially created.
+        // The test on $this->ID is used for when records are initially created. Note that this only works with
-        // Note that this only works with cleartext passwords, as we can't rehash
+        // cleartext passwords, as we can't rehash existing passwords. Checking passwordChangesToWrite prevents
-        // existing passwords.
+        // recursion between changePassword and this method.
-        if ((!$this->ID && $this->Password) || $this->isChanged('Password')) {
+        if ((!$this->ID && $this->Password) || ($this->isChanged('Password') && !$this->passwordChangesToWrite)) {
-            //reset salt so that it gets regenerated - this will invalidate any persistent login cookies
+            $this->changePassword($this->Password, false);
-            // or other information encrypted with this Member's settings (see self::encryptWithUserSettings)
-            $this->Salt = '';
-            // Password was changed: encrypt the password according the settings
-            $encryption_details = Security::encrypt_password(
-                $this->Password, // this is assumed to be cleartext
-                $this->Salt,
-                ($this->PasswordEncryption) ?
-                    $this->PasswordEncryption : Security::config()->get('password_encryption_algorithm'),
-                $this
-            );
-
-            // Overwrite the Password property with the hashed value
-            $this->Password = $encryption_details['password'];
-            $this->Salt = $encryption_details['salt'];
-            $this->PasswordEncryption = $encryption_details['algorithm'];
-
-            // If we haven't manually set a password expiry
-            if (!$this->isChanged('PasswordExpiry')) {
-                // then set it for us
-                if (static::config()->get('password_expiry_days')) {
-                    $this->PasswordExpiry = date('Y-m-d', time() + 86400 * static::config()->get('password_expiry_days'));
-                } else {
-                    $this->PasswordExpiry = null;
-                }
-            }
         }
 
         // save locale
@@ -1670,25 +1648,21 @@ class Member extends DataObject
             }
         }
 
-        if ((!$this->ID && $this->SetPassword) || $this->isChanged('SetPassword')) {
-            if ($this->SetPassword && $validator) {
-                $valid->combineAnd($validator->validate($this->SetPassword, $this));
-            }
-        }
-
         return $valid;
     }
 
     /**
      * 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. Note that the onAfterChangePassword extension point receives a clone of
+     * and can check it after the write also.
-     * the validation result which cannot be modified.
+     *
+     * This method will encrypt the password prior to writing.
      *
      * @param string $password  Cleartext password
+     * @param bool   $write     Whether to write the member afterwards
      * @return ValidationResult
      */
-    public function changePassword($password)
+    public function changePassword($password, $write = true)
     {
         $this->Password = $password;
         $valid = $this->validate();
@@ -1697,14 +1671,57 @@ class Member extends DataObject
 
         if ($valid->isValid()) {
             $this->AutoLoginHash = null;
+
+            $this->encryptPassword();
+
+            if ($write) {
+                $this->passwordChangesToWrite = true;
                 $this->write();
             }
+        }
 
         $this->extend('onAfterChangePassword', $password, $valid);
 
         return $valid;
     }
 
+    /**
+     * Takes a plaintext password (on the Member object) and encrypts it
+     *
+     * @return $this
+     */
+    protected function encryptPassword()
+    {
+        // reset salt so that it gets regenerated - this will invalidate any persistent login cookies
+        // or other information encrypted with this Member's settings (see self::encryptWithUserSettings)
+        $this->Salt = '';
+
+        // Password was changed: encrypt the password according the settings
+        $encryption_details = Security::encrypt_password(
+            $this->Password,
+            $this->Salt,
+            $this->PasswordEncryption ?: Security::config()->get('password_encryption_algorithm'),
+            $this
+        );
+
+        // Overwrite the Password property with the hashed value
+        $this->Password = $encryption_details['password'];
+        $this->Salt = $encryption_details['salt'];
+        $this->PasswordEncryption = $encryption_details['algorithm'];
+
+        // If we haven't manually set a password expiry
+        if (!$this->isChanged('PasswordExpiry')) {
+            // then set it for us
+            if (static::config()->get('password_expiry_days')) {
+                $this->PasswordExpiry = date('Y-m-d', time() + 86400 * static::config()->get('password_expiry_days'));
+            } else {
+                $this->PasswordExpiry = null;
+            }
+        }
+
+        return $this;
+    }
+
     /**
      * Tell this member that someone made a failed attempt at logging in as them.
      * This can be used to lock the user out temporarily if too many failed attempts are made.