Commit 42ccb598 authored by Sean McGivern's avatar Sean McGivern

Only do complicated confidentiality checks when necessary

When we are filtering by a single project, and the current user has access to
see confidential issues on that project, we don't need to filter by
confidentiality at all - just as if the user were an admin.

The filter by confidentiality often picks a non-optimal query plan: for
instance, AND-ing the results of all issues in the project (a relatively small
set), and all issues in the states requested (a huge set), rather than just
starting small and winnowing further.
parent 4a9ffb23
......@@ -16,14 +16,30 @@
# sort: string
#
class IssuesFinder < IssuableFinder
CONFIDENTIAL_ACCESS_LEVEL = Gitlab::Access::REPORTER
def klass
Issue
end
def not_restricted_by_confidentiality
return Issue.where('issues.confidential IS NOT TRUE') if user_cannot_see_confidential_issues?
return Issue.all if user_can_see_all_confidential_issues?
Issue.where('
issues.confidential IS NOT TRUE
OR (issues.confidential = TRUE
AND (issues.author_id = :user_id
OR EXISTS (SELECT TRUE FROM issue_assignees WHERE user_id = :user_id AND issue_id = issues.id)
OR issues.project_id IN(:project_ids)))',
user_id: current_user.id,
project_ids: current_user.authorized_projects(CONFIDENTIAL_ACCESS_LEVEL).select(:id))
end
private
def init_collection
IssuesFinder.not_restricted_by_confidentiality(current_user)
not_restricted_by_confidentiality
end
def by_assignee(items)
......@@ -38,22 +54,20 @@ class IssuesFinder < IssuableFinder
end
end
def self.not_restricted_by_confidentiality(user)
return Issue.where('issues.confidential IS NOT TRUE') if user.blank?
def item_project_ids(items)
items&.reorder(nil)&.select(:project_id)
end
return Issue.all if user.full_private_access?
def user_can_see_all_confidential_issues?
return false unless current_user
return true if current_user.full_private_access?
Issue.where('
issues.confidential IS NOT TRUE
OR (issues.confidential = TRUE
AND (issues.author_id = :user_id
OR EXISTS (SELECT TRUE FROM issue_assignees WHERE user_id = :user_id AND issue_id = issues.id)
OR issues.project_id IN(:project_ids)))',
user_id: user.id,
project_ids: user.authorized_projects(Gitlab::Access::REPORTER).select(:id))
project? &&
project &&
project.team.max_member_access(current_user.id) >= CONFIDENTIAL_ACCESS_LEVEL
end
def item_project_ids(items)
items&.reorder(nil)&.select(:project_id)
def user_cannot_see_confidential_issues?
current_user.blank?
end
end
......@@ -28,7 +28,7 @@
%span
Issues
- if @project.default_issues_tracker?
%span.badge.count.issue_counter= number_with_delimiter(IssuesFinder.new(current_user, project_id: @project.id).execute.opened.count)
%span.badge.count.issue_counter= number_with_delimiter(IssuesFinder.new(current_user, project_id: @project.id, state: :opened).execute.count)
- if project_nav_tab? :merge_requests
- controllers = [:merge_requests, 'projects/merge_requests/conflicts']
......@@ -37,7 +37,7 @@
= link_to namespace_project_merge_requests_path(@project.namespace, @project), title: 'Merge Requests', class: 'shortcuts-merge_requests' do
%span
Merge Requests
%span.badge.count.merge_counter.js-merge-counter= number_with_delimiter(MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened.count)
%span.badge.count.merge_counter.js-merge-counter= number_with_delimiter(MergeRequestsFinder.new(current_user, project_id: @project.id, state: :opened).execute.count)
- if project_nav_tab? :pipelines
= nav_link(controller: [:pipelines, :builds, :environments, :artifacts]) do
......
......@@ -295,22 +295,121 @@ describe IssuesFinder do
end
end
describe '.not_restricted_by_confidentiality' do
let(:authorized_user) { create(:user) }
let(:project) { create(:empty_project, namespace: authorized_user.namespace) }
let!(:public_issue) { create(:issue, project: project) }
let!(:confidential_issue) { create(:issue, project: project, confidential: true) }
describe '#not_restricted_by_confidentiality' do
let(:guest) { create(:user) }
set(:authorized_user) { create(:user) }
set(:project) { create(:empty_project, namespace: authorized_user.namespace) }
set(:public_issue) { create(:issue, project: project) }
set(:confidential_issue) { create(:issue, project: project, confidential: true) }
context 'when no project filter is given' do
let(:params) { {} }
context 'for an anonymous user' do
subject { described_class.new(nil, params).not_restricted_by_confidentiality }
it 'returns only public issues' do
expect(subject).to include(public_issue)
expect(subject).not_to include(confidential_issue)
end
end
context 'for a user without project membership' do
subject { described_class.new(user, params).not_restricted_by_confidentiality }
it 'returns only public issues' do
expect(subject).to include(public_issue)
expect(subject).not_to include(confidential_issue)
end
end
context 'for a guest user' do
subject { described_class.new(guest, params).not_restricted_by_confidentiality }
before do
project.add_guest(guest)
end
it 'returns only public issues' do
expect(subject).to include(public_issue)
expect(subject).not_to include(confidential_issue)
end
end
context 'for a project member with access to view confidential issues' do
subject { described_class.new(authorized_user, params).not_restricted_by_confidentiality }
it 'returns all issues' do
expect(subject).to include(public_issue, confidential_issue)
end
end
end
context 'when searching within a specific project' do
let(:params) { { project_id: project.id } }
context 'for an anonymous user' do
subject { described_class.new(nil, params).not_restricted_by_confidentiality }
it 'returns only public issues' do
expect(subject).to include(public_issue)
expect(subject).not_to include(confidential_issue)
end
it 'does not filter by confidentiality' do
expect(Issue).not_to receive(:where).with(a_string_matching('confidential'), anything)
it 'returns non confidential issues for nil user' do
expect(described_class.send(:not_restricted_by_confidentiality, nil)).to include(public_issue)
subject
end
end
context 'for a user without project membership' do
subject { described_class.new(user, params).not_restricted_by_confidentiality }
it 'returns non confidential issues for user not authorized for the issues projects' do
expect(described_class.send(:not_restricted_by_confidentiality, user)).to include(public_issue)
it 'returns only public issues' do
expect(subject).to include(public_issue)
expect(subject).not_to include(confidential_issue)
end
it 'returns all issues for user authorized for the issues projects' do
expect(described_class.send(:not_restricted_by_confidentiality, authorized_user)).to include(public_issue, confidential_issue)
it 'filters by confidentiality' do
expect(Issue).to receive(:where).with(a_string_matching('confidential'), anything)
subject
end
end
context 'for a guest user' do
subject { described_class.new(guest, params).not_restricted_by_confidentiality }
before do
project.add_guest(guest)
end
it 'returns only public issues' do
expect(subject).to include(public_issue)
expect(subject).not_to include(confidential_issue)
end
it 'filters by confidentiality' do
expect(Issue).to receive(:where).with(a_string_matching('confidential'), anything)
subject
end
end
context 'for a project member with access to view confidential issues' do
subject { described_class.new(authorized_user, params).not_restricted_by_confidentiality }
it 'returns all issues' do
expect(subject).to include(public_issue, confidential_issue)
end
it 'does not filter by confidentiality' do
expect(Issue).not_to receive(:where).with(a_string_matching('confidential'), anything)
subject
end
end
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment