When changing password or email address, require to type old password #13683
Conversation
tsteur
added
not-in-changelog
| { | ||
| $requirePasswordConfirmation = self::$UPDATE_USER_REQUIRE_PASSWORD_CONFIRMATION; |
There was a problem hiding this comment.
Couldn't find a better way to make updateUser verify the password but sometimes not. Eg passwordResetter wouldn't know the current password of the user as it is stored hashed...
things like only checking it when Request::isRootRequestApiRequest() would not work (cause some plugins call it through API) and neither $requirePasswordConfirmation = Request::isCurrentApiRequestTheRootApiRequest() cause eg because people could avoid it by using bulk requests etc.
There was a problem hiding this comment.
Could you just use the UserUpdater that you introduced? For PasswordResetter I assume it would be ok, since it doesn't use Request::processRequest().
There was a problem hiding this comment.
Oh I see below that class just issues an API request. Could that class instead just use the model & set the password directly? I assume all it would need to do is hash the password and call Model::updateUser().
There was a problem hiding this comment.
I thought about it as well and then figured it may be beneficial to still use the API just to have all the permission checks running again, the code is covered by tests, deletes tracker cache after updating user etc. Otherwise might be too easy to regress something
| private function verifyPasswordCorrect($userLogin, $password) | ||
| { | ||
| /** @var \Piwik\Auth $authAdapter */ | ||
| $authAdapter = StaticContainer::get('Piwik\Auth'); |
There was a problem hiding this comment.
I'm hoping this works with other auth plugins
There was a problem hiding this comment.
I haven't looked at your other PR, but will this be an issue for the new two factor auth plugin?
There was a problem hiding this comment.
I don't think it is. It's only needed for login initially and the password is only a double check here. We will need to update the brute force prevention PR though (lock down IP #2888 ).
| public function updateUserWithoutCurrentPassword($userLogin, $password = false, $email = false, $alias = false, | ||
| $_isPasswordHashed = false) | ||
| { | ||
| API::$UPDATE_USER_REQUIRE_PASSWORD_CONFIRMATION = false; |
There was a problem hiding this comment.
Added this class so each time this is needed it doesn't need to be re-developed and no mistakes can be made. (eg forgetting to reset API::$UPDATE_USER_REQUIRE_PASSWORD_CONFIRMATION = true;)
|
Noticed one thing while testing locally: whenever the password confirm comes up, I would immediately enter my password (which would work since it is focused), then press enter. But that doesn't submit the modal, so I'd have to click Yes explicitly. Might be good to add this behavior, was annoying to me at least. |
|
@diosmosis I'm resolving merge conflicts and fixing tests... is this otherwise good to merge? |
I'll quickly look into it but have a feeling it might not be so easy |
|
added the "enter" functionality @diosmosis |
|
@tsteur 👍 looks good |
When changing email or password, the password will be required for the current user.
refs matomo-org/plugin-LoginLdap#174
fix #2932
Todo: We need to update either this PR or #13472 depending on which one gets merged first to apply brute force / lock down changes