diff options
-rw-r--r-- | app/controllers/concerns/issuable_collections.rb | 2 | ||||
-rw-r--r-- | app/controllers/concerns/issuable_collections_action.rb | 4 | ||||
-rw-r--r-- | app/controllers/projects_controller.rb | 2 | ||||
-rw-r--r-- | app/helpers/issuables_helper.rb | 2 | ||||
-rw-r--r-- | app/models/concerns/issuable.rb | 6 | ||||
-rw-r--r-- | app/models/issue.rb | 4 | ||||
-rw-r--r-- | app/models/merge_requests_closing_issues.rb | 35 | ||||
-rw-r--r-- | app/views/shared/_issuable_meta_data.html.haml | 2 | ||||
-rw-r--r-- | changelogs/unreleased/security-59581-related-merge-requests-count.yml | 5 | ||||
-rw-r--r-- | lib/api/entities.rb | 6 | ||||
-rw-r--r-- | lib/api/issues.rb | 6 | ||||
-rw-r--r-- | lib/api/merge_requests.rb | 2 | ||||
-rw-r--r-- | lib/api/todos.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/issuable_metadata.rb | 4 | ||||
-rw-r--r-- | spec/lib/gitlab/issuable_metadata_spec.rb | 8 | ||||
-rw-r--r-- | spec/requests/api/issues/get_group_issues_spec.rb | 30 | ||||
-rw-r--r-- | spec/requests/api/issues/get_project_issues_spec.rb | 58 | ||||
-rw-r--r-- | spec/requests/api/issues/issues_spec.rb | 31 | ||||
-rw-r--r-- | spec/support/shared_examples/requests/api/issues/merge_requests_count_shared_examples.rb | 37 |
19 files changed, 192 insertions, 54 deletions
diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index 88a0690938a..21b3949e361 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/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 diff --git a/app/controllers/concerns/issuable_collections_action.rb b/app/controllers/concerns/issuable_collections_action.rb index 18ed4027eac..4ad287c4a13 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 12db493978b..330e2d0f8a5 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 045de105b77..cd2669ef6ad 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 127430cc68f..299e413321d 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 30e29911758..982a94315bd 100644 --- a/app/models/issue.rb +++ b/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 diff --git a/app/models/merge_requests_closing_issues.rb b/app/models/merge_requests_closing_issues.rb index 61af50841ee..22cedf57b86 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 31a5370a5f8..71b13a5d741 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 00000000000..83faa2f7c13 --- /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 ead01dc53f7..0f4332506a7 100644 --- a/lib/api/entities.rb +++ b/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 diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 039ebf92187..d687acf3423 100644 --- a/lib/api/issues.rb +++ b/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 diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index bf87e9ec2ff..6b8c1a2c0e8 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 871eaabc887..7260ecfb5ee 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 351d15605e0..be73bcd5506 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 916f3876a8e..032467b8b4e 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/requests/api/issues/get_group_issues_spec.rb b/spec/requests/api/issues/get_group_issues_spec.rb index 8b02cf56e9f..9a41d790945 100644 --- a/spec/requests/api/issues/get_group_issues_spec.rb +++ b/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 diff --git a/spec/requests/api/issues/get_project_issues_spec.rb b/spec/requests/api/issues/get_project_issues_spec.rb index 0b0f754ab57..f7ca6fd1e0a 100644 --- a/spec/requests/api/issues/get_project_issues_spec.rb +++ b/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) diff --git a/spec/requests/api/issues/issues_spec.rb b/spec/requests/api/issues/issues_spec.rb index f32ffd1c77b..d195f54be11 100644 --- a/spec/requests/api/issues/issues_spec.rb +++ b/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 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 00000000000..5f4e178f2e5 --- /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 |