From d5e6afd6c77ad7a62d49b57000a650df14d26352 Mon Sep 17 00:00:00 2001 From: Jerry Cheung Date: Mon, 9 Nov 2015 22:17:50 -0800 Subject: [PATCH 01/11] lazy init Net::LDAP::Connection's internal sock --- lib/net/ldap/connection.rb | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/lib/net/ldap/connection.rb b/lib/net/ldap/connection.rb index 4e3f6dd0..e987a443 100644 --- a/lib/net/ldap/connection.rb +++ b/lib/net/ldap/connection.rb @@ -6,16 +6,10 @@ class Net::LDAP::Connection #:nodoc: LdapVersion = 3 MaxSaslChallenges = 10 - def initialize(server) + def initialize(server = {}) + @server = server @instrumentation_service = server[:instrumentation_service] - 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 @@ -195,7 +189,7 @@ def message_queue def read(syntax = Net::LDAP::AsnSyntax) ber_object = instrument "read.net_ldap_connection", :syntax => syntax do |payload| - @conn.read_ber(syntax) do |id, content_length| + socket.read_ber(syntax) do |id, content_length| payload[:object_type_id] = id payload[:content_length] = content_length end @@ -225,7 +219,7 @@ def read(syntax = Net::LDAP::AsnSyntax) def write(request, controls = nil, message_id = next_msgid) instrument "write.net_ldap_connection" do |payload| packet = [message_id.to_ber, request, controls].compact.to_ber_sequence - payload[:content_length] = @conn.write(packet) + payload[:content_length] = socket.write(packet) end end private :write @@ -600,4 +594,18 @@ def delete(args) pdu end + + private + + # Returns a Socket like object used internally to communicate with LDAP server + # + # Typically a TCPSocket, but can be a OpenSSL::SSL::SSLSocket + def socket + return @conn if defined? @conn + + # First refactoring uses the existing methods open_connection and + # prepare_socket to set @conn. Next cleanup would centralize connection + # handling here. + open_connection(@server) + end end # class Connection From 4a415bcc4f43c2f40ed037266089a9405b0d2768 Mon Sep 17 00:00:00 2001 From: Jerry Cheung Date: Mon, 9 Nov 2015 23:29:48 -0800 Subject: [PATCH 02/11] preserve existing socket init code --- lib/net/ldap/connection.rb | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/net/ldap/connection.rb b/lib/net/ldap/connection.rb index e987a443..84164ef7 100644 --- a/lib/net/ldap/connection.rb +++ b/lib/net/ldap/connection.rb @@ -606,6 +606,13 @@ def socket # First refactoring uses the existing methods open_connection and # prepare_socket to set @conn. Next cleanup would centralize connection # handling here. - open_connection(@server) + if @server[:socket] + prepare_socket(@server) + else + @server[:hosts] = [[@server[:host], @server[:port]]] if @server[:hosts].nil? + open_connection(@server) + end + + @conn end end # class Connection From b8568061cf1d55966aa87d75bf8825f1fe3e143e Mon Sep 17 00:00:00 2001 From: Jerry Cheung Date: Mon, 9 Nov 2015 23:30:36 -0800 Subject: [PATCH 03/11] #socket internal for easier testing --- lib/net/ldap/connection.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/net/ldap/connection.rb b/lib/net/ldap/connection.rb index 84164ef7..0d419c4d 100644 --- a/lib/net/ldap/connection.rb +++ b/lib/net/ldap/connection.rb @@ -595,9 +595,8 @@ def delete(args) pdu end - private - - # Returns a Socket like object used internally to communicate with LDAP server + # Internal: Returns a Socket like object used internally to communicate with + # LDAP server. # # Typically a TCPSocket, but can be a OpenSSL::SSL::SSLSocket def socket From 76dde7b25b130e7e847b72700fde593a9ca86024 Mon Sep 17 00:00:00 2001 From: Jerry Cheung Date: Mon, 9 Nov 2015 23:30:50 -0800 Subject: [PATCH 04/11] parameterize socket_class for testing --- lib/net/ldap/connection.rb | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/net/ldap/connection.rb b/lib/net/ldap/connection.rb index 0d419c4d..ef703dd2 100644 --- a/lib/net/ldap/connection.rb +++ b/lib/net/ldap/connection.rb @@ -13,6 +13,15 @@ def initialize(server = {}) yield self if block_given? end + # Allows tests to parameterize what socket class to use + def socket_class + @socket_class || TCPSocket + end + + def socket_class=(socket_class) + @socket_class = socket_class + end + def prepare_socket(server) socket = server[:socket] encryption = server[:encryption] @@ -28,7 +37,7 @@ def open_connection(server) errors = [] hosts.each do |host, port| begin - prepare_socket(server.merge(socket: TCPSocket.new(host, port))) + prepare_socket(server.merge(socket: socket_class.new(host, port))) return rescue Net::LDAP::Error, SocketError, SystemCallError, OpenSSL::SSL::SSLError => e From 53cc6b501e5dbe25ab1498968eafec0a7c927fa9 Mon Sep 17 00:00:00 2001 From: Jerry Cheung Date: Mon, 9 Nov 2015 23:32:12 -0800 Subject: [PATCH 05/11] remove tcpsocket stubbing with FakeTCPSocket class --- test/test_ldap_connection.rb | 54 ++++++++++++++++++++++-------------- 1 file changed, 33 insertions(+), 21 deletions(-) diff --git a/test/test_ldap_connection.rb b/test/test_ldap_connection.rb index b4c77615..c75ad410 100644 --- a/test/test_ldap_connection.rb +++ b/test/test_ldap_connection.rb @@ -9,41 +9,53 @@ def capture_stderr $stderr = stderr end + # Fake socket for testing + # + # FakeTCPSocket.new("success", 636) + # FakeTCPSocket.new("fail.SocketError", 636) # raises SocketError + class FakeTCPSocket + def initialize(host, port) + status, error = host.split(".") + if status == "fail" + raise Object.const_get(error) + end + end + end + def test_list_of_hosts_with_first_host_successful hosts = [ - ['test.mocked.com', 636], - ['test2.mocked.com', 636], - ['test3.mocked.com', 636], + ["success.host", 636], + ["fail.SocketError", 636], + ["fail.SocketError", 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) + + connection = Net::LDAP::Connection.new(:hosts => hosts) + connection.socket_class = FakeTCPSocket + connection.socket end def test_list_of_hosts_with_first_host_failure hosts = [ - ['test.mocked.com', 636], - ['test2.mocked.com', 636], - ['test3.mocked.com', 636], + ["fail.SocketError", 636], + ["success.host", 636], + ["fail.SocketError", 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 - Net::LDAP::Connection.new(:hosts => hosts) + connection = Net::LDAP::Connection.new(:hosts => hosts) + connection.socket_class = FakeTCPSocket + connection.socket end def test_list_of_hosts_with_all_hosts_failure hosts = [ - ['test.mocked.com', 636], - ['test2.mocked.com', 636], - ['test3.mocked.com', 636], + ["fail.SocketError", 636], + ["fail.SocketError", 636], + ["fail.SocketError", 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) - flexmock(TCPSocket).should_receive(:new).ordered.never + + connection = Net::LDAP::Connection.new(:hosts => hosts) + connection.socket_class = FakeTCPSocket assert_raise Net::LDAP::ConnectionError do - Net::LDAP::Connection.new(:hosts => hosts) + connection.socket end end From e9a1bf19603e51cd5b3718e30309f574311037e5 Mon Sep 17 00:00:00 2001 From: Jerry Cheung Date: Tue, 10 Nov 2015 00:13:22 -0800 Subject: [PATCH 06/11] add initialize docs --- lib/net/ldap/connection.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/net/ldap/connection.rb b/lib/net/ldap/connection.rb index ef703dd2..6d58f6ea 100644 --- a/lib/net/ldap/connection.rb +++ b/lib/net/ldap/connection.rb @@ -6,6 +6,14 @@ class Net::LDAP::Connection #:nodoc: LdapVersion = 3 MaxSaslChallenges = 10 + # Initialize a connection to an LDAP server + # + # :server + # :hosts Array of tuples specifying host, port + # :host host + # :port port + # :socket prepared socket + # def initialize(server = {}) @server = server @instrumentation_service = server[:instrumentation_service] From 9a2e26ef08e0781b6a1ad294c5c073918f2f2384 Mon Sep 17 00:00:00 2001 From: Jerry Cheung Date: Tue, 10 Nov 2015 00:30:58 -0800 Subject: [PATCH 07/11] preserve existing behavior --- lib/net/ldap.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/net/ldap.rb b/lib/net/ldap.rb index 0ec7fbb7..d952c484 100644 --- a/lib/net/ldap.rb +++ b/lib/net/ldap.rb @@ -1237,12 +1237,16 @@ def use_connection(args) # Establish a new connection to the LDAP server def new_connection - Net::LDAP::Connection.new \ + connection = Net::LDAP::Connection.new \ :host => @host, :port => @port, :hosts => @hosts, :encryption => @encryption, :instrumentation_service => @instrumentation_service + + # Force connect to see if there's a connection error + connection.socket + connection rescue Errno::ECONNREFUSED, Net::LDAP::ConnectionRefusedError => e @result = { :resultCode => 52, From 259f18af42b56efe3d371ba59e9349dda736d55d Mon Sep 17 00:00:00 2001 From: Jerry Cheung Date: Tue, 10 Nov 2015 00:31:17 -0800 Subject: [PATCH 08/11] update tests --- test/test_ldap_connection.rb | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/test/test_ldap_connection.rb b/test/test_ldap_connection.rb index c75ad410..a8620eb7 100644 --- a/test/test_ldap_connection.rb +++ b/test/test_ldap_connection.rb @@ -59,6 +59,7 @@ def test_list_of_hosts_with_all_hosts_failure end end + # This belongs in test_ldap, not test_ldap_connection def test_result_for_connection_failed_is_set flexmock(TCPSocket).should_receive(:new).and_raise(Errno::ECONNREFUSED) @@ -73,33 +74,36 @@ def test_result_for_connection_failed_is_set end def test_unresponsive_host + connection = Net::LDAP::Connection.new(:host => "fail.Errno::ETIMEDOUT", :port => 636) + connection.socket_class = FakeTCPSocket assert_raise Net::LDAP::Error do - Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636) + connection.socket end end def test_blocked_port - flexmock(TCPSocket).should_receive(:new).and_raise(SocketError) + connection = Net::LDAP::Connection.new(:host => "fail.SocketError", :port => 636) + connection.socket_class = FakeTCPSocket assert_raise Net::LDAP::Error do - Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636) + connection.socket end end def test_connection_refused - flexmock(TCPSocket).should_receive(:new).and_raise(Errno::ECONNREFUSED) + connection = Net::LDAP::Connection.new(:host => "fail.Errno::ECONNREFUSED", :port => 636) + connection.socket_class = FakeTCPSocket stderr = capture_stderr do assert_raise Net::LDAP::ConnectionRefusedError do - Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636) + connection.socket end end assert_equal("Deprecation warning: Net::LDAP::ConnectionRefused will be deprecated. Use Errno::ECONNREFUSED instead.\n", stderr) end def test_raises_unknown_exceptions - error = Class.new(StandardError) - flexmock(TCPSocket).should_receive(:new).and_raise(error) - assert_raise error do - Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636) + connection = Net::LDAP::Connection.new(:host => "fail.StandardError", :port => 636) + assert_raise Net::LDAP::Error do + connection.socket end end From f6ad189c2e07f55a9c1c17c54c236a98a5727caa Mon Sep 17 00:00:00 2001 From: Jerry Cheung Date: Tue, 10 Nov 2015 17:30:23 -0800 Subject: [PATCH 09/11] use fake for auth adapter test --- test/test_auth_adapter.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/test_auth_adapter.rb b/test/test_auth_adapter.rb index 7cec57bc..ee7fb4cc 100644 --- a/test/test_auth_adapter.rb +++ b/test/test_auth_adapter.rb @@ -1,9 +1,14 @@ require 'test_helper' class TestAuthAdapter < Test::Unit::TestCase + class FakeSocket + def initialize(*args) + end + end + def test_undefined_auth_adapter - flexmock(TCPSocket).should_receive(:new).ordered.with('ldap.example.com', 379).once.and_return(nil) conn = Net::LDAP::Connection.new(host: 'ldap.example.com', port: 379) + conn.socket_class = FakeSocket assert_raise Net::LDAP::AuthMethodUnsupportedError, "Unsupported auth method (foo)" do conn.bind(method: :foo) end From e7cc5ae51ecf21053d21afa1970eced85106233b Mon Sep 17 00:00:00 2001 From: Jerry Cheung Date: Tue, 10 Nov 2015 17:30:43 -0800 Subject: [PATCH 10/11] replace ldap tests with fake connection object --- lib/net/ldap.rb | 5 +++++ test/test_ldap.rb | 41 +++++++++++++++++++++++++++++++---------- 2 files changed, 36 insertions(+), 10 deletions(-) diff --git a/lib/net/ldap.rb b/lib/net/ldap.rb index d952c484..febad64c 100644 --- a/lib/net/ldap.rb +++ b/lib/net/ldap.rb @@ -1212,6 +1212,11 @@ def inspect inspected end + # Internal: Set @open_connection for testing + def connection=(connection) + @open_connection = connection + end + private # Yields an open connection if there is one, otherwise establishes a new diff --git a/test/test_ldap.rb b/test/test_ldap.rb index f30416b2..b8c8afdf 100644 --- a/test/test_ldap.rb +++ b/test/test_ldap.rb @@ -1,6 +1,28 @@ require 'test_helper' class TestLDAPInstrumentation < Test::Unit::TestCase + # Fake Net::LDAP::Connection for testing + class FakeConnection + # It's difficult to instantiate Net::LDAP::PDU objects. Faking out what we + # need here until that object is brought under test and has it's constructor + # cleaned up. + class Result < Struct.new(:success?, :result_code); end + + def initialize + @bind_success = Result.new(true, Net::LDAP::ResultCodeSuccess) + @search_success = Result.new(true, Net::LDAP::ResultCodeSizeLimitExceeded) + end + + def bind(args = {}) + @bind_success + end + + def search(*args) + yield @search_success if block_given? + @search_success + end + end + def setup @connection = flexmock(:connection, :close => true) flexmock(Net::LDAP::Connection).should_receive(:new).and_return(@connection) @@ -15,8 +37,9 @@ def setup def test_instrument_bind events = @service.subscribe "bind.net_ldap" - bind_result = flexmock(:bind_result, :success? => true) - flexmock(@connection).should_receive(:bind).with(Hash).and_return(bind_result) + fake_connection = FakeConnection.new + @subject.connection = fake_connection + bind_result = fake_connection.bind assert @subject.bind @@ -28,10 +51,9 @@ def test_instrument_bind def test_instrument_search events = @service.subscribe "search.net_ldap" - flexmock(@connection).should_receive(:bind).and_return(flexmock(:bind_result, :result_code => Net::LDAP::ResultCodeSuccess)) - flexmock(@connection).should_receive(:search).with(Hash, Proc). - yields(entry = Net::LDAP::Entry.new("uid=user1,ou=users,dc=example,dc=com")). - and_return(flexmock(:search_result, :success? => true, :result_code => Net::LDAP::ResultCodeSuccess)) + fake_connection = FakeConnection.new + @subject.connection = fake_connection + entry = fake_connection.search refute_nil @subject.search(:filter => "(uid=user1)") @@ -44,10 +66,9 @@ def test_instrument_search def test_instrument_search_with_size events = @service.subscribe "search.net_ldap" - flexmock(@connection).should_receive(:bind).and_return(flexmock(:bind_result, :result_code => Net::LDAP::ResultCodeSuccess)) - flexmock(@connection).should_receive(:search).with(Hash, Proc). - yields(entry = Net::LDAP::Entry.new("uid=user1,ou=users,dc=example,dc=com")). - and_return(flexmock(:search_result, :success? => true, :result_code => Net::LDAP::ResultCodeSizeLimitExceeded)) + fake_connection = FakeConnection.new + @subject.connection = fake_connection + entry = fake_connection.search refute_nil @subject.search(:filter => "(uid=user1)", :size => 1) From 1aab8c9a86d88c378bbc203449341d61d6e7c2f7 Mon Sep 17 00:00:00 2001 From: Jerry Cheung Date: Fri, 8 Jan 2016 10:32:14 -0800 Subject: [PATCH 11/11] set socket_class in initialize --- lib/net/ldap/connection.rb | 26 +++++++++++++------------- test/test_auth_adapter.rb | 3 +-- test/test_ldap_connection.rb | 24 +++++++++--------------- 3 files changed, 23 insertions(+), 30 deletions(-) diff --git a/lib/net/ldap/connection.rb b/lib/net/ldap/connection.rb index e9a79414..39cfd970 100644 --- a/lib/net/ldap/connection.rb +++ b/lib/net/ldap/connection.rb @@ -21,19 +21,10 @@ def initialize(server = {}) @server = server @instrumentation_service = server[:instrumentation_service] - yield self if block_given? - end + # Allows tests to parameterize what socket class to use + @socket_class = server.fetch(:socket_class, DefaultSocket) - # Allows tests to parameterize what socket class to use - def socket_class - @socket_class || DefaultSocket - end - - # Wrap around Socket.tcp to normalize with other Socket initializers - class DefaultSocket - def self.new(host, port, socket_opts = {}) - Socket.tcp(host, port, socket_opts) - end + yield self if block_given? end def socket_class=(socket_class) @@ -59,7 +50,7 @@ def open_connection(server) errors = [] hosts.each do |host, port| begin - prepare_socket(server.merge(socket: socket_class.new(host, port, socket_opts))) + prepare_socket(server.merge(socket: @socket_class.new(host, port, socket_opts))) return rescue Net::LDAP::Error, SocketError, SystemCallError, OpenSSL::SSL::SSLError => e @@ -690,4 +681,13 @@ def socket @conn end + + private + + # Wrap around Socket.tcp to normalize with other Socket initializers + class DefaultSocket + def self.new(host, port, socket_opts = {}) + Socket.tcp(host, port, socket_opts) + end + end end # class Connection diff --git a/test/test_auth_adapter.rb b/test/test_auth_adapter.rb index ee7fb4cc..9e4c6002 100644 --- a/test/test_auth_adapter.rb +++ b/test/test_auth_adapter.rb @@ -7,8 +7,7 @@ def initialize(*args) end def test_undefined_auth_adapter - conn = Net::LDAP::Connection.new(host: 'ldap.example.com', port: 379) - conn.socket_class = FakeSocket + conn = Net::LDAP::Connection.new(host: 'ldap.example.com', port: 379, :socket_class => FakeSocket) assert_raise Net::LDAP::AuthMethodUnsupportedError, "Unsupported auth method (foo)" do conn.bind(method: :foo) end diff --git a/test/test_ldap_connection.rb b/test/test_ldap_connection.rb index 12ca3d71..51e30c3f 100644 --- a/test/test_ldap_connection.rb +++ b/test/test_ldap_connection.rb @@ -29,8 +29,7 @@ def test_list_of_hosts_with_first_host_successful ["fail.SocketError", 636], ] - connection = Net::LDAP::Connection.new(:hosts => hosts) - connection.socket_class = FakeTCPSocket + connection = Net::LDAP::Connection.new(:hosts => hosts, :socket_class => FakeTCPSocket) connection.socket end @@ -41,8 +40,7 @@ def test_list_of_hosts_with_first_host_failure ["fail.SocketError", 636], ] - connection = Net::LDAP::Connection.new(:hosts => hosts) - connection.socket_class = FakeTCPSocket + connection = Net::LDAP::Connection.new(:hosts => hosts, :socket_class => FakeTCPSocket) connection.socket end @@ -53,8 +51,7 @@ def test_list_of_hosts_with_all_hosts_failure ["fail.SocketError", 636], ] - connection = Net::LDAP::Connection.new(:hosts => hosts) - connection.socket_class = FakeTCPSocket + connection = Net::LDAP::Connection.new(:hosts => hosts, :socket_class => FakeTCPSocket) assert_raise Net::LDAP::ConnectionError do connection.socket end @@ -75,24 +72,21 @@ def test_result_for_connection_failed_is_set end def test_unresponsive_host - connection = Net::LDAP::Connection.new(:host => "fail.Errno::ETIMEDOUT", :port => 636) - connection.socket_class = FakeTCPSocket + connection = Net::LDAP::Connection.new(:host => "fail.Errno::ETIMEDOUT", :port => 636, :socket_class => FakeTCPSocket) assert_raise Net::LDAP::Error do connection.socket end end def test_blocked_port - connection = Net::LDAP::Connection.new(:host => "fail.SocketError", :port => 636) - connection.socket_class = FakeTCPSocket + connection = Net::LDAP::Connection.new(:host => "fail.SocketError", :port => 636, :socket_class => FakeTCPSocket) assert_raise Net::LDAP::Error do connection.socket end end def test_connection_refused - connection = Net::LDAP::Connection.new(:host => "fail.Errno::ECONNREFUSED", :port => 636) - connection.socket_class = FakeTCPSocket + connection = Net::LDAP::Connection.new(:host => "fail.Errno::ECONNREFUSED", :port => 636, :socket_class => FakeTCPSocket) stderr = capture_stderr do assert_raise Net::LDAP::ConnectionRefusedError do connection.socket @@ -102,7 +96,7 @@ def test_connection_refused end def test_connection_timeout - connection = Net::LDAP::Connection.new(:host => "fail.Errno::ETIMEDOUT", :port => 636) + connection = Net::LDAP::Connection.new(:host => "fail.Errno::ETIMEDOUT", :port => 636, :socket_class => FakeTCPSocket) stderr = capture_stderr do assert_raise Net::LDAP::Error do connection.socket @@ -111,8 +105,8 @@ def test_connection_timeout end def test_raises_unknown_exceptions - connection = Net::LDAP::Connection.new(:host => "fail.StandardError", :port => 636) - assert_raise Net::LDAP::Error do + connection = Net::LDAP::Connection.new(:host => "fail.StandardError", :port => 636, :socket_class => FakeTCPSocket) + assert_raise StandardError do connection.socket end end