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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃殌 Feature: TOTP (two-factor) Authentication #55

Merged
merged 14 commits into from Dec 21, 2022
Merged

馃殌 Feature: TOTP (two-factor) Authentication #55

merged 14 commits into from Dec 21, 2022

Conversation

stautonico
Copy link
Contributor

@stautonico stautonico commented Dec 18, 2022

This pull request adds a new feature: Two-factor authentication supporting TOTP, and closes #28

What Changed (Technical)

  1. Added new database model: LoginToken (used for TOTP procedure)
  2. Updated User model
    • Added loginTokens (link to all the user's login tokens)
    • Added totpEnabled (if the user has started enabling TOTP)
    • Added totpVerified (if the user has fully enabled TOTP and should be used for login)
    • Added totpSecret (16 base64 encoded random bytes used to generate the 6-digit TOTP code)
  3. Added new routes for enabling, disabling, verifying, and signing in with TOTP
  4. Changed how the login procedure works to support TOTP
  5. UNRELATED: Fixed a small bug that prevented the "change password" feature from working correctly (see comment on backend/src/auth/auth.service.ts)

How Enabling TOTP Works

  1. User logs into their account and visits the "My account" page
  2. The user enters their password into the "TOTP" tab in the "Security" card
  3. Clicking "Start" sends the user's entered password to /auth/totp/enable
  4. The backend returns: the URI for an authenticator app, the base64 encoded secret, and a base64 encoded QR code (scannable by authenticator apps).
  5. The user adds the secret to their authenticator app (either manually with the code or by scanning the QR code)
  6. The user enters the 6-digit TOTP code into the box and presses "Verify".
  7. The frontend sends another copy of the users password along with the 6-digit TOTP code to the backend (/auth/totp/verify). If the TOTP code matches, TOTP is enabled, otherwise, an error is returned to the user.

How Disabling TOTP Works

  1. User logs into their account and visits the "My account" page
  2. If TOTP is enabled, a "disable TOTP" prompt is shown in the "TOTP" section of the "Security" card
  3. The user enters their password and current 6-digit TOTP code into the fields and presses "Disable"
  4. The frontend sends the TOTP code and the user's password to /auth/totp/disable
  5. If the password and TOTP code matches, TOTP is disabled and a message is returned, otherwise, an error is returned

New Login Procedure

  1. The user enters their username/email and password into the fields like usual
  2. The frontend sends the username/email and password to /auth/signIn
  3. If the user has TOTP disabled, the backend returns the access token and refresh token like normal
  4. If the user had TOTP enabled, the backend returns a login token, which is used for step 2 of the login procedure
  5. The frontend prompts the user to enter their TOTP code
  6. Once the user enters their code, the frontend sends the username/email, password, TOTP code, and login token to /auth/signIn/totp
  7. The backend validates all the info given, and if it matches, the backend returns an access token and a refresh token
  8. If the code is incorrect, the backend will return an error

Technical Notes

Why do all the TOTP related routes/functions require the user's password
In order to securely store the user's TOTP secret, we can't hash it because we need to be able to get it back when it is time to check the user's password. Instead of using a global encryption key (which can be stolen in the case of a server breach), we encrypt the user's secret using the user's password as it is never stored anywhere (in plain text). The only downside to this is that the user's must enter their password before performing any TOTP related activities (but we can just say its a "security feature" 馃槄)

Refresh page after TOTP is enabled/disabled
Once the user finishes enabling/disabling their TOTP authenticator, the page refreshes. I would like the form to change from the enable form to the disable form, but I couldn't get it to change without the refresh. I tried to get the user service to update the user object (which should store the new status of TOTP), but I couldn't get it to work.

@stautonico
Copy link
Contributor Author

Note: The tests were failing before. I checked out the commit before I touched anything and the tests were still failing

@stonith404
Copy link
Owner

Thanks again for your awesome work!

  • I made some changes regarding the error handling. I wrote a function before toast.axiosError(), that automatically takes the error response and shows a toast when something went wrong. As you can see in my changes this really simplifies the code. (d5d5085)
  • I also fixed the page reload with only updating the user context (192eae1)
  • Regarding entering the password the whole time for totp actions, I really like that tbh., so it's definitely a feature ;)
  • You've also added a U2F tab. I would recommend to delete this because I don't really see a reason to implement this feature. In my opinion TOTP is more enough because with a hijacked Pingvin Share you can't really do much. Do you agree?
  • Also I'm not sure if we should split the 3 TOTP attributes of an user in a separate table? What do you think?

@stautonico
Copy link
Contributor Author

I agree, I think TOTP is probably sufficient. Maybe in the future, if U2F is a requested feature, we can add it, but TOTP should be enough for most people. Also, I personally think the TOTP attributes should be part of the user table. It is ALWAYS a 1:1 relationship and I don't think there's enough data to be its own table. Maybe if we supported U2F/other 2FA methods, maybe having a dedicated 2FA table would keep the user model clean, but I don't think its necessary in its current form.

Copy link
Owner

@stonith404 stonith404 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks fine for me!

@stonith404 stonith404 merged commit 16480f6 into stonith404:main Dec 21, 2022
@Ju17091
Copy link

Ju17091 commented Dec 22, 2022

Thank you very much 馃槏馃憤馃徎

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

Successfully merging this pull request may close these issues.

2 factor authentication
3 participants