Rework the way 2FA is enabled/disabled (fixes #3309) #3959
Conversation
crates/api_common/src/person.rs
Outdated
| #[cfg_attr(feature = "full", derive(TS))] | ||
| #[cfg_attr(feature = "full", ts(export))] | ||
| pub struct ToggleTotp { | ||
| pub totp_totp_token: String, |
There was a problem hiding this comment.
This needs a comment about how to disable totp also (maybe sending an empty or invalid string)
crates/api/src/lib.rs
Outdated
| .map_err(|_| LemmyErrorType::CouldntParseTotpSecret)?; | ||
|
|
||
| TOTP::new( | ||
| totp_rs::Algorithm::SHA1, |
There was a problem hiding this comment.
I see you got this one, good.
crates/api_common/src/person.rs
Outdated
| #[cfg_attr(feature = "full", derive(TS))] | ||
| #[cfg_attr(feature = "full", ts(export))] | ||
| pub struct ToggleTotp { | ||
| pub totp_totp_token: String, |
There was a problem hiding this comment.
This should probably also have a response, probably a simple success: boolean or enabled: boolean
Because right now the responses for enabling / disabling TOTP are the same.
|
I want to mention that this since this two-step process is not relevant for security and purely done for UX purposes, it's possible to do it completely client side without any server code or DB storage needed. In a different project I generate the secret client side, ask the user to input one one-time token, verifiy it client side, and the activation api just sends the client generated secret to the server. The advantage of that is simplicity since the server doesn't have to know about the incomplete non-activated secret at all. Just saying this as as note. |
|
@phiresky That makes sense. However this way seems easier for clients, as they wont have to pull in any 2fa library and figure out the correct parameters to validate the token. |
|
Ready to merge, api tests are passing locally. |
Currently 2FA is enabled in a single step which makes it easy to lock yourself out. This is fixed by using a two-step process, where the secret is generated first, and then 2FA is enabled by passing a valid 2FA token. Also fixes the problem where 2FA can be disabled without passing any 2FA token.
This PR will disable 2fa for all users who currently have it enabled. This allows users who are locked out to get into their account again. Once the js client is updated I will also add an api test to ensure that it works as expected.