laravel / fortify

Backend controllers and scaffolding for Laravel authentication.
https://laravel.com/docs/fortify
MIT License
1.61k stars 294 forks source link

Illuminate\Auth\Events\Failed not being triggered #145

Closed nckrtl closed 3 years ago

nckrtl commented 3 years ago

Description:

It seems that Illuminate\Auth\Events\Failed is not being triggered upon a failed login attempt. Other auth events are working fine as far as I can tell.

Steps To Reproduce:

  1. Setup Laravel with jetstream
  2. Add auth event listeners as described here: https://laravel.com/docs/8.x/authentication
  3. Add a dd() in the handle method of the listeners that's bound to Illuminate\Auth\Events\Failed
  4. Login with faulty credentials
  5. The dd() in the listener won't be triggered.

This issue is also being described here: https://stackoverflow.com/questions/64666382/laravel-8-auth-attempting-and-failed-events-not-firing-as-expected

The reason I made this bug report in the fortify repository is because I'm also having this issue without Jetstream and just Fortify. That's why I think it's perhaps Fortify related.

I'm not sure how to further debug to get to the root of the problem, any help would be appreciated.

Edit: After some digging around I noticed that the default order of Fortify::authenticateThrough is:

        Fortify::authenticateThrough(function (Request $request) {
            return array_filter([
                    config('fortify.limiters.login') ? null : EnsureLoginIsNotThrottled::class,
                    RedirectIfTwoFactorAuthenticatable::class,
                    AttemptToAuthenticate::class,
                    PrepareAuthenticatedSession::class,
            ]);
        });

Should AttemptToAuthenticate::class not come before RedirectIfTwoFactorAuthenticatable::class? Like:

        Fortify::authenticateThrough(function (Request $request) {
            return array_filter([
                    config('fortify.limiters.login') ? null : EnsureLoginIsNotThrottled::class,
                    AttemptToAuthenticate::class,
                    RedirectIfTwoFactorAuthenticatable::class,
                    PrepareAuthenticatedSession::class,
            ]);
        });

By making this change in the FortifyServiceProvider the events fire as expected. Not sure if this might have any unintended side effects.

driesvints commented 3 years ago

After some digging around I noticed that the default order of Fortify::authenticateThrough is:

Where do you see this? I can't find this anywhere.

nckrtl commented 3 years ago

https://github.com/laravel/fortify/blob/1.x/src/Http/Controllers/AuthenticatedSessionController.php

Thats where the default login pipeline is defined. So my initial statement that the "original" Fortify::authenticateThrough function is like that is not correct. You can alter the default pipeline with the earlier posted snippet in the fortify/jetstream service provider. As mentioned here in the jetstream docs: https://jetstream.laravel.com/1.x/features/authentication.html#customizing-the-authentication-pipeline

I hope this makes it a bit more clear.

driesvints commented 3 years ago

Switching those two is something we cannot do as the AttemptToAuthenticate will actually attempt to login the user. The event not firing does seems like a genuine concern though.

nckrtl commented 3 years ago

I could set up a repo so it can be verified, if that would help.

driesvints commented 3 years ago

No, we verified it ourselves. Just trying to figure out what the best solution could be.

driesvints commented 3 years ago

Fixed in next release thanks to @rodrigopedra