Merge branch 'bw-finder-with-group-heirarchy' into 'master'

Extract code from LablesFinder into concern FinderWithGroupHierarchy

See merge request gitlab-org/gitlab!58585

app/finders/concerns/finder_with_group_hierarchy.rb

+# frozen_string_literal: true
+
+# Module to include into finders to provide support for querying for
+# objects up and down the group hierarchy.  Extracted from LabelsFinder
+#
+# Supports params:
+#   :group
+#   :group_id
+#   :include_ancestor_groups
+#   :include_descendant_groups
+module FinderWithGroupHierarchy
+  extend ActiveSupport::Concern
+
+  private
+
+  def item_ids
+    raise NotImplementedError
+  end
+
+  # Gets redacted array of group ids
+  # which can include the ancestors and descendants of the requested group.
+  def group_ids_for(group)
+    strong_memoize(:group_ids) do
+      groups = groups_to_include(group)
+
+      # Because we are sure that all groups are in the same hierarchy tree
+      # we can preset root group for all of them to optimize permission checks
+      Group.preset_root_ancestor_for(groups)
+
+      groups_user_can_read_items(groups).map(&:id)
+    end
+  end
+
+  def groups_to_include(group)
+    groups = [group]
+
+    groups += group.ancestors if include_ancestor_groups?
+    groups += group.descendants if include_descendant_groups?
+
+    groups
+  end
+
+  def include_ancestor_groups?
+    params[:include_ancestor_groups]
+  end
+
+  def include_descendant_groups?
+    params[:include_descendant_groups]
+  end
+
+  def group?
+    params[:group].present? || params[:group_id].present?
+  end
+
+  def group
+    strong_memoize(:group) { params[:group].presence || Group.find(params[:group_id]) }
+  end
+
+  def read_permission
+    raise NotImplementedError
+  end
+
+  def authorized_to_read_item?(item_parent)
+    return true if skip_authorization
+
+    Ability.allowed?(current_user, read_permission, item_parent)
+  end
+
+  def groups_user_can_read_items(groups)
+    DeclarativePolicy.user_scope do
+      groups.select { |group| authorized_to_read_item?(group) }
+    end
+  end
+end

app/finders/labels_finder.rb

@@ -2,6 +2,7 @@
 
 class LabelsFinder < UnionFinder
   prepend FinderWithCrossProjectAccess
+  include FinderWithGroupHierarchy
   include FinderMethods
   include Gitlab::Utils::StrongMemoize
 
@@ -14,7 +15,7 @@ class LabelsFinder < UnionFinder
 
   def execute(skip_authorization: false)
     @skip_authorization = skip_authorization
-    items = find_union(label_ids, Label) || Label.none
+    items = find_union(item_ids, Label) || Label.none
     items = with_title(items)
     items = by_subscription(items)
     items = by_search(items)
@@ -26,8 +27,8 @@ class LabelsFinder < UnionFinder
   attr_reader :current_user, :params, :skip_authorization
 
   # rubocop: disable CodeReuse/ActiveRecord
-  def label_ids
-    label_ids = []
+  def item_ids
+    item_ids = []
 
     if project?
       if project
@@ -35,25 +36,25 @@ class LabelsFinder < UnionFinder
           labels_table = Label.arel_table
           group_ids = group_ids_for(project.group)
 
