Request only the LDAP attributes we need

4 files changed, 149 insertions, 28 deletions

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