Confirm OTP Token (#72)

* scaffold solution for confirming OTP before enabling OTP;

* Add instructions to confirm_otp_token form;

* move "Enable Authentication" form to separate "edit" view; reduce "show" view to display link to form if otp is disabled;

* use existing edit/update actions on otp_tokens controller for confirming/enabling Two-Factor-Authentication (rather than separate otp_confirm_tokens);

* remove token explanation from show page;

* update flash message for failed confirmation;

* move locales for OTP confirmation form to edit_otp_tokens scope; delete unneeded values;

* differentiate title of show and edit pages; move "title" value for edit_otp_tokens to "lead_in";

* revert method name to enable_top!;

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

* use enable_link config locale in otp_tokens#show;

* use locales for otp_token field and submit button; switch terminology to "Verification Code";

* match terminology to AWS MFA form;

* replace remaining reference to "Verification Code";

* add tests for enabling two-factor authentication via dedicated otp_token#edit action;

* update test helpers and initial sign_in test for new Enable Two-Factor Authentication form;

* update otp_tokens#update to redirect to show action as before (rather than redirecting via otp_credential_path);

* update disable test to confirm correct status displayed; remove accept_confirm block (not needed);

* update EnableOtpForm tests to reload user before checking whether OTP was enabled, and to check for actual page content rather than flash message content for reliability;

* add populate_otp! method for populating initial secrets; add instructions to README for existing users;

* update otp_tokens controller to populate otp secrets as needed; rename 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 instructions for existing users from README (no longer needed);

* update tests for change; add otp_failed_attempts to destroy_otp_secrets! method;

* rename destroy_otp_secrets! to clear_otp_fields! for consistency (since it includes counters and challenges as well);

* simplify populate_otp_secrets! method;

* draft CHANGELOG insertion for requiring confirmation token and populating OTP secrets as needed;

* rename "otp_token" input to "confirmation_code"; make edit_otp_token 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;

---------

Co-authored-by: Laney Stroup <[email protected]>

CHANGELOG.md

@@ -1,5 +1,44 @@
 # Changelog
 
+## UNRELEASED
+
+Summary:
+- Require confirmation token before enabling Two Factor Authentication (2FA) to ensure that user has added OTP token properly to their device
+- Update system to populate OTP secrets as needed
+
+Details:
+- Add "edit" action with Confirmation Token for enabling 2FA to otp_tokens controller
+- Make enabling of 2FA in update action conditional on valid Confirmation Token
+- Repurpose "show" view for display of OTP status and info (no form)
+
+- Update otp_tokens#edit to populate OTP secrets (rather than assuming they are populated via callbacks in OTPDeviseAuthenticatable module)
+- Repurpose otp_tokens#destroy to disable 2FA and clear OTP secrets (rather than resetting them)
+
+- Remove OtpAuthenticatable callbacks for setting OTP credentials on create action (no longer needed)
+- Replace OtpAuthenticatable "reset_otp_credentials" methods with "clear_otp_fields!" method;
+
+Changes to Locales:
+- Remove:
+  - otp_tokens.enable_request
+  - otp_tokens.status
+  - otp_tokens.submit
+- Add to otp_tokens scope:
+  - enable_link
+- Move/rename devise.otp.token_secret.reset_\* values to devise.otp.otp_tokens.disable_\* (for consistency with "enable_link")
+  - disable_link
+  - disable_explain
+  - disable_explain_warn
+- Add to new edit_otp_token scope:
+  - title
+  - lead_in
+  - step1
+  - step2
+  - confirmation_code
+  - submit
+- Move "explain" to new edit_otp_token scope
+- Add devise.otp.otp_tokens.could_not_confirm
+- Rename "successfully_reset_creds" to "successfully_disabled_otp"
+
 ## 0.4.0
 
 Breaking changes:
@@ -14,4 +53,4 @@ Other improvements:
 
 ## 0.3.0
 
-A long awaited update bringing Devise::OTP from the dead!
+A long awaited update bringing Devise::OTP from the dead!

app/controllers/devise_otp/devise/otp_tokens_controller.rb

@@ -19,24 +19,34 @@ def show
         end
       end
 
+      #
+      # Displays the QR Code and Validation Token form for enabling the OTP
+      #
+      def edit
+        resource.populate_otp_secrets!
+      end
+
       #
       # Updates the status of OTP authentication
       #
       def update
-        enabled = params[resource_name][:otp_enabled] == "1"
-        if enabled ? resource.enable_otp! : resource.disable_otp!
+        if resource.valid_otp_token?(params[:confirmation_code])
+          resource.enable_otp!
           otp_set_flash_message :success, :successfully_updated
+          redirect_to action: :show
+        else
+          otp_set_flash_message :danger, :could_not_confirm
+          render :edit
         end
-
-        render :show
       end
 
       #
       # Resets OTP authentication, generates new credentials, sets it to off
       #
       def destroy
-        if resource.reset_otp_credentials!
-          otp_set_flash_message :success, :successfully_reset_creds
+        if resource.disable_otp!
+          resource.clear_otp_fields!
+          otp_set_flash_message :success, :successfully_disabled_otp
         end
 
         redirect_to action: :show

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

@@ -1,5 +1,4 @@
 <h3><%= I18n.t('title', :scope => 'devise.otp.token_secret') %></h3>
-<p><%= I18n.t('explain', :scope => 'devise.otp.token_secret') %></p>
 
 <%= otp_authenticator_token_image(resource) %>
 
@@ -8,11 +7,11 @@
   <code><%= resource.otp_auth_secret %></code>
 </p>
 
-<p><%= button_to I18n.t('reset_otp', :scope => 'devise.otp.token_secret'), @resource, :method => :delete, :data => { "turbo-method": "DELETE" } %></p>
+<p><%= button_to I18n.t('disable_link', :scope => 'devise.otp.otp_tokens'), @resource, :method => :delete, :data => { "turbo-method": "DELETE" } %></p>
 
 <p>
-  <%= I18n.t('reset_explain', :scope => 'devise.otp.token_secret') %>
-  <strong><%= I18n.t('reset_explain_warn', :scope => 'devise.otp.token_secret') %></strong>
+  <%= I18n.t('disable_explain', :scope => 'devise.otp.otp_tokens') %>
+  <strong><%= I18n.t('disable_explain_warn', :scope => 'devise.otp.otp_tokens') %></strong>
 </p>
 
 <%- if recovery_enabled? %>

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

@@ -0,0 +1,26 @@
+<h2><%= I18n.t('title', :scope => 'devise.otp.edit_otp_token') %></h2>
+<p><%= I18n.t('explain', :scope => 'devise.otp.edit_otp_token') %></p>
+
+<h2><%= I18n.t('lead_in', :scope => 'devise.otp.edit_otp_token') %></h2>
+
+<p><%= I18n.t('step_1', :scope => 'devise.otp.edit_otp_token') %></p>
+
+<%= otp_authenticator_token_image(resource) %>
+
+<p>
+  <strong><%= I18n.t('manual_provisioning', :scope => 'devise.otp.token_secret') %>:</strong>
+  <code><%= resource.otp_auth_secret %></code>
+</p>
+
+<p><%= I18n.t('step_2', :scope => 'devise.otp.edit_otp_token') %></p>
+
+<%= form_with(:url => [resource_name, :otp_token], :method => :put) do |f| %>
+
+  <p>
+    <%= f.label :confirmation_code, I18n.t('confirmation_code', :scope => 'devise.otp.edit_otp_token') %>
+    <%= f.text_field :confirmation_code %>
+  </p>
+
+  <p><%= f.submit I18n.t('submit', :scope => 'devise.otp.edit_otp_token') %></p>
+
+<% end %>

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

@@ -1,21 +1,10 @@
 <h2><%= I18n.t('title', :scope => 'devise.otp.otp_tokens') %></h2>
-<p><%= I18n.t('explain', :scope => 'devise.otp.otp_tokens') %></p>
 
-<%= form_for(resource, :as => resource_name, :url => [resource_name, :otp_token], :html => { :method => :put, "data-turbo" => false }) do |f| %>
-
-  <%= render "devise/shared/error_messages", resource: resource %>
-
-  <h3><%= I18n.t('enable_request', :scope => 'devise.otp.otp_tokens') %></h3>
-
-  <p>
-    <%= f.label :otp_enabled, I18n.t('status', :scope => 'devise.otp.otp_tokens') %><br />
-    <%= f.check_box :otp_enabled %>
-  </p>
-
-  <p><%= f.submit I18n.t('submit', :scope => 'devise.otp.otp_tokens') %></p>
-<% end %>
+<p><strong>Status:</strong> <%= resource.otp_enabled? ? "Enabled" : "Disabled" %></p>
 
 <%- if resource.otp_enabled? %>
   <%= render :partial => 'token_secret' if resource.otp_enabled? %>
   <%= render :partial => 'trusted_devices' if trusted_devices_enabled? %>
+<% else %>
+  <%= link_to I18n.t('enable_link', :scope => 'devise.otp.otp_tokens'), edit_otp_token_path_for(resource) %>
 <% end %>

config/locales/en.yml

@@ -26,17 +26,15 @@ en:
         title: 'Your token secret'
         explain: 'Take a photo of this QR code with your mobile'
         manual_provisioning: 'Manual provisioning code'
-        reset_otp: 'Reset your Two Factors Authentication status'
-        reset_explain: 'This will reset your credentials, and disable two-factors authentication.'
-        reset_explain_warn: 'You will need to enroll your mobile device again.'
       otp_tokens:
         title: 'Two-factors Authentication:'
-        explain: 'Two factors authentication adds an additional layer of security to your account. When logging in you will be asked for a code that you can generate on a physical device, like your phone.'
-        enable_request: 'Would you like to enable Two Factors Authenticator?'
-        status: 'Enable Two-Factors Authentication.'
-        submit: 'Continue...'
+        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.'
         successfully_updated: 'Your two-factors authentication settings have been updated.'
-        successfully_reset_creds: 'Your two-factors credentials has been reset.'
+        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_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.'
@@ -48,6 +46,14 @@ en:
           code: 'Recovery Code'
           codes_list: 'Here is the list of your recovery codes'
           download_codes: 'Download recovery codes'
+      edit_otp_token:
+        title: 'Enable Two-factors Authentication'
+        explain: 'Two factors authentication adds an additional layer of security to your account. When logging in you will be asked for a code that you can generate on a physical device, like your phone.'
+        lead_in: 'To Enable Two-Factor Authentication:'
+        step_1: '1. Open your authenticator app and scan the QR code shown below:'
+        step_2: '2. Enter the 6-digit code shown in your authenticator app below:'
+        confirmation_code: "Confirmation Code"
+        submit: 'Continue...'
       trusted_browsers:
         title: 'Trusted Browsers'
         explain: 'If you set your browser as trusted, you will not be asked to provide a Two-factor authentication token when logging in from that browser.'

lib/devise_otp_authenticatable/controllers/url_helpers.rb

@@ -21,6 +21,11 @@ def otp_token_path_for(resource_or_scope, opts = {})
         send("#{scope}_otp_token_path", opts)
       end
 
+      def edit_otp_token_path_for(resource_or_scope, opts = {})
+        scope = ::Devise::Mapping.find_scope!(resource_or_scope)
+        send("edit_#{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/models/otp_authenticatable.rb

@@ -5,8 +5,6 @@ module OtpAuthenticatable
     extend ActiveSupport::Concern
 
     included do
-      before_validation :generate_otp_auth_secret, on: :create
-      before_validation :generate_otp_persistence_seed, on: :create
       scope :with_valid_otp_challenge, lambda { |time| where("otp_challenge_expires > ?", time) }
     end
 
@@ -36,21 +34,6 @@ def otp_provisioning_identifier
       email
     end
 
-    def reset_otp_credentials
-      @time_based_otp = nil
-      @recovery_otp = nil
-      generate_otp_auth_secret
-      reset_otp_persistence
-      update!(otp_enabled: false,
-        otp_session_challenge: nil, otp_challenge_expires: nil,
-        otp_recovery_counter: 0)
-    end
-
-    def reset_otp_credentials!
-      reset_otp_credentials
-      save!
-    end
-
     def reset_otp_persistence
       generate_otp_persistence_seed
     end
@@ -60,11 +43,30 @@ def reset_otp_persistence!
       save!
     end
 
-    def enable_otp!
-      if otp_persistence_seed.nil?
-        reset_otp_credentials!
+    def populate_otp_secrets!
+      if [otp_auth_secret, otp_recovery_secret, otp_persistence_seed].any? { |a| a.blank? }
+        generate_otp_auth_secret
+        generate_otp_persistence_seed
+        self.save!
       end
+    end
+
+    def clear_otp_fields!
+      @time_based_otp = nil
+      @recovery_otp = nil
+
+      self.update!(
+        :otp_auth_secret => nil,
+        :otp_recovery_secret => nil,
+        :otp_persistence_seed => nil,
+        :otp_session_challenge => nil,
+        :otp_challenge_expires => nil,
+        :otp_failed_attempts => 0,
+        :otp_recovery_counter => 0
+      )
+    end
 
+    def enable_otp!
       update!(otp_enabled: true, otp_enabled_on: Time.now)
     end
 

lib/devise_otp_authenticatable/routes.rb

@@ -4,7 +4,7 @@ class Mapper
 
     def devise_otp(mapping, controllers)
       namespace :otp, module: :devise_otp do
-        resource :token, only: [:show, :update, :destroy],
+        resource :token, only: [:show, :edit, :update, :destroy],
           path: mapping.path_names[:token], controller: controllers[:otp_tokens] do
           if Devise.otp_trust_persistence
             get :persistence, action: "get_persistence"
@@ -20,6 +20,7 @@ def devise_otp(mapping, controllers)
           get :refresh, action: "get_refresh"
           put :refresh, action: "set_refresh"
         end
+
       end
     end
   end

test/integration/enable_otp_form_test.rb

@@ -0,0 +1,57 @@
+require "test_helper"
+require "integration_tests_helper"
+
+class EnableOtpFormTest < ActionDispatch::IntegrationTest
+  def teardown
+    Capybara.reset_sessions!
+  end
+
+  test "a user should be able enable their OTP authentication by entering a confirmation code" do
+    user = sign_user_in
+
+    visit edit_user_otp_token_path
+
+    user.reload
+
+    fill_in "confirmation_code", with: ROTP::TOTP.new(user.otp_auth_secret).at(Time.now)
+
+    click_button "Continue..."
+
+    assert_equal user_otp_token_path, current_path
+    assert page.has_content?("Enabled")
+
+    user.reload
+    assert user.otp_enabled?
+  end
+
+  test "a user should not be able enable their OTP authentication with an incorrect confirmation code" do
+    user = sign_user_in
+
+    visit edit_user_otp_token_path
+
+    fill_in "confirmation_code", with: "123456"
+
+    click_button "Continue..."
+
+    assert page.has_content?("To Enable Two-Factor Authentication")
+
+    user.reload
+    assert_not user.otp_enabled?
+  end
+
+  test "a user should not be able enable their OTP authentication with a blank confirmation code" do
+    user = sign_user_in
+
+    visit edit_user_otp_token_path
+
+    fill_in "confirmation_code", with: ""
+
+    click_button "Continue..."
+
+    assert page.has_content?("To Enable Two-Factor Authentication")
+
+    user.reload
+    assert_not user.otp_enabled?
+  end
+
+end

test/integration/sign_in_test.rb

@@ -17,20 +17,16 @@ def teardown
     assert_equal posts_path, current_path
   end
 
-  test "a new user, just signed in, should be able to sign in and enable their OTP authentication" do
+  test "a new user, just signed in, should be able to see and click the 'Enable Two Factor Authentication' link" do
     user = sign_user_in
 
     visit user_otp_token_path
-    assert !page.has_content?("Your token secret")
+    assert page.has_content?("Disabled")
 
-    check "user_otp_enabled"
-    click_button "Continue..."
+    click_link "Enable Two-Factor Authentication"
 
-    assert_equal user_otp_token_path, current_path
-
-    assert page.has_content?("Your token secret")
-    assert !user.otp_auth_secret.nil?
-    assert !user.otp_persistence_seed.nil?
+    assert page.has_content?("Enable Two-factors Authentication")
+    assert_equal edit_user_otp_token_path, current_path
   end
 
   test "a new user should be able to sign in enable OTP and be prompted for their token" do

test/integration/token_test.rb

@@ -19,6 +19,8 @@ def teardown
     # disable OTP
     disable_otp
 
+    assert page.has_content? "Disabled"
+
     # logout
     sign_out