Last modified: 2014-09-23 23:52:45 UTC
Created attachment 7578 [details] more judicious use of LoginAuthenticateAudit hooks We wanted to create a log of hack attempts on our wiki but hooking in to LoginAuthenticateAudit only captures login attempts from valid users. Could the hook be added anywhere the authenticateUserData function returns? Attached is the solution we ended up using against version 1.15.3.
*Bulk BZ Change: +Patch to open bugs with patches attached that are missing the keyword*
Thanks for the patch, Michael, and sorry for the wait! Signalling that this patch needs review by adding the "need-review" keyword.
Created attachment 9446 [details] updated patch for 1.17.0
Next time, please try to use the code from subversion so we can apply it more easily r103396
Hi. I reverted this because it called the hook with the argument $u in places before $u is defined (Which broke unit tests). For example when we check if the user didn't enter a name, we don't have a user object yet. Probably way forward would be to create a new hook to audit situations where the login was rejected out of hand for the user being 'stupid'. Cheers.
This was re-applied in r103467 & r106446 but wasn't marked FIXED. I've reverted it (again) in r110797, so REOPENED is appropriate.
Created attachment 9973 [details] patch against trunk I'll submit one more, very stripped down, try at this. Previous patch was blindly calling the hook every time authenticateUserData returned which was obviously overkill. This should allow logging of failed login attempts with invalid user names.
Michael, thanks for the new patch. I have changed the "reviewed" keyword to "need-review" to indicate that the new patch has not yet been reviewed and awaits critique and/or acceptance.
Brion, could I ask you to look at this?
The patch in its current form is not OK. The hook in its current state is documented as passing a User object. If, by chance, there's an extension that decided to use type hinting on the hook call, this would cause errors in those cases. Also, it's just bad design. However, it is a good idea to be calling this hook all the time, so I'll try and think of a proper solution when I get back from lunch.
https://gerrit.wikimedia.org/r/37788 This restructures User::authenticateUserData so that it always reaches the end of the function. Also, it guarantees that a User object is always passed to the hook.
https://gerrit.wikimedia.org/r/37788 (Gerrit Change I316bbacf4d9fa5558c63fb09e8d0aef98503c556) | change ABANDONED [by Parent5446]
Change 27022 abandoned by Parent5446: Re-implemented Special:Userlogin using FormSpecialPage. Reason: The rest of the login system needs to be fixed before the UI part. https://gerrit.wikimedia.org/r/27022
