Utilize Warden Redirect

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:

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

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 %>
     <p>
-      <%= f.label :token, I18n.t('recovery_prompt', :scope => 'devise.otp.submit_token') %><br />
-      <%= f.text_field :otp_recovery_counter, :autocomplete => :off, :disabled => true, :size => 4 %>
+      <%= label_tag :token, I18n.t('recovery_prompt', :scope => 'devise.otp.submit_token') %><br />
+      <%= text_field_tag :otp_recovery_counter, resource.otp_recovery_counter, :autocomplete => :off, :disabled => true, :size => 4 %>
     </p>
   <% else %>
     <p>
-      <%= f.label :token, I18n.t('prompt', :scope => 'devise.otp.submit_token') %><br />
+      <%= label_tag :token, I18n.t('prompt', :scope => 'devise.otp.submit_token') %><br />
     </p>
   <% end %>
 
-  <%= f.text_field :token, :autocomplete => :off, :autofocus => true, :size => 6, :value => '' %><br>
+  <%= text_field_tag :token, nil, :autocomplete => :off, :autofocus => true, :size => 6 %><br>
 
   <%= label_tag :enable_persistence do %>
     <%= check_box_tag :enable_persistence, true, false %> <%= I18n.t('remember', :scope => 'devise.otp.general') %>

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"

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)

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

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

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
 

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

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

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