diff --git a/app/controllers/users/passwords_controller.rb b/app/controllers/users/passwords_controller.rb index bdc70682cd..f1c2ea6a90 100644 --- a/app/controllers/users/passwords_controller.rb +++ b/app/controllers/users/passwords_controller.rb @@ -47,12 +47,6 @@ 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 @@ -95,7 +89,6 @@ 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 64b73bb5b0..050d32d48b 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -15,9 +15,7 @@ user_name: ENV["SENDINBLUE_EMAIL"], password: ENV["SENDINBLUE_PASSWORD"], authentication: "login", - enable_starttls_auto: true, - open_timeout: 5, # Timeout for opening connection (seconds) - read_timeout: 5 # Timeout for reading response (seconds) + enable_starttls_auto: true } # 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 0b1b57e895..647f755af2 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!(:twilio_service_double) { instance_double(TwilioService) } + let!(:twillio_service_double) { instance_double(TwilioService) } let!(:short_url_service_double) { instance_double(ShortUrlService) } before do allow(TwilioService).to( receive(:new).with( org - ).and_return(twilio_service_double) + ).and_return(twillio_service_double) ) - allow(twilio_service_double).to receive(:send_sms) + allow(twillio_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(twilio_service_double).to have_received(:send_sms).once.with( + expect(twillio_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 { request }.to change { ActionMailer::Base.deliveries.count }.by(1) - expect(ActionMailer::Base.deliveries.last.to).to include(user.email) + expect_any_instance_of(User).to receive(:send_reset_password_instructions).once + request 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 { request }.to change { ActionMailer::Base.deliveries.count }.by(1) - expect(ActionMailer::Base.deliveries.last.to).to include(user.email) + expect_any_instance_of(User).to receive(:send_reset_password_instructions).once + request end it "does not send sms with reset password" do request - expect(twilio_service_double).not_to have_received(:send_sms) + expect(twillio_service_double).not_to have_received(:send_sms) end end @@ -75,13 +75,14 @@ it "sends a password reset SMS to existing user" do request - expect(twilio_service_double).to have_received(:send_sms).once.with( + expect(twillio_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 { request }.not_to change { ActionMailer::Base.deliveries.count } + expect_any_instance_of(User).not_to receive(:send_reset_password_instructions) + request end end end @@ -95,10 +96,10 @@ end end - context "with wrong parameters (non-existent user)" do + context "with wrong parameters" do let(:params) { {user: {phone_number: "13333333333"}} } - it "does not reveal if user exists (security)" do + it "sets errors correctly" 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.") @@ -106,24 +107,6 @@ 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}} } @@ -132,87 +115,12 @@ end it "does not send an sms, only an email" do - 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/) + expect_any_instance_of(User).to receive(:send_reset_password_instructions).once 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 @@ -233,58 +141,12 @@ } end - subject(:request) { put user_password_path, params: params } + subject(:submit_reset) { put user_password_path, params: params } - 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 + 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.") end end end