🗑️ Add deprecation warnings to .new and #starttls

Preparing for a (backwards-incompatible) secure-by-default
configuration, Net::IMAP.default_ssl will be used when no explicit port
or tls setting is provided.

TODO: should truthy default_ssl be used to config params when port is
993 but ssl is implicit? Another var?

Moved all deprecated option handling to DeprecatedClientOptions, which
is prepended to Net::IMAP.

Additionally, split `initialize` up into small helper methods making it
easier to understand at a glance.

Author: nevans

lib/net/imap.rb

@@ -716,11 +716,104 @@ class << self
       alias default_imap_port default_port
       alias default_imaps_port default_tls_port
       alias default_ssl_port default_tls_port
+
+      # The default value for the +ssl+ option of ::new, when +port+ is
+      # unspecified or non-standard.
+      #
+      # Defaults to +nil+ for backward compatibility, which prints a warning and
+      # does _not_ use TLS.
+      #
+      # >>>
+      #   *Note*: A future version of Net::IMAP will default to +true+, as per
+      #   RFC7525[https://tools.ietf.org/html/rfc7525],
+      #   RFC7817[https://tools.ietf.org/html/rfc7817],
+      #   and RFC8314[https://tools.ietf.org/html/rfc8314].
+      #
+      # Set to +false+ to *globally* use insecure defaults and silence warnings.
+      # Send <tt>ssl: false</tt> to ::new to explicitly silence warnings for a
+      # single connection.
+      attr_accessor :default_ssl
+    end
+
+    # Creates a new Net::IMAP object and connects it to the specified
+    # +host+.
+    #
+    # Accepts the following options:
+    #
+    # [port]
+    #   Port number (default value is 143 for imap, or 993 for imaps)
+    # [ssl]
+    #   When +true+, the connection will use TLS using the defaults chosen by
+    #   {OpenSSL::SSL::SSLContext#set_params}[https://docs.ruby-lang.org/en/master/OpenSSL/SSL/SSLContext.html#method-i-set_params].
+    #   Use a hash to override the defaults---the keys are assignment methods on
+    #   SSLContext[https://docs.ruby-lang.org/en/master/OpenSSL/SSL/SSLContext.html].
+    #   Defaults to +true+ or +false+ to match +port+, or to ::default_ssl when
+    #   +port+ is unspecified or non-standard.
+    # [open_timeout]
+    #   Seconds to wait until a connection is opened
+    # [idle_response_timeout]
+    #   Seconds to wait until an IDLE response is received
+    #
+    # The most common errors are:
+    #
+    # Errno::ECONNREFUSED:: Connection refused by +host+ or an intervening
+    #                       firewall.
+    # Errno::ETIMEDOUT:: Connection timed out (possibly due to packets
+    #                    being dropped by an intervening firewall).
+    # Errno::ENETUNREACH:: There is no route to that network.
+    # SocketError:: Hostname not known or other socket error.
+    # Net::IMAP::ByeResponseError:: Connected to the host successfully, but
+    #                               it immediately said goodbye.
+    def initialize(host,
+                   port: nil,
+                   ssl:  nil,
+                   open_timeout: 30,
+                   idle_response_timeout: 5)
+      # Basic configuration
+      @host                  = host
+      @ssl, @port            = default_ssl_and_port(ssl, port)
+      @open_timeout          = Integer(open_timeout)
+      @idle_response_timeout = Integer(idle_response_timeout)
+
+      # Basic Client state
+      super() # Mutex and condition vars (MonitorMixin#initialize)
+      @greeting = nil
+      @capabilities = nil
+      @utf8_strings = false # TODO: use @enabled instead
+      @debug_output_bol = true
+
+      # Client Protocol Reciever
+      @parser = ResponseParser.new
+      @receiver_thread = nil
+      @receiver_thread_terminating = false
+      @exception = nil
+
+      # Client Protocol Sender
+      @tag_prefix = "RUBY"
+      @tagno = 0
+
+      # Response handlers
+      @continuation_request_arrival = new_cond
+      @continuation_request_exception = nil
+      @tagged_response_arrival = new_cond
+      @tagged_responses = {}
+      @response_handlers = []
+      @responses = Hash.new {|h, k| h[k] = [] }
+
+      # Command execution state
+      @logout_command_tag = nil
+      @continued_command_tag = nil
+      @idle_done_cond = nil
+
+      # create the connection
+      @sock = nil
+      start_connection
     end
 
     def client_thread # :nodoc:
-      warn "Net::IMAP#client_thread is deprecated and will be removed soon."
-      @client_thread
+      warn "Net::IMAP#client_thread is deprecated and always returns the " \
+           "caller's current thread."
+      Thread.current
     end
 
     # Disconnects from the server.
@@ -966,15 +1059,9 @@ def logout
     # Server capabilities may change after #starttls, #login, and #authenticate.
     # Cached #capabilities will be cleared when this method completes.
     #
