Merge branch 'ability-batch-issue-checking' into 'master'

Optimize checking if a user can read multiple issues

## What does this MR do?

This optimizes various parts of the code so it can more efficiently check if a user can read a list of issues.

## Are there points in the code the reviewer needs to double check?

Yes, in particular `Ability.issues_readable_by_user` should be checked to make sure it correctly allows/restricts access to issues.

## Why was this MR needed?

Currently the general approach to checking if one can read an issue is to iterate over the issues to check and call `can?(user, :read_issue, issue)` for every issue. This is not efficient as the same work has to be done for every issue.

## What are the relevant issue numbers?

* #15607 
* #17463

See merge request !5370

CHANGELOG

@@ -15,6 +15,7 @@ v 8.11.0 (unreleased)
   - Update to gitlab_git 10.4.1 and take advantage of preserved Ref objects
   - Retrieve rendered HTML from cache in one request
   - Fix renaming repository when name contains invalid chararacters under project settings
+  - Optimize checking if a user has read access to a list of issues !5370
   - Nokogiri's various parsing methods are now instrumented
   - Add a way to send an email and create an issue based on private personal token. Find the email address from issues page. !3363
   - Add build event color in HipChat messages (David Eisner)

app/models/ability.rb

@@ -47,6 +47,16 @@ class Ability
       end
     end
 
+    # Returns an Array of Issues that can be read by the given user.
+    #
+    # issues - The issues to reduce down to those readable by the user.
+    # user - The User for which to check the issues
+    def issues_readable_by_user(issues, user = nil)
+      return issues if user && user.admin?
+
+      issues.select { |issue| issue.visible_to_user?(user) }
+    end
+
     # List of possible abilities for anonymous user
     def anonymous_abilities(user, subject)
       if subject.is_a?(PersonalSnippet)

app/models/issue.rb

@@ -230,6 +230,34 @@ class Issue < ActiveRecord::Base
       self.closed_by_merge_requests(current_user).empty?
   end
 
+  # Returns `true` if the current issue can be viewed by either a logged in User
+  # or an anonymous user.
+  def visible_to_user?(user = nil)
+    user ? readable_by?(user) : publicly_visible?
+  end
+
+  # Returns `true` if the given User can read the current Issue.
+  def readable_by?(user)
+    if user.admin?
+      true
+    elsif project.owner == user
+      true
+    elsif confidential?
+      author == user ||
+        assignee == user ||
+        project.team.member?(user, Gitlab::Access::REPORTER)
+    else
+      project.public? ||
+        project.internal? && !user.external? ||
+        project.team.member?(user)
+    end
+  end
+
+  # Returns `true` if this Issue is visible to everybody.
+  def publicly_visible?
+    project.public? && !confidential?
+  end
+
   def overdue?
     due_date.try(:past?) || false
   end

lib/banzai/reference_parser/issue_parser.rb

@@ -9,10 +9,11 @@ module Banzai
 
         issues = issues_for_nodes(nodes)
 
-        nodes.select do |node|
-          issue = issue_for_node(issues, node)
+        readable_issues = Ability.
+          issues_readable_by_user(issues.values, user).to_set
 
-          issue ? can?(user, :read_issue, issue) : false
+        nodes.select do |node|
+          readable_issues.include?(issue_for_node(issues, node))
         end
       end
 

spec/lib/banzai/reference_parser/issue_parser_spec.rb

@@ -16,17 +16,17 @@ describe Banzai::ReferenceParser::IssueParser, lib: true do
       end
 
       it 'returns the nodes when the user can read the issue' do
-        expect(Ability.abilities).to receive(:allowed?).
-          with(user, :read_issue, issue).
-          and_return(true)
+        expect(Ability).to receive(:issues_readable_by_user).
+          with([issue], user).
+          and_return([issue])
 
         expect(subject.nodes_visible_to_user(user, [link])).to eq([link])
       end
 
       it 'returns an empty Array when the user can not read the issue' do
-        expect(Ability.abilities).to receive(:allowed?).
-          with(user, :read_issue, issue).
-          and_return(false)
+        expect(Ability).to receive(:issues_readable_by_user).
+          with([issue], user).
+          and_return([])
 
         expect(subject.nodes_visible_to_user(user, [link])).to eq([])
       end

spec/models/ability_spec.rb

@@ -170,4 +170,52 @@ describe Ability, lib: true do
       end
     end
   end
