Ranged members

CHANGELOG-EE

@@ -7,6 +7,7 @@ v 8.12.0 (Unreleased)
   - Add 'Sync now' to group members page !704
   - [ES] Instrument other Gitlab::Elastic classes
   - [ES] Fix: Elasticsearch does not find partial matches in project names
+  - Faster Active Directory group membership resolution !719
   - [ES] Global code search
   - [ES] Improve logging
   - Fix projects with remote mirrors asynchronously destruction

lib/ee/gitlab/ldap/adapter.rb

@@ -31,13 +31,26 @@ module EE
           groups(*args).first
         end
 
-        def dns_for_filter(filter)
+        def group_members_in_range(dn, range_start)
           ldap_search(
-            base: config.base,
-            filter: filter,
-            scope: Net::LDAP::SearchScope_WholeSubtree,
-            attributes: %w{dn}
-          ).map(&:dn)
+            base: dn,
+            scope: Net::LDAP::SearchScope_BaseObject,
+            attributes: ["member;range=#{range_start}-*"],
+          ).first
+        end
+
+        def nested_groups(parent_dn)
+          options = {
+            base: config.group_base,
+            filter: Net::LDAP::Filter.join(
+              Net::LDAP::Filter.eq('objectClass', 'group'),
+              Net::LDAP::Filter.eq('memberOf', parent_dn)
+            )
+          }
+
+          ldap_search(options).map do |entry|
+            LDAP::Group.new(entry, self)
+          end
         end
       end
     end

lib/ee/gitlab/ldap/group.rb

@@ -39,14 +39,15 @@ module EE
           entry.memberuid
         end
 
-        def member_dns
+        def dn
+          entry.dn
+        end
+
+        def member_dns(nested_groups_to_skip = [])
           dns = []
 
-          # There's an edge-case with AD where sometimes a recursive search
-          # doesn't return all users at the top-level. Concat recursive results
-          # with regular results to be safe. See gitlab-ee#484
-          if active_directory?
-            dns = adapter.dns_for_filter(active_directory_recursive_memberof_filter)
+          if active_directory? && adapter
+            dns.concat(active_directory_members(entry, nested_groups_to_skip))
           end
 
           if (entry.respond_to? :member) && (entry.respond_to? :submember)
@@ -60,20 +61,91 @@ module EE
           else
             Rails.logger.warn("Could not find member DNs for LDAP group #{entry.inspect}")
           end
+
           dns.uniq
         end
 
         private
 
-        # We use the ActiveDirectory LDAP_MATCHING_RULE_IN_CHAIN matching rule; see
-        # http://msdn.microsoft.com/en-us/library/aa746475%28VS.85%29.aspx#code-snippet-5
-        def active_directory_recursive_memberof_filter
-          Net::LDAP::Filter.ex("memberOf:1.2.840.113556.1.4.1941", entry.dn)
-        end
-
         def entry
           @entry
         end
+
+        # Active Directory range member methods
+
+        def has_member_range?(entry)
+          member_range_attribute(entry).present?
+        end
+
+        def member_range_attribute(entry)
+          entry.attribute_names.find { |a| a.to_s.start_with?("member;range=")}.to_s
+        end
+
+        def active_directory_members(entry, nested_groups_to_skip)
+          require 'net/ldap/dn'
+
+          members = []
+
+          # Retrieve all member pages/ranges
+          members.concat(ranged_members(entry)) if has_member_range?(entry)
+          # Process nested group members
+          members.concat(nested_members(nested_groups_to_skip))
+
+          # Clean dns of groups and users outside the base
+          members.reject! { |dn| nested_groups_to_skip.include?(dn) }
+          base = Net::LDAP::DN.new(adapter.config.base.downcase).to_a
+          members.select! { |dn| Net::LDAP::DN.new(dn.downcase).to_a.last(base.length) == base }
+
+          members
+        end
+
+        # AD requires use of range retrieval for groups with more than 1500 members
+        # cf. https://msdn.microsoft.com/en-us/library/aa367017(v=vs.85).aspx
+        def ranged_members(entry)
+          members = []
+
+          # Concatenate the members in the current range
+          members.concat(entry[member_range_attribute(entry)])
+
+          # Recursively concatenate members until end of ranges
+          if has_more_member_ranges?(entry)
+            next_entry = adapter.group_members_in_range(dn, next_member_range_start(entry))
+
+            members.concat(ranged_members(next_entry))
+          end
+
+          members
+        end
+
+        # Process any AD nested groups. Use a manual process because
+        # the AD recursive member of filter is too slow and uses too
+        # much CPU on the AD server.
+        def nested_members(nested_groups_to_skip)
+          # Ignore this group if we see it again in a nested group.
+          # Prevents infinite loops.
+          nested_groups_to_skip << dn
+
+          members = []
+          nested_groups = adapter.nested_groups(dn)
+
+          nested_groups.each do |nested_group|
+            next if nested_groups_to_skip.include?(nested_group.dn)
+
+            members.concat(nested_group.member_dns(nested_groups_to_skip))
+          end
+
+          members
+        end
+
+        def has_more_member_ranges?(entry)
+          next_member_range_start(entry).present?
+        end
+
+        def next_member_range_start(entry)
+          match = member_range_attribute(entry).match /^member;range=\d+-(\d+|\*)$/
+
+          match[1].to_i + 1 if match.present? && match[1] != '*'
+        end
       end
     end
   end

spec/lib/ee/gitlab/ldap/group_spec.rb

@@ -3,47 +3,125 @@ require 'spec_helper'
 describe EE::Gitlab::LDAP::Group, lib: true do
   include LdapHelpers
 
-  let(:adapter) { ldap_adapter }
+  before do
+    stub_ldap_config(active_directory: true)
+  end
 
   describe '#member_dns' do
-    def ldif
-      Net::LDAP::Entry.from_single_ldif_string(
-        <<-EOS.strip_heredoc
-          dn: cn=ldap_group1,ou=groups,dc=example,dc=com
-          cn: ldap_group1
-          description: LDAP Group 1
-          member: uid=user1,ou=users,dc=example,dc=com
-          member: uid=user2,ou=users,dc=example,dc=com
-          member: uid=user3,ou=users,dc=example,dc=com
-          objectclass: top
-          objectclass: groupOfNames
-        EOS
+    let(:adapter) { ldap_adapter }
+
+    it 'resolves the correct member_dns when member has a range' do
+      group_entry_page1 = ldap_group_entry_with_member_range(
+        [user_dn('user1'), user_dn('user2'), user_dn('user3')],
+        range_start: '0',
+        range_end: '2'
       )
-    end
+      group_entry_page2 = ldap_group_entry_with_member_range(
+        [user_dn('user4'), user_dn('user5'), user_dn('user6')],
+        range_start: '3',
+        range_end: '*'
+      )
+      group = EE::Gitlab::LDAP::Group.new(group_entry_page1, adapter)
+      stub_ldap_adapter_group_members_in_range(group_entry_page2, adapter, range_start: '3')
+      stub_ldap_adapter_nested_groups(group.dn, [], adapter)
 
-    let(:group) { described_class.new(ldif, adapter) }
-    let(:recursive_dns) do
-      %w(
-        uid=user3,ou=users,dc=example,dc=com
-        uid=user4,ou=users,dc=example,dc=com
-        uid=user5,ou=users,dc=example,dc=com
+      expect(group.member_dns).to match_array(
+        %w(
+          uid=user1,ou=users,dc=example,dc=com
+          uid=user2,ou=users,dc=example,dc=com
+          uid=user3,ou=users,dc=example,dc=com
+          uid=user4,ou=users,dc=example,dc=com
+          uid=user5,ou=users,dc=example,dc=com
+          uid=user6,ou=users,dc=example,dc=com
+        )
       )
     end
 
-    it 'concatenates recursive and regular results and returns uniq' do
-      allow(group).to receive(:active_directory?).and_return(true)
-      allow(adapter).to receive(:dns_for_filter).and_return(recursive_dns)
+    context 'when there are nested groups' do
+      let(:group1_entry) do
+        ldap_group_entry(
+          [user_dn('user1'), user_dn('user2')],
+          objectclass: 'group',
+          member_attr: 'member'
+        )
+      end
+      let(:group2_entry) do
+        ldap_group_entry(
+          [user_dn('user3'), user_dn('user4')],
+          cn: 'ldap_group2',
+          objectclass: 'group',
+          member_attr: 'member',
+          member_of: group1_entry.dn
+        )
+      end
+      let(:group) { EE::Gitlab::LDAP::Group.new(group1_entry, adapter) }
+
+      it 'resolves the correct member_dns when there are nested groups' do
+        group3_entry = ldap_group_entry(
+          [user_dn('user5'), user_dn('user6')],
+          cn: 'ldap_group3',
+          objectclass: 'group',
+          member_attr: 'member',
+          member_of: group1_entry.dn
+        )
+        nested_groups = [group2_entry, group3_entry]
+        stub_ldap_adapter_nested_groups(group.dn, nested_groups, adapter)
+        stub_ldap_adapter_nested_groups(group2_entry.dn, [], adapter)
+        stub_ldap_adapter_nested_groups(group3_entry.dn, [], adapter)
 
-      expect(group.member_dns)
-        .to match_array(
+        expect(group.member_dns).to match_array(
           %w(
             uid=user1,ou=users,dc=example,dc=com
             uid=user2,ou=users,dc=example,dc=com
             uid=user3,ou=users,dc=example,dc=com
             uid=user4,ou=users,dc=example,dc=com
             uid=user5,ou=users,dc=example,dc=com
+            uid=user6,ou=users,dc=example,dc=com
           )
         )
+      end
+
+      it 'skips duplicate nested groups' do
+        group3_entry = ldap_group_entry(
+          [user_dn('user5'), user_dn('user6')],
+          cn: 'ldap_group3',
+          objectclass: 'group',
+          member_attr: 'member',
+          member_of: [group1_entry.dn, group2_entry.dn]
+        )
+        nested_groups = [group2_entry, group3_entry]
+        stub_ldap_adapter_nested_groups(group.dn, nested_groups, adapter)
+        stub_ldap_adapter_nested_groups(group2_entry.dn, [group3_entry], adapter)
+        stub_ldap_adapter_nested_groups(group3_entry.dn, [], adapter)
+
+        expect(adapter).to receive(:nested_groups).with(group3_entry.dn).once
+
+        group.member_dns
+      end
+
+      it 'does not include group dns or users outside of the base' do
+        # Spaces in the 3rd DN below are intentional to ensure we're sanitizing
+        # DNs before comparing and not just doing a string compare.
+        group3_entry = ldap_group_entry(
+          [
+            'cn=ldap_group2,ou=groups,dc=example,dc=com',
+            'uid=foo,ou=users,dc=other,dc=com',
+            'uid=bar,ou=users,dc=example , dc=com'
+          ],
+          cn: 'ldap_group3',
+          objectclass: 'group',
+          member_attr: 'member',
+          member_of: group1_entry.dn
+        )
+        nested_groups = [group2_entry, group3_entry]
+        stub_ldap_adapter_nested_groups(group.dn, nested_groups, adapter)
+        stub_ldap_adapter_nested_groups(group2_entry.dn, [], adapter)
+        stub_ldap_adapter_nested_groups(group3_entry.dn, [], adapter)
+
+        expect(group.member_dns).not_to include('cn=ldap_group1,ou=groups,dc=example,dc=com')
+        expect(group.member_dns).not_to include('uid=foo,ou=users,dc=other,dc=com')
+        expect(group.member_dns).to include('uid=bar,ou=users,dc=example , dc=com')
+      end
     end
   end
 end

spec/support/ee/ldap_helpers.rb

@@ -46,7 +46,8 @@ module EE
       members,
       cn: 'ldap_group1',
       objectclass: 'groupOfNames',
-      member_attr: 'uniqueMember'
+      member_attr: 'uniqueMember',
+      member_of: nil
     )
       entry = Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc)
         dn: cn=#{cn},ou=groups,dc=example,dc=com
@@ -56,9 +57,70 @@ module EE
         objectclass: #{objectclass}
       EOS
 
+      entry['memberOf'] = member_of if member_of
       members = [members].flatten
       entry[member_attr] = members if members.any?
       entry
     end
+
+    # To simulate Active Directory ranged member retrieval. Create an LDAP
+    # group entry with any number of members in a given range. A '*' signifies
+    # the end of the 'pages' has been reached.
+    #
+    # Example:
+    #   ldap_group_entry_with_member_range(
+    #     [ 'user1', 'user2' ],
+    #     cn: 'my_group',
+    #     range_start: '0',
+    #     range_end: '*'
+    #   )
+    def ldap_group_entry_with_member_range(
+      members_in_range,
+      cn: 'ldap_group1',
+      range_start: '0',
+      range_end: '*'
+    )
+      entry = Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc)
+        dn: cn=#{cn},ou=groups,dc=example,dc=com
+        cn: #{cn}
+        description: LDAP Group #{cn}
+      EOS
+
+      members_in_range = [members_in_range].flatten
+      entry["member;range=#{range_start}-#{range_end}"] = members_in_range
+      entry
+    end
+
+    # Stub Active Directory range member retrieval.
+    #
+    # Example:
+    #  adapter = ::Gitlab::LDAP::Adapter.new('ldapmain', double(:ldap))
+    #  group_entry_page1 = ldap_group_entry_with_member_range(
+    #    [user_dn('user1'), user_dn('user2'), user_dn('user3')],
+    #    range_start: '0',
+    #    range_end: '2'
+    #  )
+    #  group_entry_page2 = ldap_group_entry_with_member_range(
+    #    [user_dn('user4'), user_dn('user5'), user_dn('user6')],
+    #    range_start: '3',
+    #    range_end: '*'
+    #  )
+    #  group = EE::Gitlab::LDAP::Group.new(group_entry_page1, adapter)
+    #
+    #  stub_ldap_adapter_group_members_in_range(group_entry_page2, adapter, range_start: '3')
+    def stub_ldap_adapter_group_members_in_range(
+      entry,
+      adapter = ldap_adapter,
+      range_start: '0'
+    )
+      allow(adapter).to receive(:group_members_in_range)
+        .with(entry.dn, range_start.to_i).and_return(entry)
+    end
+
+    def stub_ldap_adapter_nested_groups(parent_dn, entries = [], adapter = ldap_adapter)
+      groups = entries.map { |entry| EE::Gitlab::LDAP::Group.new(entry, adapter) }
+
+      allow(adapter).to receive(:nested_groups).with(parent_dn).and_return(groups)
+    end
   end
 end