laravel / fortify

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

2FA invalid code redirects back to login #343

Closed talelmishali closed 2 years ago

talelmishali commented 2 years ago

Description:

With the current implementation when a user provides an invalid 2FA code, the user is been redirected to the login route. Instead, a more suitable approach would be to redirect the user back to the 2FA route for re-entering the code (same as the behavior on the forge website and many others).

Before opening a PR I wanted to know if it is something the laravel team would consider addressing at the moment, I know it was mentioned on a previous PR (#325 ) and this is why I am asking (@taylorotwell mentioned he might not have the time to vet the code at moment).

Steps To Reproduce:

  1. Log in to an account with the 2FA feature activated
  2. Enter invalid 2FA code on verification
alexandercrice commented 2 years ago

@talelmishali

I ran into a similar problem. In case there is no fix coming for this, here is a workaround I used when I needed to customize redirect behavior for responses that don't look at the fortify.php configuration file. I haven't tried the 2FA feature yet, but it looks like the same approach would work. Use the extend method to bind a new response class that wraps/decorates (whatever you want to call it) the implementation response (Laravel\Fortify\Http\Responses\FailedTwoFactorLoginResponse ) from the Fortify package. All this does is it takes the response object returned by the FailedTwoFactorLoginResponse's toResponse method, and changes the URL before passing it along.

$this->app->extend(
    \Laravel\Fortify\Contracts\FailedTwoFactorLoginResponse::class,
    function ($response, $app) {
        return new class($response) implements \Laravel\Fortify\Contracts\FailedTwoFactorLoginResponse {
            private $baseResponse;

            public function __construct(\Laravel\Fortify\Contracts\FailedTwoFactorLoginResponse $response)
            {
                $this->baseResponse = $response;
            }

            public function toResponse($request)
            {
                $response = $this->baseResponse->toResponse($request);
                if ($response instanceof \Illuminate\Http\RedirectResponse) {
                    $response->setTargetUrl(
                        // ...  Put the value of the URL you want to redirect to here
                        // You could even add a value to the fortify.php config file and read it in here.
                    );
                }
                return $response;
            }
        };
    }
);

That's a lot of code for something that could be easily handled in one line if there was a "redirect" configuration setting for this route, but it worked for me.

Also be thankful it looks like there is a response contract binding to hook into here. If you want to change the redirect behavior after, say, updating the user profile information, it is more difficult because there is no redirect configuration setting, nor any response binding to hook into. It returns a back() redirect directly from the controller.

talelmishali commented 2 years ago

@alexandercrice Thank you for sharing the code.

In order to solve it, there's a need to change the create() method in TwoFactorAuthenticatedSessionController. I believe that in #233 there was a change introducing hasChallengedUser() method in TwoFactorLoginRequest TwoFactorLoginRequest.php#L83 which prevents an unchallenged user from accessing the two-factor.login route.

There are multiple ways to solve it, I will wait to hear from the laravel team for a preferred way in their opinion.

driesvints commented 2 years ago

I think either a much more simpler PR will be needed for this or like Taylor says, it's best to fork Fortify if you need this.