From e2e1426366605379b70899a9cfbe00d0b2a30bd6 Mon Sep 17 00:00:00 2001 From: nick evans Date: Sun, 10 Sep 2023 11:15:45 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=F0=9F=97=91=EF=B8=8F=20Deprecate?= =?UTF-8?q?=20old=20Net::IMAP.new=20parameters?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Converts `Net::IMAP#initialize` to use keyword parameters for all options. Creates `Net::IMAP::DeprecatedClientOptions`, which is prepended to `Net::IMAP` to override `#initialize`. Moved all of the original argument handling into it. Obsolete, but not yet deprecated: * Using an `options` hash instead of keyword parameters. * Sending `port` as the second parameter. Deprecated (will raise a warning): * Setting `ssl` params via `usessl`, `certs`, or `verify` arguments. --- lib/net/imap.rb | 49 ++------ lib/net/imap/deprecated_client_options.rb | 112 +++++++++++++++++ test/net/imap/fake_server/test_helper.rb | 8 +- .../imap/test_deprecated_client_options.rb | 118 ++++++++++++++++++ 4 files changed, 246 insertions(+), 41 deletions(-) create mode 100644 lib/net/imap/deprecated_client_options.rb create mode 100644 test/net/imap/test_deprecated_client_options.rb diff --git a/lib/net/imap.rb b/lib/net/imap.rb index dcef290d..f5ec3903 100644 --- a/lib/net/imap.rb +++ b/lib/net/imap.rb @@ -760,7 +760,7 @@ class << self # [idle_response_timeout] # Seconds to wait until an IDLE response is received # - # See DeprecatedClientOptions for obsolete backwards compatible arguments. + # See DeprecatedClientOptions.new for deprecated arguments. # # ==== Examples # @@ -810,16 +810,15 @@ class << self # [Net::IMAP::ByeResponseError] # Connected to the host successfully, but it immediately said goodbye. # - def initialize(host, options = {}, *deprecated) + def initialize(host, port: nil, ssl: nil, + open_timeout: 30, idle_response_timeout: 5) super() - options = convert_deprecated_options(options, *deprecated) - # Config options @host = host - @port = options[:port] || (options[:ssl] ? SSL_PORT : PORT) - @open_timeout = options[:open_timeout] || 30 - @idle_response_timeout = options[:idle_response_timeout] || 5 - @ssl_ctx_params, @ssl_ctx = build_ssl_ctx(options[:ssl]) + @port = port || (ssl ? SSL_PORT : PORT) + @open_timeout = Integer(open_timeout) + @idle_response_timeout = Integer(idle_response_timeout) + @ssl_ctx_params, @ssl_ctx = build_ssl_ctx(ssl) # Basic Client State @utf8_strings = false @@ -2354,20 +2353,6 @@ def remove_response_handler(handler) @@debug = false - def convert_deprecated_options( - port_or_options = {}, usessl = false, certs = nil, verify = true - ) - 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) - end - options - end - def start_imap_connection @greeting = get_server_greeting @capabilities = capabilities_from_resp_code @greeting @@ -2695,23 +2680,6 @@ def build_ssl_ctx(ssl) 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 raise "SSL extension not installed" unless defined?(OpenSSL::SSL) raise "already using SSL" if @sock.kind_of?(OpenSSL::SSL::SSLSocket) @@ -2746,3 +2714,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 diff --git a/lib/net/imap/deprecated_client_options.rb b/lib/net/imap/deprecated_client_options.rb new file mode 100644 index 00000000..c9555d8e --- /dev/null +++ b/lib/net/imap/deprecated_client_options.rb @@ -0,0 +1,112 @@ +# frozen_string_literal: true + +module Net + class IMAP < Protocol + + # This module handles deprecated arguments to various Net::IMAP methods. + module DeprecatedClientOptions + + # :call-seq: + # Net::IMAP.new(host, **options) # standard keyword options + # Net::IMAP.new(host, options) # obsolete hash options + # Net::IMAP.new(host, port) # obsolete port argument + # Net::IMAP.new(host, port, usessl, certs = nil, verify = true) # deprecated SSL arguments + # + # Translates Net::IMAP.new arguments for backward compatibility. + # + # ==== Obsolete arguments + # + # Using obsolete arguments does not a warning. Obsolete arguments will be + # deprecated by a future release. + # + # If a second positional argument is given and it is a hash (or is + # convertable via +#to_hash+), it is converted to keyword arguments. + # + # # Obsolete: + # Net::IMAP.new("imap.example.com", options_hash) + # # Use instead: + # Net::IMAP.new("imap.example.com", **options_hash) + # + # If a second positional argument is given and it is not a hash, it is + # converted to the +port+ keyword argument. + # # Obsolete: + # Net::IMAP.new("imap.example.com", 114433) + # # Use instead: + # Net::IMAP.new("imap.example.com", port: 114433) + # + # ==== Deprecated arguments + # + # Using deprecated arguments prints a warning. Convert to keyword + # arguments to avoid the warning. Deprecated arguments will be removed in + # a future release. + # + # If +usessl+ is false, +certs+, and +verify+ are ignored. When it true, + # all three arguments are converted to the +ssl+ keyword argument. + # Without +certs+ or +verify+, it is converted to ssl: true. + # # DEPRECATED: + # Net::IMAP.new("imap.example.com", nil, true) # => prints a warning + # # Use instead: + # Net::IMAP.new("imap.example.com", ssl: true) + # + # When +certs+ is a path to a directory, it is converted to ca_path: + # certs. + # # DEPRECATED: + # Net::IMAP.new("imap.example.com", nil, true, "/path/to/certs") # => prints a warning + # # Use instead: + # Net::IMAP.new("imap.example.com", ssl: {ca_path: "/path/to/certs"}) + # + # When +certs+ is a path to a file, it is converted to ca_file: + # certs. + # # DEPRECATED: + # Net::IMAP.new("imap.example.com", nil, true, "/path/to/cert.pem") # => prints a warning + # # Use instead: + # Net::IMAP.new("imap.example.com", ssl: {ca_file: "/path/to/cert.pem"}) + # + # When +verify+ is +false+, it is converted to verify_mode: + # OpenSSL::SSL::VERIFY_NONE. + # # DEPRECATED: + # Net::IMAP.new("imap.example.com", nil, true, nil, false) # => prints a warning + # # Use instead: + # Net::IMAP.new("imap.example.com", ssl: {verify_mode: OpenSSL::SSL::VERIFY_NONE}) + # + def initialize(host, port_or_options = nil, *deprecated, **options) + if port_or_options.nil? && deprecated.empty? + super host, **options + elsif options.any? + # Net::IMAP.new(host, *__invalid__, **options) + raise ArgumentError, "Do not combine deprecated and keyword arguments" + elsif port_or_options.respond_to?(:to_hash) and deprecated.any? + # Net::IMAP.new(host, options, *__invalid__) + raise ArgumentError, "Do not use deprecated SSL params with options hash" + elsif port_or_options.respond_to?(:to_hash) + super host, **Hash.try_convert(port_or_options) + elsif deprecated.empty? + super host, port: port_or_options + elsif deprecated.shift + warn "DEPRECATED: Call Net::IMAP.new with keyword options", uplevel: 1 + super host, port: port_or_options, ssl: create_ssl_params(*deprecated) + else + warn "DEPRECATED: Call Net::IMAP.new with keyword options", uplevel: 1 + super host, port: port_or_options, ssl: false + end + end + + private + + 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 + params[:verify_mode] = + verify ? OpenSSL::SSL::VERIFY_PEER : OpenSSL::SSL::VERIFY_NONE + params + end + + end + end +end diff --git a/test/net/imap/fake_server/test_helper.rb b/test/net/imap/fake_server/test_helper.rb index cf68dd9f..c35b1d6a 100644 --- a/test/net/imap/fake_server/test_helper.rb +++ b/test/net/imap/fake_server/test_helper.rb @@ -4,10 +4,14 @@ module Net::IMAP::FakeServer::TestHelper - def run_fake_server_in_thread(timeout: 5, **opts) + def run_fake_server_in_thread(ignore_io_error: false, timeout: 5, **opts) Timeout.timeout(timeout) do server = Net::IMAP::FakeServer.new(timeout: timeout, **opts) - @threads << Thread.new do server.run end if @threads + @threads << Thread.new do + server.run + rescue IOError + raise unless ignore_io_error + end yield server ensure server&.shutdown diff --git a/test/net/imap/test_deprecated_client_options.rb b/test/net/imap/test_deprecated_client_options.rb new file mode 100644 index 00000000..445fc105 --- /dev/null +++ b/test/net/imap/test_deprecated_client_options.rb @@ -0,0 +1,118 @@ +# frozen_string_literal: true + +require "net/imap" +require "test/unit" +require_relative "fake_server" + +class DeprecatedClientOptionsTest < Test::Unit::TestCase + include Net::IMAP::FakeServer::TestHelper + + def setup + @do_not_reverse_lookup = Socket.do_not_reverse_lookup + Socket.do_not_reverse_lookup = true + @threads = [] + end + + def teardown + assert_join_threads(@threads) unless @threads.empty? + ensure + Socket.do_not_reverse_lookup = @do_not_reverse_lookup + end + + class InitializeTests < DeprecatedClientOptionsTest + + test "Convert obsolete options hash to keywords" do + run_fake_server_in_thread do |server| + with_client(server.host, {port: server.port, ssl: false}) do |client| + assert_equal server.host, client.host + assert_equal server.port, client.port + assert_equal false, client.ssl_ctx_params + end + end + end + + test "Convert obsolete port argument to :port keyword" do + run_fake_server_in_thread do |server| + with_client(server.host, server.port) do |client| + assert_equal server.host, client.host + assert_equal server.port, client.port + assert_equal false, client.ssl_ctx_params + end + end + end + + test "Convert deprecated usessl (= false) with warning" do + run_fake_server_in_thread do |server| + assert_deprecated_warning(/Call Net::IMAP\.new with keyword/i) do + with_client(server.host, server.port, false, :who, :cares) do |client| + assert_equal server.host, client.host + assert_equal server.port, client.port + assert_equal false, client.ssl_ctx_params + end + end + end + end + + test "Convert deprecated usessl (= true) and certs, with warning" do + run_fake_server_in_thread(implicit_tls: true) do |server| + certs = server.config.tls[:ca_file] + assert_deprecated_warning(/Call Net::IMAP\.new with keyword/i) do + with_client("localhost", server.port, true, certs) do |client| + assert_equal "localhost", client.host + assert_equal server.port, client.port + assert_equal( + {ca_file: certs, verify_mode: OpenSSL::SSL::VERIFY_PEER}, + client.ssl_ctx_params + ) + end + end + end + end + + test "Convert deprecated usessl (= true) and verify (= false), with warning" do + run_fake_server_in_thread(implicit_tls: true) do |server| + assert_deprecated_warning(/Call Net::IMAP\.new with keyword/i) do + with_client("localhost", server.port, true, nil, false) do |client| + assert_equal "localhost", client.host + assert_equal server.port, client.port + assert_equal( + {verify_mode: OpenSSL::SSL::VERIFY_NONE}, + client.ssl_ctx_params + ) + assert_equal OpenSSL::SSL::VERIFY_NONE, client.ssl_ctx.verify_mode + end + end + end + end + + test "combined options hash and keyword args raises ArgumentError" do + ex = nil + run_fake_server_in_thread( + ignore_io_error: true, implicit_tls: true + ) do |server| + imap = Net::IMAP.new("localhost", {port: 993}, ssl: true) + rescue => ex + nil + ensure + imap&.disconnect + end + assert_equal ArgumentError, ex.class + end + + test "combined options hash and ssl args raises ArgumentError" do + ex = nil + run_fake_server_in_thread( + ignore_io_error: true, implicit_tls: true + ) do |server| + imap = Net::IMAP.new("localhost", {port: 993}, true) + rescue => ex + nil + ensure + imap&.disconnect + end + assert_equal ArgumentError, ex.class + end + + end + +end