WIP Adding a new hashing API #8806
Conversation
New: - Hashing is now called "hashing" instead of "encryption" - There's a new interface for hash methods: "SilverStripe\Security\HashMethod\HashMethodInterface" - The new interface supports rehashing over time - Created a new _default_ hash method that just proxies through to the PHP password hashing API - Implemented a HashMethod which proxies through to the old method - Implemented a new simple service for consuming this API outside of a "Member" sense: `CryptographicHashService` - Implemented a service for using it with a member: `PasswordHashService` Implemation detail: - Detail about the passwords is stored on the same field as the password - The hash service being used is configured via config - Changing the hash using this method is non-breaking and passwords will be converted over time (when the hash is verified)
This commit also deprecates a bunch of API that is redundant because of the new API
| * | ||
| * @var bool | ||
| */ | ||
| protected $passwordChangesToWrite = false; |
There was a problem hiding this comment.
Potentially I need to restore this. I'd consider this not part of public API and it's no longer used in the new implementation
| if (!$this->ID || ($this->isChanged('Password') && !$this->passwordChangesToWrite)) { | ||
| $this->changePassword($this->Password, false); | ||
|
|
||
| if (!$this->isChanged('PasswordExpiry') && (!$this->isInDB() || $passwordIsChanged)) { |
There was a problem hiding this comment.
This is moved from the changePassword method.
|
|
||
| return $valid; | ||
| $this->changePassword($password, false, true); |
There was a problem hiding this comment.
This bit could be improved - I think. I've altered the functionality so that passwords are never set unhashed on a Member object. This is capturing calls to $member->Password = 'thing'. I'll at least like to add in a deprecation notice - but I need to update places that set a password to use PasswordHashService::setForMember first. The challenge here is Field::saveInto...
| * PasswordHashService to set passwords for members. Note that existing calls to update a password | ||
| * (`$member->Password = 'thing'`) will automatically hash now. | ||
| * | ||
| * @deprecated 4.4.0 |
There was a problem hiding this comment.
I might just be able to remove this API if we don't consider it "public".
| // Store the token for the change password form. Will be unset after reload within the ChangePasswordForm. | ||
| $session = $this->getRequest()->getSession(); | ||
| $session->set('AutoLoginHash', $token); | ||
| $session->set('PasswordResetMemberID', $member->ID); |
There was a problem hiding this comment.
The "Hash" is now a token - arguably the randomness of the token is reduced when "hashed with user info". Also now it's tracking the ID of the member in the session too, meaning that we can fetch the member and their reset tokens rather than scanning for all members.
|
I look forward to this new API @ScopeyNZ. Keep up the good work. |
| @@ -351,7 +343,8 @@ public function validateCanLogin(ValidationResult &$result = null) | |||
| $result->addError( | |||
| _t( | |||
| __CLASS__ . '.ERRORLOCKEDOUT2', | |||
| 'Your account has been temporarily disabled because of too many failed attempts at ' . 'logging in. Please try again in {count} minutes.', | |||
| 'Your account has been temporarily disabled because of too many failed attempts at ' . | |||
| 'logging in. Please try again in {count} minutes.', | |||
| null, | |||
| array('count' => static::config()->get('lock_out_delay_mins')) | |||
There was a problem hiding this comment.
Switch to shorthand array notation while you're at it?
| @@ -219,14 +221,15 @@ public function doChangePassword(array $data, $form) | |||
|
|
|||
| $session = $this->getRequest()->getSession(); | |||
| if (!$member) { | |||
| if ($session->get('AutoLoginHash')) { | |||
| $member = Member::member_from_autologinhash($session->get('AutoLoginHash')); | |||
| if ($session->get('AutoLoginHash') && ($memberId = $session->get('PasswordResetMemberID'))) { | |||
There was a problem hiding this comment.
Wouldn't this break if the session of the member has expired?
|
@ScopeyNZ Still keen to move forward with this PR? |
|
Sure. |
|
This pull request hasn't had any activity for a while. Are you going to be doing further work on it, or would you prefer to close it now? |
|
This is a bit of a pain to close - as I think it has massive value for SilverStripe. Realistically I'm not sure I'll get the enthusiasm to tackle this again, as Security is a bit of a untamed beast in SilverStripe. I'll close for now, but if anyone comes across this, feel free to reach out here or in Slack for some assistance if they want to take over. |
This is a WIP PR. I still need to get tests green, do some more testing, and write some more tests. I'm raising the PR welcoming people to have a look, give suggestions, or constructive feedback on the approach.
New:
"SilverStripe\Security\HashMethod\HashMethodInterface"
password hashing API
"Member" sense:
CryptographicHashServicePasswordHashServiceImplementation detail:
converted over time (when the hash is verified)
Related issues: