laravel / fortify

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

Fortify::authenticateUsing called twice #339

Closed oulfr closed 2 years ago

oulfr commented 2 years ago

Description:

The Fortify::authenticateUsing method is triggered twice for each request.

Steps To Reproduce:

In FortifyServiceProvider create an index: protected $i=1;

In boot method add tis function:

    Fortify::authenticateUsing(function (Request $request) {
        $this->i++;

        if( $this->i > 2){
            dd('hmm', $this->i);
        }
        $user = User::where('email', $request->email)->first();
        if ($user && Hash::check($request->password, $user->password)) {
            return $user;
        }
    });

    Fortify::loginView(function () {
        return view('user::auth.login');
    });

You will get: "hmm" 3

driesvints commented 2 years ago

I'm not really sure what the issue is. If you can post much more clear description what is amis and how it blocks you with clear steps to reproduce then we can have another look. Thanks

oulfr commented 2 years ago

The issue is the Fortify::authenticateUsing method is triggered twice for each request. It should be called one time for each login request

driesvints commented 2 years ago

I'll try it on Monday

driesvints commented 2 years ago

Bit swamped with Laravel 9 stuff right now so I'm delaying this until I find some time.

driesvints commented 2 years ago

I indeed get the same thing but since there's no actual bug and nothing's broken I'm going to close this. If you can help us optimize this we'd appreciate a PR. Thanks

ricardogobbosouza commented 2 years ago

Hi @oulfr It actually happens...see https://github.com/laravel/fortify/pull/304

driesvints commented 2 years ago

@ricardogobbosouza I didn't know about that PR anymore. Looks like this is a wont fix, sorry.

ricardogobbosouza commented 2 years ago

The authentication method is called twice when the twoFactorAuthentication feature is enabled

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

oulfr commented 2 years ago

this bug is blocked if we want to add google recaptcha validation to the authentication form, because the google recaptcha api only authorizes the first request and it considers the second call as a bot and never the validation will pass. to resolve the issue we have implemented something like this:

          class FortifyServiceProvider extends ServiceProvider
          {
              protected $isFirstCall = false;

              public function boot()
              {

                  Fortify::authenticateUsing(function ($request) {
                      if (!$this->isFirstCall) {
                          $this->isFirstCall = true;
                          $request->validate([
                              'g-recaptcha-response' => ['required', new CaptchaRule],
                          ]);
                      }
                      $validated = Auth::validate($credentials = [
                          'email' => $request->email,
                          'password' => $request->password
                      ]);
                      $user = Auth::getProvider()->retrieveByCredentials($credentials);
                      return $validated && $user->isActive() ? $user : null;
                  });

              }

          }
dirkam commented 1 year ago

This is also (or might be) a problem, if you try to authenticate the user from a remote data source. For instance, in the case of LDAP login or similar remote repository, multiple calls are made. Since these require network connections, it can cause issues. And, as @oulfr mentioned, some third-parties might consider this as a bot activity and block the request.

@driesvints would it be possible to reopen this issue and fix this behavior?

driesvints commented 1 year ago

Right now we're not actively looking to fix this sorry. But you can always try another PR to change Taylor's mind.

driesvints commented 1 year ago

I'm sorry you feel this way @oulfr. In general issues we don't want to tackle ourselves or not actually consider an issue, like this one, will be closed to keep our issue queue clear.

kingroho commented 10 months ago

Wow, I've been dealing with this problem for hours now. Turns out, this function is indeed running twice. That's why Cloudflare's Turnstile is not working for me. Anyway, glad I'm not alone. WIll try to disable 2FA see if that solves the issue.

gtim04 commented 5 months ago

this bug is blocked if we want to add google recaptcha validation to the authentication form, because the google recaptcha api only authorizes the first request and it considers the second call as a bot and never the validation will pass. to resolve the issue we have implemented something like this:

          class FortifyServiceProvider extends ServiceProvider
          {
              protected $isFirstCall = false;

              public function boot()
              {

                  Fortify::authenticateUsing(function ($request) {
                      if (!$this->isFirstCall) {
                          $this->isFirstCall = true;
                          $request->validate([
                              'g-recaptcha-response' => ['required', new CaptchaRule],
                          ]);
                      }
                      $validated = Auth::validate($credentials = [
                          'email' => $request->email,
                          'password' => $request->password
                      ]);
                      $user = Auth::getProvider()->retrieveByCredentials($credentials);
                      return $validated && $user->isActive() ? $user : null;
                  });

              }

          }

this helped me so much. thank you

driesvints commented 2 months ago

I'm very sorry everyone but looks like this is an absolute no-fix for us. We just don't want to risk breaking something for people.

What you could always do is extend and bind the AttemptToAuthenticate action in a service provider to the container and then overwrite the handleUsingCustomCallback like in PR https://github.com/laravel/fortify/pull/532. Likewise for RedirectIfTwoFactorAuthenticatable.