<%= I18n.t('explain', :scope => 'devise.otp.token_secret') %>
<%= otp_authenticator_token_image(resource) %> @@ -8,11 +7,11 @@<%= resource.otp_auth_secret %>
-<%= button_to I18n.t('reset_otp', :scope => 'devise.otp.token_secret'), @resource, :method => :delete, :data => { "turbo-method": "DELETE" } %>
+<%= button_to I18n.t('disable_link', :scope => 'devise.otp.otp_tokens'), @resource, :method => :delete, :data => { "turbo-method": "DELETE" } %>
- <%= I18n.t('reset_explain', :scope => 'devise.otp.token_secret') %> - <%= I18n.t('reset_explain_warn', :scope => 'devise.otp.token_secret') %> + <%= I18n.t('disable_explain', :scope => 'devise.otp.otp_tokens') %> + <%= I18n.t('disable_explain_warn', :scope => 'devise.otp.otp_tokens') %>
<%- if recovery_enabled? %> diff --git a/app/views/devise/otp_tokens/edit.html.erb b/app/views/devise/otp_tokens/edit.html.erb new file mode 100644 index 0000000..db0e5c4 --- /dev/null +++ b/app/views/devise/otp_tokens/edit.html.erb @@ -0,0 +1,26 @@ +<%= I18n.t('explain', :scope => 'devise.otp.edit_otp_token') %>
+ +<%= I18n.t('step_1', :scope => 'devise.otp.edit_otp_token') %>
+ +<%= otp_authenticator_token_image(resource) %> + +
+ <%= I18n.t('manual_provisioning', :scope => 'devise.otp.token_secret') %>:
+ <%= resource.otp_auth_secret %>
+
<%= I18n.t('step_2', :scope => 'devise.otp.edit_otp_token') %>
+ +<%= form_with(:url => [resource_name, :otp_token], :method => :put) do |f| %> + ++ <%= f.label :confirmation_code, I18n.t('confirmation_code', :scope => 'devise.otp.edit_otp_token') %> + <%= f.text_field :confirmation_code %> +
+ +<%= f.submit I18n.t('submit', :scope => 'devise.otp.edit_otp_token') %>
+ +<% end %> diff --git a/app/views/devise/otp_tokens/show.html.erb b/app/views/devise/otp_tokens/show.html.erb index 0cbf48e..8ca6d0a 100644 --- a/app/views/devise/otp_tokens/show.html.erb +++ b/app/views/devise/otp_tokens/show.html.erb @@ -1,21 +1,10 @@<%= I18n.t('explain', :scope => 'devise.otp.otp_tokens') %>
-<%= 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 %> - -
- <%= f.label :otp_enabled, I18n.t('status', :scope => 'devise.otp.otp_tokens') %>
- <%= f.check_box :otp_enabled %>
-
<%= f.submit I18n.t('submit', :scope => 'devise.otp.otp_tokens') %>
-<% end %> +Status: <%= resource.otp_enabled? ? "Enabled" : "Disabled" %>
<%- 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 %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 09b19fa..133a162 100644 --- a/config/locales/en.yml +++ b/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.' diff --git a/lib/devise_otp_authenticatable/controllers/url_helpers.rb b/lib/devise_otp_authenticatable/controllers/url_helpers.rb index 3b6f068..4b8415e 100644 --- a/lib/devise_otp_authenticatable/controllers/url_helpers.rb +++ b/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) diff --git a/lib/devise_otp_authenticatable/models/otp_authenticatable.rb b/lib/devise_otp_authenticatable/models/otp_authenticatable.rb index d4ad988..8450ed2 100644 --- a/lib/devise_otp_authenticatable/models/otp_authenticatable.rb +++ b/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 diff --git a/lib/devise_otp_authenticatable/routes.rb b/lib/devise_otp_authenticatable/routes.rb index 00fcf04..a751306 100644 --- a/lib/devise_otp_authenticatable/routes.rb +++ b/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 diff --git a/test/integration/enable_otp_form_test.rb b/test/integration/enable_otp_form_test.rb new file mode 100644 index 0000000..fcbebf7 --- /dev/null +++ b/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 diff --git a/test/integration/sign_in_test.rb b/test/integration/sign_in_test.rb index 9345b97..7f2f2e5 100644 --- a/test/integration/sign_in_test.rb +++ b/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 diff --git a/test/integration/token_test.rb b/test/integration/token_test.rb index ffbbbc5..ed2f63d 100644 --- a/test/integration/token_test.rb +++ b/test/integration/token_test.rb @@ -19,6 +19,8 @@ def teardown # disable OTP disable_otp + assert page.has_content? "Disabled" + # logout sign_out diff --git a/test/integration_tests_helper.rb b/test/integration_tests_helper.rb index 9876012..c22a8e0 100644 --- a/test/integration_tests_helper.rb +++ b/test/integration_tests_helper.rb @@ -25,9 +25,11 @@ def enable_otp_and_sign_in_with_otp def enable_otp_and_sign_in user = create_full_user + user.populate_otp_secrets! + sign_user_in(user) - visit user_otp_token_path - check "user_otp_enabled" + visit edit_user_otp_token_path + fill_in "confirmation_code", with: ROTP::TOTP.new(user.otp_auth_secret).at(Time.now) click_button "Continue..." Capybara.reset_sessions! @@ -43,8 +45,7 @@ def otp_challenge_for(user) def disable_otp visit user_otp_token_path - uncheck "user_otp_enabled" - click_button "Continue..." + click_button "Disable Two-Factor Authentication" end def sign_out diff --git a/test/models/otp_authenticatable_test.rb b/test/models/otp_authenticatable_test.rb index 220e68e..3bad3d6 100644 --- a/test/models/otp_authenticatable_test.rb +++ b/test/models/otp_authenticatable_test.rb @@ -6,42 +6,70 @@ def setup new_user end - test "new users have a non-nil secret set" do - assert_not_nil User.first.otp_auth_secret + test "new users do not have a secret set" do + user = User.first + + [:otp_auth_secret, :otp_recovery_secret, :otp_persistence_seed].each do |field| + assert_nil user.send(field) + end end test "new users have OTP disabled by default" do assert !User.first.otp_enabled end - test "users should have an instance of TOTP/ROTP objects" do - u = User.first - assert u.time_based_otp.is_a? ROTP::TOTP - assert u.recovery_otp.is_a? ROTP::HOTP + test "populating otp secrets should populate all required fields" do + user = User.first + user.populate_otp_secrets! + + [:otp_auth_secret, :otp_recovery_secret, :otp_persistence_seed].each do |field| + assert_not_nil user.send(field) + end end - test "users should have their otp_auth_secret/persistence_seed set on creation" do - assert User.first.otp_auth_secret - assert User.first.otp_persistence_seed + test "time_based_otp and recover_otp fields should be an instance of TOTP/ROTP objects" do + user = User.first + user.populate_otp_secrets! + + assert user.time_based_otp.is_a? ROTP::TOTP + assert user.recovery_otp.is_a? ROTP::HOTP end - test "reset_otp_credentials should generate new secrets and disable OTP" do - u = User.first - u.update_attribute(:otp_enabled, true) - assert u.otp_enabled - otp_auth_secret = u.otp_auth_secret - otp_persistence_seed = u.otp_persistence_seed + test "clear_otp_fields should clear all otp fields" do + user = User.first + user.populate_otp_secrets! - u.reset_otp_credentials! - assert !(otp_auth_secret == u.otp_auth_secret) - assert !(otp_persistence_seed == u.otp_persistence_seed) - assert !u.otp_enabled + user.enable_otp! + user.generate_otp_challenge! + user.update( + :otp_failed_attempts => 1, + :otp_recovery_counter => 1 + ) + + + assert user.otp_enabled + [:otp_auth_secret, :otp_recovery_secret, :otp_persistence_seed].each do |field| + assert_not_nil user.send(field) + end + [:otp_failed_attempts, :otp_recovery_counter].each do |field| + assert_not user.send(field) == 0 + end + + user.clear_otp_fields! + [:otp_auth_secret, :otp_recovery_secret, :otp_persistence_seed].each do |field| + assert_nil user.send(field) + end + [:otp_failed_attempts, :otp_recovery_counter].each do |field| + assert user.send(field) == 0 + end end test "reset_otp_persistence should generate new persistence_seed but NOT change the otp_auth_secret" do u = User.first - u.update_attribute(:otp_enabled, true) + u.populate_otp_secrets! + u.enable_otp! assert u.otp_enabled + otp_auth_secret = u.otp_auth_secret otp_persistence_seed = u.otp_persistence_seed @@ -53,7 +81,8 @@ def setup test "generating a challenge, should retrieve the user later" do u = User.first - u.update_attribute(:otp_enabled, true) + u.populate_otp_secrets! + u.enable_otp! challenge = u.generate_otp_challenge! w = User.find_valid_otp_challenge(challenge) @@ -63,7 +92,8 @@ def setup test "expiring the challenge, should retrieve nothing" do u = User.first - u.update_attribute(:otp_enabled, true) + u.populate_otp_secrets! + u.enable_otp! challenge = u.generate_otp_challenge!(1.second) sleep(2) @@ -73,7 +103,8 @@ def setup test "expired challenges should not be valid" do u = User.first - u.update_attribute(:otp_enabled, true) + u.populate_otp_secrets! + u.enable_otp! challenge = u.generate_otp_challenge!(1.second) sleep(2) assert_equal false, u.otp_challenge_valid? @@ -81,14 +112,16 @@ def setup test "null otp challenge" do u = User.first - u.update_attribute(:otp_enabled, true) + u.populate_otp_secrets! + u.enable_otp! assert_equal false, u.validate_otp_token("") assert_equal false, u.validate_otp_token(nil) end test "generated otp token should be valid for the user" do u = User.first - u.update_attribute(:otp_enabled, true) + u.populate_otp_secrets! + u.enable_otp! secret = u.otp_auth_secret token = ROTP::TOTP.new(secret).now @@ -98,7 +131,8 @@ def setup test "generated otp token, out of drift window, should be NOT valid for the user" do u = User.first - u.update_attribute(:otp_enabled, true) + u.populate_otp_secrets! + u.enable_otp! secret = u.otp_auth_secret @@ -110,7 +144,8 @@ def setup test "recovery secrets should be valid, and valid only once" do u = User.first - u.update_attribute(:otp_enabled, true) + u.populate_otp_secrets! + u.enable_otp! recovery = u.next_otp_recovery_tokens assert u.valid_otp_recovery_token? recovery.fetch(0)