Merge pull request #1536 from eval/refactor-smtp-tls-starttls

SMTP: refactor and accept starttls :always and :auto

lib/mail/network/delivery_methods/smtp.rb

@@ -84,7 +84,7 @@ class SMTP
       :password             => nil,
       :authentication       => nil,
       :enable_starttls      => nil,
-      :enable_starttls_auto => true,
+      :enable_starttls_auto => nil,
       :openssl_verify_mode  => nil,
       :ssl                  => nil,
       :tls                  => nil,
@@ -105,23 +105,64 @@ def deliver!(mail)
     end
 
     private
+      # `k` is said to be provided when `settings` has a non-nil value for `k`.
+      def setting_provided?(k)
+        !settings[k].nil?
+      end
+
+      # Yields one of `:always`, `:auto` or `false` based on `enable_starttls` and `enable_starttls_auto` flags.
+      # Yields `false` when `smtp_tls?`.
+      def smtp_starttls
+        return false if smtp_tls?
+
+        if setting_provided?(:enable_starttls) && settings[:enable_starttls]
+          # enable_starttls: provided and truthy
+          case settings[:enable_starttls]
+          when :auto then :auto
+          when :always then :always
+          else
+            :always
+          end
+        else
+          # enable_starttls: not provided or false
+          if setting_provided?(:enable_starttls_auto)
+            settings[:enable_starttls_auto] ? :auto : false
+          else
+            # enable_starttls_auto: not provided
+            # enable_starttls: when provided then false
+            # use :auto when neither enable_starttls* provided
+            setting_provided?(:enable_starttls) ? false : :auto
+          end
+        end
+      end
+
+      def smtp_tls?
+        setting_provided?(:tls) && settings[:tls] || setting_provided?(:ssl) && settings[:ssl]
+      end
+
       def start_smtp_session(&block)
         build_smtp_session.start(settings[:domain], settings[:user_name], settings[:password], settings[:authentication], &block)
       end
 
       def build_smtp_session
+        if smtp_tls? && (settings[:enable_starttls] || settings[:enable_starttls_auto])
+          raise ArgumentError, ":enable_starttls and :tls are mutually exclusive. Set :tls if you're on an SMTPS connection. Set :enable_starttls if you're on an SMTP connection and using STARTTLS for secure TLS upgrade."
+        end
+
         Net::SMTP.new(settings[:address], settings[:port]).tap do |smtp|
-          if settings[:tls] || settings[:ssl]
-            if smtp.respond_to?(:enable_tls)
-              smtp.enable_tls(ssl_context)
-            end
-          elsif settings[:enable_starttls]
-            if smtp.respond_to?(:enable_starttls)
+          if smtp_tls?
+            smtp.disable_starttls
+            smtp.enable_tls(ssl_context)
+          else
+            smtp.disable_tls
+
+            case smtp_starttls
+            when :always
               smtp.enable_starttls(ssl_context)
-            end
-          elsif settings[:enable_starttls_auto]
-            if smtp.respond_to?(:enable_starttls_auto)
+            when :auto
               smtp.enable_starttls_auto(ssl_context)
+            else
+              smtp.disable_starttls
             end
           end
 

spec/mail/network/delivery_methods/smtp_spec.rb

@@ -5,24 +5,10 @@
 RSpec.describe "SMTP Delivery Method" do
 
   before(:each) do
+    MockSMTP.reset
+
     # Reset all defaults back to original state
-    Mail.defaults do
-      delivery_method :smtp, { :address              => "localhost",
-                               :port                 => 25,
-                               :domain               => 'localhost.localdomain',
-                               :user_name            => nil,
-                               :password             => nil,
-                               :authentication       => nil,
-                               :enable_starttls      => nil,
-                               :enable_starttls_auto => true,
-                               :openssl_verify_mode  => nil,
-                               :tls                  => nil,
-                               :ssl                  => nil,
-                               :open_timeout         => nil,
-                               :read_timeout         => nil
-                                }
-    end
-    MockSMTP.clear_deliveries
+    Mail.defaults { delivery_method :smtp, {} }
   end
 
   describe "general usage" do
@@ -207,7 +193,7 @@ def redefine_verify_none(new_value)
 
       message.deliver!
 
-      expect(MockSMTP.security).to eq :enable_starttls_auto
+      expect(MockSMTP.starttls).to eq :auto
     end
 
     it 'should allow forcing STARTTLS' do
@@ -216,19 +202,70 @@ def redefine_verify_none(new_value)
         to '[email protected]'
         subject 'Re: No way!'
         body 'Yeah sure'
-        delivery_method :smtp, { :address         => "localhost",
-                                 :port            => 25,
-                                 :domain          => 'localhost.localdomain',
-                                 :user_name       => nil,
-                                 :password        => nil,
-                                 :authentication  => nil,
-                                 :enable_starttls => true  }
+        delivery_method :smtp, { :enable_starttls => true }
+      end
 
