Change ActiveRecord::RecordInvalid to return 500, not 422 for 2FA login
What does this MR do and why?
Change ActiveRecord::RecordInvalid to return 500, not 422
This is a follow-up to a production incident gitlab-com/gl-infra/production#16022 (comment 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.
How to set up and validate locally
- Enable 2FA for a user.
- Use the following patch and try to log in.
- Before the changes you should get 422 in log/development.log after putting the OTP and after the changes, you should get 500.
PATCH
diff --git a/app/controllers/concerns/authenticates_with_two_factor.rb b/app/controllers/concerns/authenticates_with_two_factor.rb
index 9fd86e6a7e0f..fb315e4be1dd 100644
--- a/app/controllers/concerns/authenticates_with_two_factor.rb
+++ b/app/controllers/concerns/authenticates_with_two_factor.rb
@@ -78,6 +78,7 @@ def authenticate_with_two_factor_via_otp(user)
clear_two_factor_attempt!
remember_me(user) if user_params[:remember_me] == '1'
+ raise ActiveRecord::RecordInvalid
user.save!
sign_in(user, message: :two_factor_authenticated, event: :authentication)
else
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Related to #417918 (closed)