Page MenuHomePhabricator

loginAttempt.php should use a hook, not a LoginNotify instance
Closed, ResolvedPublic


@kaldari, when I created the maintenance script described in T183722 I took your advice and made it such that it creates a LoginNotify instance and calls the appropriate function. Originally, I had designed it such that it would call the AuthManagerLoginAuthenticateAudit hook, just like a real login attempt would, but you swayed me to not do so.

@MaxSem, later when I created a patch for T174492, you discouraged me from creating a dependency to LoginNotify, and encouraged me to use the AuthManagerLoginAuthenticateAudit hook instead, so that CheckUser can log failed login attempts without any need for LoginNotify to be installed.

These two can't go together! We either have to use the hook in both places, or create a dependency from CheckUser to LoginNotify. Otherwise, the maintenance script will NOT create entries in CheckUser logs. And that makes the maintenance script useless; the idea behind that script is to allow developers (like me) to generate some fake login attempts from arbitrarily assigned IPs, to easily test the code they write for other features like the Echo notification that is going to be modified through T174388 and T174562. Keep in mind that Echo *will* depend on CheckUser for those, because we want to make sure that once the old IP data is purged from CheckUser, it is also unavailable to Echo (consistent with WMF retention policies).

The purpose of this task is for you to reconcile this conflict. My vote: let's use the hook in the maintenance script.

Event Timeline

Huji removed Huji as the assignee of this task.Feb 16 2018, 1:37 AM
Huji triaged this task as High priority.
Huji created this task.
Huji edited projects, added CheckUser; removed MediaWiki-Maintenance-system.

Was making an amended version when you posted it:)

Niharika renamed this task from loginAttemp.php should use a hook, not a LoginNotify instance to loginAttempt.php should use a hook, not a LoginNotify instance.Feb 20 2018, 8:14 PM

@MaxSem thanks for fixing the patch for T174492. Now it is time to work on this task. Would you say it'd be a good idea to change loginAttempt.php to use the Hook?

Change 413084 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/LoginNotify@master] Update loginAttempt.php for new login handling code

Change 413084 merged by jenkins-bot:
[mediawiki/extensions/LoginNotify@master] Update loginAttempt.php for new login handling code

Huji claimed this task.
Huji removed a project: Patch-For-Review.