diff --git a/CHANGELOG.md b/CHANGELOG.md index a688d57..b7c91bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,17 @@ ## UNRELEASED +Utilize native Warden redirect for redirecting to OTP credentials form. + +Changes: +- Update DeviseAuthenticatable to redirect user (rather than login user) when OTP is enabled; +- Move "trusted browser" functionality to otp\_tokens controller; +- Unnest "challenge", "recovery", and token fields in OTP credentials form; +- Cleanup variable setters in otp\_credentials controller; +- Delete custom "sessions" hook (no longer needed); + +## UNRELEASED + Summary: Add reset token action, and hide/repurpose disable token action Details: diff --git a/app/controllers/devise_otp/devise/otp_credentials_controller.rb b/app/controllers/devise_otp/devise/otp_credentials_controller.rb index 7c2acfd..a6af9d9 100644 --- a/app/controllers/devise_otp/devise/otp_credentials_controller.rb +++ b/app/controllers/devise_otp/devise/otp_credentials_controller.rb @@ -5,45 +5,31 @@ class OtpCredentialsController < DeviseController prepend_before_action :authenticate_scope!, only: [:get_refresh, :set_refresh] prepend_before_action :require_no_authentication, only: [:show, :update] + before_action :set_challenge, only: [:show, :update] + before_action :set_recovery, only: [:show, :update] + before_action :set_resource, only: [:show, :update] + before_action :set_token, only: [:update] + before_action :skip_challenge_if_trusted_browser, only: [:show, :update] # # show a request for the OTP token # def show - @challenge = params[:challenge] - @recovery = (params[:recovery] == "true") && recovery_enabled? - - if @challenge.nil? - redirect_to new_session_path(resource_name) - else - self.resource = resource_class.find_valid_otp_challenge(@challenge) - if resource.nil? - redirect_to new_session_path(resource_name) - elsif @recovery - @recovery_count = resource.otp_recovery_counter - render :show - else - render :show - end + if @recovery + @recovery_count = resource.otp_recovery_counter end + + render :show end # # signs the resource in, if the OTP token is valid and the user has a valid challenge # def update - resource = resource_class.find_valid_otp_challenge(params[resource_name][:challenge]) - recovery = (params[resource_name][:recovery] == "true") && recovery_enabled? - token = params[resource_name][:token] - - if token.blank? + if @token.blank? otp_set_flash_message(:alert, :token_blank) - redirect_to otp_credential_path_for(resource_name, challenge: params[resource_name][:challenge], - recovery: recovery) - elsif resource.nil? - otp_set_flash_message(:alert, :otp_session_invalid) - redirect_to new_session_path(resource_name) - elsif resource.otp_challenge_valid? && resource.validate_otp_token(params[resource_name][:token], recovery) + redirect_to otp_credential_path_for(resource_name, challenge: @challenge, recovery: @recovery) + elsif resource.otp_challenge_valid? && resource.validate_otp_token(@token, @recovery) sign_in(resource_name, resource) otp_set_trusted_device_for(resource) if params[:enable_persistence] == "true" @@ -78,6 +64,39 @@ def set_refresh private + def set_challenge + @challenge = params[:challenge] + + unless @challenge.present? + redirect_to :root + end + end + + def set_recovery + @recovery = (recovery_enabled? && params[:recovery] == "true") + end + + def set_resource + self.resource = resource_class.find_valid_otp_challenge(@challenge) + + unless resource.present? + otp_set_flash_message(:alert, :otp_session_invalid) + redirect_to new_session_path(resource_name) + end + end + + def set_token + @token = params[:token] + end + + def skip_challenge_if_trusted_browser + if is_otp_trusted_browser_for?(resource) + sign_in(resource_name, resource) + otp_refresh_credentials_for(resource) + redirect_to after_sign_in_path_for(resource) + end + end + def done_valid_refresh otp_refresh_credentials_for(resource) respond_with resource, location: otp_fetch_refresh_return_url diff --git a/app/views/devise/otp_credentials/show.html.erb b/app/views/devise/otp_credentials/show.html.erb index f3566df..f51eba0 100644 --- a/app/views/devise/otp_credentials/show.html.erb +++ b/app/views/devise/otp_credentials/show.html.erb @@ -3,21 +3,21 @@ <%= form_for(resource, :as => resource_name, :url => [resource_name, :otp_credential], :html => { :method => :put, "data-turbo" => false }) do |f| %> - <%= f.hidden_field :challenge, {:value => @challenge} %> - <%= f.hidden_field :recovery, {:value => @recovery} %> + <%= hidden_field_tag :challenge, @challenge %> + <%= hidden_field_tag :recovery, @recovery %> <% if @recovery %>

