Skip to content

Change ActiveRecord::RecordInvalid to return 500, not 422 for 2FA login

Abdul Wadood requested to merge 417918-return-500-instead-of-422 into master

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

  1. Enable 2FA for a user.
  2. Use the following patch and try to log in.
  3. 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.

Related to #417918 (closed)

Merge request reports

Loading