Merge branch 'if-64257-warden_set_user_fix' into 'master'

Ensure Warden triggers after_authentication callback

See merge request gitlab-org/gitlab-ce!31138

app/controllers/concerns/authenticates_with_two_factor.rb

@@ -55,7 +55,7 @@ module AuthenticatesWithTwoFactor
 
       remember_me(user) if user_params[:remember_me] == '1'
       user.save!
-      sign_in(user, message: :two_factor_authenticated)
+      sign_in(user, message: :two_factor_authenticated, event: :authentication)
     else
       user.increment_failed_attempts!
       Gitlab::AppLogger.info("Failed Login: user=#{user.username} ip=#{request.remote_ip} method=OTP")
@@ -72,7 +72,7 @@ module AuthenticatesWithTwoFactor
       session.delete(:challenge)
 
       remember_me(user) if user_params[:remember_me] == '1'
-      sign_in(user, message: :two_factor_authenticated)
+      sign_in(user, message: :two_factor_authenticated, event: :authentication)
     else
       user.increment_failed_attempts!
       Gitlab::AppLogger.info("Failed Login: user=#{user.username} ip=#{request.remote_ip} method=U2F")

app/controllers/omniauth_callbacks_controller.rb

@@ -139,7 +139,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
       if user.two_factor_enabled? && !auth_user.bypass_two_factor?
         prompt_for_two_factor(user)
       else
-        sign_in_and_redirect(user)
+        sign_in_and_redirect(user, event: :authentication)
       end
     else
       fail_login(user)

app/controllers/sessions_controller.rb

@@ -26,6 +26,17 @@ class SessionsController < Devise::SessionsController
   after_action :log_failed_login, if: -> { action_name == 'new' && failed_login? }
   helper_method :captcha_enabled?
 
+  # protect_from_forgery is already prepended in ApplicationController but
+  # authenticate_with_two_factor which signs in the user is prepended before
+  # that here.
+  # We need to make sure CSRF token is verified before authenticating the user
+  # because Devise.clean_up_csrf_token_on_authentication is set to true by
+  # default to avoid CSRF token fixation attacks. Authenticating the user first
+  # would cause the CSRF token to be cleared and then
+  # RequestForgeryProtection#verify_authenticity_token would fail because of
+  # token mismatch.
+  protect_from_forgery with: :exception, prepend: true
+
   CAPTCHA_HEADER = 'X-GitLab-Show-Login-Captcha'.freeze
 
   def new

changelogs/unreleased/64257-warden_set_user_fix.yml

+---
+title: Ensure Warden triggers after_authentication callback
+merge_request: 31138
+author:
+type: fixed

lib/gitlab/auth/activity.rb

@@ -37,14 +37,17 @@ module Gitlab
 
       def user_authenticated!
         self.class.user_authenticated_counter_increment!
+
+        case @opts[:message]
+        when :two_factor_authenticated
+          self.class.user_two_factor_authenticated_counter_increment!
+        end
       end
 
       def user_session_override!
         self.class.user_session_override_counter_increment!
 
         case @opts[:message]
-        when :two_factor_authenticated
-          self.class.user_two_factor_authenticated_counter_increment!
         when :sessionless_sign_in
           self.class.user_sessionless_authentication_counter_increment!
         end

spec/features/oauth_login_spec.rb

@@ -34,6 +34,7 @@ describe 'OAuth Login', :js, :allow_forgery_protection do
 
       before do
         stub_omniauth_config(provider)
+        expect(ActiveSession).to receive(:cleanup).with(user).at_least(:once).and_call_original
       end
 
       context 'when two-factor authentication is disabled' do

spec/features/users/login_spec.rb

@@ -132,7 +132,6 @@ describe 'Login' do
       it 'does not show a "You are already signed in." error message' do
         expect(authentication_metrics)
           .to increment(:user_authenticated_counter)
-          .and increment(:user_session_override_counter)
           .and increment(:user_two_factor_authenticated_counter)
 
         enter_code(user.current_otp)
@@ -144,7 +143,6 @@ describe 'Login' do
         it 'allows login with valid code' do
           expect(authentication_metrics)
             .to increment(:user_authenticated_counter)
-            .and increment(:user_session_override_counter)
             .and increment(:user_two_factor_authenticated_counter)
 
           enter_code(user.current_otp)
