Merge branch 'restrict_ldap_return_attributes' into 'master'

Restrict ldap return attributes

## What does this MR do?

Fixes the CE part of #13821. We really only ever need uid, dn, cn, and mail attributes, and in some cases, even less. This merge request strips the request down to those four attributes by default, and allows the caller to specify others, if needed. 

## Why was this MR needed?

This will improve performance especially in cases where the connection is slow between GitLab and LDAP, or when the LDAP object has lots of attributes we don't care about.

See merge request !6187

CHANGELOG

@@ -26,6 +26,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)

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

spec/lib/gitlab/ldap/adapter_spec.rb

 require 'spec_helper'
 
 describe Gitlab::LDAP::Adapter, lib: true do
-  let(:adapter) { Gitlab::LDAP::Adapter.new 'ldapmain' }
+  include LdapHelpers
 
-  describe '#dn_matches_filter?' do
   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
     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

spec/support/ldap_helpers.rb

+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