-    def starttls(options = {}, verify = true)
+    def starttls(options = {})
       send_command("STARTTLS") do |resp|
         if resp.kind_of?(TaggedResponse) && resp.name == "OK"
-          begin
-            # for backward compatibility
-            certs = options.to_str
-            options = create_ssl_params(certs, verify)
-          rescue NoMethodError
-          end
           clear_cached_capabilities
           clear_responses
           start_tls_session(options)
@@ -2190,99 +2277,62 @@ def remove_response_handler(handler)
 
     @@debug = false
 
-    # :call-seq:
-    #    Net::IMAP.new(host, options = {})
-    #
-    # Creates a new Net::IMAP object and connects it to the specified
-    # +host+.
-    #
-    # +options+ is an option hash, each key of which is a symbol.
-    #
-    # The available options are:
-    #
-    # port::  Port number (default value is 143 for imap, or 993 for imaps)
-    # ssl::   If +options[:ssl]+ is true, then an attempt will be made
-    #         to use SSL (now TLS) to connect to the server.
-    #         If +options[:ssl]+ is a hash, it's passed to
-    #         OpenSSL::SSL::SSLContext#set_params as parameters.
-    # open_timeout:: Seconds to wait until a connection is opened
-    # idle_response_timeout:: Seconds to wait until an IDLE response is received
-    #
-    # The most common errors are:
-    #
-    # Errno::ECONNREFUSED:: Connection refused by +host+ or an intervening
-    #                       firewall.
-    # Errno::ETIMEDOUT:: Connection timed out (possibly due to packets
-    #                    being dropped by an intervening firewall).
-    # Errno::ENETUNREACH:: There is no route to that network.
-    # SocketError:: Hostname not known or other socket error.
-    # Net::IMAP::ByeResponseError:: The connected to the host was successful, but
-    #                               it immediately said goodbye.
-    def initialize(host, port_or_options = {},
-                   usessl = false, certs = nil, verify = true)
-      super()
-      @host = host
-      begin
-        options = port_or_options.to_hash
-      rescue NoMethodError
-        # for backward compatibility
-        options = {}
-        options[:port] = port_or_options
-        if usessl
-          options[:ssl] = create_ssl_params(certs, verify)
+    def default_ssl_and_port(ssl, port)
+      if ssl.nil? && port
+        ssl = true  if port == SSL_PORT || /\Aimaps\z/i === port
+        ssl = false if port == PORT
+      elsif port.nil? && !ssl.nil?
+        port = ssl ? SSL_PORT : PORT
+      end
+      if ssl.nil? && port.nil?
+        ssl = self.class.default_ssl.dup.freeze
+        port = ssl ? SSL_PORT : PORT
+        if ssl.nil?
+          warn "A future version of Net::IMAP.default_ssl " \
+               "will default to 'true', for secure connections by default.  " \
+               "Use 'Net::IMAP.new(host, ssl: false)' or set " \
+               "Net::IMAP.default_ssl = false' to silence this warning."
         end
       end
-      @port = options[:port] || (options[:ssl] ? SSL_PORT : PORT)
-      @tag_prefix = "RUBY"
-      @tagno = 0
-      @utf8_strings = false
-      @open_timeout = options[:open_timeout] || 30
-      @idle_response_timeout = options[:idle_response_timeout] || 5
-      @parser = ResponseParser.new
+      ssl &&= ssl.respond_to?(:to_hash) ? ssl.to_hash : {}
+      [ssl, port]
+    end
+
+    def start_connection
       @sock = tcp_socket(@host, @port)
       begin
-        if options[:ssl]
-          start_tls_session(options[:ssl])
-          @usessl = true
-        else
-          @usessl = false
-        end
-        @responses = Hash.new {|h, k| h[k] = [] }
-        @tagged_responses = {}
-        @response_handlers = []
-        @tagged_response_arrival = new_cond
-        @continued_command_tag = nil
-        @continuation_request_arrival = new_cond
-        @continuation_request_exception = nil
-        @idle_done_cond = nil
-        @logout_command_tag = nil
-        @debug_output_bol = true
-        @exception = nil
-
+        start_tls_session(@ssl) if @ssl
         @greeting = get_response
-        if @greeting.nil?
-          raise Error, "connection closed"
-        end
-        record_untagged_response_code @greeting
-        @capabilities = capabilities_from_resp_code @greeting
-        if @greeting.name == "BYE"
-          raise ByeResponseError, @greeting
-        end
-
-        @client_thread = Thread.current
-        @receiver_thread = Thread.start {
-          begin
-            receive_responses
-          rescue Exception
-          end
-        }
-        @receiver_thread_terminating = false
+        handle_server_greeting
+        @receiver_thread = start_receiver_thread
       rescue Exception
         @sock.close
         raise
       end
     end
 