+
+  describe '.issues_readable_by_user' do
+    context 'with an admin user' do
+      it 'returns all given issues' do
+        user = build(:user, admin: true)
+        issue = build(:issue)
+
+        expect(described_class.issues_readable_by_user([issue], user)).
+          to eq([issue])
+      end
+    end
+
+    context 'with a regular user' do
+      it 'returns the issues readable by the user' do
+        user = build(:user)
+        issue = build(:issue)
+
+        expect(issue).to receive(:readable_by?).with(user).and_return(true)
+
+        expect(described_class.issues_readable_by_user([issue], user)).
+          to eq([issue])
+      end
+
+      it 'returns an empty Array when no issues are readable' do
+        user = build(:user)
+        issue = build(:issue)
+
+        expect(issue).to receive(:readable_by?).with(user).and_return(false)
+
+        expect(described_class.issues_readable_by_user([issue], user)).to eq([])
+      end
+    end
+
+    context 'without a regular user' do
+      it 'returns issues that are publicly visible' do
+        hidden_issue = build(:issue)
+        visible_issue = build(:issue)
+
+        expect(hidden_issue).to receive(:publicly_visible?).and_return(false)
+        expect(visible_issue).to receive(:publicly_visible?).and_return(true)
+
+        issues = described_class.
+          issues_readable_by_user([hidden_issue, visible_issue])
+
+        expect(issues).to eq([visible_issue])
+      end
+    end
+  end
 end

spec/models/concerns/mentionable_spec.rb

@@ -68,7 +68,7 @@ describe Issue, "Mentionable" do
 
   describe '#create_cross_references!' do
     let(:project) { create(:project) }
-    let(:author)  { double('author') }
+    let(:author)  { build(:user) }
     let(:commit)  { project.commit }
     let(:commit2) { project.commit }
 
@@ -88,6 +88,10 @@ describe Issue, "Mentionable" do
     let(:author)  { create(:author) }
     let(:issues)  { create_list(:issue, 2, project: project, author: author) }
 
+    before do
+      project.team << [author, Gitlab::Access::DEVELOPER]
+    end
+
     context 'before changes are persisted' do
       it 'ignores pre-existing references' do
         issue = create_issue(description: issues[0].to_reference)

spec/models/issue_spec.rb

@@ -306,4 +306,257 @@ describe Issue, models: true do
       expect(user2.assigned_open_issues_count).to eq(1)
     end
   end
+
+  describe '#visible_to_user?' do
+    context 'with a user' do
+      let(:user) { build(:user) }
+      let(:issue) { build(:issue) }
+
+      it 'returns true when the issue is readable' do
+        expect(issue).to receive(:readable_by?).with(user).and_return(true)
+
+        expect(issue.visible_to_user?(user)).to eq(true)
+      end
+
+      it 'returns false when the issue is not readable' do
+        expect(issue).to receive(:readable_by?).with(user).and_return(false)
+
+        expect(issue.visible_to_user?(user)).to eq(false)
+      end
+    end
+
+    context 'without a user' do
+      let(:issue) { build(:issue) }
+
+      it 'returns true when the issue is publicly visible' do
+        expect(issue).to receive(:publicly_visible?).and_return(true)
+
+        expect(issue.visible_to_user?).to eq(true)
+      end
+
+      it 'returns false when the issue is not publicly visible' do
+        expect(issue).to receive(:publicly_visible?).and_return(false)
+
+        expect(issue.visible_to_user?).to eq(false)
+      end
+    end
+  end
+
+  describe '#readable_by?' do
+    describe 'with a regular user that is not a team member' do
+      let(:user) { create(:user) }
+
+      context 'using a public project' do
+        let(:project) { create(:empty_project, :public) }
+
+        it 'returns true for a regular issue' do
+          issue = build(:issue, project: project)
+
+          expect(issue).to be_readable_by(user)
+        end
+
+        it 'returns false for a confidential issue' do
+          issue = build(:issue, project: project, confidential: true)
+
+          expect(issue).not_to be_readable_by(user)
+        end
+      end
+
+      context 'using an internal project' do
+        let(:project) { create(:empty_project, :internal) }
+
+        context 'using an internal user' do
+          it 'returns true for a regular issue' do
+            issue = build(:issue, project: project)
+
+            expect(issue).to be_readable_by(user)
+          end
+
+          it 'returns false for a confidential issue' do
+            issue = build(:issue, :confidential, project: project)
+
+            expect(issue).not_to be_readable_by(user)
+          end
+        end
+
+        context 'using an external user' do
+          before do
+            allow(user).to receive(:external?).and_return(true)
+          end
+
+          it 'returns false for a regular issue' do
+            issue = build(:issue, project: project)
+
+            expect(issue).not_to be_readable_by(user)
+          end
+
+          it 'returns false for a confidential issue' do
+            issue = build(:issue, :confidential, project: project)
+
+            expect(issue).not_to be_readable_by(user)
+          end
+        end
+      end
+
+      context 'using a private project' do
+        let(:project) { create(:empty_project, :private) }
+
+        it 'returns false for a regular issue' do
+          issue = build(:issue, project: project)
+
+          expect(issue).not_to be_readable_by(user)
+        end
+
+        it 'returns false for a confidential issue' do
+          issue = build(:issue, :confidential, project: project)
+
+          expect(issue).not_to be_readable_by(user)
+        end
+
+        context 'when the user is the project owner' do
+          it 'returns true for a regular issue' do
+            issue = build(:issue, project: project)
+
+            expect(issue).not_to be_readable_by(user)
+          end
+
+          it 'returns true for a confidential issue' do
+            issue = build(:issue, :confidential, project: project)
+
+            expect(issue).not_to be_readable_by(user)
+          end
+        end
+      end
+    end
+
+    context 'with a regular user that is a team member' do
+      let(:user) { create(:user) }
+      let(:project) { create(:empty_project, :public) }
+
+      context 'using a public project' do
+        before do
+          project.team << [user, Gitlab::Access::DEVELOPER]
+        end
+
+        it 'returns true for a regular issue' do
+          issue = build(:issue, project: project)
+
+          expect(issue).to be_readable_by(user)
+        end
+
+        it 'returns true for a confidential issue' do
+          issue = build(:issue, :confidential, project: project)
+
+          expect(issue).to be_readable_by(user)
+        end
+      end
+
+      context 'using an internal project' do
+        let(:project) { create(:empty_project, :internal) }
+
+        before do
+          project.team << [user, Gitlab::Access::DEVELOPER]
+        end
+
+        it 'returns true for a regular issue' do
+          issue = build(:issue, project: project)
+
+          expect(issue).to be_readable_by(user)
+        end
+
+        it 'returns true for a confidential issue' do
+          issue = build(:issue, :confidential, project: project)
+
+          expect(issue).to be_readable_by(user)
+        end
+      end
+
+      context 'using a private project' do
+        let(:project) { create(:empty_project, :private) }
+
+        before do
+          project.team << [user, Gitlab::Access::DEVELOPER]
+        end
+
+        it 'returns true for a regular issue' do
+          issue = build(:issue, project: project)
+
+          expect(issue).to be_readable_by(user)
+        end
+
+        it 'returns true for a confidential issue' do
+          issue = build(:issue, :confidential, project: project)
+
+          expect(issue).to be_readable_by(user)
+        end
+      end
+    end
+
+    context 'with an admin user' do
+      let(:project) { create(:empty_project) }
+      let(:user) { create(:user, admin: true) }
+
+      it 'returns true for a regular issue' do
+        issue = build(:issue, project: project)
+
+        expect(issue).to be_readable_by(user)
+      end
+
+      it 'returns true for a confidential issue' do
+        issue = build(:issue, :confidential, project: project)
+
+        expect(issue).to be_readable_by(user)
+      end
+    end
+  end
+
+  describe '#publicly_visible?' do
+    context 'using a public project' do
+      let(:project) { create(:empty_project, :public) }
+
+      it 'returns true for a regular issue' do
+        issue = build(:issue, project: project)
+
+        expect(issue).to be_publicly_visible
+      end
+
+      it 'returns false for a confidential issue' do
+        issue = build(:issue, :confidential, project: project)
+
+        expect(issue).not_to be_publicly_visible
+      end
+    end
+
+    context 'using an internal project' do
+      let(:project) { create(:empty_project, :internal) }
+
+      it 'returns false for a regular issue' do
+        issue = build(:issue, project: project)
+
+        expect(issue).not_to be_publicly_visible
+      end
+
+      it 'returns false for a confidential issue' do
+        issue = build(:issue, :confidential, project: project)
+
+        expect(issue).not_to be_publicly_visible
+      end
+    end
+
+    context 'using a private project' do
+      let(:project) { create(:empty_project, :private) }
+
+      it 'returns false for a regular issue' do
+        issue = build(:issue, project: project)
+
+        expect(issue).not_to be_publicly_visible
+      end
+
+      it 'returns false for a confidential issue' do
+        issue = build(:issue, :confidential, project: project)
+
+        expect(issue).not_to be_publicly_visible
+      end
+    end
+  end
 end