Merge branch 'security-59581-related-merge-requests-count' into 'master'

Expose merge requests count based on user access

See merge request gitlab/gitlabhq!3157

app/controllers/concerns/issuable_collections.rb

@@ -42,7 +42,7 @@ module IssuableCollections
 
     @issuables          = @issuables.page(params[:page])
     @issuables          = per_page_for_relative_position if params[:sort] == 'relative_position'
-    @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

@@ -250,8 +250,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
 
   def labels_hook_attrs

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

@@ -498,9 +498,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
 
@@ -564,7 +564,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

@@ -96,7 +96,7 @@ module API
           with: Entities::Issue,
           with_labels_details: declared_params[:with_labels_details],
           current_user: current_user,
-          issuable_metadata: issuable_meta_data(issues, 'Issue')
+          issuable_metadata: issuable_meta_data(issues, 'Issue', current_user)
         }
 
         present issues, options
@@ -122,7 +122,7 @@ module API
           with: Entities::Issue,
           with_labels_details: declared_params[:with_labels_details],
           current_user: current_user,
-          issuable_metadata: issuable_meta_data(issues, 'Issue')
+          issuable_metadata: issuable_meta_data(issues, 'Issue', current_user)
         }
 
         present issues, options
@@ -161,7 +161,7 @@ module API
           with_labels_details: declared_params[:with_labels_details],
           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/requests/api/issues/get_group_issues_spec.rb

@@ -23,7 +23,11 @@ describe API::Issues do
 
   describe 'GET /groups/:id/issues' do
     let!(:group)            { create(:group) }
-    let!(:group_project)    { create(:project, :public, creator_id: user.id, namespace: group) }
+    let!(:group_project)    { create(:project, :public, :repository, creator_id: user.id, namespace: group) }
+    let!(:private_mrs_project) do
+      create(:project, :public, :repository, creator_id: user.id, namespace: group, merge_requests_access_level: ProjectFeature::PRIVATE)
+    end
+
     let!(:group_closed_issue) do
       create :closed_issue,
         author: user,
@@ -234,6 +238,30 @@ describe API::Issues do
             it_behaves_like 'group issues statistics'
           end
         end
+
+        context "when returns issue merge_requests_count for different access levels" do
+          let!(:merge_request1) do
+            create(:merge_request,
+                   :simple,
+                   author: user,
+                   source_project: private_mrs_project,
+                   target_project: private_mrs_project,
+                   description: "closes #{group_issue.to_reference(private_mrs_project)}")
+          end
+          let!(:merge_request2) do
+            create(:merge_request,
+                   :simple,
+                   author: user,
+                   source_project: group_project,
+                   target_project: group_project,
+                   description: "closes #{group_issue.to_reference}")
+          end
+
+          it_behaves_like 'accessible merge requests count' do
+            let(:api_url) { base_url }
+            let(:target_issue) { group_issue }
+          end
+        end
       end
     end
 

spec/requests/api/issues/get_project_issues_spec.rb

@@ -4,8 +4,9 @@ require 'spec_helper'
 
 describe API::Issues do
   set(:user) { create(:user) }
-  set(:project) do
-    create(:project, :public, creator_id: user.id, namespace: user.namespace)
+  set(:project) { create(:project, :public, :repository, creator_id: user.id, namespace: user.namespace) }
+  set(:private_mrs_project) do
+    create(:project, :public, :repository, creator_id: user.id, namespace: user.namespace, merge_requests_access_level: ProjectFeature::PRIVATE)
   end
 
   let(:user2)       { create(:user) }
@@ -60,9 +61,28 @@ describe API::Issues do
   let(:no_milestone_title) { 'None' }
   let(:any_milestone_title) { 'Any' }
 
+  let!(:merge_request1) do
+    create(:merge_request,
+           :simple,
+           author: user,
+           source_project: project,
+           target_project: project,
+           description: "closes #{issue.to_reference}")
+  end
+  let!(:merge_request2) do
+    create(:merge_request,
+           :simple,
+           author: user,
+           source_project: private_mrs_project,
+           target_project: private_mrs_project,
+           description: "closes #{issue.to_reference(private_mrs_project)}")
+  end
+
   before(:all) do
     project.add_reporter(user)
     project.add_guest(guest)
