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

Enter 2FA input token before save model #74

Closed
schmeits opened this issue Sep 6, 2020 · 23 comments · Fixed by #992
Closed

Enter 2FA input token before save model #74

schmeits opened this issue Sep 6, 2020 · 23 comments · Fixed by #992
Assignees

Comments

@schmeits
Copy link

schmeits commented Sep 6, 2020

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.

@schmeits schmeits changed the title 2FA input your token before save Enter 2FA input token before save model Sep 6, 2020
@zaknesler
Copy link
Contributor

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
Copy link
Contributor

m1guelpf commented Sep 6, 2020

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
Copy link
Member

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

@codytooker
Copy link

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
Copy link
Contributor

m1guelpf commented Sep 12, 2020

@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
Copy link

@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
Copy link

MrReeds commented Nov 13, 2020

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
Copy link
Contributor

dillingham commented Jan 9, 2021

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
Copy link

tpetry commented Jan 13, 2021

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.

@sathio
Copy link

sathio commented Mar 4, 2021

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

@driesvints
Copy link
Member

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

@dmandrade
Copy link

dmandrade commented Jun 2, 2021

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

@sathio
Copy link

sathio commented Jun 3, 2021

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
Copy link

MrReeds commented Jun 3, 2021

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
Copy link

dmandrade commented Jun 3, 2021

@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
Copy link

sicaboy commented Dec 12, 2021

any updates on this from official?

@ThisGitHubUsernameWasAvailable

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

@ps-sean
Copy link
Contributor

ps-sean commented Jan 24, 2022

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
Copy link

ghost commented Feb 23, 2022

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
Copy link

ghost commented Feb 23, 2022

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
Copy link
Member

driesvints commented Mar 1, 2022

@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
Copy link
Member

I've begun work on this: #992

@driesvints driesvints self-assigned this Mar 14, 2022
@driesvints
Copy link
Member

This will be released next Tuesday!

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

Successfully merging a pull request may close this issue.