Merge branch...

Merge branch '211456-admin-sessionscontroller-create-cannot-execute-update-in-a-read-only-transaction' into 'master'

Does not write to DB when using 2FA via OTP for admin mode

Closes #211456

See merge request gitlab-org/gitlab!27450

app/controllers/admin/concerns/authenticates_2fa_for_admin_mode.rb

@@ -37,7 +37,7 @@ module Authenticates2FAForAdminMode
       # Remove any lingering user data from login
       session.delete(:otp_user_id)
 
-      user.save!
+      user.save! unless Gitlab::Database.read_only?
 
       # The admin user has successfully passed 2fa, enable admin mode ignoring password
       enable_admin_mode

app/controllers/admin/sessions_controller.rb

@@ -64,7 +64,9 @@ class Admin::SessionsController < ApplicationController
   end
 
   def valid_otp_attempt?(user)
-    user.validate_and_consume_otp!(user_params[:otp_attempt]) ||
-        user.invalidate_otp_backup_code!(user_params[:otp_attempt])
+    valid_otp_attempt = user.validate_and_consume_otp!(user_params[:otp_attempt])
+    return valid_otp_attempt if Gitlab::Database.read_only?
+
+    valid_otp_attempt || user.invalidate_otp_backup_code!(user_params[:otp_attempt])
   end
 end

app/models/user.rb

@@ -1715,6 +1715,23 @@ class User < ApplicationRecord
     super
   end
 
+  # This is copied from Devise::Models::TwoFactorAuthenticatable#consume_otp!
+  #
+  # An OTP cannot be used more than once in a given timestep
+  # Storing timestep of last valid OTP is sufficient to satisfy this requirement
+  #
+  # See:
+  #   <https://github.com/tinfoil/devise-two-factor/blob/master/lib/devise_two_factor/models/two_factor_authenticatable.rb#L66>
+  #
+  def consume_otp!
+    if self.consumed_timestep != current_otp_timestep
+      self.consumed_timestep = current_otp_timestep
+      return Gitlab::Database.read_only? ? true : save(validate: false)
+    end
+
+    false
+  end
+
   private
 
   def default_private_profile_to_false

ee/changelogs/unreleased/211456-admin-sessionscontroller-create-cannot-execute-update-in-a-read.yml

+---
+title: Geo - Does not write to DB when using 2FA via OTP for admin mode
+merge_request: 27450
+author:
+type: fixed

spec/controllers/admin/sessions_controller_spec.rb

@@ -176,6 +176,48 @@ describe Admin::SessionsController, :do_not_mock_admin_mode do
             expect(controller.current_user_mode.admin_mode?).to be(true)
           end
         end
+
+        context 'on a read-only instance' do
+          before do
+            allow(Gitlab::Database).to receive(:read_only?).and_return(true)
+          end
+
+          it 'does not attempt to write to the database with valid otp' do
+            expect_any_instance_of(User).not_to receive(:save)
+            expect_any_instance_of(User).not_to receive(:save!)
+
+            controller.store_location_for(:redirect, admin_root_path)
+            controller.current_user_mode.request_admin_mode!
+
+            authenticate_2fa(otp_attempt: user.current_otp)
+
+            expect(response).to redirect_to admin_root_path
+          end
+
+          it 'does not attempt to write to the database with invalid otp' do
+            expect_any_instance_of(User).not_to receive(:save)
+            expect_any_instance_of(User).not_to receive(:save!)
+
+            controller.current_user_mode.request_admin_mode!
+
+            authenticate_2fa(otp_attempt: 'invalid')
+
+            expect(response).to render_template('admin/sessions/two_factor')
+            expect(controller.current_user_mode.admin_mode?).to be(false)
+          end
+
+          it 'does not attempt to write to the database with backup code' do
+            expect_any_instance_of(User).not_to receive(:save)
+            expect_any_instance_of(User).not_to receive(:save!)
+
+            controller.current_user_mode.request_admin_mode!
+
+            authenticate_2fa(otp_attempt: user.otp_backup_codes.first)
+
+            expect(response).to render_template('admin/sessions/two_factor')
+            expect(controller.current_user_mode.admin_mode?).to be(false)
+          end
+        end
       end
 
       context 'when using two-factor authentication via U2F' do