From d5f7516e0d8061bd97bf7c2cf112fd5912744c73 Mon Sep 17 00:00:00 2001 From: Jeremy Bopp Date: Mon, 28 Sep 2015 10:35:32 -0500 Subject: [PATCH 1/6] DRY up connection handling logic --- lib/net/ldap/connection.rb | 56 ++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 29 deletions(-) diff --git a/lib/net/ldap/connection.rb b/lib/net/ldap/connection.rb index 05aedfef..fdec64b5 100644 --- a/lib/net/ldap/connection.rb +++ b/lib/net/ldap/connection.rb @@ -8,56 +8,54 @@ class Net::LDAP::Connection #:nodoc: def initialize(server) @instrumentation_service = server[:instrumentation_service] - server[:hosts] = [[server[:host], server[:port]]] if server[:hosts].nil? if server[:socket] prepare_socket(server) else + server[:hosts] = [[server[:host], server[:port]]] if server[:hosts].nil? open_connection(server) end yield self if block_given? end - def prepare_socket(server) - @conn = server[:socket] + def prepare_socket(server, close = false) + socket = server[:socket] + encryption = server[:encryption] - if server[:encryption] - setup_encryption server[:encryption] - end + @conn = socket + setup_encryption encryption if encryption + rescue + # Ensure the connection is closed when requested in the event of an SSL + # setup failure. + @conn.close if close + @conn = nil + raise end def open_connection(server) + hosts = server[:hosts] + encryption = server[:encryption] + errors = [] - server[:hosts].each do |host, port| + hosts.each do |host, port| begin - return connect_to_host(host, port, server) - rescue Net::LDAP::Error - errors << $! + prepare_socket(server.merge(socket: TCPSocket.new(host, port)), true) + return + rescue Net::LDAP::Error, SocketError, SystemCallError, + OpenSSL::SSL::SSLError + errors << [$!, host, port] end end - raise errors.first if errors.size == 1 - raise Net::LDAP::Error, - "Unable to connect to any given server: \n #{errors.join("\n ")}" - end - - def connect_to_host(host, port, server) - begin - @conn = TCPSocket.new(host, port) - rescue SocketError - raise Net::LDAP::Error, "No such address or other socket error." - rescue Errno::ECONNREFUSED - raise Net::LDAP::ConnectionRefusedError, "Server #{host} refused connection on port #{port}." - rescue Errno::EHOSTUNREACH => error - raise Net::LDAP::Error, "Host #{host} was unreachable (#{error.message})" - rescue Errno::ETIMEDOUT - raise Net::LDAP::Error, "Connection to #{host} timed out." + if errors.size == 1 + error = errors.first.first + raise Net::LDAP::ConnectionRefusedError, error.message if error.kind_of? Errno::ECONNREFUSED + raise Net::LDAP::Error, error.message end - if server[:encryption] - setup_encryption server[:encryption] - end + raise Net::LDAP::Error, + "Unable to connect to any given server: \n #{errors.map { |e, h, p| "#{e.class}: #{e.message} (#{h}:#{p})" }.join("\n ")}" end module GetbyteForSSLSocket From edee2ee46b0f3bba2ea5b71019a7a11a53bbb23b Mon Sep 17 00:00:00 2001 From: Jeremy Bopp Date: Sun, 18 Oct 2015 17:32:07 -0500 Subject: [PATCH 2/6] Move connection error handling logic into a new error class * ConnectionError wraps the creation of several error types for backward compatibility * For now, ConnectionError is only created when more than 1 error is given to the constructor * In the future, ConnectionError should be used even in the single error case --- lib/net/ldap/connection.rb | 9 +-------- lib/net/ldap/error.rb | 19 +++++++++++++++++++ test/test_ldap_connection.rb | 2 +- 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/lib/net/ldap/connection.rb b/lib/net/ldap/connection.rb index fdec64b5..d28554ff 100644 --- a/lib/net/ldap/connection.rb +++ b/lib/net/ldap/connection.rb @@ -48,14 +48,7 @@ def open_connection(server) end end - if errors.size == 1 - error = errors.first.first - raise Net::LDAP::ConnectionRefusedError, error.message if error.kind_of? Errno::ECONNREFUSED - raise Net::LDAP::Error, error.message - end - - raise Net::LDAP::Error, - "Unable to connect to any given server: \n #{errors.map { |e, h, p| "#{e.class}: #{e.message} (#{h}:#{p})" }.join("\n ")}" + raise Net::LDAP::ConnectionError.new(errors) end module GetbyteForSSLSocket diff --git a/lib/net/ldap/error.rb b/lib/net/ldap/error.rb index 38b4a4a5..9f157195 100644 --- a/lib/net/ldap/error.rb +++ b/lib/net/ldap/error.rb @@ -25,6 +25,25 @@ def warn_deprecation_message warn "Deprecation warning: Net::LDAP::ConnectionRefused will be deprecated. Use Errno::ECONNREFUSED instead." end end + class ConnectionError < Error + def self.new(errors) + error = errors.first.first + if errors.size == 1 + if error.kind_of? Errno::ECONNREFUSED + return Net::LDAP::ConnectionRefusedError.new(error.message) + end + + return Net::LDAP::Error.new(error.message) + end + + super + end + + def initialize(errors) + message = "Unable to connect to any given server: \n #{errors.map { |e, h, p| "#{e.class}: #{e.message} (#{h}:#{p})" }.join("\n ")}" + super(message) + end + end class NoOpenSSLError < Error; end class NoStartTLSResultError < Error; end class NoSearchBaseError < Error; end diff --git a/test/test_ldap_connection.rb b/test/test_ldap_connection.rb index e5104838..d991bddc 100644 --- a/test/test_ldap_connection.rb +++ b/test/test_ldap_connection.rb @@ -42,7 +42,7 @@ def test_list_of_hosts_with_all_hosts_failure flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[1]).once.and_raise(SocketError) flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[2]).once.and_raise(SocketError) flexmock(TCPSocket).should_receive(:new).ordered.never - assert_raise Net::LDAP::Error do + assert_raise Net::LDAP::ConnectionError do Net::LDAP::Connection.new(:hosts => hosts) end end From e1a0d1348f2e49acb5ba67e803e9102eb1b64f14 Mon Sep 17 00:00:00 2001 From: Jeremy Bopp Date: Sun, 18 Oct 2015 17:38:03 -0500 Subject: [PATCH 3/6] Resolve rubocop violations --- test/test_ldap_connection.rb | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/test/test_ldap_connection.rb b/test/test_ldap_connection.rb index d991bddc..73752631 100644 --- a/test/test_ldap_connection.rb +++ b/test/test_ldap_connection.rb @@ -11,10 +11,10 @@ def capture_stderr def test_list_of_hosts_with_first_host_successful hosts = [ - ['test.mocked.com', 636], - ['test2.mocked.com', 636], - ['test3.mocked.com', 636], - ] + ['test.mocked.com', 636], + ['test2.mocked.com', 636], + ['test3.mocked.com', 636], + ] flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[0]).once.and_return(nil) flexmock(TCPSocket).should_receive(:new).ordered.never Net::LDAP::Connection.new(:hosts => hosts) @@ -22,10 +22,10 @@ def test_list_of_hosts_with_first_host_successful def test_list_of_hosts_with_first_host_failure hosts = [ - ['test.mocked.com', 636], - ['test2.mocked.com', 636], - ['test3.mocked.com', 636], - ] + ['test.mocked.com', 636], + ['test2.mocked.com', 636], + ['test3.mocked.com', 636], + ] flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[0]).once.and_raise(SocketError) flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[1]).once.and_return(nil) flexmock(TCPSocket).should_receive(:new).ordered.never @@ -34,10 +34,10 @@ def test_list_of_hosts_with_first_host_failure def test_list_of_hosts_with_all_hosts_failure hosts = [ - ['test.mocked.com', 636], - ['test2.mocked.com', 636], - ['test3.mocked.com', 636], - ] + ['test.mocked.com', 636], + ['test2.mocked.com', 636], + ['test3.mocked.com', 636], + ] flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[0]).once.and_raise(SocketError) flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[1]).once.and_raise(SocketError) flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[2]).once.and_raise(SocketError) From 5a4ddb14a1f1abf54202c8e7fcd5bf3ba287c91f Mon Sep 17 00:00:00 2001 From: Jeremy Bopp Date: Mon, 19 Oct 2015 14:42:08 -0500 Subject: [PATCH 4/6] Assign exceptions to a variable rather than use $! --- lib/net/ldap/connection.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/net/ldap/connection.rb b/lib/net/ldap/connection.rb index d28554ff..3c3dbfd6 100644 --- a/lib/net/ldap/connection.rb +++ b/lib/net/ldap/connection.rb @@ -43,8 +43,8 @@ def open_connection(server) prepare_socket(server.merge(socket: TCPSocket.new(host, port)), true) return rescue Net::LDAP::Error, SocketError, SystemCallError, - OpenSSL::SSL::SSLError - errors << [$!, host, port] + OpenSSL::SSL::SSLError => e + errors << [e, host, port] end end From ab320af47c6c75536ac6ab7a83a419746132f67a Mon Sep 17 00:00:00 2001 From: Jeremy Bopp Date: Mon, 19 Oct 2015 14:43:38 -0500 Subject: [PATCH 5/6] Close the socket where opened when necessary --- lib/net/ldap/connection.rb | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/lib/net/ldap/connection.rb b/lib/net/ldap/connection.rb index 3c3dbfd6..f3b42f71 100644 --- a/lib/net/ldap/connection.rb +++ b/lib/net/ldap/connection.rb @@ -19,18 +19,12 @@ def initialize(server) yield self if block_given? end - def prepare_socket(server, close = false) + def prepare_socket(server) socket = server[:socket] encryption = server[:encryption] @conn = socket setup_encryption encryption if encryption - rescue - # Ensure the connection is closed when requested in the event of an SSL - # setup failure. - @conn.close if close - @conn = nil - raise end def open_connection(server) @@ -40,10 +34,14 @@ def open_connection(server) errors = [] hosts.each do |host, port| begin - prepare_socket(server.merge(socket: TCPSocket.new(host, port)), true) + socket = TCPSocket.new(host, port) + prepare_socket(server.merge(socket: socket)) return rescue Net::LDAP::Error, SocketError, SystemCallError, OpenSSL::SSL::SSLError => e + # Ensure the connection is closed in the event a setup failure. + socket.close unless socket.nil? + socket = nil errors << [e, host, port] end end From e8290692cfd9f196c0d90f36fca29bc530e51dfe Mon Sep 17 00:00:00 2001 From: Jeremy Bopp Date: Sun, 25 Oct 2015 22:28:21 -0500 Subject: [PATCH 6/6] Move connection cleanup logic into the close method --- lib/net/ldap/connection.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/net/ldap/connection.rb b/lib/net/ldap/connection.rb index f3b42f71..691e284f 100644 --- a/lib/net/ldap/connection.rb +++ b/lib/net/ldap/connection.rb @@ -34,14 +34,12 @@ def open_connection(server) errors = [] hosts.each do |host, port| begin - socket = TCPSocket.new(host, port) - prepare_socket(server.merge(socket: socket)) + prepare_socket(server.merge(socket: TCPSocket.new(host, port))) return rescue Net::LDAP::Error, SocketError, SystemCallError, OpenSSL::SSL::SSLError => e # Ensure the connection is closed in the event a setup failure. - socket.close unless socket.nil? - socket = nil + close errors << [e, host, port] end end @@ -145,6 +143,7 @@ def setup_encryption(args) # have to call it, but perhaps it will come in handy someday. #++ def close + return if @conn.nil? @conn.close @conn = nil end