From 7b627dc14b934430f62a59fbcf09b0595ee94567 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Thu, 11 Jul 2024 16:16:54 +1000
Subject: [PATCH] FIX: Office365/Outlook auth method for group SMTP (#27854)

Both office365 and outlook SMTP servers need LOGIN
SMTP authentication instead of PLAIN (which is what
we are using by default). This commit uses that
unconditionally for these servers, and also makes
sure to use STARTTLS for them too.
---
 app/services/email_settings_validator.rb      | 20 ++++++--
 .../services/email_settings_validator_spec.rb | 46 +++++++++++++++++++
 2 files changed, 63 insertions(+), 3 deletions(-)

diff --git a/app/services/email_settings_validator.rb b/app/services/email_settings_validator.rb
index 0119d1d977b..c921b155af9 100644
--- a/app/services/email_settings_validator.rb
+++ b/app/services/email_settings_validator.rb
@@ -91,6 +91,7 @@ class EmailSettingsValidator
       end
 
       if username || password
+        authentication = provider_specific_authentication_overrides(host) if authentication.nil?
         authentication = (authentication || GlobalSetting.smtp_authentication)&.to_sym
         if !%i[plain login cram_md5].include?(authentication)
           raise ArgumentError, "Invalid authentication method. Must be plain, login, or cram_md5."
@@ -129,7 +130,11 @@ class EmailSettingsValidator
       smtp.enable_tls(ssl_context) if enable_tls
 
       smtp.open_timeout = 5
-      smtp.read_timeout = 5
+
+      # Some SMTP servers have a higher delay to respond with errors
+      # as a tarpit measure that slows down clients who are sending "bad" commands.
+      # 10s is the minimum, we might need to increase this in the future.
+      smtp.read_timeout = 10
 
       smtp.start(domain, username, password, authentication)
       smtp.finish
@@ -176,10 +181,19 @@ class EmailSettingsValidator
     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)
-    # Gmail acts weirdly if you do not use the correct combinations of
+    # 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 host == "smtp.gmail.com"
+    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
diff --git a/spec/services/email_settings_validator_spec.rb b/spec/services/email_settings_validator_spec.rb
index 78bc11fcec0..ca41c0352d4 100644
--- a/spec/services/email_settings_validator_spec.rb
+++ b/spec/services/email_settings_validator_spec.rb
@@ -275,6 +275,52 @@ RSpec.describe EmailSettingsValidator do
       }.to raise_error(ArgumentError)
     end
 
+    it "corrects tls settings for gmail based on port 587" do
+      net_smtp_stub.expects(:enable_starttls_auto).once
+      net_smtp_stub.expects(:enable_tls).never
+      described_class.validate_smtp(
+        host: host,
+        port: 587,
+        username: username,
+        password: password,
+        domain: domain,
+      )
+    end
+
+    it "corrects tls settings for gmail based on port 465" do
+      net_smtp_stub.expects(:enable_starttls_auto).never
+      net_smtp_stub.expects(:enable_tls).once
+      described_class.validate_smtp(
+        host: host,
+        port: 465,
+        username: username,
+        password: password,
+        domain: domain,
+      )
+    end
+
+    it "corrects authentication method to login for office365" do
+      net_smtp_stub.expects(:start).with("office365.com", username, password, :login)
+      described_class.validate_smtp(
+        host: "smtp.office365.com",
+        port: port,
+        username: username,
+        password: password,
+        domain: "office365.com",
+      )
+    end
+
+    it "corrects authentication method to login for outlook" do
+      net_smtp_stub.expects(:start).with("outlook.com", username, password, :login)
+      described_class.validate_smtp(
+        host: "smtp-mail.outlook.com",
+        port: port,
+        username: username,
+        password: password,
+        domain: "outlook.com",
+      )
+    end
+
     context "when the domain is not provided" do
       let(:domain) { nil }
       it "gets the domain from the host" do