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

Let users confirm their TOTP setup before enforcing the provider #156

Merged
merged 2 commits into from
Apr 3, 2017

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Mar 13, 2017

Users might have trouble get their TOTP app to work. By checking
whether their basic setup is configured correctly, we lower the
risk of users being locked out unexpectedly.

TODO

  • The 2FA provider must not be used before the user confirmed that the TOTP setup works by providing a sample key
  • Better UI and UX, maybe by splitting the interaction into different steps the user has to complete.
  • Tests

Fixes #22

@ChristophWurst ChristophWurst force-pushed the feature/confirm-totp-before-enabling branch from b1f7f3a to e1a527c Compare March 14, 2017 09:22
Users might have trouble get their TOTP app to work. By checking
whether their basic setup is configured correctly, we lower the
risk of users being locked out unexpectedly.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Copy link
Member Author

@rigrig @RobThree @MariusBluem @trockenasche @Finkregh @Myatu @My1 @z3ntu @ngrundmann @r2evans @Xuanwo @Framartin here it is, the feature you gave your 👍 for :-) Please help test and review this PR, then I can release this in about two weeks (translators should get a chance to translate the new strings) 😉

@ChristophWurst
Copy link
Member Author

If you want to see it in action, watch the automated test run: https://saucelabs.com/beta/tests/e243977543794352bbde1cba61131803/watch#38 🍿

@rigrig
Copy link

rigrig commented Mar 14, 2017

Awesome, thanks for implementing this.

I don't have flash installed, so had to test it myself ;-)

What I did:

  1. Unchecked the TOTP box
  2. Checked the TOTP box -> I was shown a new key, QR code and asked to input a valid code
  3. Verified the key matched the QR code
  4. Entered the new key in my authenticator app
  5. Entered a wrong code -> I was told the code was wrong, TOTP was not enabled
  6. Entered the right code -> TOTP was enabled
  7. Logged out
  8. Tried to log in with a wrong code -> I was told the code was wrong
  9. Entered the right code -> I was logged in

As far as I can tell everything works fine.

@ChristophWurst
Copy link
Member Author

Hey cool, thanks for testing manually :-)

Let's wait, maybe we can get a second review/test, this is a relatively big change compared to the other PRs -> "given enough eyeballs, all bugs are shallow" 😉

@rigrig
Copy link

rigrig commented Mar 14, 2017

Actually: while the code seems to be working fine, screenshots/settings.png now is wrong.

I'm using some horrible dark theme myself, so the best I can offer is edited versions of the original screenshot with the new interface
settings

Or without the NC version header (because it now looks a bit cramped without the proper spacing)
settings_no_version

:

@ChristophWurst ChristophWurst merged commit 2118710 into master Apr 3, 2017
@ChristophWurst ChristophWurst deleted the feature/confirm-totp-before-enabling branch April 3, 2017 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants