Merge branch 'security-59581-related-merge-requests-count-11-10' into '11-10-stable'

Expose merge requests count based on user access

See merge request gitlab/gitlabhq!3169

app/controllers/concerns/issuable_collections.rb

@@ -41,7 +41,7 @@ module IssuableCollections
     return if pagination_disabled?
     @issuables          = @issuables.page(params[:page])
-    @issuable_meta_data = issuable_meta_data(@issuables, collection_type)
+    @issuable_meta_data = issuable_meta_data(@issuables, collection_type, current_user)
     @total_pages        = issuable_page_count
   end
   # rubocop:enable Gitlab/ModuleWithInstanceVariables

app/controllers/concerns/issuable_collections_action.rb

@@ -11,7 +11,7 @@ module IssuableCollectionsAction
               .non_archived
               .page(params[:page])
-    @issuable_meta_data = issuable_meta_data(@issues, collection_type)
+    @issuable_meta_data = issuable_meta_data(@issues, collection_type, current_user)
     respond_to do |format|
       format.html
@@ -22,7 +22,7 @@ module IssuableCollectionsAction
   def merge_requests
     @merge_requests = issuables_collection.page(params[:page])
-    @issuable_meta_data = issuable_meta_data(@merge_requests, collection_type)
+    @issuable_meta_data = issuable_meta_data(@merge_requests, collection_type, current_user)
   end
   # rubocop:enable Gitlab/ModuleWithInstanceVariables

app/controllers/projects_controller.rb

@@ -297,7 +297,7 @@ class ProjectsController < Projects::ApplicationController
       elsif @project.feature_available?(:issues, current_user)
         @issues = issuables_collection.page(params[:page])
         @collection_type = 'Issue'
-        @issuable_meta_data = issuable_meta_data(@issues, @collection_type)
+        @issuable_meta_data = issuable_meta_data(@issues, @collection_type, current_user)
       end
       render :show

app/finders/issuable_finder.rb

@@ -29,6 +29,7 @@
 #     updated_after: datetime
 #     updated_before: datetime
 #     attempt_group_search_optimizations: boolean
+#     attempt_project_search_optimizations: boolean
 #
 class IssuableFinder
   prepend FinderWithCrossProjectAccess
@@ -184,7 +185,6 @@ class IssuableFinder
     @project = project
   end
-  # rubocop: disable CodeReuse/ActiveRecord
   def projects
     return @projects if defined?(@projects)
@@ -192,17 +192,25 @@ class IssuableFinder
     projects =
       if current_user && params[:authorized_only].presence && !current_user_related?
-        current_user.authorized_projects
+        current_user.authorized_projects(min_access_level)
       elsif group
-        finder_options = { include_subgroups: params[:include_subgroups], only_owned: true }
-        GroupProjectsFinder.new(group: group, current_user: current_user, options: finder_options).execute # rubocop: disable CodeReuse/Finder
+        find_group_projects
       else
-        ProjectsFinder.new(current_user: current_user).execute # rubocop: disable CodeReuse/Finder
+        Project.public_or_visible_to_user(current_user, min_access_level)
       end
-    @projects = projects.with_feature_available_for_user(klass, current_user).reorder(nil)
+    @projects = projects.with_feature_available_for_user(klass, current_user).reorder(nil) # rubocop: disable CodeReuse/ActiveRecord
+  end
+
+  def find_group_projects
+    return Project.none unless group
+
+    if params[:include_subgroups]
+      Project.where(namespace_id: group.self_and_descendants) # rubocop: disable CodeReuse/ActiveRecord
+    else
+      group.projects
+    end.public_or_visible_to_user(current_user, min_access_level)
   end
-  # rubocop: enable CodeReuse/ActiveRecord
   def search
     params[:search].presence
@@ -572,4 +580,8 @@ class IssuableFinder
     scope = params[:scope]
     scope == 'created_by_me' || scope == 'authored' || scope == 'assigned_to_me'
   end
+
+  def min_access_level
+    ProjectFeature.required_minimum_access_level(klass)
+  end
 end

app/finders/issues_finder.rb

@@ -48,9 +48,9 @@ class IssuesFinder < IssuableFinder
       OR (issues.confidential = TRUE
         AND (issues.author_id = :user_id
           OR EXISTS (SELECT TRUE FROM issue_assignees WHERE user_id = :user_id AND issue_id = issues.id)
-          OR issues.project_id IN(:project_ids)))',
+          OR EXISTS (:authorizations)))',
       user_id: current_user.id,
-      project_ids: current_user.authorized_projects(CONFIDENTIAL_ACCESS_LEVEL).select(:id))
+      authorizations: current_user.authorizations_for_projects(min_access_level: CONFIDENTIAL_ACCESS_LEVEL, related_project_column: "issues.project_id"))
   end
   # rubocop: enable CodeReuse/ActiveRecord

app/finders/projects_finder.rb

@@ -62,7 +62,7 @@ class ProjectsFinder < UnionFinder
     collection = by_personal(collection)
     collection = by_starred(collection)
     collection = by_trending(collection)
-    collection = by_visibilty_level(collection)
+    collection = by_visibility_level(collection)
     collection = by_tags(collection)
     collection = by_search(collection)
     collection = by_archived(collection)
@@ -71,12 +71,11 @@ class ProjectsFinder < UnionFinder
     collection
   end
-  # rubocop: disable CodeReuse/ActiveRecord
   def collection_with_user
     if owned_projects?
       current_user.owned_projects
     elsif min_access_level?
-      current_user.authorized_projects.where('project_authorizations.access_level >= ?', params[:min_access_level])
+      current_user.authorized_projects(params[:min_access_level])
     else
       if private_only?
         current_user.authorized_projects
@@ -85,7 +84,6 @@ class ProjectsFinder < UnionFinder
       end
     end
   end
-  # rubocop: enable CodeReuse/ActiveRecord
   # Builds a collection for an anonymous user.
   def collection_without_user
@@ -131,7 +129,7 @@ class ProjectsFinder < UnionFinder
   end
   # rubocop: disable CodeReuse/ActiveRecord
-  def by_visibilty_level(items)
+  def by_visibility_level(items)
     params[:visibility_level].present? ? items.where(visibility_level: params[:visibility_level]) : items
   end
   # rubocop: enable CodeReuse/ActiveRecord

app/helpers/issuables_helper.rb

@@ -277,7 +277,7 @@ module IssuablesHelper
       initialTaskStatus: issuable.task_status
     }
-    data[:hasClosingMergeRequest] = issuable.merge_requests_count != 0 if issuable.is_a?(Issue)
+    data[:hasClosingMergeRequest] = issuable.merge_requests_count(current_user) != 0 if issuable.is_a?(Issue)
     if parent.is_a?(Group)
       data[:groupPath] = parent.path

app/models/concerns/issuable.rb

@@ -29,7 +29,11 @@ module Issuable
   # This object is used to gather issuable meta data for displaying
   # upvotes, downvotes, notes and closing merge requests count for issues and merge requests
   # lists avoiding n+1 queries and improving performance.
-  IssuableMeta = Struct.new(:upvotes, :downvotes, :user_notes_count, :merge_requests_count)
+  IssuableMeta = Struct.new(:upvotes, :downvotes, :user_notes_count, :mrs_count) do
+    def merge_requests_count(user = nil)
+      mrs_count
+    end
+  end
   included do
     cache_markdown_field :title, pipeline: :single_line

app/models/issue.rb

@@ -270,8 +270,8 @@ class Issue < ApplicationRecord
   end
   # rubocop: enable CodeReuse/ServiceClass
-  def merge_requests_count
-    merge_requests_closing_issues.count
+  def merge_requests_count(user = nil)
+    ::MergeRequestsClosingIssues.count_for_issue(self.id, user)
   end
   private

app/models/merge_requests_closing_issues.rb

@@ -7,11 +7,38 @@ class MergeRequestsClosingIssues < ApplicationRecord
   validates :merge_request_id, uniqueness: { scope: :issue_id }, presence: true
   validates :issue_id, presence: true
+  scope :with_issues, ->(ids) { where(issue_id: ids) }
+  scope :with_merge_requests_enabled, -> do
+    joins(:merge_request)
+      .joins('INNER JOIN project_features ON merge_requests.target_project_id = project_features.project_id')
+      .where('project_features.merge_requests_access_level >= :access', access: ProjectFeature::ENABLED)
+  end
+
+  scope :accessible_by, ->(user) do
+    joins(:merge_request)
+      .joins('INNER JOIN project_features ON merge_requests.target_project_id = project_features.project_id')
+      .where('project_features.merge_requests_access_level >= :access OR EXISTS(:authorizations)',
+             access: ProjectFeature::ENABLED,
+             authorizations: user.authorizations_for_projects(min_access_level: Gitlab::Access::REPORTER, related_project_column: "merge_requests.target_project_id")
+            )
+  end
+
   class << self
-    def count_for_collection(ids)
-      group(:issue_id)
-        .where(issue_id: ids)
-        .pluck('issue_id', 'COUNT(*) as count')
+    def count_for_collection(ids, current_user)
+      closing_merge_requests(ids, current_user).group(:issue_id).pluck('issue_id', 'COUNT(*) as count')
+    end
+
+    def count_for_issue(id, current_user)
+      closing_merge_requests(id, current_user).count
+    end
+
+    private
+
+    def closing_merge_requests(ids, current_user)
+      return with_issues(ids) if current_user&.admin?
+      return with_issues(ids).with_merge_requests_enabled if current_user.blank?
+
+      with_issues(ids).accessible_by(current_user)
     end
   end
 end

app/models/project.rb

@@ -461,10 +461,12 @@ class Project < ApplicationRecord
   # Returns a collection of projects that is either public or visible to the
   # logged in user.
-  def self.public_or_visible_to_user(user = nil)
+  def self.public_or_visible_to_user(user = nil, min_access_level = nil)
+    min_access_level = nil if user&.admin?
+
     if user
       where('EXISTS (?) OR projects.visibility_level IN (?)',
-            user.authorizations_for_projects,
+            user.authorizations_for_projects(min_access_level: min_access_level),
             Gitlab::VisibilityLevel.levels_for_user(user))
     else
       public_to_user
@@ -474,30 +476,32 @@ class Project < ApplicationRecord
   # project features may be "disabled", "internal", "enabled" or "public". If "internal",
   # they are only available to team members. This scope returns projects where
   # the feature is either public, enabled, or internal with permission for the user.
+  # Note: this scope doesn't enforce that the user has access to the projects, it just checks
+  # that the user has access to the feature. It's important to use this scope with others
+  # that checks project authorizations first.
   #
   # This method uses an optimised version of `with_feature_access_level` for
   # logged in users to more efficiently get private projects with the given
   # feature.
   def self.with_feature_available_for_user(feature, user)
     visible = [ProjectFeature::ENABLED, ProjectFeature::PUBLIC]
-    min_access_level = ProjectFeature.required_minimum_access_level(feature)
     if user&.admin?
       with_feature_enabled(feature)
     elsif user
+      min_access_level = ProjectFeature.required_minimum_access_level(feature)
       column = ProjectFeature.quoted_access_level_column(feature)
       with_project_feature
-        .where(
-          "(projects.visibility_level > :private AND (#{column} IS NULL OR #{column} >= (:public_visible) OR (#{column} = :private_visible AND EXISTS(:authorizations))))"\
-          " OR (projects.visibility_level = :private AND (#{column} IS NULL OR #{column} >= :private_visible) AND EXISTS(:authorizations))",
-          {
-            private: Gitlab::VisibilityLevel::PRIVATE,
-            public_visible: ProjectFeature::ENABLED,
-            private_visible: ProjectFeature::PRIVATE,
-            authorizations: user.authorizations_for_projects(min_access_level: min_access_level)
-          })
+      .where("#{column} IS NULL OR #{column} IN (:public_visible) OR (#{column} = :private_visible AND EXISTS (:authorizations))",
+            {
+              public_visible: visible,
+              private_visible: ProjectFeature::PRIVATE,
+              authorizations: user.authorizations_for_projects(min_access_level: min_access_level)
+            })
     else
+      # This has to be added to include features whose value is nil in the db
+      visible << nil
       with_feature_access_level(feature, visible)
     end
   end

app/models/user.rb

@@ -761,11 +761,15 @@ class User < ApplicationRecord
   # Typically used in conjunction with projects table to get projects
   # a user has been given access to.
+  # The param `related_project_column` is the column to compare to the
+  # project_authorizations. By default is projects.id
   #
   # Example use:
   # `Project.where('EXISTS(?)', user.authorizations_for_projects)`
-  def authorizations_for_projects(min_access_level: nil)
-    authorizations = project_authorizations.select(1).where('project_authorizations.project_id = projects.id')
+  def authorizations_for_projects(min_access_level: nil, related_project_column: 'projects.id')
+    authorizations = project_authorizations
+                      .select(1)
+                      .where("project_authorizations.project_id = #{related_project_column}")
     return authorizations unless min_access_level.present?

app/views/shared/_issuable_meta_data.html.haml

@@ -2,7 +2,7 @@
 - issue_votes        = @issuable_meta_data[issuable.id]
 - upvotes, downvotes = issue_votes.upvotes, issue_votes.downvotes
 - issuable_url       = @collection_type == "Issue" ? issue_path(issuable, anchor: 'notes') : merge_request_path(issuable, anchor: 'notes')
-- issuable_mr        = @issuable_meta_data[issuable.id].merge_requests_count
+- issuable_mr        = @issuable_meta_data[issuable.id].merge_requests_count(current_user)
 - if issuable_mr > 0
   %li.issuable-mr.d-none.d-sm-block.has-tooltip{ title: _('Related merge requests') }

changelogs/unreleased/fj-59522-improve-search-controller-performance.yml

+---
+title: Add improvements to global search of issues and merge requests
+merge_request: 27817
+author:
+type: performance

changelogs/unreleased/security-59581-related-merge-requests-count.yml

+---
+title: Expose merge requests count based on user access
+merge_request:
+author:
+type: security

lib/api/entities.rb

@@ -493,9 +493,9 @@ module API
       expose :state, :created_at, :updated_at
       # Avoids an N+1 query when metadata is included
-      def issuable_metadata(subject, options, method)
+      def issuable_metadata(subject, options, method, args = nil)
         cached_subject = options.dig(:issuable_metadata, subject.id)
-        (cached_subject || subject).public_send(method) # rubocop: disable GitlabSecurity/PublicSend
+        (cached_subject || subject).public_send(method, *args) # rubocop: disable GitlabSecurity/PublicSend
       end
     end
@@ -554,7 +554,7 @@ module API
       end
       expose(:user_notes_count)     { |issue, options| issuable_metadata(issue, options, :user_notes_count) }
-      expose(:merge_requests_count) { |issue, options| issuable_metadata(issue, options, :merge_requests_count) }
+      expose(:merge_requests_count) { |issue, options| issuable_metadata(issue, options, :merge_requests_count, options[:current_user]) }
       expose(:upvotes)              { |issue, options| issuable_metadata(issue, options, :upvotes) }
       expose(:downvotes)            { |issue, options| issuable_metadata(issue, options, :downvotes) }
       expose :due_date

lib/api/issues.rb

@@ -93,7 +93,7 @@ module API
         options = {
           with: Entities::IssueBasic,
           current_user: current_user,
-          issuable_metadata: issuable_meta_data(issues, 'Issue')
+          issuable_metadata: issuable_meta_data(issues, 'Issue', current_user)
         }
         present issues, options
@@ -120,7 +120,7 @@ module API
         options = {
           with: Entities::IssueBasic,
           current_user: current_user,
-          issuable_metadata: issuable_meta_data(issues, 'Issue')
+          issuable_metadata: issuable_meta_data(issues, 'Issue', current_user)
         }
         present issues, options
@@ -150,7 +150,7 @@ module API
           with: Entities::IssueBasic,
           current_user: current_user,
           project: user_project,
-          issuable_metadata: issuable_meta_data(issues, 'Issue')
+          issuable_metadata: issuable_meta_data(issues, 'Issue', current_user)
         }
         present issues, options

lib/api/merge_requests.rb

@@ -71,7 +71,7 @@ module API
         if params[:view] == 'simple'
           options[:with] = Entities::MergeRequestSimple
         else
-          options[:issuable_metadata] = issuable_meta_data(merge_requests, 'MergeRequest')
+          options[:issuable_metadata] = issuable_meta_data(merge_requests, 'MergeRequest', current_user)
         end
         options

lib/api/todos.rb

@@ -65,7 +65,7 @@ module API
             next unless collection
             targets = collection.map(&:target)
-            options[type] = { issuable_metadata: issuable_meta_data(targets, type) }
+            options[type] = { issuable_metadata: issuable_meta_data(targets, type, current_user) }
           end
         end
       end

lib/gitlab/group_search_results.rb

@@ -2,6 +2,8 @@
 module Gitlab
   class GroupSearchResults < SearchResults
+    attr_reader :group
+
     def initialize(current_user, limit_projects, group, query, default_project_filter: false, per_page: 20)
       super(current_user, limit_projects, query, default_project_filter: default_project_filter, per_page: per_page)
@@ -26,5 +28,9 @@ module Gitlab
         .where(id: groups.select('members.user_id'))
     end
     # rubocop:enable CodeReuse/ActiveRecord
+
+    def issuable_params
+      super.merge(group_id: group.id)
+    end
   end
 end