Merge branch 'ee-fj-174-better-ldap-connection-handling' into 'master'

CE port: Add better LDAP connection handling

Closes #480 and #4941

See merge request gitlab-org/gitlab-ee!5173

app/controllers/application_controller.rb

@@ -237,10 +237,6 @@ class ApplicationController < ActionController::Base
     @event_filter ||= EventFilter.new(filters)
   end
 
-  def gitlab_ldap_access(&block)
-    Gitlab::Auth::LDAP::Access.open { |access| yield(access) }
-  end
-
   # JSON for infinite scroll via Pager object
   def pager_json(partial, count, locals = {})
     html = render_to_string(

ee/changelogs/unreleased/ee-fj-174-better-ldap-connection-handling.yml

+---
+title: Add better LDAP connection handling in EE and fixing some LDAP group syncing problems
+merge_request: 5173
+author:
+type: fixed

ee/lib/ee/gitlab/auth/ldap/sync/group.rb

@@ -11,35 +11,45 @@ module EE
               def execute_all_providers(group)
                 return unless ldap_sync_ready?(group)
 
-                group.start_ldap_sync
-                Rails.logger.debug { "Started syncing all providers for '#{group.name}' group" }
-
-                # Shuffle providers to prevent a scenario where sync fails after a time
-                # and only the first provider or two get synced. This shuffles the order
-                # so subsequent syncs should eventually get to all providers. Obviously
-                # we should avoid failure, but this is an additional safeguard.
-                ::Gitlab::Auth::LDAP::Config.providers.shuffle.each do |provider|
-                  Sync::Proxy.open(provider) do |proxy|
-                    new(group, proxy).update_permissions
+                begin
+                  group.start_ldap_sync
+                  Rails.logger.debug { "Started syncing all providers for '#{group.name}' group" }
+
+                  # Shuffle providers to prevent a scenario where sync fails after a time
+                  # and only the first provider or two get synced. This shuffles the order
+                  # so subsequent syncs should eventually get to all providers. Obviously
+                  # we should avoid failure, but this is an additional safeguard.
+                  ::Gitlab::Auth::LDAP::Config.providers.shuffle.each do |provider|
+                    Sync::Proxy.open(provider) do |proxy|
+                      new(group, proxy).update_permissions
+                    end
                   end
-                end
 
-                group.finish_ldap_sync
-                Rails.logger.debug { "Finished syncing all providers for '#{group.name}' group" }
+                  group.finish_ldap_sync
+                  Rails.logger.debug { "Finished syncing all providers for '#{group.name}' group" }
+                rescue ::Gitlab::Auth::LDAP::LDAPConnectionError
+                  Rails.logger.warn("Error syncing all providers for '#{group.name}' group")
+                  group.fail_ldap_sync
+                end
               end
 
               # Sync members across a single provider for the given group.
               def execute(group, proxy)
                 return unless ldap_sync_ready?(group)
 
-                group.start_ldap_sync
-                Rails.logger.debug { "Started syncing '#{proxy.provider}' provider for '#{group.name}' group" }
+                begin
+                  group.start_ldap_sync
+                  Rails.logger.debug { "Started syncing '#{proxy.provider}' provider for '#{group.name}' group" }
 
-                sync_group = new(group, proxy)
-                sync_group.update_permissions
+                  sync_group = new(group, proxy)
+                  sync_group.update_permissions
 
-                group.finish_ldap_sync
-                Rails.logger.debug { "Finished syncing '#{proxy.provider}' provider for '#{group.name}' group" }
+                  group.finish_ldap_sync
+                  Rails.logger.debug { "Finished syncing '#{proxy.provider}' provider for '#{group.name}' group" }
+                rescue ::Gitlab::Auth::LDAP::LDAPConnectionError
+                  Rails.logger.warn("Error syncing '#{proxy.provider}' provider for '#{group.name}' group")
+                  group.fail_ldap_sync
+                end
               end
 
               def ldap_sync_ready?(group)

ee/lib/ee/gitlab/auth/ldap/sync/groups.rb

@@ -27,14 +27,10 @@ module EE
             end
 
             def update_permissions
-              logger.debug { "Performing LDAP group sync for '#{provider}' provider" }
               sync_groups
-              logger.debug { "Finished LDAP group sync for '#{provider}' provider" }
 
               if config.admin_group.present?
-                logger.debug { "Syncing admin users for '#{provider}' provider" }
                 sync_admin_users
-                logger.debug { "Finished syncing admin users for '#{provider}' provider" }
               else
                 logger.debug { "No `admin_group` configured for '#{provider}' provider. Skipping" }
               end
@@ -42,9 +38,7 @@ module EE
               if config.external_groups.empty?
                 logger.debug { "No `external_groups` configured for '#{provider}' provider. Skipping" }
               else
-                logger.debug { "Syncing external users for '#{provider}' provider" }
                 sync_external_users
-                logger.debug { "Finished syncing external users for '#{provider}' provider" }
               end
 
               nil
@@ -53,17 +47,33 @@ module EE
             private
 
             def sync_groups
+              logger.debug { "Performing LDAP group sync for '#{provider}' provider" }
+
               groups_where_group_links_with_provider_ordered.each do |group|
                 Sync::Group.execute(group, proxy)
               end
+
+              logger.debug { "Finished LDAP group sync for '#{provider}' provider" }
             end
 
             def sync_admin_users
-              Sync::AdminUsers.execute(proxy)
+              logger.debug { "Syncing admin users for '#{provider}' provider" }
+
+              if Sync::AdminUsers.execute(proxy)
+                logger.debug { "Finished syncing admin users for '#{provider}' provider" }
+              else
+                logger.debug { "Error syncing admin users for '#{provider}' provider. LDAP connection error" }
+              end
             end
 
             def sync_external_users
-              Sync::ExternalUsers.execute(proxy)
+              logger.debug { "Syncing external users for '#{provider}' provider" }
+
+              if Sync::ExternalUsers.execute(proxy)
+                logger.debug { "Finished syncing external users for '#{provider}' provider" }
+              else
+                logger.debug { "Error syncing external users for '#{provider}' provider. LDAP connection error" }
+              end
             end
 
             def groups_where_group_links_with_provider_ordered

ee/lib/ee/gitlab/auth/ldap/sync/users.rb

@@ -17,7 +17,7 @@ module EE
 
             def update_permissions
               dns = member_dns
-              return if dns.empty?
+              return true if dns.empty?
 
               current_users_with_attribute = ::User.with_provider(provider).where(attribute => true)
               verified_users_with_attribute = []
@@ -33,6 +33,12 @@ module EE
                 user[attribute] = false
                 user.save
               end
+
+              true
+            rescue ::Gitlab::Auth::LDAP::LDAPConnectionError
+              Rails.logger.warn("Error syncing #{attribute} users for provider '#{provider}'. LDAP connection Error")
+
+              false
             end
 
             private

ee/spec/lib/ee/gitlab/auth/ldap/sync/admin_users_spec.rb

@@ -51,5 +51,21 @@ describe EE::Gitlab::Auth::LDAP::Sync::AdminUsers do
       expect { sync_admin.update_permissions }
         .not_to change { admin.reload.admin? }
     end
+
+    context 'when ldap connection fails' do
+      before do
+        unstub_ldap_group_find_by_cn
+        raise_ldap_connection_error
+      end
+
+      it 'logs a debug message' do
+        expect(Rails.logger)
+          .to receive(:warn)
+                .with("Error syncing admin users for provider 'ldapmain'. LDAP connection Error")
+                .at_least(:once)
+
+        sync_admin.update_permissions
+      end
+    end
   end
 end

ee/spec/lib/ee/gitlab/auth/ldap/sync/external_users_spec.rb

@@ -52,5 +52,21 @@ describe EE::Gitlab::Auth::LDAP::Sync::ExternalUsers do
       expect { sync_external.update_permissions }
         .not_to change { user.reload.external? }
     end
+
+    context 'when ldap connection fails' do
+      before do
+        unstub_ldap_group_find_by_cn
+        raise_ldap_connection_error
+      end
+
+      it 'logs a debug message' do
+        expect(Rails.logger)
+          .to receive(:warn)
+                .with("Error syncing external users for provider 'ldapmain'. LDAP connection Error")
+                .at_least(:once)
+
+        sync_external.update_permissions
+      end
+    end
   end
 end

ee/spec/lib/ee/gitlab/auth/ldap/sync/group_spec.rb

@@ -36,7 +36,7 @@ describe EE::Gitlab::Auth::LDAP::Sync::Group do
       execute
     end
 
-    context 'when the group ldap sync is already started' do
+    context 'when the group ldap sync has already started' do
       it 'logs a debug message' do
         group.start_ldap_sync
 
@@ -57,6 +57,26 @@ describe EE::Gitlab::Auth::LDAP::Sync::Group do
         execute
       end
     end
+
+    context 'when ldap connection fails' do
+      before do
+        unstub_ldap_group_find_by_cn
+        raise_ldap_connection_error
+      end
+
+      it 'logs a debug message' do
+        expect(Rails.logger)
+          .to receive(:warn).at_least(:once)
+
+        execute
+      end
+
+      it 'ensures group state returns to failed_ldap_sync' do
+        execute
+
+        expect(group.ldap_sync_failed?).to be_truthy
+      end
+    end
   end
 
   describe '.execute_all_providers' do
@@ -65,10 +85,10 @@ describe EE::Gitlab::Auth::LDAP::Sync::Group do
     end
 
     before do
-      stub_ldap_config(providers: %w[main secundary])
+      stub_ldap_config(providers: %w[ldapmain secundary])
 
-      adapter = ldap_adapter('main')
-      proxy = proxy(adapter, 'main')
+      adapter = ldap_adapter('ldapmain')
+      proxy = proxy(adapter, 'ldapmain')
       allow(EE::Gitlab::Auth::LDAP::Sync::Proxy).to receive(:open).and_yield(proxy)
     end
 

ee/spec/lib/ee/gitlab/auth/ldap/sync/proxy_spec.rb

@@ -102,6 +102,16 @@ describe EE::Gitlab::Auth::LDAP::Sync::Proxy do
         expect(sync_proxy.dns_for_group_cn('ldap_group1')).to match_array(dns)
       end
     end
+
+    context 'when there is a connection problem' do
+      before do
+        raise_ldap_connection_error
+      end
+
+      it 'raises exception' do
+        expect { sync_proxy.dns_for_group_cn('ldap_group1') }.to raise_error(::Gitlab::Auth::LDAP::LDAPConnectionError)
+      end
+    end
   end
 
   describe '#dn_for_uid' do
@@ -187,5 +197,15 @@ describe EE::Gitlab::Auth::LDAP::Sync::Proxy do
                 .once.and_call_original
       end
     end
+
+    context 'when there is a connection problem' do
+      before do
+        raise_ldap_connection_error
+      end
+
+      it 'raises exception' do
+        expect { sync_proxy.dns_for_group_cn('ldap_group1') }.to raise_error(::Gitlab::Auth::LDAP::LDAPConnectionError)
+      end
+    end
   end
 end

ee/spec/support/ee/ldap_helpers.rb

@@ -22,6 +22,11 @@ module EE
           .with(cn, kind_of(::Gitlab::Auth::LDAP::Adapter)).and_return(return_value)
     end
 
+    def unstub_ldap_group_find_by_cn
+      allow(EE::Gitlab::Auth::LDAP::Group)
+        .to receive(:find_by_cn).and_call_original
+    end
+
     # Create an LDAP group entry with any number of members. By default, creates
     # a groupOfNames style entry. Change the style by specifying the object class
     # and member attribute name. The last example below shows how to specify a

lib/gitlab/auth/ldap/access.rb

@@ -56,6 +56,8 @@ module Gitlab
             block_user(user, 'does not exist anymore')
             false
           end
+        rescue LDAPConnectionError
+          false
         end
 
         def adapter

lib/gitlab/auth/ldap/adapter.rb

@@ -4,6 +4,9 @@ module Gitlab
       class Adapter
         prepend ::EE::Gitlab::Auth::LDAP::Adapter
 
+        SEARCH_RETRY_FACTOR = [1, 1, 2, 3].freeze
+        MAX_SEARCH_RETRIES = Rails.env.test? ? 1 : SEARCH_RETRY_FACTOR.size.freeze
+
         attr_reader :provider, :ldap
 
         def self.open(provider, &block)
@@ -18,7 +21,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
@@ -49,8 +52,10 @@ module Gitlab
         end
 
         def ldap_search(*args)
+          retries ||= 0
+
           # Net::LDAP's `time` argument doesn't work. Use Ruby `Timeout` instead.
-          Timeout.timeout(config.timeout) do
+          Timeout.timeout(timeout_time(retries)) do
             results = ldap.search(*args)
 
             if results.nil?
@@ -65,16 +70,26 @@ module Gitlab
               results
             end
           end
-        rescue Net::LDAP::Error => error
-          Rails.logger.warn("LDAP search raised exception #{error.class}: #{error.message}")
-          []
-        rescue Timeout::Error
-          Rails.logger.warn("LDAP search timed out after #{config.timeout} seconds")
-          []
+        rescue Net::LDAP::Error, Timeout::Error => error
+          retries += 1
+          error_message = connection_error_message(error)
+
+          Rails.logger.warn(error_message)
+
+          if retries < MAX_SEARCH_RETRIES
+            renew_connection_adapter
+            retry
+          else
+            raise LDAPConnectionError, error_message
+          end
         end
 
         private
 
+        def timeout_time(retry_number)
+          SEARCH_RETRY_FACTOR[retry_number] * config.timeout
+        end
+
         def user_options(fields, value, limit)
           options = {
             attributes: Gitlab::Auth::LDAP::Person.ldap_attributes(config),
@@ -106,6 +121,18 @@ module Gitlab
             filter
           end
         end
+
+        def connection_error_message(exception)
+          if exception.is_a?(Timeout::Error)
+            "LDAP search timed out after #{config.timeout} seconds"
+          else
+            "LDAP search raised exception #{exception.class}: #{exception.message}"
+          end
+        end
+
+        def renew_connection_adapter
+          @ldap = Net::LDAP.new(config.adapter_options)
+        end
       end
     end
   end

lib/gitlab/auth/ldap/ldap_connection_error.rb

+module Gitlab
+  module Auth
+    module LDAP
+      LDAPConnectionError = Class.new(StandardError)
+    end
+  end
+end

lib/gitlab/auth/o_auth/user.rb

@@ -126,6 +126,8 @@ module Gitlab
           Gitlab::Auth::LDAP::Person.find_by_uid(auth_hash.uid, adapter) ||
             Gitlab::Auth::LDAP::Person.find_by_email(auth_hash.uid, adapter) ||
             Gitlab::Auth::LDAP::Person.find_by_dn(auth_hash.uid, adapter)
+        rescue Gitlab::Auth::LDAP::LDAPConnectionError
+          nil
         end
 
         def ldap_config

spec/lib/gitlab/auth/ldap/access_spec.rb

@@ -38,6 +38,7 @@ describe Gitlab::Auth::LDAP::Access do
     context 'when the user cannot be found' do
       before do
         allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(nil)
+        allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_email).and_return(nil)
       end
 
       it { is_expected.to be_falsey }
@@ -56,8 +57,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 +123,7 @@ describe Gitlab::Auth::LDAP::Access do
         context 'when user cannot be found' do
           before do
             allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(nil)
+            allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_email).and_return(nil)
           end
 
           it { is_expected.to be_falsey }
@@ -142,6 +146,34 @@ describe Gitlab::Auth::LDAP::Access do
             access.allowed?
           end
         end
+
+        context 'when user was previously ldap_blocked' do
+          before do
+            user.ldap_block
+          end
+
+          it 'unblocks the user if it exists' do
+            expect(access).to receive(:unblock_user).with(user, 'is available again')
+
+            access.allowed?
+          end
+        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

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

spec/lib/gitlab/auth/o_auth/user_spec.rb

 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

spec/support/ldap_helpers.rb

@@ -49,4 +49,9 @@ module LdapHelpers
 
     entry
   end
+
+  def raise_ldap_connection_error
+    allow_any_instance_of(Gitlab::Auth::LDAP::Adapter)
+      .to receive(:ldap_search).and_raise(Gitlab::Auth::LDAP::LDAPConnectionError)
+  end
 end