Add option to disable two factor auth in admin accounts panel. #2584
Conversation
|
should the Once a user gets to log in without their 2FA code, I think we should completely invalidate existing 2FA data for that user, since it's no longer a valid form of identification. |
|
we could, but I don't think it matters too much? how could these codes be used if 2fa is disabled? |
|
yeah I suppose that's right. they'll get regenerated again when the user re-enables 2FA on their account. and aren't really useful for authentication at all if the account already has 2FA disabled. the auditor inside of me still thinks it should be purged, for the sake of a clean DB. |
|
To be honest, it totally slipped my mind to purge those. Probably should clear the backup codes, just in case. I'll go ahead and update the pr after lunch 😄 |
|
Updated, now runs |
| before_action :set_account | ||
|
|
||
| def destroy | ||
| @account.user.otp_required_for_login = false |
There was a problem hiding this comment.
Can you do at least some and maybe all of:
- Roll all of this up into a method like
User#disable_two_factor!or something (which would set required to false, clear the backups, and attempt save!) - Add spec coverage for this controller action which shows that it does what it says
- Add model coverage for that method on User, if you create one like that
| private | ||
|
|
||
| def set_account | ||
| @account = Account.find(params[:account_id]) |
There was a problem hiding this comment.
This could probably hit an admin/users/:id/two_factor_authentication#destroy route, since it's only doing things to the user record and not the account record.
|
I think that's correct now, @mjankowski can you take another look for us and let me know if I should update anything? |
* Moves destroy actions behind User#disable_two_factor! * Adds spec coverage for Admin:TwoFactorAuthenticationsController and User#disable_two_factor!
…upstream Merge upstream changes up to 7a1f087
Closes #2578, adds an option to disable 2FA in the accounts panel. UI change displayed below.
This will require updated translations, if anyone can help translate that would be awesome.