+      message.deliver!
+
+      expect(MockSMTP.starttls).to eq :always
+    end
+
+    it 'should allow forcing STARTTLS via enable_starttls: :always (overriding :enable_starttls_auto)' do
+      message = Mail.new do
+        from '[email protected]'
+        to '[email protected]'
+        subject 'Re: No way!'
+        body 'Yeah sure'
+        delivery_method :smtp, { :enable_starttls => :always,
+                                 :enable_starttls_auto => true }
       end
 
       message.deliver!
 
-      expect(MockSMTP.security).to eq :enable_starttls
+      expect(MockSMTP.starttls).to eq :always
+    end
+
+    it 'should allow detecting STARTTLS via enable_starttls: :auto (overriding :enable_starttls_auto)' do
+      message = Mail.new do
+        from '[email protected]'
+        to '[email protected]'
+        subject 'Re: No way!'
+        body 'Yeah sure'
+        delivery_method :smtp, { :enable_starttls => :auto,
+                                 :enable_starttls_auto => false }
+      end
+
+      message.deliver!
+
+      expect(MockSMTP.starttls).to eq :auto
+    end
+
+    it 'should allow disabling automatic STARTTLS' do
+      message = Mail.new do
+        from '[email protected]'
+        to '[email protected]'
+        subject 'Re: No way!'
+        body 'Yeah sure'
+        delivery_method :smtp, { :enable_starttls => false }
+      end
+
+      message.deliver!
+
+      expect(MockSMTP.starttls).to eq false
+    end
+
+    it 'raises when setting STARTTLS with tls' do
+      message = Mail.new do
+        from '[email protected]'
+        to '[email protected]'
+        subject 'Re: No way!'
+        body 'Yeah sure'
+        delivery_method :smtp, { :tls => true, :enable_starttls => :always }
+      end
+
+      expect {
+        message.deliver!
+      }.to raise_error(ArgumentError, /:enable_starttls and :tls are mutually exclusive/)
     end
   end
 

spec/mail/network_spec.rb

@@ -180,7 +180,7 @@ class MyRetriever; def initialize(settings); end; end
 
     before(:each) do
       # Set the delivery method to test as the default
-      MockSMTP.clear_deliveries
+      MockSMTP.reset
     end
 
     it "should deliver a mail message" do

spec/spec_helper.rb

@@ -71,21 +71,44 @@ def strip_heredoc(string)
   string.gsub(/^[ \t]{#{indent}}/, '')
 end
 
+class Net::SMTP
+  class << self
+    alias unstubbed_new new
+  end
+
+  def self.new(*args)
+    MockSMTP.new
+  end
+end
+
 # Original mockup from ActionMailer
 class MockSMTP
   attr_accessor :open_timeout, :read_timeout
 
+  def self.reset
+    test = Net::SMTP.unstubbed_new('example.com')
+    @@tls = test.tls?
+    @@starttls = test.starttls?
+
+    @@deliveries = []
+  end
+
+  reset
+
   def self.deliveries
     @@deliveries
   end
 
-  def self.security
-    @@security
+  def self.tls
+    @@tls
+  end
+
+  def self.starttls
+    @@starttls
   end
 
   def initialize
-    @@deliveries = []
-    @@security = nil
+    self.class.reset
   end
 
   def sendmail(mail, from, to)
@@ -105,37 +128,31 @@ def finish
     return true
   end
 
-  def self.clear_deliveries
-    @@deliveries = []
-  end
-
-  def self.clear_security
-    @@security = nil
-  end
-
   def enable_tls(context)
-    raise ArgumentError, "SMTPS and STARTTLS is exclusive" if @@security && @@security != :enable_tls
-    @@security = :enable_tls
+    raise ArgumentError, "SMTPS and STARTTLS is exclusive" if @@starttls == :always
+    @@tls = true
     context
   end
 
+  def disable_tls
+    @@tls = false
+  end
+
   def enable_starttls(context = nil)
-    raise ArgumentError, "SMTPS and STARTTLS is exclusive" if @@security == :enable_tls
-    @@security = :enable_starttls
+    raise ArgumentError, "SMTPS and STARTTLS is exclusive" if @@tls
+    @@starttls = :always
     context
   end
 
   def enable_starttls_auto(context)
-    raise ArgumentError, "SMTPS and STARTTLS is exclusive" if @@security == :enable_tls
-    @@security = :enable_starttls_auto
+    raise ArgumentError, "SMTPS and STARTTLS is exclusive" if @@tls
+    @@starttls = :auto
     context
   end
-end
-
-class Net::SMTP
-  def self.new(*args)
-    MockSMTP.new
+  def disable_starttls
+    @@starttls = false
   end
+
 end
 
 class MockPopMail