Add better LDAP connection handling

3 files changed, 99 insertions, 19 deletions

diff --git a/spec/lib/gitlab/auth/ldap/access_spec.rb b/spec/lib/gitlab/auth/ldap/access_spec.rb
index 9b3916bf9e3..6b251d824f7 100644
--- a/spec/lib/gitlab/auth/ldap/access_spec.rb
+++ b/spec/lib/gitlab/auth/ldap/access_spec.rb
@@ -1,6 +1,8 @@
 require 'spec_helper'
 
 describe Gitlab::Auth::LDAP::Access do
+  include LdapHelpers
+
   let(:access) { described_class.new user }
   let(:user) { create(:omniauth_user) }
 
@@ -32,8 +34,10 @@ describe Gitlab::Auth::LDAP::Access do
     end
 
     context 'when the user is found' do
+      let(:ldap_user) { Gitlab::Auth::LDAP::Person.new(Net::LDAP::Entry.new, 'ldapmain') }
+
       before do
-        allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(:ldap_user)
+        allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(ldap_user)
       end
 
       context 'and the user is disabled via active directory' do
@@ -120,6 +124,22 @@ describe Gitlab::Auth::LDAP::Access do
         end
       end
     end
+
+    context 'when the connection fails' do
+      before do
+        raise_ldap_connection_error
+      end
+
+      it 'does not block the user' do
+        access.allowed?
+
+        expect(user.ldap_blocked?).to be_falsey
+      end
+
+      it 'denies access' do
+        expect(access.allowed?).to be_falsey
+      end
+    end
   end
 
   describe '#block_user' do
diff --git a/spec/lib/gitlab/auth/ldap/adapter_spec.rb b/spec/lib/gitlab/auth/ldap/adapter_spec.rb
index 10c60d792bd..3eeaf3862f6 100644
--- a/spec/lib/gitlab/auth/ldap/adapter_spec.rb
+++ b/spec/lib/gitlab/auth/ldap/adapter_spec.rb
@@ -124,16 +124,36 @@ describe Gitlab::Auth::LDAP::Adapter do
 
     context "when the search raises an LDAP exception" do
       before do
+        allow(adapter).to receive(:renew_connection_adapter).and_return(ldap)
         allow(ldap).to receive(:search) { raise Net::LDAP::Error, "some error" }
         allow(Rails.logger).to receive(:warn)
       end
 
-      it { is_expected.to eq [] }
+      context 'retries the operation' do
+        before do
+          stub_const("#{described_class}::MAX_SEARCH_RETRIES", 3)
+        end
+
+        it 'as many times as MAX_SEARCH_RETRIES' do
+          expect(ldap).to receive(:search).exactly(3).times
+          expect { subject }.to raise_error(Gitlab::Auth::LDAP::LDAPConnectionError)
+        end
+
+        context 'when no more retries' do
+          before do
+            stub_const("#{described_class}::MAX_SEARCH_RETRIES", 1)
+          end
 
-      it 'logs the error' do
-        subject
-        expect(Rails.logger).to have_received(:warn).with(
-          "LDAP search raised exception Net::LDAP::Error: some error")
+          it 'raises the exception' do
+            expect { subject }.to raise_error(Gitlab::Auth::LDAP::LDAPConnectionError)
+          end
+
+          it 'logs the error' do
+            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
+        end
       end
     end
   end
diff --git a/spec/lib/gitlab/auth/o_auth/user_spec.rb b/spec/lib/gitlab/auth/o_auth/user_spec.rb
index 0c71f1d8ca6..64f3d09a25b 100644
--- a/spec/lib/gitlab/auth/o_auth/user_spec.rb
+++ b/spec/lib/gitlab/auth/o_auth/user_spec.rb
@@ -1,6 +1,8 @@
 require 'spec_helper'
 
 describe Gitlab::Auth::OAuth::User do
+  include LdapHelpers
+
   let(:oauth_user) { described_class.new(auth_hash) }
   let(:gl_user) { oauth_user.gl_user }
   let(:uid) { 'my-uid' }
@@ -38,10 +40,6 @@ describe Gitlab::Auth::OAuth::User do
   end
 
   describe '#save' do
-    def stub_ldap_config(messages)
-      allow(Gitlab::Auth::LDAP::Config).to receive_messages(messages)
-    end
-
     let(:provider) { 'twitter' }
 
     describe 'when account exists on server' do
@@ -269,20 +267,47 @@ describe Gitlab::Auth::OAuth::User do
             end
 
             context 'when an LDAP person is not found by uid' do
-              it 'tries to find an LDAP person by DN and adds the omniauth identity to the user' do
+              it 'tries to find an LDAP person by email and adds the omniauth identity to the user' do
                 allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_uid).and_return(nil)
-                allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(ldap_user)
+                allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_email).and_return(ldap_user)
+
+                oauth_user.save
+
+                identities_as_hash = gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } }
+                expect(identities_as_hash).to match_array(result_identities(dn, uid))
+              end
+
+              context 'when also not found by email' do
+                it 'tries to find an LDAP person by DN and adds the omniauth identity to the user' do
+                  allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_uid).and_return(nil)
+                  allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_email).and_return(nil)
+                  allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(ldap_user)
+
+                  oauth_user.save
+
+                  identities_as_hash = gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } }
+                  expect(identities_as_hash).to match_array(result_identities(dn, uid))
+                end
+              end
+            end
 
+            def result_identities(dn, uid)
+              [
+                { provider: 'ldapmain', extern_uid: dn },
+                { provider: 'twitter', extern_uid: uid }
+              ]
+            end
+
+            context 'when there is an LDAP connection error' do
+              before do
+                raise_ldap_connection_error
+              end
+
+              it 'does not save the identity' do
                 oauth_user.save
 
                 identities_as_hash = gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } }
-                expect(identities_as_hash)
-                  .to match_array(
-                    [
-                      { provider: 'ldapmain', extern_uid: dn },
-                      { provider: 'twitter', extern_uid: uid }
-                    ]
-                  )
+                expect(identities_as_hash).to match_array([{ provider: 'twitter', extern_uid: uid }])
               end
             end
           end
@@ -739,4 +764,19 @@ describe Gitlab::Auth::OAuth::User do
       expect(oauth_user.find_user).to eql gl_user
     end
   end
+
+  describe '#find_ldap_person' do
+    context 'when LDAP connection fails' do
+      before do
+        raise_ldap_connection_error
+      end
+
+      it 'returns nil' do
+        adapter = Gitlab::Auth::LDAP::Adapter.new('ldapmain')
+        hash = OmniAuth::AuthHash.new(uid: 'whatever', provider: 'ldapmain')
+
+        expect(oauth_user.send(:find_ldap_person, hash, adapter)).to be_nil
+      end
+    end
+  end
 end