Skip to content
代码片段 群组 项目
提交 805e350b 编辑于 作者: Abdul Wadood's avatar Abdul Wadood 提交者: Roy Zwambag
浏览文件

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
上级 2d182a35
No related branches found
No related tags found
无相关合并请求
......@@ -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
......
......@@ -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 { [] }
......
......@@ -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 })
......
0% 加载中 .
You are about to add 0 people to the discussion. Proceed with caution.
先完成此消息的编辑!
想要评论请 注册