+    private_mrs_project.add_reporter(user)
+    private_mrs_project.add_guest(guest)
   end
 
   before do
@@ -257,6 +277,11 @@ describe API::Issues do
       expect_paginated_array_response(issue.id)
     end
 
+    it_behaves_like 'accessible merge requests count' do
+      let(:api_url) { "/projects/#{project.id}/issues" }
+      let(:target_issue) { issue }
+    end
+
     context 'with labeled issues' do
       let(:label_b) { create(:label, title: 'foo', project: project) }
       let(:label_c) { create(:label, title: 'bar', project: project) }
@@ -636,34 +661,26 @@ describe API::Issues do
         expect(json_response['iid']).to eq(confidential_issue.iid)
       end
     end
-  end
-
-  describe 'GET :id/issues/:issue_iid/closed_by' do
-    let(:merge_request) do
-      create(:merge_request,
-        :simple,
-        author: user,
-        source_project: project,
-        target_project: project,
-        description: "closes #{issue.to_reference}")
-    end
 
-    before do
-      create(:merge_requests_closing_issues, issue: issue, merge_request: merge_request)
+    it_behaves_like 'accessible merge requests count' do
+      let(:api_url) { "/projects/#{project.id}/issues/#{issue.iid}" }
+      let(:target_issue) { issue }
     end
+  end
 
+  describe 'GET :id/issues/:issue_iid/closed_by' do
     context 'when unauthenticated' do
       it 'return public project issues' do
         get api("/projects/#{project.id}/issues/#{issue.iid}/closed_by")
 
-        expect_paginated_array_response(merge_request.id)
+        expect_paginated_array_response(merge_request1.id)
       end
     end
 
     it 'returns merge requests that will close issue on merge' do
       get api("/projects/#{project.id}/issues/#{issue.iid}/closed_by", user)
 
-      expect_paginated_array_response(merge_request.id)
+      expect_paginated_array_response(merge_request1.id)
     end
 
     context 'when no merge requests will close issue' do
@@ -721,13 +738,6 @@ describe API::Issues do
     end
 
     it 'returns merge requests that mentioned a issue' do
-      create(:merge_request,
-        :simple,
-        author: user,
-        source_project: project,
-        target_project: project,
-        description: 'Some description')
-
       get_related_merge_requests(project.id, issue.iid, user)
 
       expect_paginated_array_response(related_mr.id)

spec/requests/api/issues/issues_spec.rb

@@ -4,8 +4,9 @@ require 'spec_helper'
 
 describe API::Issues do
   set(:user) { create(:user) }
-  set(:project) do
-    create(:project, :public, creator_id: user.id, namespace: user.namespace)
+  set(:project) { create(:project, :public, :repository, creator_id: user.id, namespace: user.namespace) }
+  set(:private_mrs_project) do
+    create(:project, :public, :repository, creator_id: user.id, namespace: user.namespace, merge_requests_access_level: ProjectFeature::PRIVATE)
   end
 
   let(:user2)       { create(:user) }
@@ -63,6 +64,8 @@ describe API::Issues do
   before(:all) do
     project.add_reporter(user)
     project.add_guest(guest)
+    private_mrs_project.add_reporter(user)
+    private_mrs_project.add_guest(guest)
   end
 
   before do
@@ -725,6 +728,30 @@ describe API::Issues do
         end
       end
     end
+
+    context "when returns issue merge_requests_count for different access levels" do
+      let!(:merge_request1) do
+        create(:merge_request,
+               :simple,
+               author: user,
+               source_project: private_mrs_project,
+               target_project: private_mrs_project,
+               description: "closes #{issue.to_reference(private_mrs_project)}")
+      end
+      let!(:merge_request2) do
+        create(:merge_request,
+               :simple,
+               author: user,
+               source_project: project,
+               target_project: project,
+               description: "closes #{issue.to_reference}")
+      end
+
+      it_behaves_like 'accessible merge requests count' do
+        let(:api_url) { "/issues" }
+        let(:target_issue) { issue }
+      end
+    end
   end
 
   describe 'DELETE /projects/:id/issues/:issue_iid' do

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