Merge branch 'ms-fix-mail-smtp-defaults-override' into 'master'

Prevents Mail::SMTP monkey patch from overriding defaults

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130185



Merged-by: Stan Hu <stanhu@gmail.com>
Approved-by: Heinrich Lee Yu <heinrich@gitlab.com>
Approved-by: Stan Hu <stanhu@gmail.com>
Reviewed-by: Heinrich Lee Yu <heinrich@gitlab.com>
Reviewed-by: Marc Saleiko <msaleiko@gitlab.com>
Co-authored-by: Marc Saleiko <msaleiko@gitlab.com>

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

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

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