After a unsuccessful verification attempt a secondary (correct) attempt always fails due to a TokenAlreadyProcessedException about laravel-two-factor-authentication HOT 6 CLOSED

Yes, seems you are right. I could swear that MessageBird allowed you to verify the same token multiple times in the past, so they must have changed that. Or maybe I am just imagining that this was the case. Not sure... It is a while ago I did anything with the "messagebird" driver.

Anyway, I agree that there is no point redirecting the user to the 2fa input screen in case of a TokenInvalidException exception, since a second attempt will always result in a TokenAlreadyProcessedException exception.

Instead of showing the token input field after an unsuccessful attempt I would suggest to show a link to a controller that generates a new token, show the error message, and hide the input field.

Not sure if I am a fan of this to be honest. I can see its use, but it would also require an extra "regenerate token" blade view and route to handle the regeneration of the token. I think it would be simpler to just handle an TokenInvalidException the same way as a TokenExpiredException or TokenAlreadyProcessedException. So to just simply call TwoFactorAuthenticatesUsers#sendKillTwoFactorAuthResponse with a custom error message so as to start the entire authentication process from scratch.

Alternatively, it might be an idea to introduce a protected TwoFactorAuthenticatesUsers#twoFactorFailed method that by default just calls TwoFactorAuthenticatesUsers#sendKillTwoFactorAuthResponse (with a custom error message), which you then could override in your TwoFactorAuthController if you want to implement your own logic in case a user enters an invalid token. You could then implement the token regeneration logic yourself.

Right now, the verifyToken method in the MichaelDzjap\TwoFactorAuth\Http\Controllers\TwoFactorAuthenticatesUsers trait seems to bypass the incrementTwoFactorAuthAttempts method when the token is expired or already processed, so I could endless retry guessing the code.

I would suggest to move that call to incrementTwoFactorAuthAttempts to the very beginning of the method so that every attempt, no matter if it is valid or not, contains the right characters or not, attributes to the amount of attempts. Better too strict than too soft imho.

There's a reason I didn't do this, which admittedly, I can't recall now anymore. But I don't think it is correct to state that a user can endlessly retry guessing the code. You are logged out in case of a TokenExpiredException or TokenAlreadyProcessedException and so a user always would have to login again (and thus will be dealing with Laravel's native login throttling mechanism) and when logged in successfully, always starts the 2fa process from scratch. But maybe I am misunderstanding you.

from laravel-two-factor-authentication.

Thanks for your extensive reply!

Not sure if I am a fan of this to be honest. I can see its use, but it would also require an extra "regenerate token" blade view and route to handle the regeneration of the token. I think it would be simpler to just handle an TokenInvalidException the same way as a TokenExpiredException or TokenAlreadyProcessedException. So to just simply call TwoFactorAuthenticatesUsers#sendKillTwoFactorAuthResponse with a custom error message so as to start the entire authentication process from scratch.

It would indeed be simpler, but generating a new code would be a bit more user friendly. The suggestion to introduce a twoFactorFailed method seems like a good idea. It would be easier to choose your own way of handling a failed two factor attempt.

You're right about the comment on incrementTwoFactorAuthAttempts I think.
In the case you would introduce a twoFactorFailed method, a call the increment method in or before calling this would be necessary.

from laravel-two-factor-authentication.

I've been thinking a bit more about this and I actually think adding the ability to override a new protected TwoFactorAuthenticatesUsers#twoFactorFailed that by default simply redirects back to the /login route is not the best solution after all. We have been reasoning from the point of view of the "messagebird" driver, where (as we have concluded) the current behaviour in case of an invalid token is not really useful. However, it might still make sense for people who have implemented their own custom 2fa provider. Changing things so that a user will be redirected to the login page in case of an invalid token by default will introduce a breaking change (i.e. will result in different behaviour as before). Alternatively, so will a redirect to a "regenerate token" view.

However, I think there is a simpler, more elegant and probably even more flexible solution: a way to customise the route a user is redirected to in case of an invalid token similar to how Illuminate\Foundation\Auth\RedirectsUsers from the Laravel ui package works. So in the default case it would then just increment the two factor auth attempts count and redirect back to the previous route with an error message (i.e. the same thing as is currently happening), but you are free to override this behaviour by specifying a custom $redirectTo property or redirectTo method on your App\Http\Controllers\Auth\TwoFactorAuthController controller. So then it is still possible to redirect to a custom "regenerate token" view or simply redirect to the /login route again, but no breaking change is introduced.

Will think a bit more about this (maybe I am overseeing something), but this solution sounds by far the most logical to me at this point.

In the case you would introduce a twoFactorFailed method, a call the increment method in or before calling this would be necessary.

I think this would only be necessary when you redirect to a custom "regenerate token" route (or in the default case, as is happening already now). Don't think it would be necessary if you simply redirect to the login page again (and hence, do not login the user automatically of course).

from laravel-two-factor-authentication.

I agree that I could break custom drivers. Your proposal of setting a $redirectTo route sounds like a great solution @michaeldzjap

I think this would only be necessary when you redirect to a custom "regenerate token" route (or in the default case, as is happening already now). Don't think it would be necessary if you simply redirect to the login page again (and hence, do not login the user automatically of course).

The current code is correct indeed, since the early returns redirect you back to /login
In a new situation this should be done slightly different I guess.

from laravel-two-factor-authentication.

@fgd007 I've added a redirect option in v2.8.1 or v3.1.0 (for Laravel 8).

Hopefully that will add enough flexibility to solve your problem.

from laravel-two-factor-authentication.

Great @michaeldzjap! Thanks

from laravel-two-factor-authentication.