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

Feature enable otp #108

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

benjaminwols
Copy link
Contributor

This PR adds an enable option, which lets the user enable and disable tfa.

If the need_two_factor_authentication? method returns true and otp is not enabled, the user is redirected to the :new view, where an QR code is shown if using an app and there is a link to ask for a direct otp code.

If the user confirms his code, otp is enabled and the user is asked next time for a otp code.

You can add a link to 'edit_user_two_factor_authentication_path' to let the user disable tfa.

I would love to get your feedback.

@chrise86
Copy link

Is it wise to be submitting the totp_secret in the form? I faced a similar situation recently and opted to:

  • generate the code and update the user first
  • display the QR code
  • when they confirm by entering the authenticator code, mark them as otp_enabled.

If they don't confirm there and then, I regenerate the code next time and ask them to confirm using the new code.

@benjaminwols
Copy link
Contributor Author

Good point.
I will take a look at this later this week.

@Xosmond
Copy link

Xosmond commented Oct 2, 2017

The problem with generating the code and update the user first, is that when the user does not confirm by entering the authenticator code we will update the user again and again.

Sending the topt_secret in the form is fine because we are showing it on the qrcode (is the same thing).

PD: Maybe if somebody sees or changes the topt_secret via javascript would be a risk. I do not know.

PD2: I did a similar way of this PR by hand on my project (I think everybody will need to write this).

@benjaminwols
Copy link
Contributor Author

@Houdini Do you have any thoughts on this matter?
I think it provides a solution to Issue #32.

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.

None yet

3 participants