From 42ccb5981a8425216f9d69372754c52510f0cade Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 20 Jun 2017 16:38:14 +0100 Subject: [PATCH] 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. --- app/finders/issues_finder.rb | 42 +++++--- app/views/layouts/nav/_project.html.haml | 4 +- spec/finders/issues_finder_spec.rb | 125 ++++++++++++++++++++--- 3 files changed, 142 insertions(+), 29 deletions(-) diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb index 3da5508aefd..3455a75b8bc 100644 --- a/app/finders/issues_finder.rb +++ b/app/finders/issues_finder.rb @@ -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 diff --git a/app/views/layouts/nav/_project.html.haml b/app/views/layouts/nav/_project.html.haml index 68024d782a6..a2c6e44425a 100644 --- a/app/views/layouts/nav/_project.html.haml +++ b/app/views/layouts/nav/_project.html.haml @@ -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 diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb index 8ace1fb5751..dfa15e859a4 100644 --- a/spec/finders/issues_finder_spec.rb +++ b/spec/finders/issues_finder_spec.rb @@ -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) } - - it 'returns non confidential issues for nil user' do - expect(described_class.send(:not_restricted_by_confidentiality, nil)).to include(public_issue) - end + 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 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 all issues' do + expect(subject).to include(public_issue, confidential_issue) + end + end 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) + 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) + + subject + 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 + + 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 -- GitLab