Add "Reset" button, and repurpose/hide "Disable" button for Mandatory…

… OTP (#83)

* distinguish helper methods for checking whether OTP is mandatory, and whether mandatory OTP is missing;

* add reset token action;

* - 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;

* add CHANGELOG entry for new reset and updated disable actions;

* - 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;

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:

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
 
@@ -95,6 +94,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

app/views/devise/otp_tokens/_token_secret.html.erb

@@ -7,11 +7,11 @@
   <code><%= resource.otp_auth_secret %></code>
 </p>
 
-<p><%= button_to I18n.t('disable_link', :scope => 'devise.otp.otp_tokens'), @resource, :method => :delete, :data => { "turbo-method": "DELETE" } %></p>
+<p><%= button_to I18n.t('reset_link', :scope => 'devise.otp.otp_tokens'), reset_otp_token_path_for(resource), :method => :post , :data => { "turbo-method": "POST" } %></p>
 
 <p>
-  <%= I18n.t('disable_explain', :scope => 'devise.otp.otp_tokens') %>
-  <strong><%= I18n.t('disable_explain_warn', :scope => 'devise.otp.otp_tokens') %></strong>
+  <%= I18n.t('reset_explain', :scope => 'devise.otp.otp_tokens') %>
+  <strong><%= I18n.t('reset_explain_warn', :scope => 'devise.otp.otp_tokens') %></strong>
 </p>
 
 <%- if recovery_enabled? %>

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 %>

config/locales/en.yml

@@ -30,11 +30,13 @@ 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.'
         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.'

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

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)

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

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],

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

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

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