From 39b2b945723389a4f1a068a25b985f95686f5d8d Mon Sep 17 00:00:00 2001 From: Laney Stroup Date: Tue, 4 Jun 2024 22:12:06 +0900 Subject: [PATCH 1/5] distinguish helper methods for checking whether OTP is mandatory, and whether mandatory OTP is missing; --- .../controllers/public_helpers.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/devise_otp_authenticatable/controllers/public_helpers.rb b/lib/devise_otp_authenticatable/controllers/public_helpers.rb index 3ce7293..30ef1ba 100644 --- a/lib/devise_otp_authenticatable/controllers/public_helpers.rb +++ b/lib/devise_otp_authenticatable/controllers/public_helpers.rb @@ -16,7 +16,7 @@ def self.define_helpers(mapping) #:nodoc: def ensure_mandatory_#{mapping}_otp! resource = current_#{mapping} if !devise_controller? - if otp_mandatory_on?(resource) + if mandatory_otp_missing_on?(resource) redirect_to edit_#{mapping}_otp_token_path end end @@ -25,10 +25,13 @@ def ensure_mandatory_#{mapping}_otp! end def otp_mandatory_on?(resource) - return true if resource.class.otp_mandatory && !resource.otp_enabled return false unless resource.respond_to?(:otp_mandatory) - resource.otp_mandatory && !resource.otp_enabled + resource.class.otp_mandatory or resource.otp_mandatory + end + + def mandatory_otp_missing_on?(resource) + otp_mandatory_on?(resource) && !resource.otp_enabled end end From 841380a98373b13dca2646f485c489c42ada1fba Mon Sep 17 00:00:00 2001 From: Laney Stroup Date: Tue, 4 Jun 2024 22:45:41 +0900 Subject: [PATCH 2/5] add reset token action; --- .../devise_otp/devise/otp_tokens_controller.rb | 9 +++++++++ app/views/devise/otp_tokens/_token_secret.html.erb | 7 +++++++ config/locales/en.yml | 4 ++++ .../controllers/url_helpers.rb | 5 +++++ lib/devise_otp_authenticatable/routes.rb | 1 + 5 files changed, 26 insertions(+) diff --git a/app/controllers/devise_otp/devise/otp_tokens_controller.rb b/app/controllers/devise_otp/devise/otp_tokens_controller.rb index b6ae3c6..1fe1bad 100644 --- a/app/controllers/devise_otp/devise/otp_tokens_controller.rb +++ b/app/controllers/devise_otp/devise/otp_tokens_controller.rb @@ -95,6 +95,15 @@ def recovery end end + def reset + if resource.disable_otp! + resource.clear_otp_fields! + otp_set_flash_message :success, :successfully_reset_otp + end + + redirect_to action: :edit + end + private def ensure_credentials_refresh diff --git a/app/views/devise/otp_tokens/_token_secret.html.erb b/app/views/devise/otp_tokens/_token_secret.html.erb index 7ae09b5..135819f 100644 --- a/app/views/devise/otp_tokens/_token_secret.html.erb +++ b/app/views/devise/otp_tokens/_token_secret.html.erb @@ -14,6 +14,13 @@ <%= I18n.t('disable_explain_warn', :scope => 'devise.otp.otp_tokens') %>

+

<%= button_to I18n.t('reset_link', :scope => 'devise.otp.otp_tokens'), reset_otp_token_path_for(resource), :method => :post , :data => { "turbo-method": "POST" } %>

+ +

+ <%= I18n.t('reset_explain', :scope => 'devise.otp.otp_tokens') %> + <%= I18n.t('reset_explain_warn', :scope => 'devise.otp.otp_tokens') %> +

+ <%- if recovery_enabled? %>

<%= I18n.t('title', :scope => 'devise.otp.otp_tokens.recovery') %>

<%= I18n.t('explain', :scope => 'devise.otp.otp_tokens.recovery') %>

diff --git a/config/locales/en.yml b/config/locales/en.yml index 133a162..426662d 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -32,9 +32,13 @@ en: disable_link: 'Disable Two-Factor Authentication' disable_explain: 'This will disable Two-Factor authentication and clear the OTP secret.' disable_explain_warn: 'To re-enable Two-Factor authentication, you will need to enroll your mobile device again.' + reset_link: 'Reset Token Secret' + reset_explain: 'Resetting your token secret will temporarilly disable Two-Factor authentication.' + reset_explain_warn: 'To re-enable Two-Factor authentication, you will need to re-enroll your mobile device with the new token secret.' successfully_updated: 'Your two-factors authentication settings have been updated.' could_not_confirm: 'The Confirmation Code you entered did not match the QR code shown below.' successfully_disabled_otp: 'Two-Factor authentication has been disabled.' + successfully_reset_otp: 'Your token secret has been reset. Please confirm your new token secret below.' successfully_set_persistence: 'Your device is now trusted.' successfully_cleared_persistence: 'Your device has been removed from the list of trusted devices.' successfully_reset_persistence: 'Your list of trusted devices has been cleared.' diff --git a/lib/devise_otp_authenticatable/controllers/url_helpers.rb b/lib/devise_otp_authenticatable/controllers/url_helpers.rb index 4b8415e..14754fb 100644 --- a/lib/devise_otp_authenticatable/controllers/url_helpers.rb +++ b/lib/devise_otp_authenticatable/controllers/url_helpers.rb @@ -26,6 +26,11 @@ def edit_otp_token_path_for(resource_or_scope, opts = {}) send("edit_#{scope}_otp_token_path", opts) end + def reset_otp_token_path_for(resource_or_scope, opts = {}) + scope = ::Devise::Mapping.find_scope!(resource_or_scope) + send("reset_#{scope}_otp_token_path", opts) + end + def otp_credential_path_for(resource_or_scope, opts = {}) scope = ::Devise::Mapping.find_scope!(resource_or_scope) send("#{scope}_otp_credential_path", opts) diff --git a/lib/devise_otp_authenticatable/routes.rb b/lib/devise_otp_authenticatable/routes.rb index a751306..45e7296 100644 --- a/lib/devise_otp_authenticatable/routes.rb +++ b/lib/devise_otp_authenticatable/routes.rb @@ -13,6 +13,7 @@ def devise_otp(mapping, controllers) end get :recovery + post :reset end resource :credential, only: [:show, :update], From 19ecaad4a44cc29a04225bcf054710fc5f3daf6d Mon Sep 17 00:00:00 2001 From: Laney Stroup Date: Tue, 4 Jun 2024 22:59:07 +0900 Subject: [PATCH 3/5] - hide disable button for mandatory OTP resources; - update disable action to preserve existing OTP token; - move disable button to bottom of otp_tokens#show page; --- app/controllers/devise_otp/devise/otp_tokens_controller.rb | 1 - app/views/devise/otp_tokens/_token_secret.html.erb | 7 ------- app/views/devise/otp_tokens/show.html.erb | 4 ++++ config/locales/en.yml | 2 -- lib/devise_otp_authenticatable/engine.rb | 2 ++ 5 files changed, 6 insertions(+), 10 deletions(-) diff --git a/app/controllers/devise_otp/devise/otp_tokens_controller.rb b/app/controllers/devise_otp/devise/otp_tokens_controller.rb index 1fe1bad..c80aa7a 100644 --- a/app/controllers/devise_otp/devise/otp_tokens_controller.rb +++ b/app/controllers/devise_otp/devise/otp_tokens_controller.rb @@ -45,7 +45,6 @@ def update # def destroy if resource.disable_otp! - resource.clear_otp_fields! otp_set_flash_message :success, :successfully_disabled_otp end diff --git a/app/views/devise/otp_tokens/_token_secret.html.erb b/app/views/devise/otp_tokens/_token_secret.html.erb index 135819f..a8c6a3c 100644 --- a/app/views/devise/otp_tokens/_token_secret.html.erb +++ b/app/views/devise/otp_tokens/_token_secret.html.erb @@ -7,13 +7,6 @@ <%= resource.otp_auth_secret %>

-

<%= button_to I18n.t('disable_link', :scope => 'devise.otp.otp_tokens'), @resource, :method => :delete, :data => { "turbo-method": "DELETE" } %>

- -

- <%= I18n.t('disable_explain', :scope => 'devise.otp.otp_tokens') %> - <%= I18n.t('disable_explain_warn', :scope => 'devise.otp.otp_tokens') %> -

-

<%= button_to I18n.t('reset_link', :scope => 'devise.otp.otp_tokens'), reset_otp_token_path_for(resource), :method => :post , :data => { "turbo-method": "POST" } %>

