Invalidate two factor sign-in when user password changes

Currently, when a user's password changes between signing in with their
password and providing the 2FA code/U2F GitLab successfully allows the
user to sign-in. GitLab should invalidate the sign-in process when
this happens. This change invalidates the sign-in process when the
user updated_at attribute changes between the two phases of sign-in.

app/controllers/concerns/authenticates_with_two_factor.rb

@@ -22,6 +22,8 @@ module AuthenticatesWithTwoFactor
     return handle_locked_user(user) unless user.can?(:log_in)
 
     session[:otp_user_id] = user.id
+    session[:user_updated_at] = user.updated_at
+
     setup_u2f_authentication(user)
     render 'devise/sessions/two_factor'
   end
@@ -39,6 +41,7 @@ module AuthenticatesWithTwoFactor
   def authenticate_with_two_factor
     user = self.resource = find_user
     return handle_locked_user(user) unless user.can?(:log_in)
+    return handle_changed_user(user) if user_changed?(user)
 
     if user_params[:otp_attempt].present? && session[:otp_user_id]
       authenticate_with_two_factor_via_otp(user)
@@ -63,12 +66,14 @@ module AuthenticatesWithTwoFactor
 
   def clear_two_factor_attempt!
     session.delete(:otp_user_id)
+    session.delete(:user_updated_at)
+    session.delete(:challenge)
   end
 
   def authenticate_with_two_factor_via_otp(user)
     if valid_otp_attempt?(user)
       # Remove any lingering user data from login
-      session.delete(:otp_user_id)
+      clear_two_factor_attempt!
 
       remember_me(user) if user_params[:remember_me] == '1'
       user.save!
@@ -85,8 +90,7 @@ module AuthenticatesWithTwoFactor
   def authenticate_with_two_factor_via_u2f(user)
     if U2fRegistration.authenticate(user, u2f_app_id, user_params[:device_response], session[:challenge])
       # Remove any lingering user data from login
-      session.delete(:otp_user_id)
-      session.delete(:challenge)
+      clear_two_factor_attempt!
 
       remember_me(user) if user_params[:remember_me] == '1'
       sign_in(user, message: :two_factor_authenticated, event: :authentication)
@@ -113,4 +117,18 @@ module AuthenticatesWithTwoFactor
     end
   end
   # rubocop: enable CodeReuse/ActiveRecord
+
+  def handle_changed_user(user)
+    clear_two_factor_attempt!
+
+    redirect_to new_user_session_path, alert: _('An error occurred. Please sign in again.')
+  end
+
+  # If user has been updated since we validated the password,
+  # the password might have changed.
+  def user_changed?(user)
+    return false unless session[:user_updated_at]
+
+    user.updated_at != session[:user_updated_at]
+  end
 end

changelogs/unreleased/security-214-dblessing-revoke-session-on-pw-change.yml

+---
+title: Invalidate two factor sign-in when user password changes
+merge_request:
+author:
+type: security

locale/gitlab.pot

@@ -2867,6 +2867,9 @@ msgstr ""
 msgid "An error occurred while validating username"
 msgstr ""
 
+msgid "An error occurred. Please sign in again."
+msgstr ""
+
 msgid "An error occurred. Please try again."
 msgstr ""
 

spec/features/users/login_spec.rb

@@ -177,6 +177,14 @@ RSpec.describe 'Login' do
         expect(page).not_to have_content(I18n.t('devise.failure.already_authenticated'))
       end
 
+      it 'does not allow sign-in if the user password is updated before entering a one-time code' do
+        user.update!(password: 'new_password')
+
+        enter_code(user.current_otp)
+
+        expect(page).to have_content('An error occurred. Please sign in again.')
+      end
+
       context 'using one-time code' do
         it 'allows login with valid code' do
           expect(authentication_metrics)
@@ -232,7 +240,7 @@ RSpec.describe 'Login' do
           expect(codes.size).to eq 10
 
           # Ensure the generated codes get saved
-          user.save
+          user.save(touch: false)
         end
 
         context 'with valid code' do
@@ -290,7 +298,7 @@ RSpec.describe 'Login' do
             code = codes.sample
             expect(user.invalidate_otp_backup_code!(code)).to eq true
 
-            user.save!
+            user.save!(touch: false)
             expect(user.reload.otp_backup_codes.size).to eq 9
 
             enter_code(code)