From f4bca105d16e3bc47c2cd2725c519d2dcd788e70 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 14 May 2014 18:10:43 +0200 Subject: [PATCH 1/9] Backport Adapter#ldap_search from EE --- lib/gitlab/ldap/adapter.rb | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/ldap/adapter.rb b/lib/gitlab/ldap/adapter.rb index 0777558d643..7bdcb4b9743 100644 --- a/lib/gitlab/ldap/adapter.rb +++ b/lib/gitlab/ldap/adapter.rb @@ -64,7 +64,7 @@ module Gitlab end end - entries = ldap.search(options).select do |entry| + entries = ldap_search(options).select do |entry| entry.respond_to? config.uid end @@ -77,6 +77,22 @@ module Gitlab users(*args).first end + def ldap_search(*args) + results = ldap.search(*args) + + if results.nil? + response = ldap.get_operation_result + + unless response.code.zero? + Rails.logger.warn("LDAP search error: #{response.message}") + end + + [] + else + results + end + end + private def config From 982d4d51e8110bec280eb00db0fb756b062103d9 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 14 May 2014 18:11:14 +0200 Subject: [PATCH 2/9] Backport Adapter#dn_matches_filter? from EE --- lib/gitlab/ldap/adapter.rb | 4 +++ spec/lib/gitlab/ldap/ldap_adapter_spec.rb | 31 +++++++++++++++++++++++ 2 files changed, 35 insertions(+) create mode 100644 spec/lib/gitlab/ldap/ldap_adapter_spec.rb diff --git a/lib/gitlab/ldap/adapter.rb b/lib/gitlab/ldap/adapter.rb index 7bdcb4b9743..e36616f0e66 100644 --- a/lib/gitlab/ldap/adapter.rb +++ b/lib/gitlab/ldap/adapter.rb @@ -77,6 +77,10 @@ module Gitlab users(*args).first end + def dn_matches_filter?(dn, filter) + ldap_search(base: dn, filter: filter, scope: Net::LDAP::SearchScope_BaseObject, attributes: %w{dn}).any? + end + def ldap_search(*args) results = ldap.search(*args) diff --git a/spec/lib/gitlab/ldap/ldap_adapter_spec.rb b/spec/lib/gitlab/ldap/ldap_adapter_spec.rb new file mode 100644 index 00000000000..c3f07334431 --- /dev/null +++ b/spec/lib/gitlab/ldap/ldap_adapter_spec.rb @@ -0,0 +1,31 @@ +require 'spec_helper' + +describe Gitlab::LDAP::Adapter do + let(:adapter) { Gitlab::LDAP::Adapter.new } + + describe :dn_matches_filter? do + let(:ldap) { double(:ldap) } + subject { adapter.dn_matches_filter?(:dn, :filter) } + before { adapter.stub(ldap: ldap) } + + context "when the search is successful" do + context "and the result is non-empty" do + before { ldap.stub(search: [:foo]) } + + it { should be_true } + end + + context "and the result is empty" do + before { ldap.stub(search: []) } + + it { should be_false } + end + end + + context "when the search encounters an error" do + before { ldap.stub(search: nil, get_operation_result: double(code: 1, message: 'some error')) } + + it { should be_false } + end + end +end From d54133b09fdb0b2e589896dc8740bb8d0c99ed54 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 14 May 2014 18:14:06 +0200 Subject: [PATCH 3/9] Add spec for LDAP::Access#allowed? --- spec/lib/gitlab/ldap/ldap_access_spec.rb | 32 ++++++++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 spec/lib/gitlab/ldap/ldap_access_spec.rb diff --git a/spec/lib/gitlab/ldap/ldap_access_spec.rb b/spec/lib/gitlab/ldap/ldap_access_spec.rb new file mode 100644 index 00000000000..e76cc4f2fde --- /dev/null +++ b/spec/lib/gitlab/ldap/ldap_access_spec.rb @@ -0,0 +1,32 @@ +require 'spec_helper' + +describe Gitlab::LDAP::Access do + let(:access) { Gitlab::LDAP::Access.new } + let(:user) { create(:user) } + + describe :allowed? do + subject { access.allowed?(user) } + + context 'when the user cannot be found' do + before { Gitlab::LDAP::Person.stub(find_by_dn: nil) } + + it { should be_false } + end + + context 'when the user is found' do + before { Gitlab::LDAP::Person.stub(find_by_dn: :ldap_user) } + + context 'and the Active Directory disabled flag is set' do + before { Gitlab::LDAP::Person.stub(ad_disabled?: true) } + + it { should be_false } + end + + context 'and the Active Directory disabled flag is not set' do + before { Gitlab::LDAP::Person.stub(ad_disabled?: false) } + + it { should be_true } + end + end + end +end From a754f0b2205d4f09092c8c7c032ad944a229be8f Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 14 May 2014 18:26:58 +0200 Subject: [PATCH 4/9] Add LDAP::Person#ad_disabled? Check the bit for disabled Active Directory users. The filter is based on http://ctogonewild.com/2009/09/03/bitmask-searches-in-ldap/ . --- lib/gitlab/ldap/person.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/gitlab/ldap/person.rb b/lib/gitlab/ldap/person.rb index 06b17c58f8c..fa57f298e16 100644 --- a/lib/gitlab/ldap/person.rb +++ b/lib/gitlab/ldap/person.rb @@ -1,6 +1,8 @@ module Gitlab module LDAP class Person + AD_USER_DISABLED = Net::LDAP::Filter.ex("userAccountControl:1.2.840.113556.1.4.803", 2) + def self.find_by_uid(uid, adapter=nil) adapter ||= Gitlab::LDAP::Adapter.new adapter.user(config.uid, uid) @@ -11,6 +13,11 @@ module Gitlab adapter.user('dn', dn) end + def self.ad_disabled?(dn, adapter=nil) + adapter ||= Gitlab::LDAP::Adapter.new + adapter.dn_matches_filter?(dn, AD_USER_DISABLED) + end + def initialize(entry) Rails.logger.debug { "Instantiating #{self.class.name} with LDIF:\n#{entry.to_ldif}" } @entry = entry From a6e4153878eda841b0a71e5e1666e6bed0a050ae Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 14 May 2014 18:32:40 +0200 Subject: [PATCH 5/9] Check for the AD disabled flag in Access#allowed? --- lib/gitlab/ldap/access.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index 8f492e5c012..71931b79f62 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -14,7 +14,11 @@ module Gitlab end def allowed?(user) - !!Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter) + if Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter) + !Gitlab::LDAP::Person.ad_disabled?(user.extern_uid, adapter) + else + false + end rescue false end From 11dba4cee7dc43f88c340bccd553cff0e3d874e4 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 14 May 2014 18:54:05 +0200 Subject: [PATCH 6/9] Fix syntax error in AD disabled user filter --- lib/gitlab/ldap/person.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/ldap/person.rb b/lib/gitlab/ldap/person.rb index fa57f298e16..17ffde0e84f 100644 --- a/lib/gitlab/ldap/person.rb +++ b/lib/gitlab/ldap/person.rb @@ -1,7 +1,7 @@ module Gitlab module LDAP class Person - AD_USER_DISABLED = Net::LDAP::Filter.ex("userAccountControl:1.2.840.113556.1.4.803", 2) + AD_USER_DISABLED = Net::LDAP::Filter.ex("userAccountControl:1.2.840.113556.1.4.803", "2") def self.find_by_uid(uid, adapter=nil) adapter ||= Gitlab::LDAP::Adapter.new From 797e807249076920d6c4bb71f6258ca05ee0db34 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 14 May 2014 19:04:00 +0200 Subject: [PATCH 7/9] Use LDAP::Access.open to reuse the LDAP connection --- lib/gitlab/git_access.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index eefdb1833fc..f3e781ac4e9 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -66,8 +66,8 @@ module Gitlab if Gitlab.config.ldap.enabled if user.ldap_user? # Check if LDAP user exists and match LDAP user_filter - unless Gitlab::LDAP::Access.new.allowed?(user) - return false + Gitlab::LDAP::Access.open do |adapter| + return false unless adapter.allowed?(user) end end end From a966f72224427fe6830426f459d445cd19ecd5a0 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 14 May 2014 19:08:42 +0200 Subject: [PATCH 8/9] Document the Active Directory magic numbers --- lib/gitlab/ldap/person.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/gitlab/ldap/person.rb b/lib/gitlab/ldap/person.rb index 17ffde0e84f..3a97944b122 100644 --- a/lib/gitlab/ldap/person.rb +++ b/lib/gitlab/ldap/person.rb @@ -1,6 +1,9 @@ module Gitlab module LDAP class Person + # Active Directory-specific LDAP filter that checks if bit 2 of the + # userAccountControl attribute is set. + # Source: http://ctogonewild.com/2009/09/03/bitmask-searches-in-ldap/ AD_USER_DISABLED = Net::LDAP::Filter.ex("userAccountControl:1.2.840.113556.1.4.803", "2") def self.find_by_uid(uid, adapter=nil) From be1120e9681bfb83084c7aeadae3e83692901de9 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 14 May 2014 19:13:06 +0200 Subject: [PATCH 9/9] Improve ad_disabled method name --- lib/gitlab/ldap/access.rb | 2 +- lib/gitlab/ldap/person.rb | 2 +- spec/lib/gitlab/ldap/ldap_access_spec.rb | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index 71931b79f62..4e48ff11871 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -15,7 +15,7 @@ module Gitlab def allowed?(user) if Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter) - !Gitlab::LDAP::Person.ad_disabled?(user.extern_uid, adapter) + !Gitlab::LDAP::Person.active_directory_disabled?(user.extern_uid, adapter) else false end diff --git a/lib/gitlab/ldap/person.rb b/lib/gitlab/ldap/person.rb index 3a97944b122..9ad6618bd46 100644 --- a/lib/gitlab/ldap/person.rb +++ b/lib/gitlab/ldap/person.rb @@ -16,7 +16,7 @@ module Gitlab adapter.user('dn', dn) end - def self.ad_disabled?(dn, adapter=nil) + def self.active_directory_disabled?(dn, adapter=nil) adapter ||= Gitlab::LDAP::Adapter.new adapter.dn_matches_filter?(dn, AD_USER_DISABLED) end diff --git a/spec/lib/gitlab/ldap/ldap_access_spec.rb b/spec/lib/gitlab/ldap/ldap_access_spec.rb index e76cc4f2fde..d8c107502ba 100644 --- a/spec/lib/gitlab/ldap/ldap_access_spec.rb +++ b/spec/lib/gitlab/ldap/ldap_access_spec.rb @@ -17,13 +17,13 @@ describe Gitlab::LDAP::Access do before { Gitlab::LDAP::Person.stub(find_by_dn: :ldap_user) } context 'and the Active Directory disabled flag is set' do - before { Gitlab::LDAP::Person.stub(ad_disabled?: true) } + before { Gitlab::LDAP::Person.stub(active_directory_disabled?: true) } it { should be_false } end context 'and the Active Directory disabled flag is not set' do - before { Gitlab::LDAP::Person.stub(ad_disabled?: false) } + before { Gitlab::LDAP::Person.stub(active_directory_disabled?: false) } it { should be_true } end