Refactor user authorization check for a single project to avoid querying all user projects18586-user-authorized_projects-is-slow

Currently, even when searching for all authorized issues of *one* project, we run the
`Users#authorized_projects` query (which can be rather slow). This update checks if
we are handling issues of just one project and does the authorization check locally.
It does have the downside of basically repeating the logic of `Users#authorized_projects`
on `Project#authorized_for_user`.

7 files changed, 157 insertions, 1 deletions

diff --git a/CHANGELOG b/CHANGELOG
index a5d26db7626..623c4ae8dae 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -32,6 +32,7 @@ v 8.10.0 (unreleased)
   - Display tooltip for mentioned users and groups !5261 (winniehell)
   - Allow build email service to be tested
   - Added day name to contribution calendar tooltips
+  - Refactor user authorization check for a single project to avoid querying all user projects
   - Make images fit to the size of the viewport !4810
   - Fix check for New Branch button on Issue page !4630 (winniehell)
   - Fix MR-auto-close text added to description. !4836
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 60abd47409e..60af8c15340 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -52,10 +52,50 @@ class Issue < ActiveRecord::Base
     attributes
   end
 
+  class << self
+    private
+
+    # Returns the project that the current scope belongs to if any, nil otherwise.
+    #
+    # Examples:
+    # - my_project.issues.without_due_date.owner_project => my_project
+    # - Issue.all.owner_project => nil
+    def owner_project
+      # No owner if we're not being called from an association
+      return unless all.respond_to?(:proxy_association)
+
+      owner = all.proxy_association.owner
+
+      # Check if the association is or belongs to a project
+      if owner.is_a?(Project)
+        owner
+      else
+        begin
+          owner.association(:project).target
+        rescue ActiveRecord::AssociationNotFoundError
+          nil
+        end
+      end
+    end
+  end
+
   def self.visible_to_user(user)
     return where('issues.confidential IS NULL OR issues.confidential IS FALSE') if user.blank?
     return all if user.admin?
 
+    # Check if we are scoped to a specific project's issues
+    if owner_project
+      if owner_project.authorized_for_user?(user, Gitlab::Access::REPORTER)
+        # If the project is authorized for the user, they can see all issues in the project
+        return all
+      else
+        # else only non confidential and authored/assigned to them
+        return where('issues.confidential IS NULL OR issues.confidential IS FALSE
+          OR issues.author_id = :user_id OR issues.assignee_id = :user_id',
+          user_id: user.id)
+      end
+    end
+
     where('
       issues.confidential IS NULL
       OR issues.confidential IS FALSE
diff --git a/app/models/project.rb b/app/models/project.rb
index a805f5d97bc..85fa3211512 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1164,4 +1164,44 @@ class Project < ActiveRecord::Base
   def ensure_dir_exist
     gitlab_shell.add_namespace(repository_storage_path, namespace.path)
   end
+
+  # Checks if `user` is authorized for this project, with at least the
+  # `min_access_level` (if given).
+  #
+  # If you change the logic of this method, please also update `User#authorized_projects`
+  def authorized_for_user?(user, min_access_level = nil)
+    return false unless user
+
+    return true if personal? && namespace_id == user.namespace_id
+
+    authorized_for_user_by_group?(user, min_access_level) ||
+      authorized_for_user_by_members?(user, min_access_level) ||
+      authorized_for_user_by_shared_projects?(user, min_access_level)
+  end
+
+  private
+
+  def authorized_for_user_by_group?(user, min_access_level)
+    member = user.group_members.find_by(source_id: group)
+
+    member && (!min_access_level || member.access_level >= min_access_level)
+  end
+
+  def authorized_for_user_by_members?(user, min_access_level)
+    member = members.find_by(user_id: user)
+
+    member && (!min_access_level || member.access_level >= min_access_level)
+  end
+
+  def authorized_for_user_by_shared_projects?(user, min_access_level)
+    shared_projects = user.group_members.joins(group: :shared_projects).
+      where(project_group_links: { project_id: self })
+
+    if min_access_level
+      members_scope = Gitlab::Access.access_level_scope(min_access_level)
+      shared_projects = shared_projects.where(members: members_scope)
+    end
+
+    shared_projects.any?
+  end
 end
diff --git a/app/models/user.rb b/app/models/user.rb
index 3d0a033785c..68ae81fb1c3 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -412,6 +412,8 @@ class User < ActiveRecord::Base
   end
 
   # Returns projects user is authorized to access.
+  #
+  # If you change the logic of this method, please also update `Project#authorized_for_user`
   def authorized_projects(min_access_level = nil)
     Project.where("projects.id IN (#{projects_union(min_access_level).to_sql})")
   end
@@ -854,7 +856,7 @@ class User < ActiveRecord::Base
                  groups.joins(:shared_projects).select(:project_id)]
 
     if min_access_level
