Fix logging of failed logins & unknown users

[axllent]

Fixes the authenticationFailed() method to correctly return the login email, and adds authenticationFailedUnknownUser() method to log unknown user login attempts.

Issue

• Logging of failed logins & unknown users broken #63

[GuySartorelli]

Thank you for submitting this fix.
Can you please add a unit test for this?
Please also create or link to an associated issue as per the contribution guide.

[axllent]

@GuySartorelli - My apologies, I have created #63 to explain the issue in greater detail referencing this PR.

As for the unit tests however, I haven't a clue where to begin as both of these presumably require an actual failed login attempt via the form, unlike all the other unit tests which assume an identity using $this->logInWithPermission() (which may be why there were never any unit tests for these to start with).

[GuySartorelli]

No worries.

I've added a test. I also noticed that this affects the 2.x line, so I've retargetted this PR to the current patch release branch for that release line.

[GuySartorelli]

I also took the liberty to remove the authenticationFailedUnknownUser() method you were adding - it doesn't seem related to fixing the actual bug, so if you want to introduce that new API please raise a new PR for it, and make a good case for it being needed. It doesn't seem needed to me though.

[axllent]

Thanks @GuySartorelli, you're a star. If the auditor module is supposed to include "Login attempts (failed and successful)", then surely that also includes failed logins when no matching Email is found (ie: authenticationFailedUnknownUser())?

[GuySartorelli]

. If the auditor module is supposed to include "Login attempts (failed and successful)", then surely that also includes failed logins when no matching Email is found (ie: authenticationFailedUnknownUser())?

That scenario covered with the logged warning when no email is present. I couldn't find an easy way to test the warning is logged but the functionality is there.

[axllent]

That scenario covered with the logged warning when no email is present. I couldn't find an easy way to test the warning is logged but the functionality is there.

No it's not actually - that is my point. Unless both my understanding and testing is incorrect, according to the logic here, if a member exists with the email (incorrect password though) then authenticationFailed() is called, however if no member with the email exists, then authenticationFailedUnknownUser() is called. This means that currently the only failed logins that are tracked in the audit are those attempts where a member with the email exists, however login attempts using invalid emails (ie: no matching member) are not.

The warning you are referring to in authenticationFailed() is when neither $data['Login'] nor (once patched) $data['Email'] exist - although I don't think that's possible - at least not when called via MemberAuthenticator::class as it requires $data['Email'] (else a InvalidArgumentException is returned before the audit logs ~ potentially redundant logic).

[GuySartorelli]

Ahhh, I see. I misunderstood what you meant and there was no context until now about what that new method does. Thank you for providing that extra context.

I've added that extension hook method back in and added a test for it.

[axllent]

Whew - I was starting to doubt my end-of-year sanity :) Glad it makes sense now!

[emteknetnz]

This will be automatically tagged as 2.6.2 shortly