Use `EXISTS` instead of `WHERE id IN (...)` for authorized groups

2 files changed, 25 insertions, 18 deletions

diff --git a/app/finders/group_descendants_finder.rb b/app/finders/group_descendants_finder.rb
index 1e6523b70a2..3f231bd8b10 100644
--- a/app/finders/group_descendants_finder.rb
+++ b/app/finders/group_descendants_finder.rb
@@ -1,6 +1,4 @@
 class GroupDescendantsFinder
-  include Gitlab::Allowable
-
   attr_reader :current_user, :parent_group, :params
 
   def initialize(current_user: nil, parent_group:, params: {})
@@ -40,7 +38,7 @@ class GroupDescendantsFinder
   end
 
   def paginator
-    Gitlab::MultiCollectionPaginator.new(*collections, per_page: params[:per_page])
+    @paginator ||= Gitlab::MultiCollectionPaginator.new(*collections, per_page: params[:per_page])
   end
 
   def direct_child_groups
@@ -51,18 +49,21 @@ class GroupDescendantsFinder
 
   def all_visible_descendant_groups
     groups_table = Group.arel_table
-    visible_for_user = groups_table[:visibility_level]
-                         .in(Gitlab::VisibilityLevel.levels_for_user(current_user))
-    visible_for_user = if current_user
-                         visible_projects = GroupsFinder.new(current_user).execute.as('visible')
-                         authorized = groups_table.project(1).from(visible_projects)
-                                        .where(visible_projects[:id].eq(groups_table[:id]))
-                                        .exists
-                         visible_for_user.or(authorized)
-                       end
+    visible_to_user = groups_table[:visibility_level]
+                      .in(Gitlab::VisibilityLevel.levels_for_user(current_user))
+    if current_user
+      authorized_groups = GroupsFinder.new(current_user,
+                                           all_available: false)
+                            .execute.as('authorized')
+      authorized_to_user = groups_table.project(1).from(authorized_groups)
+                             .where(authorized_groups[:id].eq(groups_table[:id]))
+                             .exists
+      visible_to_user = visible_to_user.or(authorized_to_user)
+    end
+
     hierarchy_for_parent
       .descendants
-      .where(visible_for_user)
+      .where(visible_to_user)
   end
 
   def subgroups_matching_filter
@@ -94,7 +95,6 @@ class GroupDescendantsFinder
 
   def subgroups
     return Group.none unless Group.supports_nested_groups?
-    return Group.none unless can?(current_user, :read_group, parent_group)
 
     # When filtering subgroups, we want to find all matches withing the tree of
     # descendants to show to the user
@@ -122,8 +122,6 @@ class GroupDescendantsFinder
   end
 
   def projects
-    return Project.none unless can?(current_user, :read_group, parent_group)
-
     projects = if params[:filter]
                  projects_matching_filter
                else
diff --git a/spec/finders/group_descendants_finder_spec.rb b/spec/finders/group_descendants_finder_spec.rb
index 86a7a793457..1aef49613ee 100644
--- a/spec/finders/group_descendants_finder_spec.rb
+++ b/spec/finders/group_descendants_finder_spec.rb
@@ -39,7 +39,7 @@ describe GroupDescendantsFinder do
 
   context 'with nested groups', :nested_groups do
     let!(:project) { create(:project, namespace: group) }
-    let!(:subgroup) { create(:group, parent: group) }
+    let!(:subgroup) { create(:group, :private, parent: group) }
 
     describe '#execute' do
       it 'contains projects and subgroups' do
@@ -59,6 +59,15 @@ describe GroupDescendantsFinder do
         expect(finder.execute).to contain_exactly(public_subgroup, other_subgroup)
       end
 
+      it 'only includes public groups when no user is given' do
+        public_subgroup = create(:group, :public, parent: group)
+        _private_subgroup = create(:group, :private, parent: group)
+
+        finder = described_class.new(current_user: nil, parent_group: group)
+
+        expect(finder.execute).to contain_exactly(public_subgroup)
+      end
+
       context 'with a filter' do
         let(:params) { { filter: 'test' } }
 
@@ -86,7 +95,7 @@ describe GroupDescendantsFinder do
 
         context 'with matching children' do
           it 'includes a group that has a subgroup matching the query and its parent' do
-            matching_subgroup = create(:group, name: 'testgroup', parent: subgroup)
+            matching_subgroup = create(:group, :private, name: 'testgroup', parent: subgroup)
 
             expect(finder.execute).to contain_exactly(subgroup, matching_subgroup)
           end