diff --git a/app/controllers/concerns/authenticates_with_two_factor.rb b/app/controllers/concerns/authenticates_with_two_factor.rb index 9fd86e6a7e0fff1ce7923737c5a223bb0fe4f135..41a3ee3e1c8ab5055aa91156e4b1ac01bd4d9841 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 a9d5da93bc55f0e2f855412bb68640d219d5ce37..67c857165fc585d90651330d8d2b46ceb0ef96a3 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 3bff9555834b3d688eebc9d7e1f52f7730bda6b1..8e069427678649af73d29630b5b4ded29ccdd001 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 })