Is TwoFactorAuthenticatedSessionController missing PrepareAuthenticatedSession?

[LeoniePhiline]

• Fortify Version: 1.7.2
• Laravel Version: 8.19
• PHP Version: 7.4

Description:

Authentication with vs. without 2fa

When authenticating without the Features::twoFactorAuthentication() feature then AuthenticatedSessionController pipes $request through PrepareAuthenticatedSession. If 2fa is used, though, then this step is skipped and we continue in TwoFactorAuthenticatedSessionController.
The latter still performs a $session->migrate(true) through $guard->login(...) (like AttemptToAuthenticate for non-2fa logins), but does not regenerate the CSRF token ($session->regenerate()), like PrepareAuthenticatedSession would perform for non-2fa logins. Neither is the limiter cleared after successful 2fa login. These points made me wonder if TwoFactorAuthenticatedSessionController was missing a continuation of AuthenticatedSessionController's login pipeline.

Missing login rate limiter increment at failed 2fa challenge

When failing to enter the correct 2fa code, LoginRateLimiter::increment() is not called - only if the initial password challenge fails. Doesn't this make brute-forcing the second factor possible?

Using existing user provider methods

On a side note, I noticed that RedirectIfTwoFactorAuthenticatable::validateCredentials() performs its own credentials check, where AttemptToAuthenticate::handle() uses the provider's retrieveByCredentials() method. It might make sense to use \Illuminate\Contracts\Auth\UserProvider::retrieveByCredentials() and \Illuminate\Contracts\Auth\UserProvider::validateCredentials() in RedirectIfTwoFactorAuthenticatable as well, instead of the current custom ad-hoc implementations, even if Fortify is meant to depend on Eloquent as users provider.

[driesvints]

Thanks for the clear issue. I'll let Taylor check in on these.

[driesvints]

@LeoniePhiline I think I got all of them. Taylor is going to review the PRs I made. Thanks for reporting 👍

[driesvints]

I'm going to close this now that we merged the main PR for this.

[andreasnij]

@driesvints The rate limiter still doesn't seem to be cleared after two factor login. This line is missing from the two factor login flow:
https://github.com/laravel/fortify/blob/master/src/Actions/PrepareAuthenticatedSession.php#L38

[driesvints]

@jandreasn can you send in a PR?

[andreasnij]

@driesvints I was planning to but after looking into it a bit more I'm not sure what the best solution is. Clearing just the LoginRateLimiter is easy but there's another separate two factor limiter now too.