Merge branch 'siemens/gitlab-ce-fix/ldap-access-errors' into 'master'

Log LDAP lookup errors and don't swallow unrelated exceptions

- Previously all exceptions were ignored, now only `Net::LDAP::Error` and exceptions that inherit from it are caught by the `rescue` clause. There might be other exceptions that should also be ignored / dealt with.
- Not sure if the Rails production log is a good choice for this, or if the GitLab application log would be more appropriate.

See merge request !6558

CHANGELOG

@@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 v 8.13.0 (unreleased)
   - Use gitlab-shell v3.6.2 (GIT TRACE logging)
   - Speed-up group milestones show page
+  - Log LDAP lookup errors and don't swallow unrelated exceptions. !6103 (Markus Koller)
   - Add more tests for calendar contribution (ClemMakesApps)
   - Fix robots.txt disallowing access to groups starting with "s" (Matt Harrison)
   - Only update issuable labels if they have been changed

doc/administration/auth/ldap.md

@@ -275,3 +275,9 @@ If you are getting 'Connection Refused' errors when trying to connect to the
 LDAP server please double-check the LDAP `port` and `method` settings used by
 GitLab. Common combinations are `method: 'plain'` and `port: 389`, OR
 `method: 'ssl'` and `port: 636`.
+
+### Login with valid credentials rejected
+
+If there is an unexpected error while authenticating the user with the LDAP
+backend, the login is rejected and details about the error are logged to
+`production.log`.

lib/gitlab/ldap/access.rb

@@ -51,8 +51,6 @@ module Gitlab
           user.ldap_block
           false
         end
-      rescue
-        false
       end
 
       def adapter

lib/gitlab/ldap/adapter.rb

@@ -62,6 +62,9 @@ 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")
         []

spec/lib/gitlab/ldap/adapter_spec.rb

@@ -73,17 +73,33 @@ describe Gitlab::LDAP::Adapter, lib: true do
   describe '#dn_matches_filter?' do
     subject { adapter.dn_matches_filter?(:dn, :filter) }
 
+    context "when the search result is non-empty" do
+      before { allow(adapter).to receive(:ldap_search).and_return([:foo]) }
+
+      it { is_expected.to be_truthy }
+    end
+
+    context "when the search result is empty" do
+      before { allow(adapter).to receive(:ldap_search).and_return([]) }
+
+      it { is_expected.to be_falsey }
+    end
+  end
+
+  describe '#ldap_search' do
+    subject { adapter.ldap_search(base: :dn, filter: :filter) }
+
     context "when the search is successful" do
       context "and the result is non-empty" do
         before { allow(ldap).to receive(:search).and_return([:foo]) }
 
-        it { is_expected.to be_truthy }
+        it { is_expected.to eq [:foo] }
       end
 
       context "and the result is empty" do
         before { allow(ldap).to receive(:search).and_return([]) }
 
-        it { is_expected.to be_falsey }
+        it { is_expected.to eq [] }
       end
     end
 
@@ -95,7 +111,22 @@ describe Gitlab::LDAP::Adapter, lib: true do
         )
       end
 
-      it { is_expected.to be_falsey }
+      it { is_expected.to eq [] }
+    end
+
+    context "when the search raises an LDAP exception" do
+      before do
+        allow(ldap).to receive(:search) { raise Net::LDAP::Error, "some error" }
+        allow(Rails.logger).to receive(:warn)
+      end
+
+      it { is_expected.to eq [] }
+
+      it 'logs the error' do
+        subject
+        expect(Rails.logger).to have_received(:warn).with(
+          "LDAP search raised exception Net::LDAP::Error: some error")
+      end
     end
   end
 end