Shift session expiry login failure to filter (#4711)

rubygems · May 16, 2024 · 8ce49fd · 8ce49fd
1 parent 81fa594
commit 8ce49fd
Show file tree

Hide file tree

Showing 2 changed files with 5 additions and 8 deletions.
diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb
@@ -30,10 +30,6 @@ def create
   end
 
   def webauthn_create
-    unless mfa_session_active?
-      login_failure(t("multifactor_auths.session_expired"))
-      return
-    end
     return login_failure(@webauthn_error) unless webauthn_credential_verified?
 
     record_mfa_login_duration(mfa_type: "webauthn")
@@ -60,8 +56,6 @@ def otp_create
       record_mfa_login_duration(mfa_type: "otp")
 
       do_login(two_factor_label: "OTP", two_factor_method: "otp", authentication_method: "password")
-    elsif !mfa_session_active?
-      login_failure(t("multifactor_auths.session_expired"))
     else
       login_failure(t("multifactor_auths.incorrect_otp"))
     end
@@ -136,7 +130,7 @@ def find_user
   end
 
   def find_mfa_user
-    @user = User.find_by(id: session[:mfa_user]) if session[:mfa_user]
+    @user = User.find_by(id: session[:mfa_user]) if mfa_session_active? && session[:mfa_user]
     return if @user
     delete_mfa_session
     login_failure t("multifactor_auths.session_expired")

diff --git a/test/functional/sessions_controller_test.rb b/test/functional/sessions_controller_test.rb
@@ -661,6 +661,8 @@ class SessionsControllerTest < ActionController::TestCase
     context "when providing credentials but the session expired" do
       setup do
         travel 30.minutes
+        @existing_webauthn = @controller.session[:webauthn_authentication]
+
         post(
           :webauthn_create,
           params: {
@@ -680,7 +682,6 @@ class SessionsControllerTest < ActionController::TestCase
         assert_nil @controller.session[:mfa_expires_at]
         assert_nil @controller.session[:mfa_login_started_at]
         assert_nil @controller.session[:mfa_user]
-        assert_nil @controller.session[:webauthn_authentication]
       end
 
       should "not sign in the user" do
@@ -693,6 +694,8 @@ class SessionsControllerTest < ActionController::TestCase
 
       should "render sign in page" do
         assert_template "sessions/new"
+        refute_nil @controller.session[:webauthn_authentication]
+        refute_equal @existing_webauthn, @controller.session[:webauthn_authentication]
       end
     end
   end