- <%= f.label :token, I18n.t('recovery_prompt', :scope => 'devise.otp.submit_token') %>
- <%= f.text_field :otp_recovery_counter, :autocomplete => :off, :disabled => true, :size => 4 %> + <%= label_tag :token, I18n.t('recovery_prompt', :scope => 'devise.otp.submit_token') %>
+ <%= text_field_tag :otp_recovery_counter, resource.otp_recovery_counter, :autocomplete => :off, :disabled => true, :size => 4 %>

<% else %>

- <%= f.label :token, I18n.t('prompt', :scope => 'devise.otp.submit_token') %>
+ <%= label_tag :token, I18n.t('prompt', :scope => 'devise.otp.submit_token') %>

<% end %> - <%= f.text_field :token, :autocomplete => :off, :autofocus => true, :size => 6, :value => '' %>
+ <%= text_field_tag :token, nil, :autocomplete => :off, :autofocus => true, :size => 6 %>
<%= label_tag :enable_persistence do %> <%= check_box_tag :enable_persistence, true, false %> <%= I18n.t('remember', :scope => 'devise.otp.general') %> diff --git a/lib/devise-otp.rb b/lib/devise-otp.rb index 7b01010..2107a33 100644 --- a/lib/devise-otp.rb +++ b/lib/devise-otp.rb @@ -13,8 +13,6 @@ # define DeviseOtpAuthenticatable module, and autoload hooks and helpers # module DeviseOtpAuthenticatable - autoload :Hooks, "devise_otp_authenticatable/hooks" - module Controllers autoload :Helpers, "devise_otp_authenticatable/controllers/helpers" autoload :UrlHelpers, "devise_otp_authenticatable/controllers/url_helpers" diff --git a/lib/devise/strategies/database_authenticatable.rb b/lib/devise/strategies/database_authenticatable.rb new file mode 100644 index 0000000..df9b6d4 --- /dev/null +++ b/lib/devise/strategies/database_authenticatable.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +require 'devise/strategies/authenticatable' + +module Devise + module Strategies + # Default strategy for signing in a user, based on their email and password in the database. + class DatabaseAuthenticatable < Authenticatable + def authenticate! + resource = password.present? && mapping.to.find_for_database_authentication(authentication_hash) + hashed = false + + if validate(resource){ hashed = true; resource.valid_password?(password) } + if otp_challenge_required_on?(resource) + # Redirect to challenge + challenge = resource.generate_otp_challenge! + redirect!(otp_challenge_url, {:challenge => challenge}) + else + # Sign in user as usual + remember_me(resource) + resource.after_database_authentication + success!(resource) + end + end + + # In paranoid mode, hash the password even when a resource doesn't exist for the given authentication key. + # This is necessary to prevent enumeration attacks - e.g. the request is faster when a resource doesn't + # exist in the database if the password hashing algorithm is not called. + mapping.to.new.password = password if !hashed && Devise.paranoid + unless resource + Devise.paranoid ? fail(:invalid) : fail(:not_found_in_database) + end + end + + private + + # + # resource should be challenged for otp + # + def otp_challenge_required_on?(resource) + resource.respond_to?(:otp_enabled?) && resource.otp_enabled? + end + + def otp_challenge_url + if Rails.env.development? + host = "#{request.host}:#{request.port}" + else + host = "#{request.host}" + end + + path_fragments = ["otp", mapping.path_names[:credentials]] + if mapping.fullpath == "/" + path = mapping.fullpath + path_fragments.join("/") + else + path = path_fragments.prepend(mapping.fullpath).join("/") + end + + request.protocol + host + path + end + end + end +end + +Warden::Strategies.add(:database_authenticatable, Devise::Strategies::DatabaseAuthenticatable) diff --git a/lib/devise_otp_authenticatable/engine.rb b/lib/devise_otp_authenticatable/engine.rb index 6b9f168..e3ab152 100644 --- a/lib/devise_otp_authenticatable/engine.rb +++ b/lib/devise_otp_authenticatable/engine.rb @@ -3,11 +3,6 @@ class Engine < ::Rails::Engine config.devise_otp = ActiveSupport::OrderedOptions.new config.devise_otp.precompile_assets = true - # We use to_prepare instead of after_initialize here because Devise is a Rails engine; - config.to_prepare do - DeviseOtpAuthenticatable::Hooks.apply - end - initializer "devise-otp", group: :all do |app| ActiveSupport.on_load(:devise_controller) do include DeviseOtpAuthenticatable::Controllers::UrlHelpers diff --git a/lib/devise_otp_authenticatable/hooks.rb b/lib/devise_otp_authenticatable/hooks.rb deleted file mode 100644 index 1ff9bd7..0000000 --- a/lib/devise_otp_authenticatable/hooks.rb +++ /dev/null @@ -1,11 +0,0 @@ -module DeviseOtpAuthenticatable - module Hooks - autoload :Sessions, "devise_otp_authenticatable/hooks/sessions.rb" - - class << self - def apply - ::Devise::SessionsController.send(:include, Hooks::Sessions) - end - end - end -end diff --git a/lib/devise_otp_authenticatable/hooks/sessions.rb b/lib/devise_otp_authenticatable/hooks/sessions.rb deleted file mode 100644 index d983ea6..0000000 --- a/lib/devise_otp_authenticatable/hooks/sessions.rb +++ /dev/null @@ -1,43 +0,0 @@ -module DeviseOtpAuthenticatable::Hooks - module Sessions - extend ActiveSupport::Concern - include DeviseOtpAuthenticatable::Controllers::UrlHelpers - - included do - alias_method :create, :create_with_otp - end - - # - # replaces Devise::SessionsController#create - # - def create_with_otp - resource = warden.authenticate!(auth_options) - - devise_stored_location = stored_location_for(resource) # Grab the current stored location before it gets lost by warden.logout - store_location_for(resource, devise_stored_location) # Restore it since #stored_location_for removes it - - yield resource if block_given? - if otp_challenge_required_on?(resource) - challenge = resource.generate_otp_challenge! - warden.logout - store_location_for(resource, devise_stored_location) # restore the stored location - respond_with resource, location: otp_credential_path_for(resource, {challenge: challenge}) - else - sign_in(resource_name, resource) - respond_with resource, location: after_sign_in_path_for(resource) - end - end - - private - - # - # resource should be challenged for otp - # - def otp_challenge_required_on?(resource) - return false unless resource.respond_to?(:otp_enabled) && resource.respond_to?(:otp_auth_secret) - - resource.otp_enabled && !is_otp_trusted_browser_for?(resource) - end - - end -end diff --git a/test/integration/disable_token_test.rb b/test/integration/disable_token_test.rb index ff3efbe..9e51e3d 100644 --- a/test/integration/disable_token_test.rb +++ b/test/integration/disable_token_test.rb @@ -9,7 +9,7 @@ def setup 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) + fill_in "token", with: ROTP::TOTP.new(@user.otp_auth_secret).at(Time.now) click_button "Submit Token" assert_equal root_path, current_path end diff --git a/test/integration/refresh_test.rb b/test/integration/refresh_test.rb index eedac6c..4a769b7 100644 --- a/test/integration/refresh_test.rb +++ b/test/integration/refresh_test.rb @@ -89,7 +89,7 @@ def teardown assert_equal admin_otp_credential_path, current_path - fill_in "admin_token", with: ROTP::TOTP.new(admin.otp_auth_secret).at(Time.now) + fill_in "token", with: ROTP::TOTP.new(admin.otp_auth_secret).at(Time.now) click_button "Submit Token" assert_equal "/", current_path diff --git a/test/integration/reset_token_test.rb b/test/integration/reset_token_test.rb index a6eba69..c51cf82 100644 --- a/test/integration/reset_token_test.rb +++ b/test/integration/reset_token_test.rb @@ -9,7 +9,7 @@ def setup 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) + fill_in "token", with: ROTP::TOTP.new(@user.otp_auth_secret).at(Time.now) click_button "Submit Token" assert_equal root_path, current_path end diff --git a/test/integration/sign_in_test.rb b/test/integration/sign_in_test.rb index 7f2f2e5..a7985b0 100644 --- a/test/integration/sign_in_test.rb +++ b/test/integration/sign_in_test.rb @@ -39,7 +39,7 @@ def teardown enable_otp_and_sign_in assert_equal user_otp_credential_path, current_path - fill_in "user_token", with: "123456" + fill_in "token", with: "123456" click_button "Submit Token" assert_equal new_user_session_path, current_path @@ -49,7 +49,7 @@ def teardown enable_otp_and_sign_in assert_equal user_otp_credential_path, current_path - fill_in "user_token", with: "" + fill_in "token", with: "" click_button "Submit Token" assert_equal user_otp_credential_path, current_path @@ -58,7 +58,7 @@ def teardown test "successful token authentication" do user = enable_otp_and_sign_in - fill_in "user_token", with: ROTP::TOTP.new(user.otp_auth_secret).at(Time.now) + fill_in "token", with: ROTP::TOTP.new(user.otp_auth_secret).at(Time.now) click_button "Submit Token" assert_equal root_path, current_path @@ -72,7 +72,7 @@ def teardown sleep(2) - fill_in "user_token", with: ROTP::TOTP.new(user.otp_auth_secret).at(Time.now) + fill_in "token", with: ROTP::TOTP.new(user.otp_auth_secret).at(Time.now) click_button "Submit Token" User.otp_authentication_timeout = old_timeout diff --git a/test/integration/trackable_test.rb b/test/integration/trackable_test.rb new file mode 100644 index 0000000..31e6ed6 --- /dev/null +++ b/test/integration/trackable_test.rb @@ -0,0 +1,50 @@ +require "test_helper" +require "integration_tests_helper" + +class TrackableTest < ActionDispatch::IntegrationTest + + def setup + @user = sign_user_in + + @user.reload + + @sign_in_count = @user.sign_in_count + @current_sign_in_at = @user.current_sign_in_at + + sign_out + end + + def teardown + Capybara.reset_sessions! + end + + test "if otp is disabled, it should update devise trackable fields as usual when the user signs in" do + sign_user_in(@user) + + @user.reload + + assert_not_equal @sign_in_count, @user.sign_in_count + assert_not_equal @current_sign_in_at, @user.current_sign_in_at + end + + test "if otp is enabled, it should not update devise trackable fields until user enters their user token to complete their sign in" do + @user.populate_otp_secrets! + @user.enable_otp! + + sign_user_in(@user) + + @user.reload + + assert_equal @sign_in_count, @user.sign_in_count + assert_equal @current_sign_in_at, @user.current_sign_in_at + + fill_in "token", with: ROTP::TOTP.new(@user.otp_auth_secret).at(Time.now) + click_button "Submit Token" + + @user.reload + + assert_not_equal @sign_in_count, @user.sign_in_count + assert_not_equal @current_sign_in_at, @user.current_sign_in_at + end + +end diff --git a/test/integration_tests_helper.rb b/test/integration_tests_helper.rb index b11f47b..a1f84e6 100644 --- a/test/integration_tests_helper.rb +++ b/test/integration_tests_helper.rb @@ -29,7 +29,7 @@ def create_full_admin def enable_otp_and_sign_in_with_otp enable_otp_and_sign_in.tap do |user| - fill_in "user_token", with: ROTP::TOTP.new(user.otp_auth_secret).at(Time.now) + fill_in "token", with: ROTP::TOTP.new(user.otp_auth_secret).at(Time.now) click_button "Submit Token" end end @@ -50,7 +50,7 @@ def enable_otp_and_sign_in end def otp_challenge_for(user) - fill_in "user_token", with: ROTP::TOTP.new(user.otp_auth_secret).at(Time.now) + fill_in "token", with: ROTP::TOTP.new(user.otp_auth_secret).at(Time.now) click_button "Submit Token" end