Merge pull request #3873 from evanrolfe/fix/ldap_reconnect

[api][webui] LDAP retries when connection fails.

src/api/app/models/user_ldap_strategy.rb

@@ -197,33 +197,46 @@ def self.find_with_ldap(login, password)
     # @@ldap_search_con.bound? doesn't catch it as well. So when an error occurs, we
     # simply it try it a seccond time, which forces the ldap connection to
     # reinitialize (@@ldap_search_con is unbound and nil).
+    ldap_first_try = true
     user = nil
     user_filter = ''
 
-    if @@ldap_search_con.nil?
-      @@ldap_search_con = initialize_ldap_con(CONFIG['ldap_search_user'], CONFIG['ldap_search_auth'])
-    end
-    ldap_con = @@ldap_search_con
-    if ldap_con.nil?
-      Rails.logger.info("Unable to connect to LDAP server")
-      return
-    end
+    1.times do # Important! Do not remove this, this block may need to be re-run (by line 235)
+      if @@ldap_search_con.nil?
+        @@ldap_search_con = initialize_ldap_con(CONFIG['ldap_search_user'], CONFIG['ldap_search_auth'])
+      end
 
-    if CONFIG.has_key?('ldap_user_filter')
-      user_filter = "(&(#{CONFIG['ldap_search_attr']}=#{login})#{CONFIG['ldap_user_filter']})"
-    else
-      user_filter = "(#{CONFIG['ldap_search_attr']}=#{login})"
-    end
-    Rails.logger.debug("Search for #{CONFIG['ldap_search_base']} #{user_filter}")
-    begin
-      ldap_con.search(CONFIG['ldap_search_base'], LDAP::LDAP_SCOPE_SUBTREE, user_filter) do |entry|
-        user = entry.to_hash
+      ldap_con = @@ldap_search_con
+
+      if ldap_con.nil?
+        Rails.logger.info("Unable to connect to LDAP server")
+        return
+      end
+
+      if CONFIG.has_key?('ldap_user_filter')
+        user_filter = "(&(#{CONFIG['ldap_search_attr']}=#{login})#{CONFIG['ldap_user_filter']})"
+      else
+        user_filter = "(#{CONFIG['ldap_search_attr']}=#{login})"
+      end
+
+      Rails.logger.debug("Search for #{CONFIG['ldap_search_base']} #{user_filter}")
+
+      begin
+        ldap_con.search(CONFIG['ldap_search_base'], LDAP::LDAP_SCOPE_SUBTREE, user_filter) do |entry|
+          user = entry.to_hash
+        end
+      rescue StandardError
+        Rails.logger.info("Search failed:  error #{ @@ldap_search_con.err}: #{ @@ldap_search_con.err2string(@@ldap_search_con.err)}")
+        @@ldap_search_con.unbind
+        @@ldap_search_con = nil
+
+        if ldap_first_try
+          ldap_first_try = false
+          redo
+        end
+
+        return
       end
-    rescue StandardError
-      Rails.logger.info("Search failed:  error #{ @@ldap_search_con.err}: #{ @@ldap_search_con.err2string(@@ldap_search_con.err)}")
-      @@ldap_search_con.unbind
-      @@ldap_search_con = nil
-      return
     end
 
     if user.nil?

src/api/spec/models/user_ldap_strategy_spec.rb

@@ -378,6 +378,41 @@
           is_expected.to eq(['John', 'S'])
         end
       end
+
+      # This particular case occurs when a connection is made to the server, and stored in
+      # @@ldap_search_con but then the server closes the connection. UserLdapStrategy has no way of
+      # knowing if the connection was closed by the server so we need to make sure that
+      # UserLdapStrategy attempts to reconnect.
+      context 'when the connection is closed by the server' do
+        include_context 'setup ldap mock with user mock'
+        include_context 'an ldap connection'
+        include_context 'mock searching a user' do
+          let(:ldap_user) { double(:ldap_user, to_hash: { 'dn' => 'tux', 'sn' => ['John', 'Smith'] }) }
+        end
+
+        before do
+          times_called = 0
+          # First time LDAP works, second time is raises an error, third time it works etc.
+          allow(ldap_mock).to receive(:search) do
+            raise StandardError if times_called == 1
+            times_called += 1
+          end
+          allow(ldap_mock).to receive(:err).and_return('something went wrong')
+          allow(ldap_mock).to receive(:err2string).and_return('something went wrong')
+          allow(ldap_mock).to receive(:unbind)
+          # This connects to LDAP and stores the connection in a class var
+          UserLdapStrategy.find_with_ldap('tux', 'tux_password')
+        end
+
+        subject! do
+          # This attempts to use the LDAP connection which already exists in the class var
+          UserLdapStrategy.find_with_ldap('tux', 'tux_password')
+        end
+
+        it 'returns nil' do
+          is_expected.to eq(['John', 'tux'])
+        end
+      end
     end
   end
 end

src/api/test/unit/code_quality_test.rb

@@ -92,7 +92,7 @@ def setup
       'SearchController#search'                                                 => 81.15,
       'SourceController#project_command_copy'                                   => 140.04,
       'SourceController#update_project_meta'                                    => 98.09,
-      'UserLdapStrategy::find_with_ldap'                                        => 87.47,
+      'UserLdapStrategy::find_with_ldap'                                        => 94.35,
       'User::find_with_credentials_via_ldap'                                    => 88.93,
       'UserLdapStrategy::render_grouplist_ldap'                                 => 89.86,
       'Webui::DriverUpdateController#save'                                      => 91.69,