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

Two-factor authentication support #388

Merged
merged 23 commits into from
Oct 26, 2022
Merged

Two-factor authentication support #388

merged 23 commits into from
Oct 26, 2022

Conversation

doobry-systemli
Copy link
Contributor

@doobry-systemli doobry-systemli commented Jul 16, 2022

Adds support for TOTP-based two-factor authentication.

Overview

  • Users can enable and disable two-factor authentication in their account settings. Both requires the user to enter their password. When two-factor authentication is enabled, the user has to verify that it's working by entering a TOTP token.
  • Admins can remove the configured TOTP secret in order to restore accounts.
  • The TOTP secret doesn't get cleared during the recovery process for security reasons. Otherwise, a single factor (recovery secret) would be enough to reset two factors (password + TOTP secret).

Fixes: #115

Screenshots

Enabling two-factor authentication in account settings

  1. Settings overview
    Screenshot 2022-07-17 at 17-51-47 example org - Account Settings

  2. 2FA settings, 2FA not enabled
    Screenshot 2022-07-16 at 18-34-06 example org - Two-factor authentication

  3. 2FA settings, enabling 2FA
    Screenshot 2022-07-16 at 18-34-26 example org - Two-factor authentication

  4. 2FA settings, enabling 2FA, invalid token
    Screenshot 2022-07-16 at 18-34-47 example org - Two-factor authentication

  5. 2FA settings, 2FA enabled
    Screenshot 2022-07-16 at 18-35-18 example org - Two-factor authentication

Two-factor form during login

Screenshot 2022-07-16 at 18-35-45 example org - Two-factor authentication

@doobry-systemli doobry-systemli added the enhancement New feature or request label Jul 16, 2022
@doobry-systemli doobry-systemli self-assigned this Jul 16, 2022
@codecov
Copy link

codecov bot commented Jul 16, 2022

Codecov Report

Merging #388 (5bb1177) into main (b761d1e) will decrease coverage by 1.73%.
The diff coverage is 15.87%.

❗ Current head 5bb1177 differs from pull request most recent head ded11c4. Consider uploading reports for the commit ded11c4 to get more accurate results

@@             Coverage Diff              @@
##               main     #388      +/-   ##
============================================
- Coverage     36.92%   35.19%   -1.74%     
- Complexity     1059     1120      +61     
============================================
  Files           175      183       +8     
  Lines          3967     4325     +358     
============================================
+ Hits           1465     1522      +57     
- Misses         2502     2803     +301     
Impacted Files Coverage Δ
src/Admin/UserAdmin.php 0.00% <0.00%> (ø)
src/Controller/StartController.php 0.00% <0.00%> (ø)
src/Controller/TwofactorController.php 0.00% <0.00%> (ø)
src/Entity/User.php 90.90% <ø> (ø)
src/Form/TwofactorBackupAckType.php 0.00% <0.00%> (ø)
src/Form/TwofactorType.php 0.00% <0.00%> (ø)
src/Handler/UserAuthenticationHandler.php 100.00% <ø> (ø)
src/Repository/UserRepository.php 0.00% <0.00%> (ø)
src/Validator/Constraints/TotpSecret.php 0.00% <0.00%> (ø)
src/Traits/TwofactorBackupCodeTrait.php 40.90% <40.90%> (ø)
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@doobry-systemli doobry-systemli marked this pull request as draft July 16, 2022 17:45
@doobry-systemli doobry-systemli changed the title Draft: Two-factor authentication support Two-factor authentication support Jul 16, 2022
@doobry-systemli
Copy link
Contributor Author

doobry-systemli commented Jul 16, 2022

Open questions where I would appreciate input:

Do we want to reset 2FA during recovery process?

  • Pro: User can restore their account when 2FA device got lost
  • Con: From security perspective, recovery token is one-factor authentication. Users are asked to store it in a secure place, but still.

If we have support for 2FA backup codes, we could argue that users have to use them if they loose their 2FA device.

Do we want admins to be able to reset 2FA for accounts?

  • Pro: Accounts where 2FA device got lost can be restored by admins.
  • Con: Admins can downgrade the security of accounts.

Again, with 2FA backup codes we could say that users have an option to restore 2FA themselves.

Do we want to prod users to enable 2FA during login?

The only problem I see is that users might be overwhelmed by all the security-relevant information they need to store somewhere:

  1. They have to choose a secret password and remember or store it.
  2. They have to store their recovery code somewhere safe.
  3. They have to configure 2FA in thir 2FA app.
  4. They have to store 2FA backup codes somewhere safe.

That's not exactly a nice onboarding experience, is it? 🤔

@doobry-systemli doobry-systemli force-pushed the enh/2fa_totp branch 4 times, most recently from fe77c84 to c033826 Compare July 17, 2022 16:54
@t2d
Copy link
Contributor

t2d commented Jul 20, 2022

Thanks for your awesome work. I haven't looked at the code yet, but I would like to provide some input for your questions:

Do we want to reset 2FA during recovery process?

Yes. Recovery process is like a fresh start for your account. There'll be no more mails inside. It just makes sense to also clear the 2FA token.

Do we want admins to be able to reset 2FA for accounts?

That's how it is commonly done and i think, it's a sensible thing to do. Especially for domain admins it'll be helpful. They can assist, but not give them selves access.

Do we want to prod users to enable 2FA during login?

From the example I guess you're talking about signup, not login. Then my anwser: Not yet. I agree, that the onboarding experience would be degraded. Also, 2FA is not that common to regular end-users.

In general, I'm a bit worried about confusing users with additional backup codes. But I also don't think it's possible to get rid of 2fa backup codes as you don't want to reset your whole mail account when you lose your phone. What do you think about the following idea?

When a user inserts their recovery code, give them them the choice to just delete their 2FA token or reset the whole password (and clear inbox).

@doobry-systemli doobry-systemli force-pushed the enh/2fa_totp branch 2 times, most recently from ba74e57 to c6985b2 Compare September 11, 2022 11:02
@doobry-systemli doobry-systemli marked this pull request as ready for review September 11, 2022 16:31
@doobry-systemli
Copy link
Contributor Author

Support for backup codes is now implemented (and covered by behat tests).

Six backup codes are generated automatically during twofactor configuration. The user is asked to acknowledge that they stored the backup codes at a secure place. Only afterwards twofactor authentication is enabled.

@doobry-systemli
Copy link
Contributor Author

Thanks for your awesome work. I haven't looked at the code yet, but I would like to provide some input for your questions:

Do we want to reset 2FA during recovery process?

Yes. Recovery process is like a fresh start for your account. There'll be no more mails inside. It just makes sense to also clear the 2FA token.

That's wrong. Recovery process restores your account along with all the content.

What do you think about the following idea?

When a user inserts their recovery code, give them them the choice to just delete their 2FA token or reset the whole password (and clear inbox).

We decided against resetting 2FA configuration with the recovery process for now. Otherwise, we would compromise the security of two-factor authentication. Being able to reset both your password and your two-factor secret using the recovery token (regardless whether it's two options in the process or one) means that one factor (recovery token) is enough to reset both factors of your account. That's not a good idea IMHO.

Six backup codes are generated automatically during twofactor
configuration. The user is asked to acknowledge that they stored the
backup codes at a secure place. Only afterwards twofactor authentication
is enabled.
config/packages/scheb_2fa.yaml Outdated Show resolved Hide resolved
@doobry-systemli doobry-systemli merged commit 5b598ae into main Oct 26, 2022
@doobry-systemli doobry-systemli deleted the enh/2fa_totp branch October 26, 2022 21:00
@t2d t2d mentioned this pull request Oct 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Two factor authentication
3 participants