diff --git a/app/views/devise/otp_tokens/show.html.erb b/app/views/devise/otp_tokens/show.html.erb index 8ca6d0a..5a663c2 100644 --- a/app/views/devise/otp_tokens/show.html.erb +++ b/app/views/devise/otp_tokens/show.html.erb @@ -5,6 +5,10 @@ <%- if resource.otp_enabled? %> <%= render :partial => 'token_secret' if resource.otp_enabled? %> <%= render :partial => 'trusted_devices' if trusted_devices_enabled? %> + + <% unless otp_mandatory_on?(resource) %> + <%= button_to I18n.t('disable_link', :scope => 'devise.otp.otp_tokens'), @resource, :method => :delete, :data => { "turbo-method": "DELETE" } %> + <% end %> <% else %> <%= link_to I18n.t('enable_link', :scope => 'devise.otp.otp_tokens'), edit_otp_token_path_for(resource) %> <% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 426662d..40e17cb 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -30,8 +30,6 @@ en: title: 'Two-factors Authentication:' enable_link: 'Enable Two-Factor Authentication' disable_link: 'Disable Two-Factor Authentication' - disable_explain: 'This will disable Two-Factor authentication and clear the OTP secret.' - disable_explain_warn: 'To re-enable Two-Factor authentication, you will need to enroll your mobile device again.' reset_link: 'Reset Token Secret' reset_explain: 'Resetting your token secret will temporarilly disable Two-Factor authentication.' reset_explain_warn: 'To re-enable Two-Factor authentication, you will need to re-enroll your mobile device with the new token secret.' diff --git a/lib/devise_otp_authenticatable/engine.rb b/lib/devise_otp_authenticatable/engine.rb index 035c83a..6b9f168 100644 --- a/lib/devise_otp_authenticatable/engine.rb +++ b/lib/devise_otp_authenticatable/engine.rb @@ -12,11 +12,13 @@ class Engine < ::Rails::Engine ActiveSupport.on_load(:devise_controller) do include DeviseOtpAuthenticatable::Controllers::UrlHelpers include DeviseOtpAuthenticatable::Controllers::Helpers + include DeviseOtpAuthenticatable::Controllers::PublicHelpers end ActiveSupport.on_load(:action_view) do include DeviseOtpAuthenticatable::Controllers::UrlHelpers include DeviseOtpAuthenticatable::Controllers::Helpers + include DeviseOtpAuthenticatable::Controllers::PublicHelpers end # See: https://guides.rubyonrails.org/engines.html#separate-assets-and-precompiling From 38de4691c456d0ce0e76b29d851b493ae451bc7b Mon Sep 17 00:00:00 2001 From: Laney Stroup Date: Wed, 5 Jun 2024 04:18:45 +0900 Subject: [PATCH 4/5] add CHANGELOG entry for new reset and updated disable actions; --- CHANGELOG.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5bf0f74..a688d57 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,24 @@ ## UNRELEASED +Summary: Add reset token action, and hide/repurpose disable token action + +Details: +- Add reset token action to disable OTP, reset token secret, and redirect to otp_tokens#edit to re-enable with new token secret; +- Update disable action to preserve the existing token secret (since the reset action now accomplishes this functionality); +- Hide disable button when mandatory OTP; +- Move disable button to bottom of page; + +Breaking Changes (config/locales/en.yml): +- Add: + - reset\_link + - successfully\_reset\_otp +- Move/Update + - disable\_explain > reset\_explain + - disable\_explain\_warn > reset\_explain\_warn + +## UNRELEASED + Fix regression due to warden session scope usage Details: From 3cf94b97c3aa917a0fc0f69a746439750ab785c9 Mon Sep 17 00:00:00 2001 From: Laney Stroup Date: Wed, 5 Jun 2024 05:15:56 +0900 Subject: [PATCH 5/5] - add tests for reset token functionality; - add test to ensure that disable preserves existing token secrets; - move existing token tests to disable_token for clarity; --- test/integration/disable_token_test.rb | 53 ++++++++++++++++++++++++++ test/integration/reset_token_test.rb | 45 ++++++++++++++++++++++ test/integration/token_test.rb | 32 ---------------- test/integration_tests_helper.rb | 5 +++ 4 files changed, 103 insertions(+), 32 deletions(-) create mode 100644 test/integration/disable_token_test.rb create mode 100644 test/integration/reset_token_test.rb delete mode 100644 test/integration/token_test.rb diff --git a/test/integration/disable_token_test.rb b/test/integration/disable_token_test.rb new file mode 100644 index 0000000..ff3efbe --- /dev/null +++ b/test/integration/disable_token_test.rb @@ -0,0 +1,53 @@ +require "test_helper" +require "integration_tests_helper" + +class DisableTokenTest < ActionDispatch::IntegrationTest + + def setup + # log in 1fa + @user = enable_otp_and_sign_in + assert_equal user_otp_credential_path, current_path + + # otp 2fa + fill_in "user_token", with: ROTP::TOTP.new(@user.otp_auth_secret).at(Time.now) + click_button "Submit Token" + assert_equal root_path, current_path + end + + def teardown + Capybara.reset_sessions! + end + + test "disabling OTP after successfully enabling" do + # disable OTP + disable_otp + + assert page.has_content? "Disabled" + + # logout + sign_out + + # log back in 1fa + sign_user_in(@user) + + assert_equal root_path, current_path + end + + test "disabling OTP does not reset token secrets" do + # get otp secrets + @user.reload + auth_secret = @user.otp_auth_secret + recovery_secret = @user.otp_recovery_secret + + # disable OTP + disable_otp + + # compare otp secrets + assert_not_nil @user.otp_auth_secret + assert_equal @user.otp_auth_secret, auth_secret + + assert_not_nil @user.otp_recovery_secret + assert_equal @user.otp_recovery_secret, recovery_secret + end + +end diff --git a/test/integration/reset_token_test.rb b/test/integration/reset_token_test.rb new file mode 100644 index 0000000..a6eba69 --- /dev/null +++ b/test/integration/reset_token_test.rb @@ -0,0 +1,45 @@ +require "test_helper" +require "integration_tests_helper" + +class ResetTokenTest < ActionDispatch::IntegrationTest + + def setup + # log in 1fa + @user = enable_otp_and_sign_in + assert_equal user_otp_credential_path, current_path + + # otp 2fa + fill_in "user_token", with: ROTP::TOTP.new(@user.otp_auth_secret).at(Time.now) + click_button "Submit Token" + assert_equal root_path, current_path + end + + + def teardown + Capybara.reset_sessions! + end + + test "redirects to otp_tokens#edit page" do + reset_otp + + assert_equal "/users/otp/token/edit", current_path + end + + test "generates new token secrets" do + # get auth secrets + auth_secret = @user.otp_auth_secret + recovery_secret = @user.otp_recovery_secret + + # reset otp + reset_otp + + # compare auth secrets + @user.reload + assert_not_nil @user.otp_auth_secret + assert_not_equal @user.otp_auth_secret, auth_secret + + assert_not_nil @user.otp_recovery_secret + assert_not_equal @user.otp_recovery_secret, recovery_secret + end + +end diff --git a/test/integration/token_test.rb b/test/integration/token_test.rb deleted file mode 100644 index ed2f63d..0000000 --- a/test/integration/token_test.rb +++ /dev/null @@ -1,32 +0,0 @@ -require "test_helper" -require "integration_tests_helper" - -class TokenTest < ActionDispatch::IntegrationTest - def teardown - Capybara.reset_sessions! - end - - test "disabling OTP after successfully enabling" do - # log in 1fa - user = enable_otp_and_sign_in - assert_equal user_otp_credential_path, current_path - - # otp 2fa - fill_in "user_token", with: ROTP::TOTP.new(user.otp_auth_secret).at(Time.now) - click_button "Submit Token" - assert_equal root_path, current_path - - # disable OTP - disable_otp - - assert page.has_content? "Disabled" - - # logout - sign_out - - # log back in 1fa - sign_user_in(user) - - assert_equal root_path, current_path - end -end diff --git a/test/integration_tests_helper.rb b/test/integration_tests_helper.rb index 36ca0a0..b11f47b 100644 --- a/test/integration_tests_helper.rb +++ b/test/integration_tests_helper.rb @@ -59,6 +59,11 @@ def disable_otp click_button "Disable Two-Factor Authentication" end + def reset_otp + visit user_otp_token_path + click_button "Reset Token Secret" + end + def sign_out logout :user end