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

Expose merge requests count based on user access

See merge request gitlab/gitlabhq!3168

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

@@ -298,7 +298,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/helpers/issuables_helper.rb

@@ -280,7 +280,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

@@ -248,8 +248,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/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/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

@@ -72,7 +72,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/issuable_metadata.rb

@@ -2,7 +2,7 @@
 
 module Gitlab
   module IssuableMetadata
-    def issuable_meta_data(issuable_collection, collection_type)
+    def issuable_meta_data(issuable_collection, collection_type, user = nil)
       # ActiveRecord uses Object#extend for null relations.
       if !(issuable_collection.singleton_class < ActiveRecord::NullRelation) &&
           issuable_collection.respond_to?(:limit_value) &&
@@ -23,7 +23,7 @@ module Gitlab
       issuable_votes_count = ::AwardEmoji.votes_for_collection(issuable_ids, collection_type)
       issuable_merge_requests_count =
         if collection_type == 'Issue'
-          ::MergeRequestsClosingIssues.count_for_collection(issuable_ids)
+          ::MergeRequestsClosingIssues.count_for_collection(issuable_ids, user)
         else
           []
         end

spec/lib/gitlab/issuable_metadata_spec.rb

@@ -7,11 +7,11 @@ describe Gitlab::IssuableMetadata do
   subject { Class.new { include Gitlab::IssuableMetadata }.new }
 
   it 'returns an empty Hash if an empty collection is provided' do
-    expect(subject.issuable_meta_data(Issue.none, 'Issue')).to eq({})
+    expect(subject.issuable_meta_data(Issue.none, 'Issue', user)).to eq({})
   end
 
   it 'raises an error when given a collection with no limit' do
-    expect { subject.issuable_meta_data(Issue.all, 'Issue') }.to raise_error(/must have a limit/)
+    expect { subject.issuable_meta_data(Issue.all, 'Issue', user) }.to raise_error(/must have a limit/)
   end
 
   context 'issues' do
@@ -23,7 +23,7 @@ describe Gitlab::IssuableMetadata do
     let!(:closing_issues) { create(:merge_requests_closing_issues, issue: issue, merge_request: merge_request) }
 
     it 'aggregates stats on issues' do
-      data = subject.issuable_meta_data(Issue.all.limit(10), 'Issue')
+      data = subject.issuable_meta_data(Issue.all.limit(10), 'Issue', user)
 
       expect(data.count).to eq(2)
       expect(data[issue.id].upvotes).to eq(1)
@@ -46,7 +46,7 @@ describe Gitlab::IssuableMetadata do
     let!(:note) { create(:note_on_merge_request, author: user, project: project, noteable: merge_request, note: "a comment on a MR") }
 
     it 'aggregates stats on merge requests' do
-      data = subject.issuable_meta_data(MergeRequest.all.limit(10), 'MergeRequest')
+      data = subject.issuable_meta_data(MergeRequest.all.limit(10), 'MergeRequest', user)
 
       expect(data.count).to eq(2)
       expect(data[merge_request.id].upvotes).to eq(1)

spec/support/shared_examples/requests/api/issues/merge_requests_count_shared_examples.rb

+def get_issue
+  json_response.is_a?(Array) ? json_response.detect {|issue| issue['id'] == target_issue.id} : json_response
+end
+
+shared_examples 'accessible merge requests count' do
+  it 'returns anonymous accessible merge requests count' do
+    get api(api_url), params: { scope: 'all' }
+
+    issue = get_issue
+    expect(issue).not_to be_nil
+    expect(issue['merge_requests_count']).to eq(1)
+  end
+
+  it 'returns guest accessible merge requests count' do
+    get api(api_url, guest), params: { scope: 'all' }
+
+    issue = get_issue
+    expect(issue).not_to be_nil
+    expect(issue['merge_requests_count']).to eq(1)
+  end
+
+  it 'returns reporter accessible merge requests count' do
+    get api(api_url, user), params: { scope: 'all' }
+
+    issue = get_issue
+    expect(issue).not_to be_nil
+    expect(issue['merge_requests_count']).to eq(2)
+  end
+
+  it 'returns admin accessible merge requests count' do
+    get api(api_url, admin), params: { scope: 'all' }
+
+    issue = get_issue
+    expect(issue).not_to be_nil
+    expect(issue['merge_requests_count']).to eq(2)
+  end
+end