Merge branch '18586-user-authorized_projects-is-slow' into 'master'

Refactor user authorization check for a single project to avoid querying all user projects

See merge request !5102

6 files changed, 152 insertions, 0 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 991df661331..9f59d2f20ad 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -40,6 +40,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 GFM autocomplete not working on wiki pages
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 9b9d497fbd8..f09d915f20e 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1221,4 +1221,44 @@ class Project < ActiveRecord::Base
       { key: variable.key, value: variable.value, public: false }
     end
   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 = { access_level: Gitlab::Access.values.select { |access| access >= 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 7cbd97c0069..db747434959 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
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 a9f46348c28..9b017288488 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1276,4 +1276,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