diff --git a/docs/en/04_Changelogs/4.0.0.md b/docs/en/04_Changelogs/4.0.0.md index a9b21db12..30157d20c 100644 --- a/docs/en/04_Changelogs/4.0.0.md +++ b/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. diff --git a/src/Security/Member.php b/src/Security/Member.php index b77764341..c8039c0b6 100644 --- a/src/Security/Member.php +++ b/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. - // Note that this only works with cleartext passwords, as we can't rehash - // existing passwords. - if ((!$this->ID && $this->Password) || $this->isChanged('Password')) { - //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 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; - } - } + // 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->Password) || ($this->isChanged('Password') && !$this->passwordChangesToWrite)) { + $this->changePassword($this->Password, false); } // 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 - * the validation result which cannot be modified. + * and can check it after the write also. * - * @param string $password Cleartext password + * 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,7 +1671,13 @@ class Member extends DataObject if ($valid->isValid()) { $this->AutoLoginHash = null; - $this->write(); + + $this->encryptPassword(); + + if ($write) { + $this->passwordChangesToWrite = true; + $this->write(); + } } $this->extend('onAfterChangePassword', $password, $valid); @@ -1705,6 +1685,43 @@ class Member extends DataObject 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.