Merge branch '42877-fix-visibility-change-performance' into 'master'

Revert Project.public_or_visible_to_user changes but apply change to SnippetsFinder

Closes #42877

See merge request gitlab-org/gitlab-ce!17476

5 files changed, 77 insertions, 40 deletions

diff --git a/app/finders/snippets_finder.rb b/app/finders/snippets_finder.rb
index ec61fe1892e..8deef84555e 100644
--- a/app/finders/snippets_finder.rb
+++ b/app/finders/snippets_finder.rb
@@ -55,8 +55,37 @@ class SnippetsFinder < UnionFinder
     Snippet.where(feature_available_projects.or(not_project_related)).public_or_visible_to_user(current_user)
   end
 
+  # Returns a collection of projects that is either public or visible to the
+  # logged in user.
+  #
+  # A caller must pass in a block to modify individual parts of
+  # the query, e.g. to apply .with_feature_available_for_user on top of it.
+  # This is useful for performance as we can stick those additional filters
+  # at the bottom of e.g. the UNION.
+  def projects_for_user
+    return yield(Project.public_to_user) unless current_user
+
+    # If the current_user is allowed to see all projects,
+    # we can shortcut and just return.
+    return yield(Project.all) if current_user.full_private_access?
+
+    authorized_projects = yield(Project.where('EXISTS (?)', current_user.authorizations_for_projects))
+
+    levels = Gitlab::VisibilityLevel.levels_for_user(current_user)
+    visible_projects = yield(Project.where(visibility_level: levels))
+
+    # We use a UNION here instead of OR clauses since this results in better
+    # performance.
+    union = Gitlab::SQL::Union.new([authorized_projects.select('projects.id'), visible_projects.select('projects.id')])
+
+    Project.from("(#{union.to_sql}) AS #{Project.table_name}")
+  end
+
   def feature_available_projects
-    projects = Project.public_or_visible_to_user(current_user, use_where_in: false) do |part|
+    # Don't return any project related snippets if the user cannot read cross project
+    return table[:id].eq(nil) unless Ability.allowed?(current_user, :read_cross_project)
+
+    projects = projects_for_user do |part|
       part.with_feature_available_for_user(:snippets, current_user)
     end.select(:id)
 
diff --git a/app/models/project.rb b/app/models/project.rb
index 79058d51af8..194e9a2593b 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -316,42 +316,13 @@ class Project < ActiveRecord::Base
 
   # Returns a collection of projects that is either public or visible to the
   # logged in user.
-  #
-  # A caller may pass in a block to modify individual parts of
-  # the query, e.g. to apply .with_feature_available_for_user on top of it.
-  # This is useful for performance as we can stick those additional filters
-  # at the bottom of e.g. the UNION.
-  #
-  # Optionally, turning `use_where_in` off leads to returning a
-  # relation using #from instead of #where. This can perform much better
-  # but leads to trouble when used in conjunction with AR's #merge method.
-  def self.public_or_visible_to_user(user = nil, use_where_in: true, &block)
-    # If we don't get a block passed, use identity to avoid if/else repetitions
-    block = ->(part) { part } unless block_given?
-
-    return block.call(public_to_user) unless user
-
-    # If the user is allowed to see all projects,
-    # we can shortcut and just return.
-    return block.call(all) if user.full_private_access?
-
-    authorized = user
-      .project_authorizations
-      .select(1)
-      .where('project_authorizations.project_id = projects.id')
-    authorized_projects = block.call(where('EXISTS (?)', authorized))
-
-    levels = Gitlab::VisibilityLevel.levels_for_user(user)
-    visible_projects = block.call(where(visibility_level: levels))
-
-    # We use a UNION here instead of OR clauses since this results in better
-    # performance.
-    union = Gitlab::SQL::Union.new([authorized_projects.select('projects.id'), visible_projects.select('projects.id')])
-
-    if use_where_in
-      where("projects.id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection
+  def self.public_or_visible_to_user(user = nil)
+    if user
+      where('EXISTS (?) OR projects.visibility_level IN (?)',
+            user.authorizations_for_projects,
+            Gitlab::VisibilityLevel.levels_for_user(user))
     else
-      from("(#{union.to_sql}) AS #{table_name}")
+      public_to_user
     end
   end
 
@@ -370,14 +341,11 @@ class Project < ActiveRecord::Base
     elsif user
       column = ProjectFeature.quoted_access_level_column(feature)
 
-      authorized = user.project_authorizations.select(1)
-        .where('project_authorizations.project_id = projects.id')
-
       with_project_feature
         .where("#{column} IN (?) OR (#{column} = ? AND EXISTS (?))",
               visible,
               ProjectFeature::PRIVATE,
-              authorized)
+              user.authorizations_for_projects)
     else
       with_feature_access_level(feature, visible)
     end
diff --git a/app/models/user.rb b/app/models/user.rb
index e81e555c090..9093ba2a04f 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -596,6 +596,15 @@ class User < ActiveRecord::Base
     authorized_projects(min_access_level).exists?({ id: project.id })
   end
 
+  # Typically used in conjunction with projects table to get projects
+  # a user has been given access to.
+  #
+  # Example use:
+  # `Project.where('EXISTS(?)', user.authorizations_for_projects)`
+  def authorizations_for_projects
+    project_authorizations.select(1).where('project_authorizations.project_id = projects.id')
+  end
+
   # Returns the projects this user has reporter (or greater) access to, limited
   # to at most the given projects.
   #
diff --git a/changelogs/unreleased/revert-project-visibility-changes.yml b/changelogs/unreleased/revert-project-visibility-changes.yml
new file mode 100644
index 00000000000..df44fdb79b1
--- /dev/null
+++ b/changelogs/unreleased/revert-project-visibility-changes.yml
@@ -0,0 +1,5 @@
+---
+title: Revert Project.public_or_visible_to_user changes and only apply to snippets
+merge_request:
+author:
+type: fixed
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index dd1bfba1421..970a5ed056e 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1604,6 +1604,32 @@ describe User do
     it { is_expected.to eq([private_group]) }
   end
 
+  describe '#authorizations_for_projects' do
+    let!(:user) { create(:user) }
+    subject { Project.where("EXISTS (?)", user.authorizations_for_projects) }
+
+    it 'includes projects that belong to a user, but no other projects' do
+      owned = create(:project, :private, namespace: user.namespace)
+      member = create(:project, :private).tap { |p| p.add_master(user) }
+      other = create(:project)
+
+      expect(subject).to include(owned)
+      expect(subject).to include(member)
+      expect(subject).not_to include(other)
+    end
+
+    it 'includes projects a user has access to, but no other projects' do
+      other_user = create(:user)
+      accessible = create(:project, :private, namespace: other_user.namespace) do |project|
+        project.add_developer(user)
+      end
+      other = create(:project)
+
+      expect(subject).to include(accessible)
+      expect(subject).not_to include(other)
+    end
+  end
+
   describe '#authorized_projects', :delete do
     context 'with a minimum access level' do
       it 'includes projects for which the user is an owner' do