Password confirmation fix for custom login plugins. #15945
Conversation
plugins/UsersManager/API.php
Outdated
| @@ -1317,19 +1315,14 @@ public function createAppSpecificTokenAuth($userLogin, $md5Password, $descriptio | |||
| } | |||
| } | |||
|
|
|||
| if (empty($user) || !$this->password->verify($md5Password, $user['password'])) { | |||
| if (empty($user) || !$this->passwordVerifier->isPasswordCorrect($userLogin, $passwordConfirmation)) { | |||
| /** | |||
| * @ignore | |||
| * @internal | |||
| */ | |||
| Piwik::postEvent('Login.authenticate.failed', array($userLogin)); | |||
There was a problem hiding this comment.
btw @diosmosis the login failed event should be only triggered when empty($user) since isPasswordCorrect will already trigger Piwik::postEvent('Login.recordFailedLoginAttempt'); which was the goal of triggering that event. Maybe could even simply remove the empty($user) then we no longer need to post that event at all? Assuming the authenticate method should fail anyway when no user is found.
There was a problem hiding this comment.
So Login.recordFailedLoginAttempt and Login.authenticate.failed are the same?
There was a problem hiding this comment.
pretty much yes. Would have actually been better and more clear initially to use Login.recordFailedLoginAttempt there.
plugins/UsersManager/API.php
Outdated
| @@ -1297,18 +1297,16 @@ private function isUserTheOnlyUserHavingSuperUserAccess($userLogin) | |||
| * If the username/password combination is incorrect an invalid token will be returned. | |||
| * | |||
| * @param string $userLogin Login or Email address | |||
| * @param string $md5Password hashed string of the password (using current hash function; MD5-named for historical reasons) | |||
| * @param string $passwordConfirmation the current user's password. | |||
There was a problem hiding this comment.
Could we leave a comment: We recommend to always set the passwordConfirmation in a $_POST so the plain password won't appear in access logs. Ideally we would be able to force that it's sent over $_POST even? But of course this would only work if the root request is an API request for this method. We probably wouldn't be able to check !empty($_POST['passwordConfirmation']) && empty($_GET['passwordConfirmation]) in other cases. Not sure if we can implement this check using getRootApiRequestMethod() but could be interesting to force people to POST the password and to force best practice here.
There was a problem hiding this comment.
I'll update the comment, but forcing $_POST seems non-trivial. Can we create an issue for it?
There was a problem hiding this comment.
I reckon it's not too important. Can simply put a comment there then. Although it might be trivial when checking if Request::getRootApiRequestMethod() === 'UsersManager.create...'. After all we also allow the token to be passed as GET so there's not really any difference and it's not needed. Comment is definitely enough.
|
@tsteur applied review feedback |
|
Looks good if tests pass 👍 |
|
@diosmosis the parameter rename will need to be done here as well: https://github.com/matomo-org/matomo-log-analytics/blob/7266244870722c912a00b0ac51b85aa788bd6244/import_logs.py#L1017 |
mattab
added
the
not-in-changelog
edit by matt: refs #16281