-      scope = { access_level: Gitlab::Access.values.select { |access| access >= min_access_level } }
+      scope = Gitlab::Access.access_level_scope(min_access_level)
       relations = [relations.shift] + relations.map { |relation| relation.where(members: scope) }
     end
 
diff --git a/lib/gitlab/access.rb b/lib/gitlab/access.rb
index de41ea415a6..a376feac848 100644
--- a/lib/gitlab/access.rb
+++ b/lib/gitlab/access.rb
@@ -64,6 +64,10 @@ module Gitlab
       def protection_values
         protection_options.values
       end
+
+      def access_level_scope(min_access_level)
+        { access_level: values.select { |access| access >= min_access_level } }
+      end
     end
 
     def human_access
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index b87d68283e6..6a897c96690 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -22,6 +22,26 @@ describe Issue, models: true do
     it { is_expected.to have_db_index(:deleted_at) }
   end
 
+  describe 'visible_to_user' do
+    let(:user) { create(:user) }
+    let(:authorized_user) { create(:user) }
+    let(:project) { create(: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(Issue.visible_to_user(nil).count).to be(1)
+    end
+
+    it 'returns non confidential issues for user not authorized for the issues projects' do
+      expect(Issue.visible_to_user(user).count).to be(1)
+    end
+
+    it 'returns all issues for user authorized for the issues projects' do
+      expect(Issue.visible_to_user(authorized_user).count).to be(2)
+    end
+  end
+
   describe '#to_reference' do
     it 'returns a String reference to the object' do
       expect(subject.to_reference).to eq "##{subject.iid}"
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 9dc34276f18..e0fc965aa6f 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1146,4 +1146,53 @@ describe Project, models: true do
       end
     end
   end
+
+  describe 'authorized_for_user' do
+    let(:group) { create(:group) }
+    let(:developer) { create(:user) }
+    let(:master) { create(:user) }
+    let(:personal_project) { create(:project, namespace: developer.namespace) }
+    let(:group_project) { create(:project, namespace: group) }
+    let(:members_project) { create(:project) }
+    let(:shared_project) { create(:project) }
+
+    before do
+      group.add_master(master)
+      group.add_developer(developer)
+
+      members_project.team << [developer, :developer]
+      members_project.team << [master, :master]
+
+      create(:project_group_link, project: shared_project, group: group)
+    end
+
+    it 'returns false for no user' do
+      expect(personal_project.authorized_for_user?(nil)).to be(false)
+    end
+
+    it 'returns true for personal projects of the user' do
+      expect(personal_project.authorized_for_user?(developer)).to be(true)
+    end
+
+    it 'returns true for projects of groups the user is a member of' do
+      expect(group_project.authorized_for_user?(developer)).to be(true)
+    end
+
+    it 'returns true for projects for which the user is a member of' do
+      expect(members_project.authorized_for_user?(developer)).to be(true)
+    end
+
+    it 'returns true for projects shared on a group the user is a member of' do
+      expect(shared_project.authorized_for_user?(developer)).to be(true)
+    end
+
+    it 'checks for the correct minimum level access' do
+      expect(group_project.authorized_for_user?(developer, Gitlab::Access::MASTER)).to be(false)
+      expect(group_project.authorized_for_user?(master, Gitlab::Access::MASTER)).to be(true)
+      expect(members_project.authorized_for_user?(developer, Gitlab::Access::MASTER)).to be(false)
+      expect(members_project.authorized_for_user?(master, Gitlab::Access::MASTER)).to be(true)
+      expect(shared_project.authorized_for_user?(developer, Gitlab::Access::MASTER)).to be(false)
+      expect(shared_project.authorized_for_user?(master, Gitlab::Access::MASTER)).to be(true)
+    end
+  end
 end