From bf8a48e179119830f83f3b358f66f8a95af17963 Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Fri, 2 Sep 2016 05:52:13 -0500 Subject: [PATCH] Request only the LDAP attributes we need --- CHANGELOG | 1 + lib/gitlab/ldap/adapter.rb | 58 +++++++++++++---------- spec/lib/gitlab/ldap/adapter_spec.rb | 71 ++++++++++++++++++++++++++-- spec/support/ldap_helpers.rb | 47 ++++++++++++++++++ 4 files changed, 149 insertions(+), 28 deletions(-) create mode 100644 spec/support/ldap_helpers.rb diff --git a/CHANGELOG b/CHANGELOG index eaa1e007ca9..82c2174a3ff 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -23,6 +23,7 @@ v 8.12.0 (unreleased) - Set path for all JavaScript cookies to honor GitLab's subdirectory setting !5627 (Mike Greiling) - Fix blame table layout width - Fix bug where pagination is still displayed despite all todos marked as done (ClemMakesApps) + - Request only the LDAP attributes we need !6187 - Center build stage columns in pipeline overview (ClemMakesApps) - Rename behaviour to behavior in bug issue template for consistency (ClemMakesApps) - Remove suggested colors hover underline (ClemMakesApps) diff --git a/lib/gitlab/ldap/adapter.rb b/lib/gitlab/ldap/adapter.rb index 9a5bcfb5c9b..9100719da87 100644 --- a/lib/gitlab/ldap/adapter.rb +++ b/lib/gitlab/ldap/adapter.rb @@ -23,31 +23,7 @@ module Gitlab end def users(field, value, limit = nil) - if field.to_sym == :dn - options = { - base: value, - scope: Net::LDAP::SearchScope_BaseObject - } - else - options = { - base: config.base, - filter: Net::LDAP::Filter.eq(field, value) - } - end - - if config.user_filter.present? - user_filter = Net::LDAP::Filter.construct(config.user_filter) - - options[:filter] = if options[:filter] - Net::LDAP::Filter.join(options[:filter], user_filter) - else - user_filter - end - end - - if limit.present? - options.merge!(size: limit) - end + options = user_options(field, value, limit) entries = ldap_search(options).select do |entry| entry.respond_to? config.uid @@ -90,6 +66,38 @@ module Gitlab Rails.logger.warn("LDAP search timed out after #{config.timeout} seconds") [] end + + private + + def user_options(field, value, limit) + options = { attributes: %W(#{config.uid} cn mail dn) } + options[:size] = limit if limit + + if field.to_sym == :dn + options[:base] = value + options[:scope] = Net::LDAP::SearchScope_BaseObject + options[:filter] = user_filter + else + options[:base] = config.base + options[:filter] = user_filter(Net::LDAP::Filter.eq(field, value)) + end + + options + end + + def user_filter(filter = nil) + if config.user_filter.present? + user_filter = Net::LDAP::Filter.construct(config.user_filter) + end + + if user_filter && filter + Net::LDAP::Filter.join(filter, user_filter) + elsif user_filter + user_filter + else + filter + end + end end end end diff --git a/spec/lib/gitlab/ldap/adapter_spec.rb b/spec/lib/gitlab/ldap/adapter_spec.rb index 4847b5f3b0e..0600893f4cf 100644 --- a/spec/lib/gitlab/ldap/adapter_spec.rb +++ b/spec/lib/gitlab/ldap/adapter_spec.rb @@ -1,12 +1,77 @@ require 'spec_helper' describe Gitlab::LDAP::Adapter, lib: true do - let(:adapter) { Gitlab::LDAP::Adapter.new 'ldapmain' } + include LdapHelpers + + let(:ldap) { double(:ldap) } + let(:adapter) { ldap_adapter('ldapmain', ldap) } + + describe '#users' do + before do + stub_ldap_config(base: 'dc=example,dc=com') + end + + it 'searches with the proper options when searching by uid' do + # Requires this expectation style to match the filter + expect(adapter).to receive(:ldap_search) do |arg| + expect(arg[:filter].to_s).to eq('(uid=johndoe)') + expect(arg[:base]).to eq('dc=example,dc=com') + expect(arg[:attributes]).to match(%w{uid cn mail dn}) + end.and_return({}) + + adapter.users('uid', 'johndoe') + end + + it 'searches with the proper options when searching by dn' do + expect(adapter).to receive(:ldap_search).with( + base: 'uid=johndoe,ou=users,dc=example,dc=com', + scope: Net::LDAP::SearchScope_BaseObject, + attributes: %w{uid cn mail dn}, + filter: nil + ).and_return({}) + + adapter.users('dn', 'uid=johndoe,ou=users,dc=example,dc=com') + end + + it 'searches with the proper options when searching with a limit' do + expect(adapter) + .to receive(:ldap_search).with(hash_including(size: 100)).and_return({}) + + adapter.users('uid', 'johndoe', 100) + end + + it 'returns an LDAP::Person if search returns a result' do + entry = ldap_user_entry('johndoe') + allow(adapter).to receive(:ldap_search).and_return([entry]) + + results = adapter.users('uid', 'johndoe') + + expect(results.size).to eq(1) + expect(results.first.uid).to eq('johndoe') + end + + it 'returns empty array if search entry does not respond to uid' do + entry = Net::LDAP::Entry.new + entry['dn'] = user_dn('johndoe') + allow(adapter).to receive(:ldap_search).and_return([entry]) + + results = adapter.users('uid', 'johndoe') + + expect(results).to be_empty + end + + it 'uses the right uid attribute when non-default' do + stub_ldap_config(uid: 'sAMAccountName') + expect(adapter).to receive(:ldap_search).with( + hash_including(attributes: %w{sAMAccountName cn mail dn}) + ).and_return({}) + + adapter.users('sAMAccountName', 'johndoe') + end + end describe '#dn_matches_filter?' do - let(:ldap) { double(:ldap) } subject { adapter.dn_matches_filter?(:dn, :filter) } - before { allow(adapter).to receive(:ldap).and_return(ldap) } context "when the search is successful" do context "and the result is non-empty" do diff --git a/spec/support/ldap_helpers.rb b/spec/support/ldap_helpers.rb new file mode 100644 index 00000000000..079f244475c --- /dev/null +++ b/spec/support/ldap_helpers.rb @@ -0,0 +1,47 @@ +module LdapHelpers + def ldap_adapter(provider = 'ldapmain', ldap = double(:ldap)) + ::Gitlab::LDAP::Adapter.new(provider, ldap) + end + + def user_dn(uid) + "uid=#{uid},ou=users,dc=example,dc=com" + end + + # Accepts a hash of Gitlab::LDAP::Config keys and values. + # + # Example: + # stub_ldap_config( + # group_base: 'ou=groups,dc=example,dc=com', + # admin_group: 'my-admin-group' + # ) + def stub_ldap_config(messages) + messages.each do |config, value| + allow_any_instance_of(::Gitlab::LDAP::Config) + .to receive(config.to_sym).and_return(value) + end + end + + # Stub an LDAP person search and provide the return entry. Specify `nil` for + # `entry` to simulate when an LDAP person is not found + # + # Example: + # adapter = ::Gitlab::LDAP::Adapter.new('ldapmain', double(:ldap)) + # ldap_user_entry = ldap_user_entry('john_doe') + # + # stub_ldap_person_find_by_uid('john_doe', ldap_user_entry, adapter) + def stub_ldap_person_find_by_uid(uid, entry, provider = 'ldapmain') + return_value = ::Gitlab::LDAP::Person.new(entry, provider) if entry.present? + + allow(::Gitlab::LDAP::Person) + .to receive(:find_by_uid).with(uid, any_args).and_return(return_value) + end + + # Create a simple LDAP user entry. + def ldap_user_entry(uid) + entry = Net::LDAP::Entry.new + entry['dn'] = user_dn(uid) + entry['uid'] = uid + + entry + end +end