@@ -170,7 +168,6 @@ describe 'Login' do
         it 'allows login with invalid code, then valid code' do
           expect(authentication_metrics)
             .to increment(:user_authenticated_counter)
-            .and increment(:user_session_override_counter)
             .and increment(:user_two_factor_authenticated_counter)
 
           enter_code('foo')
@@ -179,6 +176,15 @@ describe 'Login' do
           enter_code(user.current_otp)
           expect(current_path).to eq root_path
         end
+
+        it 'triggers ActiveSession.cleanup for the user' do
+          expect(authentication_metrics)
+            .to increment(:user_authenticated_counter)
+            .and increment(:user_two_factor_authenticated_counter)
+          expect(ActiveSession).to receive(:cleanup).with(user).once.and_call_original
+
+          enter_code(user.current_otp)
+        end
       end
 
       context 'using backup code' do
@@ -195,7 +201,6 @@ describe 'Login' do
           it 'allows login' do
             expect(authentication_metrics)
               .to increment(:user_authenticated_counter)
-              .and increment(:user_session_override_counter)
               .and increment(:user_two_factor_authenticated_counter)
 
             enter_code(codes.sample)
@@ -206,7 +211,6 @@ describe 'Login' do
           it 'invalidates the used code' do
             expect(authentication_metrics)
               .to increment(:user_authenticated_counter)
-              .and increment(:user_session_override_counter)
               .and increment(:user_two_factor_authenticated_counter)
 
             expect { enter_code(codes.sample) }
@@ -216,7 +220,6 @@ describe 'Login' do
           it 'invalidates backup codes twice in a row' do
             expect(authentication_metrics)
               .to increment(:user_authenticated_counter).twice
-              .and increment(:user_session_override_counter).twice
               .and increment(:user_two_factor_authenticated_counter).twice
               .and increment(:user_session_destroyed_counter)
 
@@ -230,6 +233,15 @@ describe 'Login' do
             expect { enter_code(codes.sample) }
               .to change { user.reload.otp_backup_codes.size }.by(-1)
           end
+
+          it 'triggers ActiveSession.cleanup for the user' do
+            expect(authentication_metrics)
+              .to increment(:user_authenticated_counter)
+                    .and increment(:user_two_factor_authenticated_counter)
+            expect(ActiveSession).to receive(:cleanup).with(user).once.and_call_original
+
+            enter_code(codes.sample)
+          end
         end
 
         context 'with invalid code' do
@@ -274,7 +286,7 @@ describe 'Login' do
 
           expect(authentication_metrics)
             .to increment(:user_authenticated_counter)
-            .and increment(:user_session_override_counter)
+          expect(ActiveSession).to receive(:cleanup).with(user).once.and_call_original
 
           sign_in_using_saml!
 
@@ -287,8 +299,8 @@ describe 'Login' do
         it 'shows 2FA prompt after OAuth login' do
           expect(authentication_metrics)
             .to increment(:user_authenticated_counter)
-            .and increment(:user_session_override_counter)
             .and increment(:user_two_factor_authenticated_counter)
+          expect(ActiveSession).to receive(:cleanup).with(user).once.and_call_original
 
           sign_in_using_saml!
 
@@ -329,6 +341,14 @@ describe 'Login' do
 
         expect(page).not_to have_content(I18n.t('devise.failure.already_authenticated'))
       end
+
+      it 'triggers ActiveSession.cleanup for the user' do
+        expect(authentication_metrics)
+          .to increment(:user_authenticated_counter)
+        expect(ActiveSession).to receive(:cleanup).with(user).once.and_call_original
+
+        gitlab_sign_in(user)
+      end
     end
 
     context 'with invalid username and password' do
@@ -649,7 +669,6 @@ describe 'Login' do
         it 'asks the user to accept the terms' do
           expect(authentication_metrics)
             .to increment(:user_authenticated_counter)
-            .and increment(:user_session_override_counter)
             .and increment(:user_two_factor_authenticated_counter)
 
           visit new_user_session_path
@@ -708,7 +727,6 @@ describe 'Login' do
       it 'asks the user to accept the terms before setting an email' do
         expect(authentication_metrics)
           .to increment(:user_authenticated_counter)
-          .and increment(:user_session_override_counter)
 
         gitlab_sign_in_via('saml', user, 'my-uid')