Merge pull request #1951 from codidact/0valt/1119/2fa-confirmation-fix

Fix premature confirmation of sign in for users with app 2FA set up

Author: Oaphi

app/controllers/users/sessions_controller.rb

@@ -106,14 +106,17 @@ def post_sign_in(user, remember_me = false)
 
   def handle_2fa_login(user, remember_me = false)
     sign_out user
+
+    # prevents premature sign in confirmation
+    flash[:notice] = nil
+
     case user.two_factor_method
     when 'app'
       id = user.id
       @@first_factor << id
       redirect_to login_verify_2fa_path(uid: id, remember_me: remember_me)
     when 'email'
       TwoFactorMailer.with(user: user, host: request.hostname).login_email.deliver_now
-      flash[:notice] = nil
       flash[:info] = 'Please check your email inbox for a link to sign in.'
       redirect_to after_sign_in_path_for(user)
     end

test/controllers/users/sessions_controller_test.rb

@@ -4,8 +4,9 @@ class Users::SessionsControllerTest < ActionController::TestCase
   include Devise::Test::ControllerHelpers
   include ApplicationHelper
 
+  setup :set_mapping
+
   test 'should sign in with 2fa backup code' do
-    @request.env['devise.mapping'] = Devise.mappings[:user]
     Users::SessionsController.first_factor << users(:enabled_2fa).id
 
     try_verify_2fa_code(users(:enabled_2fa))
@@ -18,7 +19,6 @@ class Users::SessionsControllerTest < ActionController::TestCase
   end
 
   test 'should remember users with 2FA if requested' do
-    @request.env['devise.mapping'] = Devise.mappings[:user]
     Users::SessionsController.first_factor << users(:enabled_2fa).id
 
     try_verify_2fa_code(users(:enabled_2fa), remember_me: true)
@@ -28,9 +28,26 @@ class Users::SessionsControllerTest < ActionController::TestCase
     assert @controller.remember_me_is_active?(current_user)
   end
 
+  test 'should redirect users with code-based 2FA to code verification' do
+    pass = 'temp password for testing manual 2FA signin'
+    user = users(:enabled_2fa)
+    user.update!(password: pass)
+
+    post :create, params: { user: { email: user.email, password: pass } }
+
+    assert_response(:found)
+    assert_nil flash[:notice]
+    assert_redirected_to(login_verify_2fa_path(uid: user.id, remember_me: false))
+  end
+
   private
 
-  # @param user [User] user to very code for
+  def set_mapping
+    @request.env['devise.mapping'] = Devise.mappings[:user]
+  end
+
+  # @param user [User] user to verify code for
+  # @param opts [Hash] options hash - any additional optional params to merge in
   def try_verify_2fa_code(user, **opts)
     post :verify_code, params: { uid: user.id, code: 'M8lENyehyCvo9F9MbyTl1aOL' }.merge(opts)
   end

test/fixtures/users.yml

@@ -155,6 +155,7 @@ enabled_2fa:
   username: enabled_2fa
   confirmed_at: 2020-01-01T00:00:00.000000Z
   enabled_2fa: true
+  two_factor_method: app
   two_factor_token: WT65ANYXBB2SBR7III7IVWNJDS4PQF2T
   backup_2fa_code: M8lENyehyCvo9F9MbyTl1aOL