Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2FA does not work #6

Closed
sten opened this issue May 10, 2022 · 6 comments · Fixed by #7
Closed

2FA does not work #6

sten opened this issue May 10, 2022 · 6 comments · Fixed by #7

Comments

@sten
Copy link

sten commented May 10, 2022

When I register a new user and then enable 2FA, I am stuck on the page with the "Enable two factor auth". I get a success message, but it does not switch to the QR-code page.

I debugged this a bit and it is related to the condition in the two-factor.blade.php file. If I change the default:

@if(auth()->user()->hasEnabledTwoFactorAuthentication())

to:

@if(! is_null(auth()->user()->two_factor_secret))

I can see the QR-code.

The issue is that in the hasEnabledTwoFactorAuthentication() function, the existence of the two_factor_confirmed_at field is checked, which is only set after you scan the QR-code.

Then there is an additional issue, when I change the condition and scan the QR-code, it does not ask me to enter the code that I receive after scanning the QR-code, so the two_factor_confirmed_at is never set. I do not see such a form in the blade file. The need for the form is described in the Laravel Fortify docs, see https://laravel.com/docs/9.x/fortify#confirming-two-factor-authentication

Thanks for the great package!

@wychoong
Copy link
Owner

Hi @sten thanks for using this

can you help me to verify it’s due to these changes in fortify that causing the issue?
laravel/fortify#357
laravel/fortify#358

it seems like is the confirmable flow and default config change
laravel/fortify@a6caadc

you can try if comment out ‘confirm’ key in fortify config solve the issue
// 'confirm' => true,

@sten
Copy link
Author

sten commented May 11, 2022

@wychoong Thanks for the swift reply!

I tried with 'confirm' = true. This works but when you login you need to enter both the code of the Authenticator app and a recovery code (as specified in the LoginTwoFactor class.

In Jetstream you only need to enter the code of the app. I assume because you have initially confirmed the app and the two_factor_confirmed_at field is set.

Are you planning to support the same behaviour as in Jetstream? Thank you very much!

@wychoong
Copy link
Owner

Is 'confirm' = false works for you?

I believe the issue you facing is due to the change of confirmable 2fa in fortify is enabled by default now, and this confirmable flow was not implemented at that time thus the error. Would be great if you can confirm things work fine if set to 'confirm' = false then we can be sure the issue is due to that.

Are you planning to support the same behaviour as in Jetstream? Thank you very much!
yea sure! But might not be so soon as I’m quite packed now and I don’t need that for now. Would appreciate if you can send a PR for it 😁

@sten
Copy link
Author

sten commented May 11, 2022

If I set 'confirm' = false, it kind of works. However I need to go twice through the enabling.

  • first, I click the enable button
  • then, I need to enter my password
  • I am redirected to the 2 factor auth page with the enable button again and the Fortify Action EnableTwoFactorAuthentication is not called.
  • then, I click again on the enable button and the EnableTwoFactorAuthentication action is called and I am send to the QR code page

@wychoong
Copy link
Owner

need to go twice through the enabling.

Will try if I can replicate this. Do send a PR if you manage to fix it

@wychoong
Copy link
Owner

wychoong commented Jun 4, 2022

need to go twice through the enabling.

hi, this should be solved with the next release

and 'confirm' = true' 2fa confirmation flow is implemented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants