Confirm OTP Token #72

strouptl · 2024-05-17T14:59:53Z

Hi there,

I think it is standard now to confirm that the user has properly configured the OTP token in their devise before enabling it. Otherwise, if they navigate away from the page without actually scanning the QR code or saving the recovery tokens, then they will be locked out of their account.

I have scaffolded a solution for this that simply asks you to enter a OTP successfully before actually enabling Two Factor Authentication. The key changes are as follows:

Add an "Edit" action with a two step form for enabling MFA
Require the user to enter one OTP as confirmation in the second step before enabling MFA
Distinguish the "populate_otp!" and "enable_otp!" methods in the otp_authenticable module

Please let me know if you like this solution. Also, please let me know if you have any information on the current status of the test suite: currently hitting an error and a number of deprecation notices with Rails 7.1.3/Ruby 3.2.3.

NOTE: this PR includes #73 .

Thanks!

strouptl · 2024-05-17T15:11:51Z

Here is a screenshot from AWS for reference:

strouptl · 2024-05-18T06:10:28Z

OK, I have resolved the main errors (#73), and updated the test suite for this change.

strzibny · 2024-05-18T10:27:40Z

Thanks for the PR. I merged the first one. Can you please rebase and make sure we have green test run?

strouptl · 2024-05-18T10:43:55Z

OK, I have rebased. Waiting on the test run now.

…ow" view to display link to form if otp is disabled;

…ing/enabling Two-Factor-Authentication (rather than separate otp_confirm_tokens);

…te unneeded values;

…it_otp_tokens to "lead_in";

… to "Verification Code";

…ken#edit action;

…r Authentication form;

… than redirecting via otp_credential_path);

…t_confirm block (not needed);

… was enabled, and to check for actual page content rather than flash message content for reliability;

strouptl · 2024-05-18T14:40:37Z

OK, the CI checks are passing now! Let me know if you have any additional requests/comments.

strzibny · 2024-05-19T08:49:12Z

Hi, I tried adding this change to an existing project by adding the new "edit" view. But importing the QR code stopped working and I don't see the manual provisioning code. Is there something more that you expect for upgrading existing apps?

strouptl · 2024-05-19T09:21:13Z

Ah, sorry about that. I had added a "populate_otp!" method so that existing apps can populate the OTP auth and recovery secrets for each user (since existing users have skipped the create callback), but removed it to try to minimize the delta for this PR. It sounds like it may be necessary after all, though.

I will reapply the change and update the README with instructions for existing apps so that you can test it out.

app/controllers/devise_otp/devise/otp_tokens_controller.rb

strouptl · 2024-05-19T10:44:57Z

When the secret is populated the screen works, however then /otp/token path takes you to the enabled OTP settings. So something is not working right.

Interesting. otp_tokens#show should only show the token and secret if otp_enabled? returns true for that user.

Is OTP enabled in the database for this user (user.otp_enabled?)? Maybe somehow the otp_enabled boolean got set to "true" in the master branch?

strzibny · 2024-05-19T11:27:49Z

I'll double check

strzibny · 2024-05-19T19:14:05Z

@strouptl seems I faced the issue because it does enable OTP but it's not possible to disable it. When I go for disabling it sends me to an edit it seems but in database the otp_enabled is still true. Can you check?

strouptl · 2024-05-19T19:59:58Z

@strouptl seems I faced the issue because it does enable OTP but it's not possible to disable it. When I go for disabling it sends me to an edit it seems but in database the otp_enabled is still true. Can you check?

Hi @strzibny, I just double checked, and it is working for me, so I am still thinking you must have enabled OTP somehow in the master branch.

Perhaps it is indeed better to populate the OTP secrets as needed in the otp_tokens#edit action, though, as it would resolve potential confusion like this altogether.

Here are the steps I followed, if you still want to verify them for the current issue you are seeing:

First round (without populated secrets)

Create a user in an existing application with no OTP
Add OTP to application and user model
Login as user and visit /otp/token path (shows "Disabled")
Click "Enable..." (i.e. visit /otp/token/edit) and view form (shows blank provisioning code, as you saw)
Try submitting form with blank and false Confirmation Codes (displays "did not match" error)

Second round (with populated secrets)

Call "populate_otp!" on user
Visit /otp/token/edit again (shows populated provisioning code)
Visit /otp/token path again (still shows "Disabled")

strouptl · 2024-05-19T20:12:32Z

@strouptl seems I faced the issue because it does enable OTP but it's not possible to disable it. When I go for disabling it sends me to an edit it seems but in database the otp_enabled is still true. Can you check?

@strzibny, or are you wondering about how to disable OTP with the new functionality?

In master, you do indeed use a form for both enabling and disabling OTP. However, with the new functionality, the edit action and the form it displays are only for enabling OTP.

To disable OTP, you must click the "Reset your Two Factors Authentication status" button, which points to the "destroy" action. This follows the same workflow as AWS, where disabling 2FA == removing your secret, as well.

strzibny · 2024-05-19T20:21:40Z

app/controllers/devise_otp/devise/otp_tokens_controller.rb

      #
      # Updates the status of OTP authentication
      #
      def update
-        enabled = params[resource_name][:otp_enabled] == "1"
-        if enabled ? resource.enable_otp! : resource.disable_otp!


I think is the broken behaviour I am seeing. Before your change you could temporarily disable OTP and I think we should preserve the functionality for the time being.

strzibny · 2024-05-19T20:22:38Z

I confirm that completely removing it works and commented inline where I think it's broken for the simple 'disabling of OTP'. Any reason why it shouldn't stay there?

strouptl · 2024-05-19T20:54:13Z

I confirm that completely removing it works and commented inline where I think it's broken for the simple 'disabling of OTP'. Any reason why it shouldn't stay there?

I think the argument would be that there is no way to distinguish between temporarily and permantly disabling MFA. For example, a person could click "disable 2FA" intending to reenable it shortly, but then change authenticator devices, and then come back a month later to reenable OTP not realizing that they lost the secret.

So we need to require a confirmation code anytime we enable OTP. And if this is the case, we might as well reset the secrets so that there is no confusion over whether you need to scan the QR code or not at the "Enable 2FA" form.

Maybe the "Reset functionality" is the one that needs to change. Could we use the current button and destroy action for disabling OTP, and add a new "reset" action which resets your secrets and disables OTP, but then redirects you to the enable OTP form so that you can obtain your new QR Code/secret?

strouptl · 2024-05-19T20:56:24Z

I confirm that completely removing it works and commented inline where I think it's broken for the simple 'disabling of OTP'. Any reason why it shouldn't stay there?

We could do a fuller survey to see how others have handled this, but it looks to me like both AWS and Authy do not allow temporarily disabling MFA (i.e. they will always reset the secret).

strzibny · 2024-05-19T21:14:42Z

Yes that's fair. We'll just document the change and cancel it then.

strouptl · 2024-05-19T21:29:05Z

I confirm that completely removing it works and commented inline where I think it's broken for the simple 'disabling of OTP'. Any reason why it shouldn't stay there?

We could do a fuller survey to see how others have handled this, but it looks to me like both AWS and Authy do not allow temporarily disabling MFA (i.e. they will always reset the secret).

UPDATED: I double checked Google's 2FA functionality: Google does allow you to disable 2FA in general, but this is safer for them, as they allow multiple 2FA options (phone number, in-app Prompt, etc.), so you are less susceptible to getting locked out if you lose the OTP secret. And they include warnings with this functionality, as well:

So I think we should consider this functionality as out-of-scope for devise-otp: you can implement it if desired, but there are risks that come with this, so you would want to warn your users, and would probably want to make sure you had several different MFA options available (not just OTP).

strouptl · 2024-05-19T21:33:21Z

Yes that's fair. We'll just document the change and cancel it then.

Sorry for the confusion. Are you still good with this solution?

strzibny · 2024-05-19T21:35:01Z

Yes, I am on board. But let's populate the secret on edit so nobody has to go running some code in the terminal (especially for production)

strouptl · 2024-05-19T21:36:28Z

Sounds good! I'll update the PR with the new functionality.

…

On Mon, May 20, 2024 at 6:35 AM Josef Strzibny ***@***.***> wrote: Yes, I am on board. But let's populate the secret on edit so nobody has to go running some code in the terminal (especially for production) — Reply to this email directly, view it on GitHub <#72 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AATPWQY52MOEUP2J2CBDYZDZDELJVAVCNFSM6AAAAABH4JVAD6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJZGM3DMNRSGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

…e method to "populate_otp_secrets!", replace reset_otp_credentials! method with destroy_otp_secrets!, and remove "on create" callbacks for consistency;

…s for existing users from README (no longer needed);

…ts! method;

…ce it includes counters and challenges as well);

strouptl · 2024-05-19T23:54:52Z

Hi @strzibny, I have updated the PR so that the system operates on the assumption that nobody has OTP credentials unless they need them (i.e. by clicking "Enable...").

See details below, and let me know if you have any thoughts!

Update otp_tokens#edit action to populate otp tokens as needed
Replace "reset_otp_credentials!" method with "clear_otp_fields!" (blanks out all OTP values)
Remove "on create" callback for generating OTP credentials
Updated tests for changes

strzibny · 2024-05-20T07:31:59Z

Looking good!

As a last thing can you write an entry for https://github.com/wmlele/devise-otp/blob/master/CHANGELOG.md? I am missing last two versions but I'll update it retrospectively, you can put the new version as 'Unreleased'.

I think we should mention all removed I18n keys as well as new ones, the new edit action and view, replacement of reset_otp_credentials and removal of the callback.

…ting OTP secrets as needed;

…scope singular; fix spelling issue in "Enable Two Factor Authentication" link; expand "Changes to Locales" description in CHANGELOG;

strouptl · 2024-05-20T10:07:07Z

Done.

strzibny · 2024-05-20T11:50:16Z

Thank you @strouptl

strouptl · 2024-05-20T12:07:45Z

My pleasure!

strouptl mentioned this pull request May 17, 2024

Confirm OTP before enabling #70

Closed

strzibny self-requested a review May 18, 2024 10:30

strouptl force-pushed the confirm_otp_token branch from bc74ee1 to 6ca8906 Compare May 18, 2024 10:42

strouptl added 19 commits May 18, 2024 23:37

scaffold solution for confirming OTP before enabling OTP;

a83a5dd

Add instructions to confirm_otp_token form;

e3b929d

move "Enable Authentication" form to separate "edit" view; reduce "sh…

f960474

…ow" view to display link to form if otp is disabled;

use existing edit/update actions on otp_tokens controller for confirm…

01a402f

…ing/enabling Two-Factor-Authentication (rather than separate otp_confirm_tokens);

remove token explanation from show page;

cc9067e

update flash message for failed confirmation;

fed4602

move locales for OTP confirmation form to edit_otp_tokens scope; dele…

fab7c1c

…te unneeded values;

differentiate title of show and edit pages; move "title" value for ed…

7920bd3

…it_otp_tokens to "lead_in";

revert method name to enable_top!;

5ba39da

revert "h2" for otp_tokens#show to locale file;

7074c2f

use enable_link config locale in otp_tokens#show;

2b3f893

use locales for otp_token field and submit button; switch terminology…

ffdf493

… to "Verification Code";

match terminology to AWS MFA form;

ebd03ca

replace remaining reference to "Verification Code";

debe1c1

add tests for enabling two-factor authentication via dedicated otp_to…

f30aa59

…ken#edit action;

update test helpers and initial sign_in test for new Enable Two-Facto…

0a9dc5a

…r Authentication form;

update otp_tokens#update to redirect to show action as before (rather…

8bc2b83

… than redirecting via otp_credential_path);

update disable test to confirm correct status displayed; remove accep…

074b67e

…t_confirm block (not needed);

update EnableOtpForm tests to reload user before checking whether OTP…

1e94af0

… was enabled, and to check for actual page content rather than flash message content for reliability;

strouptl force-pushed the confirm_otp_token branch from 6ca8906 to 1e94af0 Compare May 18, 2024 14:38

strzibny reviewed May 19, 2024

View reviewed changes

app/controllers/devise_otp/devise/otp_tokens_controller.rb Show resolved Hide resolved

strzibny reviewed May 19, 2024

View reviewed changes

