Commit 88a061af authored by Douglas Barbosa Alexandre's avatar Douglas Barbosa Alexandre Committed by Bob Van Landuyt

Don't write to DB when using two-factor authentication via OTP

With this change we do not allow backup tokens
for admin mode in read-only replicas.
parent 16f380e3
...@@ -37,7 +37,7 @@ module Authenticates2FAForAdminMode ...@@ -37,7 +37,7 @@ module Authenticates2FAForAdminMode
# Remove any lingering user data from login # Remove any lingering user data from login
session.delete(:otp_user_id) 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 # The admin user has successfully passed 2fa, enable admin mode ignoring password
enable_admin_mode enable_admin_mode
......
...@@ -64,7 +64,9 @@ class Admin::SessionsController < ApplicationController ...@@ -64,7 +64,9 @@ class Admin::SessionsController < ApplicationController
end end
def valid_otp_attempt?(user) def valid_otp_attempt?(user)
user.validate_and_consume_otp!(user_params[:otp_attempt]) || valid_otp_attempt = user.validate_and_consume_otp!(user_params[:otp_attempt])
user.invalidate_otp_backup_code!(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
end end
...@@ -1710,6 +1710,23 @@ class User < ApplicationRecord ...@@ -1710,6 +1710,23 @@ class User < ApplicationRecord
super super
end 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 private
def default_private_profile_to_false def default_private_profile_to_false
......
---
title: Geo - Does not write to DB when using 2FA via OTP for admin mode
merge_request: 27450
author:
type: fixed
...@@ -176,6 +176,48 @@ describe Admin::SessionsController, :do_not_mock_admin_mode do ...@@ -176,6 +176,48 @@ describe Admin::SessionsController, :do_not_mock_admin_mode do
expect(controller.current_user_mode.admin_mode?).to be(true) expect(controller.current_user_mode.admin_mode?).to be(true)
end end
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 end
context 'when using two-factor authentication via U2F' do context 'when using two-factor authentication via U2F' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment