-
Notifications
You must be signed in to change notification settings - Fork 825
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
Enter 2FA input token before save model #74
Comments
I think it would also be beneficial to display a token that the user can copy into their generator, because sometimes they aren't able to scan the QR code. |
This is a common practice and I was surprised to see it wasn't provided by default. Is there any interest for a PR here? |
Probably won't make it in before release today but we will include in a major release soon. |
Also, it might be useful to have to type in a valid code to remove 2FA as well. At the very least a confirm modal. |
@codytooker I think entering the password is more appropriate when disabling, since you're authorizing the action. When enabling however, you want to verify you actually set the thing up, so entering a code for verification would make more sense. |
@m1guelpf Having them enter their 2FA code to disable is more secure...seeing as it prevents someone who has hacked your computer disabling your 2FA. For example if you are logged in (having logged in with your 2FA) and you have your password saved in password manager (as most people do) then hacker can use your existing session with your saved password to disable 2FA on your account. They now can login whenever they like (and even enable a new 2FA code to lock you out) |
I already counter 2 problems with the current, "unconfirmed enable". |
Would like to just |
How about a As jetstream is based on fortify this functionality needs first implemented in fortify (laravel/fortify#47) with a breaking change and therefore new major release. |
is there an ETA for this? looks really broken from the point of view of the UX |
No sorry. We're still planning on this but atm don't have an ETA. |
I created two patchs to fix this issue directly in the fortify/jetstream package, with this solution the 2FA will only be required after the complete configuration: To apply it automatically first install the vaimo/composer-patches package. Put two patch files in PROJECT_ROOT/patches folder. And in composer.json add: "extra": {
...
"patcher": {
"search": "patches"
}
} After adding the patch files and update composer.json run composer patch:apply Then publish the new fortify config and migration php artisan vendor:publish --tag=fortify-config
php artisan vendor:publish --tag=fortify-migrations
php artisan migrate After installing both patches update your resources according to the stack used using as a reference the files present in vendor/laravel/jetstream/stubs Livewire - update file:
Inertia - update file:
OBS: I didn't get to test the solution with jetstream + inertia, but with livewire works well |
Can this be merged? It's an issue with at least one hundred people waiting for it to be fixed, I think it makes sense to look into it right now. cheers |
Tried it, got it working. awesome! |
@MrReeds I fixed this error, thanks. to update your version, first run composer patch:undo then update the jetstream-confirm-2fa.patch file with the new version in gist. And then just apply the patch again composer patch:apply. -- I will do a PR on fortify as soon as possible but apparently this would only be released in a major version if approved. |
any updates on this from official? |
A very helpful suggestion. Hopefully this will be implemented soon. |
Any updates on this? Had a user today try to enable 2FA but their authenticator was built into the browser so they cant scan the QR code, and as its enabled without confirmation, they had locked themselves out of their account. |
Looks like the forty side of this has been fixed/added in 1.11.0. If I find some time I'll have ago at updating the Jetstream patch above to reflect the 1.11.0 implementation. |
Looks like just updating just the Jetstream patch file will not be enough as the new column was added to the 2014_10_12_200000_add_two_factor_columns_to_users_table.php migration file and not as a new migration. Seem odd if that's the practise when adding new features in laravel (I'm still new to laravel) or if its a mistake/bug as that makes upgrading more cumbersome. |
@ChrisRiddell the migration files are meant for when you first install the package. Upgrading apps to newer versions always require to add your own migrations. This is to not conflict with apps who are overriding migrations which can cause breaking changes. |
I've begun work on this: #992 |
This will be released next Tuesday! |
If you enable 2FA and "forget" to scan the QR code, the user can't login anymore :)
Normal after you scan your 2FA code you have to enter it again on the page before the two_factor_secret is saved into the user model, this way you kinda ensure you registered your login in your 2FA application.
The text was updated successfully, but these errors were encountered: