Fixed issue with login throttle condition and conflict with Laravel middleware

[brnquester]

Overview

This change will fix an issue where the EnsureLoginIsNotThrottled verification is never reached if fortify.limiters.login has any value different than false or totally empty. The current ternary condition logic is inverted. Main issue report here #543 .

Also, it solves the conflict of the Laravel throttle middleware cutting the flow to Fortify throttle's own handlers by adding more granular control over the rate limits for Fortify, Laravel middleware, and the decay in seconds.

Finally, this solution will keep both throttle protections of Fortify's own handler and Laravel default middleware. If the configs proposed here are followed, the Fortify EnsureLoginIsNotThrottled will be triggered first returning a friendly message Too many requests. Please try again in X seconds. which plays nicely with InertiaJS (Other issue #312) and if the requests continue replicating a brute force behaviour, then Laravel kicks in.

In the current setup, there is a lack of clarity and differentiation between Laravel middleware throttle and Fortify's own throttle handler considering one limiter config only. Also, it has been demonstrated through multiple issues reported that there is a problem with overlapping and conflicting two approaches.

Steps to reproduce without this fix

1. Make sure to have the limiters defined in config/fortify.php as follows:

'limiters' => [
        'login' => 5,
        'two-factor' => 5,
    ],

2. Remove the throttle middleware from the Route::post(RoutePath::for('login', '/login') keeping everything else as is, see below: (Fortify has its own throttle error handlers, it shouldn't need to rely on Laravel defaults throttle middleware)

Route::post(RoutePath::for('login', '/login'), [AuthenticatedSessionController::class, 'store'])
        ->middleware(array_filter([
            'guest:'.config('fortify.guard')
        ]));

4. Now attempt multiple logins passing the limiter mark, you can try even 20 times. You'll never see the error "Too many login attempts. Please try again in X seconds.".
5. Now go to the default AuthenticatedSessionController in Fortify changing the line for the EnsureLoginIsNotThrottled below to config('fortify.limiters.login') ? EnsureLoginIsNotThrottled::class : null. Just invert it.
   

   fortify/src/Http/Controllers/AuthenticatedSessionController.php

   Lines 85 to 87 in a725684

   	return (new Pipeline(app()))->send($request)->through(array_filter([
   	config('fortify.limiters.login') ? null : EnsureLoginIsNotThrottled::class,
   	config('fortify.lowercase_usernames') ? CanonicalizeUsername::class : null,

6. Now try to log in multiple times, at some point, you'll see the error "Too many requests. Please try again in X seconds.".

Relevant issues found

• Lockout event is never fired  #336
• Throttle middleware does not play nicely with Inertia #312
• Login throttle ternary condition inside AuthenticatedSessionController seems incorrect #543

[taylorotwell]

I don't agree that anything needs to be changed here.

[brnquester]

I don't agree that anything needs to be changed here.

@taylorotwell - OK. I'll try my best to defend the point here with facts. The current code have one config (fortify.limiters.login) that provides the option to configure the throttle rate. By default, it is only used in two places, in the routes which is passed to the Laravel middleware, and to Fortify's login pipeline. It is not passed to Fortify EnsureLoginIsNotThrottled nor LoginRateLimiter. The LoginRateLimiter has all the rates hardcoded. So what is the point of that config then? At this moment, it is to generate confusion only.

Finally, we have one config option that is mutually exclusive. If I set the fortify.limiters.login it doesn't run EnsureLoginIsNotThrottled and run Laravel Throttle, if I set it to empty or false, it breaks the Laravel middleware and uses the Fortify's handler.

You have to agree that at minimum it is misleading and lacks a lot of clarification. You can see that through multiple issues already reported in the past. I pointed some of them here with 5 min searching. There are threads in many websites about it.

[taylorotwell]

No, I don't agree it is confusing. You only set foritfy.limiters.login if you want to totally override the entire rate limiting strategy for logins with your own user defined rate limiter. It should be a string name, not a number - I think this is where you are confused.

`login` => 'the-name-of-my-rate-limiter`

https://laravel.com/docs/11.x/routing#rate-limiting

[brnquester]

No, I don't agree it is confusing. You only set foritfy.limiters.login if you want to totally override the entire rate limiting strategy for logins with your own user defined rate limiter. It should be a string name, not a number - I think this is where you are confused.

`login` => 'the-name-of-my-rate-limiter`

https://laravel.com/docs/11.x/routing#rate-limiting

@taylorotwell - I'm not confused about that. I might not be doing a good job communicating this because I feel you're hundreds of miles away from the point of my PR. The point is when and how can Fortify's throttle system (verified here: EnsureLoginIsNotThrottled) be used so we can leverage the friendlier error message instead of the flow cut middleware. To do that, I would need to either set foritfy.limiters.login to false or leave it empty, which becomes confusing and with little to no flexibility to control since your rate limits are hard coded. That's all.

Anyway. I tried. Feel free to close this PR if you still don't agree. I have solved it on my end. Regardless of the disagreement, awesome package, great community, good job with the eco-system of Laravel.

[crynobone]

Upon investigating on this yes @taylorotwell is correct on the matter.

The value of fortify.limiters.login is either null of the rate limiter name such as:

`login` => 'the-name-of-my-rate-limiter`

Which later then you can define the rate limiter in your service provider such as:

RateLimiter::for('the-name-of-my-rate-limiter', function (Request $request) {
    $throttleKey = Str::transliterate(Str::lower($request->input(Fortify::username())).'|'.$request->IP());

    return Limit::perMinute(5)->by($throttleKey);
});

fortify/stubs/FortifyServiceProvider.php

Lines 36 to 40 in 5c2e9cd

	RateLimiter::for('login', function (Request $request) {
	$throttleKey = Str::transliterate(Str::lower($request->input(Fortify::username())).'|'.$request->ip());
	return Limit::perMinute(5)->by($throttleKey);
	});

[brnquester]

@taylorotwell @crynobone - thanks for taking the time to review and for the input. Closing the PR.