laravel / fortify

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

Two Factor Authentication returns error on subsequent code entries if first one was invalid #351

Closed devstack-be closed 2 years ago

devstack-be commented 2 years ago

Description:

I am building an API by using Laravel 8 with Fortify and with vueJS on frontend. I enabled and integrated two-factor authentication and it works well, but just stumbled upon an issue which I do not know if it's by design or a bug.

So, the problem is when user is logging in and he enters the 2fa code wrongly for the first time. In this case, on all subsequent entries, even if the code is correct and valid, it will reject it. After some digging around, I noticed that the login.id stored in session in Laravel\Fortify\Actions\RedirectIfTwoFactorAuthenticatable::twoFactorChallengeResponse is cleared out if the code entered first time is invalid. On all subsequent requests, the login.id in Laravel\Fortify\Http\Requests\TwoFactorLoginRequest::challengedUser() is returning null.

So, my question is: is this intentional and by design? Or did I discover a bug?

My reasoning is that this is a bug because user already provided username and password, now he only needs to enter valid 2fa code to login completely, so not sure why forcing user to enter username and password again if they typed 2fa code wrongly for the first time.

The issue happens in Laravel\Fortify\Http\Requests\TwoFactorLoginRequest::challengedUser() where the code executes $model::find($this->session()->pull('login.id')). pull() is designed to get an item from session storage and then forget it immediately after. Once I changed that into get(), it started working as it should.

driesvints commented 2 years ago

Hey @devstack-be. So I believe this to be intentionally but agree it's a bit odd as that's not the behavior of most systems.

I've attempted a PR here: https://github.com/laravel/fortify/pull/353. I'm going to close this one as it's more a request to change the behavior here rather than an actual bug. Let's see how the PR goes.

Thanks

devstack-be commented 2 years ago

@driesvints Thanks for your quick answer.