[5.2] dont increment login attempt if the user is locked out

[tomschlick]

fixes #11967

[GrahamCampbell]

Surely this isn't correct?

[GrahamCampbell]

If someone attempts, the attempt could should increment. The fact that this fixes the other bug may be true, but it introduces another bug.

[tomschlick]

This bug (#11967) would be especially troublesome with lower rate limiting thresholds (around 3 or so).

I don't see another solution based on how the RateLimiter class increments. We would need to change that to a rate limit "session" which increments to the same instance even after lockout.

[stuartwagner]

Not to resurrect a four-month old issue, but the original bug (#11967) still persists. Furthermore, I believe the changes merged here don't work as intended.

From what I understand, the issue lies with the RateLimiter's decay value being represented in minutes, whereas the ThrottlesLogins trait determines lockouts in seconds. Additionally, ThrottlesLogins does not allow for the RateLimiter's decay value to be passed along, meaning the RateLimiter's default value of 1 minute is used at all times.

The bug seems to happen whenever the specified lockout time is less than that one-minute default, as the RateLimiter's cache sticks around longer than the user is denied access. In that small window of time, between the lockout lifting and the cache expiring, ThrottlesLogins will see the still-active failure count and immediately lock the user out again. Only when the user waits a full minute from when they were locked out does that count expire.

I would attempt to fix this and send a PR; however, the changes needed, while minor, go as deep as the Cache Stores, which send decay durations of $minutes * 60 to their drivers. At the very least, allowing ThrottlesLogins to set the decay might be nice to have.

[tomschlick]

What if when the lockout flag is added we reset the login attempt count? Then further requests while locked out would't increment the count at all. Once that lockout expires you would already have the reset counter to try again...

Thoughts?

[stuartwagner]

Yes, I believe that would work.

[tomschlick]

Ok, I didn't look at the actual code before commenting that but I'm glad it sounded sane :)

I'll look into it and submit a PR unless you want to do it yourself?

[tomschlick]

Nevermind, it was an easy fix. Pushing PR now.

[stuartwagner]

Cool. In fact, in the clear() method there, you could even change the second call to forget() to this:

$this->resetAttempts($key.':lockout');

[tomschlick]

eh, i think that should stay as it is because its not really resetting an attempt, its removing the :lockout key

[stuartwagner]

I do have a further point of concern. I would put this in a separate issue, but it's really part of the same discussion.

The login() method in AuthenticatesUsers checks for lockout only once, before an auth attempt is made. When the user fails the maximum number of times, the count is incremented to indicate a lockout, but they are not actually locked out until the next request occurs, and the login() method checks for a lockout again. This does two things; 1) it gives the user the false impression that they have one more chance to log in successfully than they actually do, and 2) it starts the lockout time on the subsequent request, which could be some time later.

My opinion is that login() needs to check for lockout twice, once before the database is hit, and once more after the current request fails, so that the user is locked out right away (and told as such). My only hesitation is that it might be a bit repetitive to call hasTooManyLoginAttempts() twice.

[tomschlick]

Yeah I would raise that in a separate issue and see what Taylor thinks. I don't think an extra call to hasTooManyLoginAttempts() would be that big of a deal as it would remove that extra (already banned) login request.

[stuartwagner]

Alright, I'll do that. Thanks!

[JoostK]

This commit makes no sense. If $lockedOut were true, then we would immediately bail at line 69. So, the added check at line 81 is always true.

[stuartwagner]

I agree. That's what I meant above when I said "the changes merged here don't work as intended".

However, I believe the original issue is now fixed as the follow-up PR to this was merged recently.