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

@@ -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

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

spec/finders/issues_finder_spec.rb

@@ -311,32 +311,167 @@ describe IssuesFinder do
     end
   end
 
-  describe '.not_restricted_by_confidentiality' do
-    let(:authorized_user) { create(:user) }
-    let(:admin_user) { create(:admin) }
+  describe '#not_restricted_by_confidentiality' do
+    let(:guest) { create(:user) }
+    set(:authorized_user) { create(:user) }
+    let(:admin_user) { create(:user, :admin) }
     let(:auditor_user) { create(:user, :auditor) }
-    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) }
+    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) }
 
-    it 'returns non confidential issues for nil user' do
-      expect(described_class.send(:not_restricted_by_confidentiality, nil)).to include(public_issue)
+    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 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
+      end
+
+      context 'for a guest user' do
+        subject { described_class.new(guest, params).not_restricted_by_confidentiality }
 
-    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)
+        before do
+          project.add_guest(guest)
         end
 
-    it 'returns all issues for an admin user' do
-      expect(described_class.send(:not_restricted_by_confidentiality, admin_user)).to include(public_issue, confidential_issue)
+        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
+
+      context 'for an auditor' do
+        subject { described_class.new(auditor_user, params).not_restricted_by_confidentiality }
+
+        it 'returns all issues' do
+          expect(subject).to include(public_issue, confidential_issue)
+        end
       end
 
-    it 'returns all issues for an auditor user' do
-      expect(described_class.send(:not_restricted_by_confidentiality, auditor_user)).to include(public_issue, confidential_issue)
+      context 'for an admin' do
+        subject { described_class.new(admin_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)
+
+          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
+
+      context 'for an auditor' do
+        subject { described_class.new(auditor_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
+
+      context 'for an admin' do
+        subject { described_class.new(auditor_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