+    def handle_server_greeting
+      if @greeting.nil?
+        raise Error, "connection closed"
+      end
+      record_untagged_response_code(@greeting)
+      @capabilities = capabilities_from_resp_code @greeting
+      if @greeting.name == "BYE"
+        raise ByeResponseError, @greeting
+      end
+    end
+
+    def start_receiver_thread
+      Thread.start do
+        receive_responses
+      rescue Exception
+        # don't exit the thread with an exception
+      end
+    rescue Exception
+      @sock.close
+      raise
+    end
+
     def tcp_socket(host, port)
       s = Socket.tcp(host, port, :connect_timeout => @open_timeout)
       s.setsockopt(:SOL_SOCKET, :SO_KEEPALIVE, true)
@@ -2569,35 +2619,13 @@ def normalize_searching_criteria(keys)
       end
     end
 
-    def create_ssl_params(certs = nil, verify = true)
-      params = {}
-      if certs
-        if File.file?(certs)
-          params[:ca_file] = certs
-        elsif File.directory?(certs)
-          params[:ca_path] = certs
-        end
-      end
-      if verify
-        params[:verify_mode] = VERIFY_PEER
-      else
-        params[:verify_mode] = VERIFY_NONE
-      end
-      return params
-    end
-
     def start_tls_session(params = {})
       unless defined?(OpenSSL::SSL)
         raise "SSL extension not installed"
       end
       if @sock.kind_of?(OpenSSL::SSL::SSLSocket)
         raise RuntimeError, "already using SSL"
       end
-      begin
-        params = params.to_hash
-      rescue NoMethodError
-        params = {}
-      end
       context = SSLContext.new
       context.set_params(params)
       if defined?(VerifyCallbackProc)
@@ -2632,3 +2660,6 @@ def self.saslprep(string, **opts)
 require_relative "imap/response_data"
 require_relative "imap/response_parser"
 require_relative "imap/authenticators"
+
+require_relative "imap/deprecated_client_options"
+Net::IMAP.prepend Net::IMAP::DeprecatedClientOptions

lib/net/imap/deprecated_client_options.rb

@@ -0,0 +1,83 @@
+# frozen_string_literal: true
+
+module Net
+  class IMAP < Protocol
+
+    # This module handles deprecated arguments to various methods.  It will be
+    # deleted in a future release.
+    module DeprecatedClientOptions
+      UNDEFINED = Module.new.freeze
+      private_constant :UNDEFINED
+
+      # Passing any arguments *both* positionally and as a keyword will raise an
+      # ArgumentError.
+      #
+      # Allows port to be sent as an integer or string without warning or error.
+      #
+      # SSL options sent positionally will print a deprecation warning (and,
+      # eventually, raise an argument error).
+      def initialize(host, *deprecated, **options)
+        unless deprecated.empty?
+          port_or_options, sslopts = deprecated
+          if !port_or_options.nil? &&
+              (port_or_options.respond_to?(:to_hash) ?
+              options.empty? : options.key?(:port)) ||
+              !sslopts.nil? && options.key?(:ssl)
+            raise ArgumentError, "Don't use both positional and keyword options"
+          end
+          # handle port_or_options => options
+          if port_or_options.respond_to?(:to_hash)
+            warn "DEPRECATED: options should be set by keyword arguments"
+            options = port_or_options.to_hash
+          elsif !port_or_options.nil?
+            warn "DEPRECATED: port should be set by keyword argument"
+            options[:port] = port_or_options
+          end
+          # handle ssl options
+          unless sslopts.nil?
+            warn "DEPRECATED: SSL options should be set by keyword argument"
+            usessl, certs, verify = sslopts
+            if usessl
+              options[:ssl] = create_ssl_params(certs, verify)
+            end
+          end
+        end
+        super(host, **options)
+      end
+
+      # +call-seq:
+      #   starttls(options = {})
+      #   starttls(certs, verify = true)
+      #
+      # For backward compatibility.  A future release will only accept
+      # OpenSSL::SSL::SSLContext.set_params options.
+      def starttls(options = {}, verify = UNDEFINED)
+        if options.respond_to?(:to_str)
+          warn "DEPRECATED: starttls(certs, verify).  Use starttls(ssl_params)"
+          certs   = options.to_str
+          verify  = true if verify == UNDEFINED
+          options = create_ssl_params(certs, verify)
+        end
+        super(options)
+      end
+
+      private
+
+      def create_ssl_params(certs, verify)
+        params = {}
+        if certs
+          if File.file?(certs)
+            params[:ca_file] = certs
+          elsif File.directory?(certs)
+            params[:ca_path] = certs
+          end
+        end
+        params[:verify_mode] = verify ? VERIFY_PEER : VERIFY_NONE
+        params
+      end
+
+    end
+
+    prepend DeprecatedClientOptions
+  end
+end