Log LDAP lookup errors and don't swallow unrelated exceptions

Signed-off-by: Roger Meier <r.meier@siemens.com>

5 files changed, 44 insertions, 5 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 10f1da4c681..3afaf2896ec 100644
--- a/CHANGELOG
+++ b/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
diff --git a/doc/administration/auth/ldap.md b/doc/administration/auth/ldap.md
index 7186f707ad6..bf7814875bf 100644
--- a/doc/administration/auth/ldap.md
+++ b/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`.
diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb
index 2f326d00a2f..7e06bd2b0fb 100644
--- a/lib/gitlab/ldap/access.rb
+++ b/lib/gitlab/ldap/access.rb
@@ -51,8 +51,6 @@ module Gitlab
           user.ldap_block
           false
         end
-      rescue
-        false
       end
 
       def adapter
diff --git a/lib/gitlab/ldap/adapter.rb b/lib/gitlab/ldap/adapter.rb
index 82cb8cef754..8b38cfaefb6 100644
--- a/lib/gitlab/ldap/adapter.rb
+++ b/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")
         []
diff --git a/spec/lib/gitlab/ldap/adapter_spec.rb b/spec/lib/gitlab/ldap/adapter_spec.rb
index 0600893f4cf..563c074017a 100644
--- a/spec/lib/gitlab/ldap/adapter_spec.rb
+++ b/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