From 87ee11332f85cde5eeaa24ce9d179885becb5259 Mon Sep 17 00:00:00 2001
From: Drew Blessing <drew@gitlab.com>
Date: Wed, 13 Apr 2016 11:05:42 -0500
Subject: [PATCH] Concat AD group recursive member results with reguler member
 results

---
 CHANGELOG-EE                       |  1 +
 lib/gitlab/ldap/group.rb           | 16 ++++++----
 spec/lib/gitlab/ldap/group_spec.rb | 50 ++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+), 6 deletions(-)
 create mode 100644 spec/lib/gitlab/ldap/group_spec.rb

diff --git a/CHANGELOG-EE b/CHANGELOG-EE
index a9583715081c8e..b6e826293077cf 100644
--- a/CHANGELOG-EE
+++ b/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
diff --git a/lib/gitlab/ldap/group.rb b/lib/gitlab/ldap/group.rb
index 03a0ab636ebf0b..c5b28782bfa1a9 100644
--- a/lib/gitlab/ldap/group.rb
+++ b/lib/gitlab/ldap/group.rb
@@ -39,23 +39,27 @@ def member_uids
       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
diff --git a/spec/lib/gitlab/ldap/group_spec.rb b/spec/lib/gitlab/ldap/group_spec.rb
new file mode 100644
index 00000000000000..bff80e54f5993f
--- /dev/null
+++ b/spec/lib/gitlab/ldap/group_spec.rb
@@ -0,0 +1,50 @@
+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
-- 
GitLab