Code review comments appliedfj-174-better-ldap-connection-handling

6 files changed, 16 insertions, 9 deletions

diff --git a/lib/gitlab/auth/ldap/access.rb b/lib/gitlab/auth/ldap/access.rb
index ecfeef3246f..15282338c8d 100644
--- a/lib/gitlab/auth/ldap/access.rb
+++ b/lib/gitlab/auth/ldap/access.rb
@@ -52,7 +52,7 @@ module Gitlab
             block_user(user, 'does not exist anymore')
             false
           end
-        rescue Net::LDAP::Error, Timeout::Error
+        rescue Gitlab::Auth::LDAP::LDAPConnectionError
           false
         end
 
diff --git a/lib/gitlab/auth/ldap/adapter.rb b/lib/gitlab/auth/ldap/adapter.rb
index 6b1b35bcd1c..e87b8798a25 100644
--- a/lib/gitlab/auth/ldap/adapter.rb
+++ b/lib/gitlab/auth/ldap/adapter.rb
@@ -19,7 +19,7 @@ module Gitlab
 
         def initialize(provider, ldap = nil)
           @provider = provider
-          @ldap = ldap || Net::LDAP.new(config.adapter_options)
+          @ldap = ldap || renew_connection_adapter
         end
 
         def config
@@ -76,7 +76,7 @@ module Gitlab
             renew_connection_adapter
             retry
           else
-            raise
+            raise Gitlab::Auth::LDAP::LDAPConnectionError
           end
         end
 
@@ -119,7 +119,7 @@ module Gitlab
         end
 
         def connection_error_message(exception)
-          if exception.class == Timeout::Error
+          if exception.class.is_a?(Timeout::Error)
             "LDAP search timed out after #{config.timeout} seconds"
           else
             "LDAP search raised exception #{exception.class}: #{exception.message}"
diff --git a/lib/gitlab/auth/ldap/ldap_connection_error.rb b/lib/gitlab/auth/ldap/ldap_connection_error.rb
new file mode 100644
index 00000000000..ef0a695742b
--- /dev/null
+++ b/lib/gitlab/auth/ldap/ldap_connection_error.rb
@@ -0,0 +1,7 @@
+module Gitlab
+  module Auth
+    module LDAP
+      LDAPConnectionError = Class.new(StandardError)
+    end
+  end
+end
diff --git a/lib/gitlab/auth/o_auth/user.rb b/lib/gitlab/auth/o_auth/user.rb
index b74e2e6d004..d0c6b0386ba 100644
--- a/lib/gitlab/auth/o_auth/user.rb
+++ b/lib/gitlab/auth/o_auth/user.rb
@@ -125,7 +125,7 @@ module Gitlab
             Gitlab::Auth::LDAP::Person.find_by_email(auth_hash.uid, adapter) ||
             Gitlab::Auth::LDAP::Person.find_by_dn(auth_hash.uid, adapter)
 
-        rescue Net::LDAP::Error, Timeout::Error
+        rescue Gitlab::Auth::LDAP::LDAPConnectionError
           nil
         end
 
diff --git a/spec/lib/gitlab/auth/ldap/adapter_spec.rb b/spec/lib/gitlab/auth/ldap/adapter_spec.rb
index ee90c08bfa3..3eeaf3862f6 100644
--- a/spec/lib/gitlab/auth/ldap/adapter_spec.rb
+++ b/spec/lib/gitlab/auth/ldap/adapter_spec.rb
@@ -136,7 +136,7 @@ describe Gitlab::Auth::LDAP::Adapter do
 
         it 'as many times as MAX_SEARCH_RETRIES' do
           expect(ldap).to receive(:search).exactly(3).times
-          expect { subject }.to raise_error(Net::LDAP::Error)
+          expect { subject }.to raise_error(Gitlab::Auth::LDAP::LDAPConnectionError)
         end
 
         context 'when no more retries' do
@@ -145,11 +145,11 @@ describe Gitlab::Auth::LDAP::Adapter do
           end
 
           it 'raises the exception' do
-            expect { subject }.to raise_error(Net::LDAP::Error)
+            expect { subject }.to raise_error(Gitlab::Auth::LDAP::LDAPConnectionError)
           end
 
           it 'logs the error' do
-            expect { subject }.to raise_error(Net::LDAP::Error)
+            expect { subject }.to raise_error(Gitlab::Auth::LDAP::LDAPConnectionError)
             expect(Rails.logger).to have_received(:warn).with(
               "LDAP search raised exception Net::LDAP::Error: some error")
           end
diff --git a/spec/support/ldap_helpers.rb b/spec/support/ldap_helpers.rb
index 975194d10d5..0e87b3d359d 100644
--- a/spec/support/ldap_helpers.rb
+++ b/spec/support/ldap_helpers.rb
@@ -44,6 +44,6 @@ module LdapHelpers
 
   def raise_ldap_connection_error
     allow_any_instance_of(Gitlab::Auth::LDAP::Adapter)
-      .to receive(:ldap_search).and_raise(Timeout::Error)
+      .to receive(:ldap_search).and_raise(Gitlab::Auth::LDAP::LDAPConnectionError)
   end
 end