diff options
author | Rémy Coutable <remy@rymai.me> | 2016-09-28 06:51:08 +0000 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2016-09-28 06:51:08 +0000 |
commit | 684baf7e7e8f7263bba6cccb7457bbc242135484 (patch) | |
tree | d91a1cf384f812f586d55e47dc1a8601dca1cc18 | |
parent | 9a28756c07f0f73bd134385bf11b44527ee81631 (diff) | |
parent | 68364fe2f03a543c4ad89553f50b6fa30d143331 (diff) | |
download | gitlab-ce-684baf7e7e8f7263bba6cccb7457bbc242135484.tar.gz |
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
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | doc/administration/auth/ldap.md | 6 | ||||
-rw-r--r-- | lib/gitlab/ldap/access.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/ldap/adapter.rb | 3 | ||||
-rw-r--r-- | spec/lib/gitlab/ldap/adapter_spec.rb | 37 |
5 files changed, 44 insertions, 5 deletions
diff --git a/CHANGELOG b/CHANGELOG index 8ea765b4ea9..df93f89a41a 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 |