Two factor authentication#7751
Conversation
There was a problem hiding this comment.
First of all, thank you very much for working on this. The flow looks good in my opinion, no global blocking points, so you should be able to continue that way and polish your work.
Two remarks:
- I don't see where the hashing algorithm is set. Can you assure it's a modern one, not SHA1 which was the default some years ago?
- I'm a bit afraid of the support load this new feature can bring. Framasphere moderators are already spending a lot of time with users who lost access to their account (that's why we have feature requests like #7442). The question is, is 2FA added to protect users from a stolen password (if so, then when resetting the password by e-mail, the 2FA code should not be asked) or is it also aimed to protect them if their email gets compromised, like you did here. (In that case, you're in trouble anyway but eh, that's in part why 2FA has been built), At least the podmin should be able to access recovery code easily I guess, as we will probably be asked them a lot...
It's not hashing, it's encryption, but see #7751 (comment) for more information about that topic.
Yes, probably some users will try to ask you to reset their 2fa, but you shouldn't reset it, otherwise 2fa is completely useless. It is optional (so everybody activating it should be aware of this) and there are backup keys (and they are there for a reason). If people lose their 2fa secret and their backup keys, they can create a new account. There is no (official) possibility to reset 2fa if you lost it (there is no "forgot 2fa" similar to the "forgot password", obviously you as a podmin can reset your 2fa if you want, but you shouldn't do that for other users you don't know personally). So you can just create a default answer for everybody asking to reset their 2fa and tell them that it isn't possible and they should create an new account and next time backup their backup keys. (personally I already see setting another mail-address as critical, when you can't verify if it's the real owner of the account, and the mail-address is the only way to do this. But that's another topic.) |
|
Thanks for the comments so far <3 I'll look into updating the PR this weekend! |
That really doens't matter. If you have forgotten your password, you can reset it, and you can get a new one via email. It doesn't matter if we ask for a 2fa token or not (and in fact, the industry standard is not to ask, because it's kinda pointless). In any case, resetting the password does not reset the 2fa token, and it should not. So even if your eMail address is somehow borked and someone manages to reset the password, there is still no way to get into the account, which is the point of 2fa. As @SuperTux88 already outlined, there is no way to reset/disable 2fa without a valid TOTP, and there shouldn't be one. Now, there is a risk of someone losing their phone (which generates the TOTP) without having a backup, and that's bad. Like most large applications supporting 2fa, we should generate some backup tokens for users to store in a secret place (like a password manager) to be used as a valid token if they lost their generator. The gem used here has built-in support for this, and we should expose that. If people enable 2fa, but fail to backup their keys and the recovery key, they'll have a hard (read: impossible) time to get their account back. This is by design. And this is just the way it works here on GitHub as well: If you cannot use the provided validation schemes, you are goofed, the support will not reset your account. Neither will Google, fwiw. Besides me rambling around... @lislis, thanks for working on this! |
|
Update: found some time to work on this again. Here the current state of the UI:
|
|
It looks very nice, awesome work! Is there a screen where the user can enter the recovery codes too? |
|
Recovery codes would go in the same form. |
jhass
left a comment
jhass
left a comment
There was a problem hiding this comment.
Sorry for the delayed review. Looking very good, I think the below are the last things we need here :)
|
@jhass there is still a problem in Travis where it can't find the |
|
Ah, the cucumber test for resetting passwords is outdated. |
Flaburgan
left a comment
Flaburgan
left a comment
There was a problem hiding this comment.
Two minor remarks, and maybe a bug with the disabling and recovery code? I don't know what's going on there as this is covered by the tests... Otherwise it works very well! This should be integrated soon ;)
|
|
||
| .col-md-6 | ||
| %p= t("two_factor_auth.confirm.manual_explanation") | ||
| %pre.well= current_user.otp_secret.scan(/.{4}/).join(" ") |
There was a problem hiding this comment.
I successfully activated OTP with the code but first did the mistake to enter the space displayed inside the secret here. Can we indicate somewhere that those are only for readability and not actually part of the secret?
|
🎉 🍪 🎊 |
|
Great that this is finally finished 🎉 🍪 @jhass you didn't add a changelog entry when you merged this, but I'm sure we all agree that this deserves one ;) But this leads me to the question: do we want to uplift that to the |
|
🤦♂ I knew using Github's squash merge was going to be too easy... I'm fine with uplifting this to |
|
Thank you for finishing this! |
10 participants
Hey there!
This is my attempt to implement 2fa, ticket #4299
I know, there are no tests (:see_no_evil:), all strings are hardcoded, and the UI is far from 'done' but I'm at a point where I need some feedback on the general direction.
Some screenshots:
New link on the settings page, 2fa deactivated
Scan the code and confirm
Confirmation and backup codes
Settings page, 2fa activated
After sign-in, prompt for token
And that is it so far.
Missing:
and anything else that's missing