laravel / jetstream

Tailwind scaffolding for the Laravel framework.
https://jetstream.laravel.com
MIT License
3.98k stars 821 forks source link

Enter 2FA input token before save model #74

Closed schmeits closed 2 years ago

schmeits commented 4 years ago

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.

zaknesler commented 4 years ago

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.

m1guelpf commented 4 years ago

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?

taylorotwell commented 4 years ago

Probably won't make it in before release today but we will include in a major release soon.

codytooker commented 4 years ago

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.

m1guelpf commented 4 years ago

@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.

AdvancedStyle commented 4 years ago

@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)

MrReeds commented 4 years ago

I already counter 2 problems with the current, "unconfirmed enable". First, and more common would be to require it on people who are not tech savvy. If they are with a mentality "next-next-next" and don't actually know what they are clicking next to, they will be locked out; If they are forced to use it, they don't bother to read more than "enable->enter password->enable" and then not to do anything else since it is shown as enabled. Second would be less common but still plausible; when enabling it and someone happens to distract you after you enter the confirmation password and you forgot it later. With IT personnel i have seen that exact situation happen where they basically forget to complete some task, because they where interrupted (sometimes even multiple times for the same task). I would see that in addition to what is already there, require OTP on enable, before inserting it to database. And why is in addition to current password confirm is to prevent sessions that have been logged in and left unattended be locked out because someone third party may have just "trolled" them (offices, public computers)

dillingham commented 3 years ago

Would like to just notify the user a code and let them enter it. Defaults to email but would be nice to opt into sms or phone call like banks.

tpetry commented 3 years ago

How about a two_factor_confirmed column? The 2fa secret and recovery tokens are generated but not active for logins and only if a correct 2fa totp value is provided the boolean flag (or date?) is set to enable 2fa for the login. If there is a problem on the user's end (code not scanned) the 2fa is just not active.

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.

sathia-musso commented 3 years ago

is there an ETA for this? looks really broken from the point of view of the UX

driesvints commented 3 years ago

No sorry. We're still planning on this but atm don't have an ETA.

dmandrade commented 3 years ago

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: Fortify Patch Jetstream Patch

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:

/resources/views/profile/two-factor-authentication-form.blade.php

Inertia - update file:

/resources/js/Pages/Profile/TwoFactorAuthenticationForm.vue

OBS: I didn't get to test the solution with jetstream + inertia, but with livewire works well

sathia-musso commented 3 years ago

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

MrReeds commented 3 years ago

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: Fortify Patch Jetstream Patch

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:

/resources/views/profile/two-factor-authentication-form.blade.php

Inertia - update file:

/resources/js/Pages/Profile/TwoFactorAuthenticationForm.vue

OBS: I didn't get to test the solution with jetstream + inertia, but with livewire works well

Tried it, got it working. awesome! One error i found was with when clicking "show recovery codes" and then disable i will get a "Illuminate\Contracts\Encryption\DecryptException" invalid payload. Maybe just me

dmandrade commented 3 years ago

@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.

sicaboy commented 2 years ago

any updates on this from official?

ThisGitHubUsernameWasAvailable commented 2 years ago

A very helpful suggestion. Hopefully this will be implemented soon.

ps-sean commented 2 years ago

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.

ghost commented 2 years ago

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.

ghost commented 2 years ago

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.

driesvints commented 2 years ago

@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.

driesvints commented 2 years ago

I've begun work on this: https://github.com/laravel/jetstream/pull/992

driesvints commented 2 years ago

This will be released next Tuesday!