strouptl added 5 commits May 20, 2024 07:38

update otp_tokens controller to populate otp secrets as needed; renam…

e9215f4

…e method to "populate_otp_secrets!", replace reset_otp_credentials! method with destroy_otp_secrets!, and remove "on create" callbacks for consistency;

update button text and warnings for disabling 2FA; remove instruction…

747cefa

…s for existing users from README (no longer needed);

update tests for change; add otp_failed_attempts to destroy_otp_secre…

3293345

…ts! method;

rename destroy_otp_secrets! to clear_otp_fields! for consistency (sin…

dfe9d45

…ce it includes counters and challenges as well);

simplify populate_otp_secrets! method;

0ca939f

strouptl and others added 3 commits May 20, 2024 16:56

draft CHANGELOG insertion for requiring confirmation token and popula…

51dca03

…ting OTP secrets as needed;

rename "otp_token" input to "confirmation_code"; make edit_otp_token …

04fa4a7

…scope singular; fix spelling issue in "Enable Two Factor Authentication" link; expand "Changes to Locales" description in CHANGELOG;

Update CHANGELOG.md to fix list indentation issue;

8cbc133

strzibny merged commit 57e4c92 into wmlele:master May 20, 2024
1 check passed

strouptl deleted the confirm_otp_token branch May 20, 2024 12:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Confirm OTP Token #72

Confirm OTP Token #72

strouptl commented May 17, 2024 •

edited

Loading

strouptl commented May 17, 2024

strouptl commented May 18, 2024

strzibny commented May 18, 2024 •

edited

Loading

strouptl commented May 18, 2024 •

edited

Loading

strouptl commented May 18, 2024

strzibny commented May 19, 2024 •

edited

Loading

strouptl commented May 19, 2024 •

edited

Loading

strouptl commented May 19, 2024

strzibny commented May 19, 2024

strzibny commented May 19, 2024

strouptl commented May 19, 2024 •

edited

Loading

strouptl commented May 19, 2024 •

edited

Loading

strzibny May 19, 2024

strzibny commented May 19, 2024

strouptl commented May 19, 2024

strouptl commented May 19, 2024

strzibny commented May 19, 2024

strouptl commented May 19, 2024

strouptl commented May 19, 2024

strzibny commented May 19, 2024

strouptl commented May 19, 2024 via email

strouptl commented May 19, 2024 •

edited

Loading

strzibny commented May 20, 2024

strouptl commented May 20, 2024

strzibny commented May 20, 2024

strouptl commented May 20, 2024

Confirm OTP Token #72

Confirm OTP Token #72

Conversation

strouptl commented May 17, 2024 • edited Loading

strouptl commented May 17, 2024

strouptl commented May 18, 2024

strzibny commented May 18, 2024 • edited Loading

strouptl commented May 18, 2024 • edited Loading

strouptl commented May 18, 2024

strzibny commented May 19, 2024 • edited Loading

strouptl commented May 19, 2024 • edited Loading

strouptl commented May 19, 2024

strzibny commented May 19, 2024

strzibny commented May 19, 2024

strouptl commented May 19, 2024 • edited Loading

strouptl commented May 19, 2024 • edited Loading

strzibny May 19, 2024

Choose a reason for hiding this comment

strzibny commented May 19, 2024

strouptl commented May 19, 2024

strouptl commented May 19, 2024

strzibny commented May 19, 2024

strouptl commented May 19, 2024

strouptl commented May 19, 2024

strzibny commented May 19, 2024

strouptl commented May 19, 2024 via email

strouptl commented May 19, 2024 • edited Loading

strzibny commented May 20, 2024

strouptl commented May 20, 2024

strzibny commented May 20, 2024

strouptl commented May 20, 2024

strouptl commented May 17, 2024 •

edited

Loading

strzibny commented May 18, 2024 •

edited

Loading

strouptl commented May 18, 2024 •

edited

Loading

strzibny commented May 19, 2024 •

edited

Loading

strouptl commented May 19, 2024 •

edited

Loading

strouptl commented May 19, 2024 •

edited

Loading

strouptl commented May 19, 2024 •

edited

Loading

strouptl commented May 19, 2024 •

edited

Loading