Page MenuHomePhabricator

IP Masking (CommTech): LoginNotify
Closed, ResolvedPublic

Description

This task is for tracking work to update the LoginNotify extension ahead of IP Masking being enabled on WMF sites.

See T326816: Update features for temporary accounts, particularly What will be affected.

Event Timeline

A very quick cursory look at LoginNotify seems to suggest we should (at least) rework this section:

/**
 * On login failure, record failure and maybe send notice
 *
 * @param User $user User in question
 */
public function recordFailure( User $user ) {
	$this->incrStats( 'fail.total' );
		if ( $user->isAnon() ) {
		// Login failed because user doesn't exist
		// skip this user.
		$this->log->debug( "Skipping recording failure for {user} - no account",
			[ 'user' => $user->getName() ]
		);
		return;
	}
[...]

to ensure we skip temporary users.

tstarling subscribed.

If someone goes to the login form and enters the name of a temporary user and an incorrect password, there's no need to notify the temporary user because there is no risk of account compromise, because temporary users never have a password. Currently, a notification will be sent.

If you're logged in as a temporary user, and you click the "log in" link in the skin, and attempt to log in as someone else but enter the wrong password, a notification should be sent. Currently, there is a core bug which prevents LoginNotify's hook from being called with usable information, so no notification will be sent in this case.

Change 954529 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/extensions/LoginNotify@master] Don't notify of failed logins for system or temporary users

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

Change 954759 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] Don't set AuthenticationRequest::$username on login

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

Change 954529 merged by jenkins-bot:

[mediawiki/extensions/LoginNotify@master] Don't notify of failed logins for system or temporary users

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

Change 954759 merged by jenkins-bot:

[mediawiki/core@master] Don't set AuthenticationRequest::$username on login

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

MusikAnimal subscribed.

QA notes:

  • After a few notifications are sent, LoginNotify doesn't always notify on each subsequent failed login. I'm not sure what the logic is, but just keep submitting the wrong password and eventually you'll get an echo notification.
  • No need to test if emails are sent, if that makes QA easier, since this logic was not touched. Testing if you get an echo notification is sufficient (go to Special:Preferences#mw-prefsection-echo to adjust as desired)
    • If testing on local, make sure all jobs are ran (i.e. force run them with maintenance/run runJobs.php)
  • See T329774#9139001 for criteria on temporary users
dom_walden subscribed.

When attempting to login as a temporary user, I don't see any emails or echo notifications, nothing new in echo_event database, and in the logs I see:

[LoginNotify] Skipping recording failure for user *Unregistered 232 - can't authenticate

I verified on beta that login failure notifications are still sent for regular users.

Test environments: