Make the GroupFinder specs more strict

Ensure the results match exactly and project authorizations do allow access to
sibling groups/projects deeper down.

Also apply WHERE scopes before running the UNION, to increase performance.

2 files changed, 47 insertions, 21 deletions

diff --git a/app/finders/groups_finder.rb b/app/finders/groups_finder.rb
index cb4ab6eacc1..e6fb112e7f2 100644
--- a/app/finders/groups_finder.rb
+++ b/app/finders/groups_finder.rb
@@ -5,8 +5,10 @@ class GroupsFinder < UnionFinder
   end
 
   def execute
-    groups = find_union(all_groups, Group).with_route.order_id_desc
-    by_parent(groups)
+    items = all_groups.map do |item|
+      by_parent(item)
+    end
+    find_union(items, Group).with_route.order_id_desc
   end
 
   private
@@ -17,8 +19,6 @@ class GroupsFinder < UnionFinder
     groups = []
 
     if current_user
-      groups_for_ancestors = find_union([current_user.authorized_groups, authorized_project_groups], Group)
-      groups_for_descendants = current_user.authorized_groups
       groups << Gitlab::GroupHierarchy.new(groups_for_ancestors, groups_for_descendants).all_groups
     end
     groups << Group.unscoped.public_to_user(current_user)
@@ -26,6 +26,14 @@ class GroupsFinder < UnionFinder
     groups
   end
 
+  def groups_for_ancestors
+    current_user.authorized_groups
+  end
+
+  def groups_for_descendants
+    current_user.groups
+  end
+
   def by_parent(groups)
     return groups unless params[:parent]
 
diff --git a/spec/finders/groups_finder_spec.rb b/spec/finders/groups_finder_spec.rb
index 9867d002561..9e70cccc3c4 100644
--- a/spec/finders/groups_finder_spec.rb
+++ b/spec/finders/groups_finder_spec.rb
@@ -45,23 +45,23 @@ describe GroupsFinder do
       let!(:private_subgroup) { create(:group, :private, parent: parent_group) }
 
       context 'without a user' do
-        it 'only returns public subgroups' do
-          expect(described_class.new(nil, parent: parent_group).execute).to contain_exactly(public_subgroup)
+        it 'only returns parent and public subgroups' do
+          expect(described_class.new(nil).execute).to contain_exactly(parent_group, public_subgroup)
         end
       end
 
       context 'with a user' do
-        subject { described_class.new(user, parent: parent_group).execute }
+        subject { described_class.new(user).execute }
 
-        it 'returns public and internal subgroups' do
-          is_expected.to contain_exactly(public_subgroup, internal_subgroup)
+        it 'returns parent, public, and internal subgroups' do
+          is_expected.to contain_exactly(parent_group, public_subgroup, internal_subgroup)
         end
 
         context 'being member' do
-          it 'returns public subgroups, internal subgroups, and private subgroups user is member of' do
+          it 'returns parent, public subgroups, internal subgroups, and private subgroups user is member of' do
             private_subgroup.add_guest(user)
 
-            is_expected.to contain_exactly(public_subgroup, internal_subgroup, private_subgroup)
+            is_expected.to contain_exactly(parent_group, public_subgroup, internal_subgroup, private_subgroup)
           end
         end
 
@@ -74,24 +74,42 @@ describe GroupsFinder do
             it 'returns all subgroups' do
               parent_group.add_guest(user)
 
-              is_expected.to contain_exactly(public_subgroup, internal_subgroup, private_subgroup)
+              is_expected.to contain_exactly(parent_group, public_subgroup, internal_subgroup, private_subgroup)
             end
           end
 
           context 'authorized to private project' do
-            it 'returns the subgroup of the project' do
-              subproject = create(:empty_project, :private, namespace: private_subgroup)
-              subproject.add_guest(user)
+            context 'project one level deep' do
+              let!(:subproject) { create(:empty_project, :private, namespace: private_subgroup) }
+              before do
+                subproject.add_guest(user)
+              end
+
+              it 'includes the subgroup of the project' do
+                is_expected.to include(private_subgroup)
+              end
 
-              is_expected.to include(private_subgroup)
+              it 'does not include private subgroups deeper down' do
+                subsubgroup = create(:group, :private, parent: private_subgroup)
+
+                is_expected.not_to include(subsubgroup)
+              end
             end
 
-            it 'returns all the parent groups if project is several levels deep' do
-              private_subsubgroup = create(:group, :private, parent: private_subgroup)
-              subsubproject = create(:empty_project, :private, namespace: private_subsubgroup)
-              subsubproject.add_guest(user)
+            context 'project two levels deep' do
+              let!(:private_subsubgroup) { create(:group, :private, parent: private_subgroup) }
+              let!(:subsubproject) { create(:empty_project, :private, namespace: private_subsubgroup) }
+              before do
+                subsubproject.add_guest(user)
+              end
+
+              it 'returns all the ancestor groups' do
+                is_expected.to include(private_subsubgroup, private_subgroup, parent_group)
+              end
 
-              expect(described_class.new(user).execute).to include(private_subsubgroup, private_subgroup, parent_group)
+              it 'returns the groups for a given parent' do
+                expect(described_class.new(user, parent: parent_group).execute).to include(private_subgroup)
+              end
             end
           end
         end