From 67aa0b8c4cbf762211ad178efb537f1649d91776 Mon Sep 17 00:00:00 2001
From: Drew Blessing <drew@gitlab.com>
Date: Thu, 31 Dec 2015 13:22:51 -0600
Subject: [PATCH] Optimize LDAP and add a search timeout

---
 CHANGELOG                         |  1 +
 config/gitlab.yml.example         |  5 +++++
 config/initializers/1_settings.rb |  1 +
 doc/integration/ldap.md           |  5 +++++
 lib/gitlab/ldap/access.rb         |  8 ++++++--
 lib/gitlab/ldap/adapter.rb        | 24 +++++++++++++++---------
 lib/gitlab/ldap/config.rb         |  4 ++++
 7 files changed, 37 insertions(+), 11 deletions(-)

diff --git a/CHANGELOG b/CHANGELOG
index 9ff4820c12c..ea05231937e 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -37,6 +37,7 @@ v 8.4.0 (unreleased)
 v 8.3.3 (unreleased)
   - Preserve CE behavior with JIRA integration by only calling API if URL is set
   - Fix duplicated branch creation/deletion events when using Web UI (Stan Hu)
+  - Add configurable LDAP server query timeout
   - Get "Merge when build succeeds" to work when commits were pushed to MR target branch while builds were running
   - Suppress e-mails on failed builds if allow_failure is set (Stan Hu)
   - Fix project transfer e-mail sending incorrect paths in e-mail notification (Stan Hu)
diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example
index 2d9f730c183..d6e2c9380a5 100644
--- a/config/gitlab.yml.example
+++ b/config/gitlab.yml.example
@@ -204,6 +204,11 @@ production: &base
         bind_dn: '_the_full_dn_of_the_user_you_will_bind_with'
         password: '_the_password_of_the_bind_user'
 
+        # Set a timeout, in seconds, for LDAP queries. This helps avoid blocking
+        # a request if the LDAP server becomes unresponsive.
+        # A value of 0 means there is no timeout.
+        timeout: 10
+
         # This setting specifies if LDAP server is Active Directory LDAP server.
         # For non AD servers it skips the AD specific queries.
         # If your LDAP server is not AD, set this to false.
diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb
index 4fbd84ee890..a9c5b2caf0a 100644
--- a/config/initializers/1_settings.rb
+++ b/config/initializers/1_settings.rb
@@ -108,6 +108,7 @@ if Settings.ldap['enabled'] || Rails.env.test?
 
   Settings.ldap['servers'].each do |key, server|
     server['label'] ||= 'LDAP'
+    server['timeout'] ||= 10.seconds
     server['block_auto_created_users'] = false if server['block_auto_created_users'].nil?
     server['allow_username_or_email_login'] = false if server['allow_username_or_email_login'].nil?
     server['active_directory'] = true if server['active_directory'].nil?
diff --git a/doc/integration/ldap.md b/doc/integration/ldap.md
index 845f588f913..f256477196b 100644
--- a/doc/integration/ldap.md
+++ b/doc/integration/ldap.md
@@ -48,6 +48,11 @@ main: # 'main' is the GitLab 'provider ID' of this LDAP server
   bind_dn: '_the_full_dn_of_the_user_you_will_bind_with'
   password: '_the_password_of_the_bind_user'
 
+  # Set a timeout, in seconds, for LDAP queries. This helps avoid blocking
+  # a request if the LDAP server becomes unresponsive.
+  # A value of 0 means there is no timeout.
+  timeout: 10
+
   # This setting specifies if LDAP server is Active Directory LDAP server.
   # For non AD servers it skips the AD specific queries.
   # If your LDAP server is not AD, set this to false.
diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb
index c438a3d167b..b2bdbc10d7f 100644
--- a/lib/gitlab/ldap/access.rb
+++ b/lib/gitlab/ldap/access.rb
@@ -5,7 +5,7 @@
 module Gitlab
   module LDAP
     class Access
-      attr_reader :adapter, :provider, :user
+      attr_reader :provider, :user
 
       def self.open(user, &block)
         Gitlab::LDAP::Adapter.open(user.ldap_identity.provider) do |adapter|
@@ -32,7 +32,7 @@ module Gitlab
       end
 
       def allowed?
-        if Gitlab::LDAP::Person.find_by_dn(user.ldap_identity.extern_uid, adapter)
+        if ldap_user
           return true unless ldap_config.active_directory
 
           # Block user in GitLab if he/she was blocked in AD
@@ -59,6 +59,10 @@ module Gitlab
       def ldap_config
         Gitlab::LDAP::Config.new(provider)
       end
+
+      def ldap_user
+        @ldap_user ||= Gitlab::LDAP::Person.find_by_dn(user.ldap_identity.extern_uid, adapter)
+      end
     end
   end
 end
diff --git a/lib/gitlab/ldap/adapter.rb b/lib/gitlab/ldap/adapter.rb
index 577a890a7d9..df65179bfea 100644
--- a/lib/gitlab/ldap/adapter.rb
+++ b/lib/gitlab/ldap/adapter.rb
@@ -70,19 +70,25 @@ module Gitlab
       end
 
       def ldap_search(*args)
-        results = ldap.search(*args)
+        # Net::LDAP's `time` argument doesn't work. Use Ruby `Timeout` instead.
+        Timeout.timeout(config.timeout) do
+          results = ldap.search(*args)
 
-        if results.nil?
-          response = ldap.get_operation_result
+          if results.nil?
+            response = ldap.get_operation_result
 
-          unless response.code.zero?
-            Rails.logger.warn("LDAP search error: #{response.message}")
+            unless response.code.zero?
+              Rails.logger.warn("LDAP search error: #{response.message}")
+            end
+
+            []
+          else
+            results
           end
-
-          []
-        else
-          results
         end
+      rescue Timeout::Error
+        Rails.logger.warn("LDAP search timed out after #{config.timeout} seconds")
+        []
       end
     end
   end
diff --git a/lib/gitlab/ldap/config.rb b/lib/gitlab/ldap/config.rb
index 101a3285f4b..aff7ccb157f 100644
--- a/lib/gitlab/ldap/config.rb
+++ b/lib/gitlab/ldap/config.rb
@@ -88,6 +88,10 @@ module Gitlab
         options['attributes']
       end
 
+      def timeout
+        options['timeout'].to_i
+      end
+
       protected
       def base_config
         Gitlab.config.ldap