Merge branch 'some-ad-edge-case-causes-missing-members-484' into 'master'

Concat AD group recursive member results with reguler member results

Closes #484 



See merge request !333

CHANGELOG-EE

@@ -9,6 +9,7 @@ v 8.7.0 (unreleased)
 
 v 8.6.6
   - Fix LDAP group sync regression for groups with member value `uid=<username>` !335
+  - Concat AD group recursive member results with regular member results
 
 v 8.6.5
   - No EE-specific changes

lib/gitlab/ldap/group.rb

@@ -39,23 +39,27 @@ module Gitlab
       end
 
       def member_dns
+        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)
-          return dns unless dns.empty?
         end
 
         if (entry.respond_to? :member) && (entry.respond_to? :submember)
-          entry.member + entry.submember
+          dns.concat(entry.member + entry.submember)
         elsif entry.respond_to? :member
-          entry.member
+          dns.concat(entry.member)
         elsif entry.respond_to? :uniquemember
-          entry.uniquemember
+          dns.concat(entry.uniquemember)
         elsif entry.respond_to? :memberof
-          entry.memberof
+          dns.concat(entry.memberof)
         else
           Rails.logger.warn("Could not find member DNs for LDAP group #{entry.inspect}")
-          []
         end
+        dns.uniq
       end
 
       private

spec/lib/gitlab/ldap/group_spec.rb

+require 'spec_helper'
+
+describe Gitlab::LDAP::Group, lib: true do
+  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
+      )
+    end
+
+    def adapter
+      @adapter ||= Gitlab::LDAP::Adapter.new('ldapmain')
+    end
+
+    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
+      )
+    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)
+
+      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
+          )
+        )
+    end
+
+  end
+end