[go: up one dir, main page]

Page MenuHomePhabricator

Show the reason(s) for failed logins in checkuser results
Closed, ResolvedPublicFeature

Description

Feature summary (what you would like to be able to do and where):
Instead of showing in checkuser results "Failed to log in" when a failed login attempt is made, show the reason(s) for the failed login attempt. Currently, when someone tries to log in to a locked account the associated action description only says that the login attempt failed even if the password was correct.

Use case(s) (list the steps that you performed to discover that problem, and describe the actual underlying problem which you want to solve. Do not describe only a solution):
Take an example where a sockmaster logs in with a sock that now cannot edit either through being blocked or locked. They then create a new account and proceed to edit using it on the same or very similar IP address (i.e. similar range). A checkuser then checks the new user or associated IP address, and finds the two accounts:

  • If the user was only blocked, the login attempt to the already blocked account would be shown as successful in the CU results which means the CU can be sure that the person who logged in had the correct password and thus (unless compromised) was the person who created the account
  • If the user was (also) locked, the login attempt fails before the password is checked and so the CU results only show that the login attempt failed. The CU cannot know whether the person trying to log in had the correct password, and so cannot say whether the login attempt was made by the owner of the account.

If the Checkuser knows that both the account probably isn't compromised and that the password was correct, they can say that the account was signed into by the creator of that account. If the previously blocked / locked account was a sockpuppet, this may mean that the checkuser can through comparing technical evidence say the new account is a sockpuppet. If the CU log entry only says it was a failed attempt, the checkuser cannot be sure that the login attempt wasn't made to try joe-job the locked account.

If the password was correct, this feature request would mean that the checkuser would be able to see that the password was correct but the login attempt still failed due to the account being locked. Without this feature request being implemented there is no way to know whether the password was correct when the account is locked.

Benefits (why should this be implemented?):
Being able to see why a login attempt failed would make the CU results more useful as detailed in the use case above. Furthermore, with the upcoming changes to User Agent strings in Chrome (T242825), the User Agent string / information may become too generic to be able to fingerprint users. At the moment, a User Agent string being the same between the failed login attempt and the creation of the new account is a way to more strongly say that the two accounts could be related. Once / If UA strings become very generic, being able to see off a joe-job attempt would be harder.

Extended details
I could not see a pre-existing ticket for this, but if there is please do merge.

Although this adds reasons to why a login attempt failed, checkusers already can see if a login attempt was successful (and thus unless the account is locked) will know if the password provided was or was not correct, because it's easy to verify if the account existed and if it's locked (which are the only other usual reasons for the login to fail). However, I understand that this might need a review to see it if complies with the privacy policy.

I've had a look at the checkuser extension / central auth extension source code, and think that the way this could be achieved would be to:

  • Modify CentralAuthPrimaryAuthenticationProvider.php's beginPrimaryAuthentication() so that the check to see if the account is locked is placed underneath the function call to CentralAuthUser.php's authenticate()
  • Modify CentralAuthUser.php's authenticate() so that it can return whether the password was correct and whether the account is locked. This could be done by adding a new possible string return type such as "lockedbadpassword" / "lockedgoodpassword", or even better (but more breaking) by changing the return type to a list of strings with an empty list meaning "ok" and a non-empty list meaning not okay (such as a list of "locked" and "badpassword").
  • Modify CentralAuthPrimaryAuthenticationProvider.php's beginPrimaryAuthentication() to return a AuthenticationResponse::fail that indicates the possibly multiple reasons the request failed, which may require modification to AuthenticationResponse to achieve.
  • Modify the Checkuser extension Hooks.php function onAuthManagerLoginAuthenticateAudit() to use a message that indicates the reasons for why the login attempt failed. This could be "checkuser-login-failure-badpassword", "checkuser-login-failure-locked-badpassword" "checkuser-login-failure-locked-goodpassword", and "checkuser-login-failure-nouser". The message used would depend on the information from the provided AuthenticationResponse object.

Event Timeline

Adding CentralAuth as a project tag as it would require modification to that extension too to make this change work if I understand correctly.

Although I have a rough idea how to implement this, I'm not really used to submitting patches / pull requests to mediawiki code. If I have the time I might see if I can write a patch for this, but won't start until it's clear that this would be an allowed change under privacy policy.

I'm going to see if I can write a patch for this as I have a bit more time to spend focusing on the wiki and also because there doesn't seem to be opposition to such a patch.

Change 791719 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/core@master] Changes to MediaWiki Auth to allow other commits associated with T303192 to work

https://gerrit.wikimedia.org/r/791719

Change 791721 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/extensions/CentralAuth@master] Changes to the CentralAuth extension for T303192

https://gerrit.wikimedia.org/r/791721

Change 791747 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/extensions/CheckUser@master] Changes to the CheckUser extension for T303192

https://gerrit.wikimedia.org/r/791747

I've uploaded three patches that all need to be merged for this to be resolved. The change to core allows the CentralAuth extension to communicate with any extensions that hook onto the returned AuthenticationResponse. This allows information not intended for the client to be shared across extensions.
This is needed because if an account has it's password leaked and the account is then locked, clients attempting to login to the account shouldn't be able to know if the password was correct as this is the original behaviour. The message string can be read by the client if they use the qqx language, so this cannot be used to store this info. There was no other places that I could see to put this info, so I created an array for it. If there is something I have missed please say.
The changes to the CentralAuth and CheckUser extensions allows CentralAuth to say that the password was correct but the account couldn't be logged into for some reason. The CheckUser extension can then see this and display a new message that indicates the login attempt had the correct password but failed for another reason. At the moment the CentralAuth extension only will return this if the account was locked.

This is my first patch to MediaWiki, so if I have done something wrong please say as I will have probably missed it. I ran the phpunit tests locally using docker and also manually tested locally. I also followed the pre-commit checklist on mediawiki.

Change 791755 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/core@master] Add doc for failReasons parameter in AuthenticationResponse::newFail

https://gerrit.wikimedia.org/r/791755

Change 791755 abandoned by Dreamy Jazz:

[mediawiki/core@master] Add doc for failReasons parameter in AuthenticationResponse::newFail

Reason:

hasn't combined with the other patch

https://gerrit.wikimedia.org/r/791755

I will review the comments and address the issues in an hour or two. Thanks for the suggestions and comments so far.

Change 791719 merged by jenkins-bot:

[mediawiki/core@master] Allow AuthenticationResponse to store private failure reasons

https://gerrit.wikimedia.org/r/791719

As I've written patches for this I'll mark myself as assigned to it. Let me know if this is wrong.

Change 791721 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] Make failed login attempts store whether the password was correct

https://gerrit.wikimedia.org/r/791721

Change 791747 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] Show CUs whether the password matched on login attempts to locked accounts

https://gerrit.wikimedia.org/r/791747

With thanks to Zabe for the merge. Tested locally just now and it's working.