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 diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index 8f492e5c012..4e48ff11871 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.active_directory_disabled?(user.extern_uid, adapter) + else + false + end rescue false end diff --git a/lib/gitlab/ldap/adapter.rb b/lib/gitlab/ldap/adapter.rb index 0777558d643..e36616f0e66 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,26 @@ 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) + + 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 diff --git a/lib/gitlab/ldap/person.rb b/lib/gitlab/ldap/person.rb index 06b17c58f8c..9ad6618bd46 100644 --- a/lib/gitlab/ldap/person.rb +++ b/lib/gitlab/ldap/person.rb @@ -1,6 +1,11 @@ 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) adapter ||= Gitlab::LDAP::Adapter.new adapter.user(config.uid, uid) @@ -11,6 +16,11 @@ module Gitlab adapter.user('dn', dn) end + def self.active_directory_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 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..d8c107502ba --- /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(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(active_directory_disabled?: false) } + + it { should be_true } + end + end + end +end 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