isLockedOut, Forgot Password flow and LoginAttempts table creates a confusing user experience

[muppsy007]

Affected Version

All versions. 3.x and 4.x

Description

In older versions of Silverstripe, the lockout mechanic was pretty simple. You set a max number of attempts, and number of minutes you want to lock people out if they hit that max number of failed attempts. Member.FailedLoginCount tracked how many bad attempts were made. Member.LockedOutUntil was set to a date in the future if max attempts were made. If the latter was not null, user is locked out.

At some point, LoginAttempts was introduced to track every actual login attempt. With this, some auxiliary logic was added to Member.isLockedOut().

This logic appears redundant unless the intention is to do away with the original Member fields. They effectively do exactly the same thing. With the added risk of exponential overhead as LoginAttempts table becomes large, while the Members table is much more finite and predictable.

But moreover, this added mechanic creates a confusing state for locked out users should they attempt to expedite their locked out state by doing a password reset. This is the primary focus of this issue below.

Steps to Reproduce

1. Set lock_out_after_incorrect_logins to 4
2. Set lock_out_delay_mins to 60
3. User attempts to login with bad password four times in a row
4. Member.LockedOutUntil is set to future date. Four records are added to LoginAttempt table with state of Failed. Login page tells user they are locked out for n minutes.
5. User goes through lost password process, which is not blocked even when locked out
6. After successfully changing password, Member.LockedOutUntil is nulled, but user is still locked out because the last four records in the LoginAttempt table are Failures.

Desired/Suggested outcome

It seems to me that it's a discussion regarding the overall security of what locked out means. There's multiple options. This issue was discussed here, where it was suggested that it was fixed. But it actually wasn't. All that happened in this PR was Member.LockedOutUntil was set to null when a change of password was made. It did nothing about the LoginAttempt logic that was the actual cause.

Should a locked out user be able to get around a locked out period by resetting their password using the email account that only they should have access to? If yes, then this check for LoginAttempts needs to either go from isLockedOut(), or a new record added to LoginAttempts at the point of changing the password. If no, the user should never be able to start the forgot password process in the first place, because once they are done they are still locked out when they would likely expect to not be. And until that locked out window has passed, they don't even know if their password is even changed. Consideration also needs to be given here to situations where lock out policy is actually days, not hours.

Then there's the notion of the LoginAttempts table being added, and especially the added logic running at every single login for every single user without consideration to how big that table might get over time when new records are added at every single login attempt for every single user. I can absolutely see the benefit of the added meta. But perhaps this issue of never having any control over the size of that table adds weight to redundant nature of querying it in isLockedOut()?

[lekoala]

There should be no dependency on the LoginAttempt table in my opinion. I had the issue so many times in my projects I had to add a "unlock" button in the admin so that I can manually unlock users when needed by removing the failed LoginAttempts

https://github.com/lekoala/silverstripe-base/blob/ebebc08eea8b1806e7a458d41e50857c1cea5940/src/Security/BaseMemberExtension.php#L291

(which is not great because you lose track of the failed attempts, which is why i think this table should be there for logging purposes only)

[wilr]

PR at #11176