summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAlexandru Croitor <acroitor@gitlab.com>2019-06-12 19:28:25 +0300
committerAlexandru Croitor <acroitor@gitlab.com>2019-06-18 15:28:04 +0300
commitaab5e6a42aa3c8495b7626b218c47e19d4fdf1b6 (patch)
tree56f415a86a5ef07d860a26b28bcf911ee2e878c5
parent3c240b7aea7fee1c4267d0ceb717ba0234e5e788 (diff)
downloadgitlab-ce-aab5e6a42aa3c8495b7626b218c47e19d4fdf1b6.tar.gz
Expose merge requests count based on user access
Count issues related merge requests based on user access level. And issue can have related MRs from projects where user does not have access so the number of related merge requests should be adjusted based on user's ability to access the related MRs. https://gitlab.com/gitlab-org/gitlab-ce/issues/59581
-rw-r--r--app/controllers/concerns/issuable_collections.rb2
-rw-r--r--app/controllers/concerns/issuable_collections_action.rb4
-rw-r--r--app/controllers/projects_controller.rb2
-rw-r--r--app/helpers/issuables_helper.rb2
-rw-r--r--app/models/concerns/issuable.rb6
-rw-r--r--app/models/issue.rb4
-rw-r--r--app/models/merge_requests_closing_issues.rb35
-rw-r--r--app/views/shared/_issuable_meta_data.html.haml2
-rw-r--r--changelogs/unreleased/security-59581-related-merge-requests-count.yml5
-rw-r--r--lib/api/entities.rb6
-rw-r--r--lib/api/issues.rb6
-rw-r--r--lib/api/merge_requests.rb2
-rw-r--r--lib/api/todos.rb2
-rw-r--r--lib/gitlab/issuable_metadata.rb4
-rw-r--r--spec/lib/gitlab/issuable_metadata_spec.rb8
-rw-r--r--spec/support/shared_examples/requests/api/issues/merge_requests_count_shared_examples.rb37
16 files changed, 100 insertions, 27 deletions
diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb
index 6d6e0cc6c7f..c51041513b5 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 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 62b97fc2590..4ab23a0b937 100644
--- a/app/controllers/projects_controller.rb
+++ b/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
diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb
index 52c49498e9b..c61c43c9090 100644
--- a/app/helpers/issuables_helper.rb
+++ b/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
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 17f94b4bd9b..2523d0070a6 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 eb4c87e05d5..44e7119239d 100644
--- a/app/models/issue.rb
+++ b/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
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 079ee7f5ccc..44996b0c737 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 999a9cb5a82..54ee62e3e8a 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 f164e1c1eeb..460fb2ed6b8 100644
--- a/lib/api/merge_requests.rb
+++ b/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
diff --git a/lib/api/todos.rb b/lib/api/todos.rb
index d2196f05173..f332a554c41 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 6ec86163233..a0a6353bb5c 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 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