diff --git a/app/controllers/users/passwords_controller.rb b/app/controllers/users/passwords_controller.rb index f1c2ea6a90..bdc70682cd 100644 --- a/app/controllers/users/passwords_controller.rb +++ b/app/controllers/users/passwords_controller.rb @@ -47,6 +47,12 @@ def send_password def send_password_reset_mail @reset_token = @resource.send_reset_password_instructions # generate a reset token and call devise mailer + rescue Net::ReadTimeout, Net::OpenTimeout => e + # Log the error but don't expose it to the user for security reasons + Rails.logger.error("Password reset email failed to send: #{e.class} - #{e.message}") + Bugsnag.notify(e) if defined?(Bugsnag) + # Still generate a token so SMS can potentially work + @reset_token = @resource.generate_password_reset_token end def send_password_reset_sms @@ -89,6 +95,7 @@ def empty_fields_error end def invalid_phone_number_error(error_message) + @resource ||= resource @resource.errors.add(:phone_number, error_message) false diff --git a/config/environments/production.rb b/config/environments/production.rb index 050d32d48b..64b73bb5b0 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -15,7 +15,9 @@ user_name: ENV["SENDINBLUE_EMAIL"], password: ENV["SENDINBLUE_PASSWORD"], authentication: "login", - enable_starttls_auto: true + enable_starttls_auto: true, + open_timeout: 5, # Timeout for opening connection (seconds) + read_timeout: 5 # Timeout for reading response (seconds) } # Code is not reloaded between requests. config.enable_reloading = false diff --git a/spec/requests/users/passwords_spec.rb b/spec/requests/users/passwords_spec.rb index 647f755af2..0b1b57e895 100644 --- a/spec/requests/users/passwords_spec.rb +++ b/spec/requests/users/passwords_spec.rb @@ -4,17 +4,17 @@ let!(:org) { create(:casa_org) } let!(:user) { create(:user, phone_number: "+12222222222", casa_org: org) } - let!(:twillio_service_double) { instance_double(TwilioService) } + let!(:twilio_service_double) { instance_double(TwilioService) } let!(:short_url_service_double) { instance_double(ShortUrlService) } before do allow(TwilioService).to( receive(:new).with( org - ).and_return(twillio_service_double) + ).and_return(twilio_service_double) ) - allow(twillio_service_double).to receive(:send_sms) + allow(twilio_service_double).to receive(:send_sms) allow(ShortUrlService).to receive(:new).and_return(short_url_service_double) @@ -37,14 +37,14 @@ it "sends a password reset SMS to existing user" do request - expect(twillio_service_double).to have_received(:send_sms).once.with( + expect(twilio_service_double).to have_received(:send_sms).once.with( {From: org.twilio_phone_number, Body: a_string_matching("reset_url"), To: user.phone_number} ) end it "sends a password reset email to existing user" do - expect_any_instance_of(User).to receive(:send_reset_password_instructions).once - request + expect { request }.to change { ActionMailer::Base.deliveries.count }.by(1) + expect(ActionMailer::Base.deliveries.last.to).to include(user.email) end it { is_expected.to redirect_to(user_session_url) } @@ -60,13 +60,13 @@ let(:params) { {user: {email: user.email, phone_number: ""}} } it "sends a password reset email to existing user" do - expect_any_instance_of(User).to receive(:send_reset_password_instructions).once - request + expect { request }.to change { ActionMailer::Base.deliveries.count }.by(1) + expect(ActionMailer::Base.deliveries.last.to).to include(user.email) end it "does not send sms with reset password" do request - expect(twillio_service_double).not_to have_received(:send_sms) + expect(twilio_service_double).not_to have_received(:send_sms) end end @@ -75,14 +75,13 @@ it "sends a password reset SMS to existing user" do request - expect(twillio_service_double).to have_received(:send_sms).once.with( + expect(twilio_service_double).to have_received(:send_sms).once.with( {From: org.twilio_phone_number, Body: a_string_matching("reset_url"), To: user.phone_number} ) end it "does not send email with reset password" do - expect_any_instance_of(User).not_to receive(:send_reset_password_instructions) - request + expect { request }.not_to change { ActionMailer::Base.deliveries.count } end end end @@ -96,10 +95,10 @@ end end - context "with wrong parameters" do + context "with wrong parameters (non-existent user)" do let(:params) { {user: {phone_number: "13333333333"}} } - it "sets errors correctly" do + it "does not reveal if user exists (security)" do request expect(flash[:notice]).to( eq("If the account exists you will receive an email or SMS with instructions on how to reset your password in a few minutes.") @@ -107,6 +106,24 @@ end end + context "with invalid phone number format" do + let(:params) { {user: {email: "", phone_number: "1234"}} } + + it "shows phone number validation error" do + request + expect(request.parsed_body.to_html).to include("phone_number") + end + end + + context "with non-numeric phone number" do + let(:params) { {user: {email: "", phone_number: "abc"}} } + + it "shows phone number validation error" do + request + expect(request.parsed_body.to_html).to include("phone_number") + end + end + context "when twilio is disabled" do let(:params) { {user: {email: user.email, phone_number: user.phone_number}} } @@ -115,12 +132,87 @@ end it "does not send an sms, only an email" do - expect_any_instance_of(User).to receive(:send_reset_password_instructions).once + expect { request }.to change { ActionMailer::Base.deliveries.count }.by(1) + expect(flash[:notice]).to( + eq("If the account exists you will receive an email or SMS with instructions on how to reset your password in a few minutes.") + ) + end + end + + context "when email sending times out with Net::ReadTimeout" do + let(:params) { {user: {email: user.email, phone_number: user.phone_number}} } + + before do + allow(user).to receive(:send_reset_password_instructions).and_raise(Net::ReadTimeout) + allow(User).to receive(:find_by).and_return(user) + end + + it "handles the timeout gracefully and still shows success message" do + expect(Rails.logger).to receive(:error).with(/Password reset email failed to send/) + request + expect(flash[:notice]).to( + eq("If the account exists you will receive an email or SMS with instructions on how to reset your password in a few minutes.") + ) + end + + it "does not crash the request" do + expect { request }.not_to raise_error + expect(response).to redirect_to(user_session_url) + end + + it "notifies Bugsnag of the error" do + expect(Bugsnag).to receive(:notify).with(instance_of(Net::ReadTimeout)) + request + end + + it "generates a fallback token for SMS to use" do + expect(user).to receive(:generate_password_reset_token).and_call_original + request + end + + it "still sends SMS with the fallback token" do + request + expect(twilio_service_double).to have_received(:send_sms).once + end + end + + context "when email sending times out with Net::OpenTimeout" do + let(:params) { {user: {email: user.email, phone_number: ""}} } + + before do + allow(user).to receive(:send_reset_password_instructions).and_raise(Net::OpenTimeout) + allow(User).to receive(:find_by).and_return(user) + end + + it "handles the timeout gracefully and still shows success message" do + expect(Rails.logger).to receive(:error).with(/Password reset email failed to send/) request expect(flash[:notice]).to( eq("If the account exists you will receive an email or SMS with instructions on how to reset your password in a few minutes.") ) end + + it "does not crash the request" do + expect { request }.not_to raise_error + expect(response).to redirect_to(user_session_url) + end + + it "notifies Bugsnag of the error" do + expect(Bugsnag).to receive(:notify).with(instance_of(Net::OpenTimeout)) + request + end + end + + context "when SMS sending fails" do + let(:params) { {user: {email: "", phone_number: user.phone_number}} } + + before do + allow(twilio_service_double).to receive(:send_sms).and_raise(Twilio::REST::TwilioError.new("Service unavailable")) + end + + it "raises the error (no rescue in controller)" do + expect { request }.to raise_error(Twilio::REST::TwilioError) + end end end @@ -141,12 +233,58 @@ } end - subject(:submit_reset) { put user_password_path, params: params } + subject(:request) { put user_password_path, params: params } - it "successfully resets the password" do - submit_reset - expect(response).to redirect_to(new_user_session_path) - expect(flash[:notice]).to eq("Your password has been changed successfully.") + context "with valid token and password" do + it "successfully resets the password" do + request + expect(response).to redirect_to(new_user_session_path) + expect(flash[:notice]).to eq("Your password has been changed successfully.") + end + + it "allows user to sign in with new password" do + request + user.reload + expect(user.valid_password?("newpassword123!")).to be true + end + end + + context "with password mismatch" do + let(:params) do + { + user: { + reset_password_token: token, + password: "newpassword123!", + password_confirmation: "differentpassword123!" + } + } + end + + it "does not reset the password" do + old_password_digest = user.encrypted_password + request + user.reload + expect(user.encrypted_password).to eq(old_password_digest) + end + end + + context "with password too short" do + let(:params) do + { + user: { + reset_password_token: token, + password: "abc", + password_confirmation: "abc" + } + } + end + + it "does not reset the password" do + old_password_digest = user.encrypted_password + request + user.reload + expect(user.encrypted_password).to eq(old_password_digest) + end end end end