From 805e350b20348837068593ff33c8a961683a3c5b Mon Sep 17 00:00:00 2001
From: Abdul Wadood <awadood@gitlab.com>
Date: Tue, 25 Jul 2023 08:50:59 +0000
Subject: [PATCH] Change ActiveRecord::RecordInvalid to return 500, not 422 for
2FA login
This is a follow-up to a production incident
See https://gitlab.com/gitlab-com/gl-infra/production/-/issues/16022#note_1467672642.
The incident happened because some new validations were added that
were preventing existing user records from getting updated.
We save the user when doing 2FA sign-in so users not passing the
validations were unable to sign in.
ActiveRecord::RecordInvalid caused the SessionsController to return
422 but we have changed that to 500 to improve observability/alerting.
Changelog: other
---
.../concerns/authenticates_with_two_factor.rb | 8 +++++
spec/factories/users.rb | 5 ++++
spec/requests/sessions_spec.rb | 29 +++++++++++++++++++
3 files changed, 42 insertions(+)
diff --git a/app/controllers/concerns/authenticates_with_two_factor.rb b/app/controllers/concerns/authenticates_with_two_factor.rb
index 9fd86e6a7e0ff..41a3ee3e1c8ab 100644
--- a/app/controllers/concerns/authenticates_with_two_factor.rb
+++ b/app/controllers/concerns/authenticates_with_two_factor.rb
@@ -52,6 +52,14 @@ def authenticate_with_two_factor
elsif user && user.valid_password?(user_params[:password])
prompt_for_two_factor(user)
end
+ rescue ActiveRecord::RecordInvalid => e
+ # We expect User to always be valid.
+ # Otherwise, raise internal server error instead of unprocessable entity to improve observability/alerting
+ if e.record.is_a?(User)
+ raise e.message
+ else
+ raise e
+ end
end
private
diff --git a/spec/factories/users.rb b/spec/factories/users.rb
index a9d5da93bc55f..67c857165fc58 100644
--- a/spec/factories/users.rb
+++ b/spec/factories/users.rb
@@ -148,6 +148,11 @@
end
end
+ trait :invalid do
+ first_name { 'A' * 130 } # Exceed `first_name` character limit in model to make it invalid
+ to_create { |user| user.save!(validate: false) }
+ end
+
transient do
developer_projects { [] }
maintainer_projects { [] }
diff --git a/spec/requests/sessions_spec.rb b/spec/requests/sessions_spec.rb
index 3bff9555834b3..8e06942767864 100644
--- a/spec/requests/sessions_spec.rb
+++ b/spec/requests/sessions_spec.rb
@@ -38,6 +38,35 @@
end
end
+ context 'when using two-factor authentication via OTP' do
+ let(:user) { create(:user, :two_factor, :invalid) }
+ let(:user_params) { { login: user.username, password: user.password } }
+
+ def authenticate_2fa(otp_attempt:)
+ post(user_session_path(params: { user: user_params })) # First sign-in request for password, second for OTP
+ post(user_session_path(params: { user: user_params.merge(otp_attempt: otp_attempt) }))
+ end
+
+ context 'with an invalid user' do
+ it 'raises StandardError when ActiveRecord::RecordInvalid is raised to return 500 instead of 422' do
+ otp = user.current_otp
+
+ expect { authenticate_2fa(otp_attempt: otp) }.to raise_error(StandardError)
+ end
+ end
+
+ context 'with an invalid record other than user' do
+ it 'raises ActiveRecord::RecordInvalid for invalid record to return 422f' do
+ otp = user.current_otp
+ allow_next_instance_of(ActiveRecord::RecordInvalid) do |instance|
+ allow(instance).to receive(:record).and_return(nil) # Simulate it's not a user
+ end
+
+ expect { authenticate_2fa(otp_attempt: otp) }.to raise_error(ActiveRecord::RecordInvalid)
+ end
+ end
+ end
+
context 'when user signs out' do
before do
post user_session_path(user: { login: user.username, password: user.password })
--
GitLab