diff --git a/config/initializers/mail_starttls_patch.rb b/config/initializers/mail_starttls_patch.rb index c4fe102edfb72d7a2a055d28b208ab33af423344..51d454238cabe5c30d594007c96b8cfd551990a5 100644 --- a/config/initializers/mail_starttls_patch.rb +++ b/config/initializers/mail_starttls_patch.rb @@ -9,7 +9,7 @@ module Mail class SMTP def initialize(values) - self.settings = DEFAULTS + self.settings = DEFAULTS.dup settings[:enable_starttls_auto] = nil settings.merge!(values) end diff --git a/spec/initializers/mail_starttls_patch_spec.rb b/spec/initializers/mail_starttls_patch_spec.rb index 99c8edddd125d5eae2119d538544254b53844ae6..0ceeb78a5b82bd60ec784d0ec8af14153c0dbca2 100644 --- a/spec/initializers/mail_starttls_patch_spec.rb +++ b/spec/initializers/mail_starttls_patch_spec.rb @@ -17,6 +17,37 @@ end end + # As long as this monkey patch exists and overrides the constructor + # we should test that the defaults of Mail::SMTP are not overriden. + # + # @see issue https://gitlab.com/gitlab-org/gitlab/-/issues/423268 + # @see incident https://gitlab.com/gitlab-com/gl-infra/production/-/issues/16223 + it 'does not override default constants values' do + expected_settings = Mail::SMTP.new({}).settings.dup + + Mail.new.delivery_method(Mail::SMTP, { user_name: 'user@example.com' }) + + expect(Mail::SMTP.new({}).settings).to eq(expected_settings) + end + + describe 'enable_starttls_auto setting' do + let(:settings) { {} } + + subject(:smtp) { Mail::SMTP.new(settings) } + + it 'uses default for enable_starttls_auto' do + expect(smtp.settings).to include(enable_starttls_auto: nil) + end + + context 'when set to false' do + let(:settings) { { enable_starttls_auto: false } } + + it 'overrides default and sets value' do + expect(smtp.settings).to include(enable_starttls_auto: false) + end + end + end + # Taken from https://github.com/mikel/mail/pull/1536#issue-1490438378 where(:ssl, :tls, :enable_starttls, :enable_starttls_auto, :smtp_tls, :smtp_starttls_mode) do true | nil | nil | nil | true | false diff --git a/spec/mailers/emails/service_desk_spec.rb b/spec/mailers/emails/service_desk_spec.rb index e7775022e9a5564e61b64a945a050a83e018bd0b..ae676fcf8b1932de61b2f825ef717abda1cee506 100644 --- a/spec/mailers/emails/service_desk_spec.rb +++ b/spec/mailers/emails/service_desk_spec.rb @@ -438,6 +438,18 @@ end describe '.service_desk_custom_email_verification_email' do + # Use strict definition here because Mail::SMTP.new({}).settings + # might have been changed before. + let(:expected_delivery_method_defaults) do + { + address: 'localhost', + domain: 'localhost.localdomain', + port: 25, + password: nil, + user_name: nil + } + end + subject { Notify.service_desk_custom_email_verification_email(service_desk_setting) } it_behaves_like 'a custom email verification process email' @@ -448,6 +460,13 @@ it 'forcibly uses SMTP delivery method and has correct settings' do expect_service_desk_custom_email_delivery_options(service_desk_setting) + + # defaults are unchanged after email overrode settings + expect(Mail::SMTP.new({}).settings).to include(expected_delivery_method_defaults) + + # other mailers are unchanged after email overrode settings + other_mail = Notify.test_email(email, 'Test subject', 'Test body') + expect(other_mail.delivery_method).to be_a(Mail::TestMailer) end it 'uses verification email address as recipient' do