AttemptToAuthenticate is not throwing validation exceptions by default

[sebdesign]

• Fortify Version: 1.6.1
• Laravel Version: 8.9.0
• PHP Version: 7.4.9
• Database Driver & Version: MySQL 5.7.31

Description:

This is the first time I'm using Fortify. I was trying to customize the login flow, because I want to change the default error bag on the ValidationException. But I noticed that the throwFailedAuthenticationException method on the AttemptToAuthenticate action is not being called, because the RedirectIfTwoFactorAuthenticatable action is calling validateCredentials, despite that I'm not using 2FA.

So the login pipeline is stopping at RedirectIfTwoFactorAuthenticatable::validateCredentials(), and the AttemptToAuthenticate, does not do anything, even though it's supposed to attempt to login or throw a validation exception.

Steps To Reproduce:

1. Override AttemptToAuthenticate:

   
   use Illuminate\Validation\ValidationException;
   use Laravel\Fortify\Actions\AttemptToAuthenticate;

class CustomAttemptToAuthenticate extends AttemptToAuthenticate { protected function throwFailedAuthenticationException($request) { try { parent::throwFailedAuthenticationException($request); } catch (ValidationException $e) { throw $e->errorBag('login'); } } }

2. Override login pipeline:
```php

Fortify::loginThrough(function () {
    return [
        config('fortify.limiters.login') ? null : EnsureLoginIsNotThrottled::class,
        RedirectIfTwoFactorAuthenticatable::class,
        CustomAttemptToAuthenticate::class,
        PrepareAuthenticatedSession::class,
    ];
});

3. Test the custom error bag:

   $response = $this->post('login', [
   'email' => 'invalid@email.com',
   'password' => 'password',
   ]);
   
   // this assertion fails
   $response->assertSessionHasErrors('email', null, 'login');

4. Note the exception stack trace:

   
   $this->withoutExceptionHandling();

1 vendor/laravel/fortify/src/Actions/RedirectIfTwoFactorAuthenticatable.php:98 Illuminate\Validation\ValidationException::withMessages()

2 vendor/laravel/fortify/src/Actions/RedirectIfTwoFactorAuthenticatable.php:80 Laravel\Fortify\Actions\RedirectIfTwoFactorAuthenticatable::throwFailedAuthenticationException()

3 vendor/laravel/framework/src/Illuminate/Support/helpers.php:263 Laravel\Fortify\Actions\RedirectIfTwoFactorAuthenticatable::Laravel\Fortify\Actions{closure}()

4 vendor/laravel/fortify/src/Actions/RedirectIfTwoFactorAuthenticatable.php:82 tap()

5 vendor/laravel/fortify/src/Actions/RedirectIfTwoFactorAuthenticatable.php:50 Laravel\Fortify\Actions\RedirectIfTwoFactorAuthenticatable::validateCredentials()

6 vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:167 Laravel\Fortify\Actions\RedirectIfTwoFactorAuthenticatable::handle()

7 vendor/laravel/fortify/src/Actions/EnsureLoginIsNotThrottled.php:39 Illuminate\Pipeline\Pipeline::Illuminate\Pipeline{closure}()

8 vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:167 Laravel\Fortify\Actions\EnsureLoginIsNotThrottled::handle()

9 vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:103 Illuminate\Pipeline\Pipeline::Illuminate\Pipeline{closure}()

10 vendor/laravel/fortify/src/Http/Controllers/AuthenticatedSessionController.php:60 Illuminate\Pipeline\Pipeline::then()

[driesvints]

I think this is fixed with https://github.com/laravel/fortify/pull/127. Can you confirm that you have disabled 2FA? And try the changes from the PR? I'll tag those in a bit.

[sebdesign]

@driesvints will check now

[sebdesign]

@driesvints I can confirm that with disabled 2FA, the RedirectIfTwoFactorAuthenticatable action is not executed. Which solves the issue for failed non-2FA login attempts.

However, If I enable 2FA, I need to override/extend RedirectIfTwoFactorAuthenticatable in order to customize the error bag on the ValidationException, because this action does it own validation.

But if the user doesn't have a two_factor_secret and does not use the TwoFactorAuthenticatable trait, the AttemptToAuthenticate action will be executed next, and the credentials will be validated once again.

I don't know if that's important, but that leads to a duplicate query to validate/fetch the user.

[driesvints]

What prevents you from overriding RedirectIfTwoFactorAuthenticatable?

[sebdesign]

I don't mind overriding it, since there's nothing that prevents me to do so.

What bugs me is that I have to do the exact change twice, because the credentials validation is kicking in twice, and there's also the "issue" of the duplicate queries:

(1) Handle `RedirectIfTwoFactorAuthenticatable`
(2) Validate credentials (first query)
    (3a) Throw exception if (2) fails
    (3b) Otherwise, check `two_factor_secret` and `TwoFactorAuthenticatable`
        (4a) If both are true, return the challange response
        (4b) Otherwise, handle `AttemptToAuthenticate `
            (5) Attempt to authenticate again (same query as (2))
                (6a) If login succeeded, handle `PrepareAuthenticatedSession`
                (6b) Otherwise, throw `ValidationException` like (3a)

[driesvints]

We're gonna look into that eventually but definitely feel free to send in a PR if you'd like.

[driesvints]

I'm going to close this one already since the only issue is a duplicate query which I don't consider a huge problem.