-          label_ids << Label.where(
+          item_ids << Label.where(
             labels_table[:type].eq('GroupLabel').and(labels_table[:group_id].in(group_ids)).or(
               labels_table[:type].eq('ProjectLabel').and(labels_table[:project_id].eq(project.id))
             )
           )
         else
-          label_ids << project.labels
+          item_ids << project.labels
         end
       end
     else
       if group?
-        label_ids << Label.where(group_id: group_ids_for(group))
+        item_ids << Label.where(group_id: group_ids_for(group))
       end
 
-      label_ids << Label.where(group_id: projects.group_ids)
-      label_ids << Label.where(project_id: ids_user_can_read_labels(projects)) unless only_group_labels?
+      item_ids << Label.where(group_id: projects.group_ids)
+      item_ids << Label.where(project_id: ids_user_can_read_labels(projects)) unless only_group_labels?
     end
 
-    label_ids
+    item_ids
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
@@ -94,49 +95,6 @@ class LabelsFinder < UnionFinder
     params[:subscribed] == 'true'
   end
 
-  # Gets redacted array of group ids
-  # which can include the ancestors and descendants of the requested group.
-  def group_ids_for(group)
-    strong_memoize(:group_ids) do
-      groups = groups_to_include(group)
-
-      # Because we are sure that all groups are in the same hierarchy tree
-      # we can preset root group for all of them to optimize permission checks
-      Group.preset_root_ancestor_for(groups)
-
-      groups_user_can_read_labels(groups).map(&:id)
-    end
-  end
-
-  def groups_to_include(group)
-    groups = [group]
-
-    groups += group.ancestors if include_ancestor_groups?
-    groups += group.descendants if include_descendant_groups?
-
-    groups
-  end
-
-  def include_ancestor_groups?
-    params[:include_ancestor_groups]
-  end
-
-  def include_descendant_groups?
-    params[:include_descendant_groups]
-  end
-
-  def group?
-    params[:group].present? || params[:group_id].present?
-  end
-
-  def group
-    strong_memoize(:group) { params[:group].presence || Group.find(params[:group_id]) }
-  end
-
-  def project?
-    params[:project].present? || params[:project_id].present?
-  end
-
   def projects?
     params[:project_ids]
   end
@@ -153,12 +111,16 @@ class LabelsFinder < UnionFinder
     params[:title] || params[:name]
   end
 
+  def project?
+    params[:project].present? || params[:project_id].present?
+  end
+
   def project
     return @project if defined?(@project)
 
     if project?
       @project = params[:project] || Project.find(params[:project_id])
-      @project = nil unless authorized_to_read_labels?(@project)
+      @project = nil unless authorized_to_read_item?(@project)
     else
       @project = nil
     end
@@ -191,16 +153,8 @@ class LabelsFinder < UnionFinder
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
-  def authorized_to_read_labels?(label_parent)
-    return true if skip_authorization
-
-    Ability.allowed?(current_user, :read_label, label_parent)
-  end
-
-  def groups_user_can_read_labels(groups)
-    DeclarativePolicy.user_scope do
-      groups.select { |group| authorized_to_read_labels?(group) }
-    end
+  def read_permission
+    :read_label
   end
 
   # rubocop: disable CodeReuse/ActiveRecord

spec/finders/concerns/finder_with_group_hierarchy_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe FinderWithGroupHierarchy do
+  let(:finder_class) do
+    Class.new do
+      include FinderWithGroupHierarchy
+      include Gitlab::Utils::StrongMemoize
+
+      def initialize(current_user, params = {})
+        @current_user, @params = current_user, params
+      end
+
+      def execute(skip_authorization: false)
+        @skip_authorization = skip_authorization
+
+        item_ids
+      end
+
+      # normally an array of item ids would be returned,
+      # however for this spec just return the group ids
+      def item_ids
+        group? ? group_ids_for(group) : []
+      end
+
+      private
+
+      attr_reader :current_user, :params, :skip_authorization
+
+      def read_permission
+        :read_label
+      end
+    end
+  end
+
+  let_it_be(:parent_group) { create(:group) }
+  let_it_be(:group) { create(:group, parent: parent_group) }
+  let_it_be(:private_group) { create(:group, :private) }
+  let_it_be(:private_subgroup) { create(:group, :private, parent: private_group) }
+
+  let(:user) { create(:user) }
+
+  context 'when specifying group' do
+    it 'returns only the group by default' do
+      finder = finder_class.new(user, group: group)
+
+      expect(finder.execute).to match_array([group.id])
+    end
+  end
+
+  context 'when specifying group_id' do
+    it 'returns only the group by default' do
+      finder = finder_class.new(user, group_id: group.id)
+
+      expect(finder.execute).to match_array([group.id])
+    end
+  end
+
+  context 'when including items from group ancestors' do
+    before do
+      private_subgroup.add_developer(user)
+    end
+
+    it 'returns group and its ancestors' do
+      private_group.add_developer(user)
+
+      finder = finder_class.new(user, group: private_subgroup, include_ancestor_groups: true)
+
+      expect(finder.execute).to match_array([private_group.id, private_subgroup.id])
+    end
+
+    it 'ignores groups which user can not read' do
+      finder = finder_class.new(user, group: private_subgroup, include_ancestor_groups: true)
+
+      expect(finder.execute).to match_array([private_subgroup.id])
+    end
+
+    it 'returns them all when skip_authorization is true' do
+      finder = finder_class.new(user, group: private_subgroup, include_ancestor_groups: true)
+
+      expect(finder.execute(skip_authorization: true)).to match_array([private_group.id, private_subgroup.id])
+    end
+  end
+
+  context 'when including items from group descendants' do
+    before do
+      private_subgroup.add_developer(user)
+    end
+
+    it 'returns items from group and its descendants' do
+      private_group.add_developer(user)
+
+      finder = finder_class.new(user, group: private_group, include_descendant_groups: true)
+
+      expect(finder.execute).to match_array([private_group.id, private_subgroup.id])
+    end
+
+    it 'ignores items from groups which user can not read' do
+      finder = finder_class.new(user, group: private_group, include_descendant_groups: true)
+
+      expect(finder.execute).to match_array([private_subgroup.id])
+    end
+
+    it 'returns them all when skip_authorization is true' do
+      finder = finder_class.new(user, group: private_group, include_descendant_groups: true)
+
+      expect(finder.execute(skip_authorization: true)).to match_array([private_group.id, private_subgroup.id])
+    end
+  end
+end