FIX: Use login SMTP auth for office365 in group mailer (#27931)

Followup 7b627dc14b

In this other commit, I changed the email settings validator
to always use the `login` authentication method for
office365 and outlook, but I didn't change the actual
group SMTP mailer to do this.

This commit fixes that issue and does some minor refactoring.

app/mailers/group_smtp_mailer.rb

@@ -15,7 +15,9 @@ class GroupSmtpMailer < ActionMailer::Base
       domain: from_group.email_username_domain,
       user_name: from_group.email_username,
       password: from_group.email_password,
-      authentication: GlobalSetting.smtp_authentication,
+      # NOTE: Might be better at some point to store this authentication method in the database
+      # against the group.
+      authentication: SmtpProviderOverrides.authentication_override(from_group.smtp_server),
       enable_starttls_auto: from_group.smtp_ssl,
       return_response: true,
       open_timeout: GlobalSetting.group_smtp_open_timeout,

app/services/email_settings_validator.rb

@@ -84,15 +84,15 @@ class EmailSettingsValidator
   )
     begin
       port, enable_tls, enable_starttls_auto =
-        provider_specific_ssl_overrides(host, port, enable_tls, enable_starttls_auto)
+        SmtpProviderOverrides.ssl_override(host, port, enable_tls, enable_starttls_auto)
 
       if enable_tls && enable_starttls_auto
         raise ArgumentError, "TLS and STARTTLS are mutually exclusive"
       end
 
       if username || password
-        authentication = provider_specific_authentication_overrides(host) if authentication.nil?
-        authentication = (authentication || GlobalSetting.smtp_authentication)&.to_sym
+        authentication = SmtpProviderOverrides.authentication_override(host) if authentication.nil?
+        authentication = authentication.to_sym
         if !%i[plain login cram_md5].include?(authentication)
           raise ArgumentError, "Invalid authentication method. Must be plain, login, or cram_md5."
         end
@@ -180,29 +180,4 @@ class EmailSettingsValidator
     end
     raise err
   end
-
-  # Ideally we (or net-smtp) would automatically detect the correct authentication
-  # method, but this is sufficient for our purposes because we know certain providers
-  # need certain authentication methods. This may need to change when we start to
-  # use XOAUTH2 for SMTP.
-  def self.provider_specific_authentication_overrides(host)
-    return :login if %w[smtp.office365.com smtp-mail.outlook.com].include?(host)
-    :plain
-  end
-
-  def self.provider_specific_ssl_overrides(host, port, enable_tls, enable_starttls_auto)
-    # Certain mail servers act weirdly if you do not use the correct combinations of
-    # TLS settings based on the port, we clean these up here for the user.
-    if %w[smtp.gmail.com smtp.office365.com smtp-mail.outlook.com].include?(host)
-      if port.to_i == 587
-        enable_starttls_auto = true
-        enable_tls = false
-      elsif port.to_i == 465
-        enable_starttls_auto = false
-        enable_tls = true
-      end
-    end
-
-    [port, enable_tls, enable_starttls_auto]
-  end
 end

lib/smtp_provider_overrides.rb

@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+class SmtpProviderOverrides
+  # Ideally we (or net-smtp) would automatically detect the correct authentication
+  # method, but this is sufficient for our purposes because we know certain providers
+  # need certain authentication methods. This may need to change when we start to
+  # use XOAUTH2 for SMTP.
+  def self.authentication_override(host)
+    return "login" if %w[smtp.office365.com smtp-mail.outlook.com].include?(host)
+    GlobalSetting.smtp_authentication
+  end
+
+  def self.ssl_override(host, port, enable_tls, enable_starttls_auto)
+    # Certain mail servers act weirdly if you do not use the correct combinations of
+    # TLS settings based on the port, we clean these up here for the user.
+    if %w[smtp.gmail.com smtp.office365.com smtp-mail.outlook.com].include?(host)
+      if port.to_i == 587
+        enable_starttls_auto = true
+        enable_tls = false
+      elsif port.to_i == 465
+        enable_starttls_auto = false
+        enable_tls = true
+      end
+    end
+
+    [port, enable_tls, enable_starttls_auto]
+  end
+end

spec/mailers/group_smtp_mailer_spec.rb

@@ -135,6 +135,18 @@ RSpec.describe GroupSmtpMailer do
     )
   end
 
+  it "uses the login SMTP authentication method for office365" do
+    group.update!(smtp_server: "smtp.office365.com")
+    mail = GroupSmtpMailer.send_mail(group, user.email, Fabricate(:post))
+    expect(mail.delivery_method.settings[:authentication]).to eq("login")
+  end
+
+  it "uses the login SMTP authentication method for outlook" do
+    group.update!(smtp_server: "smtp-mail.outlook.com")
+    mail = GroupSmtpMailer.send_mail(group, user.email, Fabricate(:post))
+    expect(mail.delivery_method.settings[:authentication]).to eq("login")
+  end
+
   context "when the site has a reply by email address configured" do
     before do
       SiteSetting.manual_polling_enabled = true