@kaldari @Legoktm can I ask your opinion here?

I am working on a patch that is modeled after how AbuseFilter stores its logs in CheckUser. The issue is, AbuseFilter actually has a log, and so it takes an entry from that log to create a RecentChange object using [[ https://phabricator.wikimedia.org/source/mediawiki/browse/master/includes/logging/LogEntry.php;eff03050215f757bf6e04f021abb9b9d4b0629b4$682 | RecentChage::newLogEntry ]] (via [[https://phabricator.wikimedia.org/diffusion/EABF/browse/master/includes/AbuseFilter.class.php;0019dc226502c3f6d5ab0cff864cc5ad6a5d344c$1204 | LogEntry::getRecentChange ]]) and then [[ https://phabricator.wikimedia.org/diffusion/EABF/browse/master/includes/AbuseFilter.class.php;0019dc226502c3f6d5ab0cff864cc5ad6a5d344c$1205 | passes that RecentChange object to CheckUser ]]. LoginNotify, on the other hand, does not have a log.

Should we create a log for LoginNotify (but restrict access to it, and most likely not give anyone access to it on WMF wikis)? Or do you know of a better strategy for generating an RC object?

Update: I am now more inclined to just add a new hook specifically for this use case. If you think that is the better approach, please abandon my patch below.

Change 408022 had a related patch set uploaded (by Huji; owner: Huji):
[mediawiki/extensions/LoginNotify@master] WIP: DO NOT MERGE

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

I created an initial non-working draft of the idea I have, in the patch above. Your guidance is much appreciated!

Another important question to answer at the same time: do we want to log the events in CU only for the initiating IP, or both for that and for that target user? In other words, if IP 1.2.3.4 makes a failed attempt to log into my account (User:Huji), when I run a check on 1.2.3.4 I will see that; but should I be able to also see that by running a check on User:Huji?

It would be beneficial to see that, because if a user complains about multiple failed login attempts, then I can find the origin of those and take action. But CU currently does not support that (the only way to achieve it is to generate two rows of CU data, one for the originator, one for the target).

Alternatively, we can say that only one row of data should be stored in CU, and that is for the originating IP. Users who have a complaint should report the IP to the CU themselves (that assumes we show the IP to the target users via Echo, subject of discussion in T174388; if we decide to only show a location and the IP, then this approach won't be realistic).

Change 408049 had a related patch set uploaded (by Huji; owner: Huji):
[mediawiki/extensions/CheckUser@master] Log unsuccessful login attempts in CheckUser

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

Change 408022 abandoned by Huji:
WIP: DO NOT MERGE

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

Change 408050 had a related patch set uploaded (by Huji; owner: Huji):
[mediawiki/extensions/LoginNotify@master] Log unsuccessful login attempts in CheckUser

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

This approach (combinatino of https://gerrit.wikimedia.org/r/#/c/408050/ to call a hook and https://gerrit.wikimedia.org/r/#/c/408049/ to define the callback function) seems more reasonable. I cannot get it to work though; when I try it, I get this error message: MWException from line 167 of /var/www/html/includes/Hooks.php: Invalid callback CheckUserHooks::logLoginAttempt in hooks for LoginNotifyFailedAttempt

Not sure what I have done wrong.

@Hugi: I think CheckUserHooks::logLoginAttempt will need to be public, not protected.

@Platonides you are absolutely correct!

Now, when I run $dwb->insert() is there a way for me to get back the cuc_id of the newly inserted row? We would need this to be used in T174388

Use $dbw->insertId()

Change 408050 abandoned by Huji:
Log unsuccessful login attempts in CheckUser

Reason:
Change of strategy in the dependency

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

Change 408049 merged by jenkins-bot:
[mediawiki/extensions/CheckUser@master] Log unsuccessful login attempts in CheckUser

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

@Huji Sorry I'm late to the party - can you tell me where are you surfacing these in the UI? I'm wondering if we can do the same for the new Special:Investigate page.

In T174492#6187387, @Niharika wrote:

@Huji Sorry I'm late to the party - can you tell me where are you surfacing these in the UI? I'm wondering if we can do the same for the new Special:Investigate page.

It chucks a row in cu_changes :)

https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/CheckUser/+/408049/14/includes/CheckUserHooks.php

So anywhere that queries it should bring them up...

Except, it's not enabled on WMF wikis just yet as per T253802: Configure WMF wikis to log login attempts in CheckUser

Gotcha! Thanks @Reedy!