diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index 91e875dca544eefec32b2e186aececd4e9de1687..8a6d7d1dfbf9ab355b1af1231614ba9309aa23d5 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/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 diff --git a/app/controllers/concerns/issuable_collections_action.rb b/app/controllers/concerns/issuable_collections_action.rb index 18ed4027eac88e986b921148f826fe3405a843c0..4ad287c4a13c748bc10c502676bdec00a4e6e47f 100644 --- a/app/controllers/concerns/issuable_collections_action.rb +++ b/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 diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 12db493978b00810142ce5fb2d72c87e6455c846..330e2d0f8a5eaacad37b3ef4d482504f5a16edbe 100644 --- a/app/controllers/projects_controller.rb +++ b/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 diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb index 9a12db258d569916a0466a61ed937d87b1419a9f..941b77c405bd8c55d1b6d2793340b6b2fc6a2884 100644 --- a/app/helpers/issuables_helper.rb +++ b/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 diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 127430cc68fe3b069dcc89cf706d2b4003fdd2a3..299e413321db8fd686b81fb822df62f70cf4dfa5 100644 --- a/app/models/concerns/issuable.rb +++ b/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 diff --git a/app/models/issue.rb b/app/models/issue.rb index eb5544f2a12cb6f82224af3dc8d4fdfb7ac527f5..42597bda0e6187c0c1fe5c71d162fd28e8bfedb7 100644 --- a/app/models/issue.rb +++ b/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 diff --git a/app/models/merge_requests_closing_issues.rb b/app/models/merge_requests_closing_issues.rb index 61af50841ee2801494f6db9874b2a1a54cc8e38e..22cedf57b86e20f7411415d4b48b0ea5b4d98273 100644 --- a/app/models/merge_requests_closing_issues.rb +++ b/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 diff --git a/app/views/shared/_issuable_meta_data.html.haml b/app/views/shared/_issuable_meta_data.html.haml index 31a5370a5f8d66f74f5711f754a24614fb763fe6..71b13a5d741667280da243831bb7c7d9983ffe8f 100644 --- a/app/views/shared/_issuable_meta_data.html.haml +++ b/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') } diff --git a/changelogs/unreleased/security-59581-related-merge-requests-count.yml b/changelogs/unreleased/security-59581-related-merge-requests-count.yml new file mode 100644 index 0000000000000000000000000000000000000000..83faa2f7c1375cd55d9518d257784ec49c5311dd --- /dev/null +++ b/changelogs/unreleased/security-59581-related-merge-requests-count.yml @@ -0,0 +1,5 @@ +--- +title: Expose merge requests count based on user access +merge_request: +author: +type: security diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 90ed24a2ded76d3a4ff50f94a7324f4b3083ee3b..9458c375b01132ca1ca12371dc36b56beff9bca7 100644 --- a/lib/api/entities.rb +++ b/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 diff --git a/lib/api/issues.rb b/lib/api/issues.rb index d0a93b779512f74e233d5baebe2fb42acef4df89..1e4d14a9a4cc0e580509e107caa0367d6a56d3f7 100644 --- a/lib/api/issues.rb +++ b/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 diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index ce85772e4ed441590db3fceace2b2c1a74635bf5..9a1f7d44bcb3fbdf57d737ddff11d5bd2f0ff927 100644 --- a/lib/api/merge_requests.rb +++ b/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 diff --git a/lib/api/todos.rb b/lib/api/todos.rb index d2196f05173e197705791ee307587e7ea5697d55..f332a554c41a719378baeebf5e68b2736cb7fba9 100644 --- a/lib/api/todos.rb +++ b/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 diff --git a/lib/gitlab/issuable_metadata.rb b/lib/gitlab/issuable_metadata.rb index 351d15605e0cff8119dbe503de85f60b0ff54a58..be73bcd5506ba690ca732711b8287d5099f81a8d 100644 --- a/lib/gitlab/issuable_metadata.rb +++ b/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 diff --git a/spec/lib/gitlab/issuable_metadata_spec.rb b/spec/lib/gitlab/issuable_metadata_spec.rb index 916f3876a8e916e1e0d06b6560e73b43203cd94f..032467b8b4ee59e9fcd09ff68a6f10bb0cf62ed1 100644 --- a/spec/lib/gitlab/issuable_metadata_spec.rb +++ b/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) diff --git a/spec/support/shared_examples/requests/api/issues/merge_requests_count_shared_examples.rb b/spec/support/shared_examples/requests/api/issues/merge_requests_count_shared_examples.rb new file mode 100644 index 0000000000000000000000000000000000000000..5f4e178f2e5d95be9cd0ba11f23caeb7f94fa910 --- /dev/null +++ b/spec/support/shared_examples/requests/api/issues/merge_requests_count_shared_examples.rb @@ -0,0 +1,37 @@ +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