Read about our upcoming Code of Conduct on this issue

Commit 77cf557c authored by Yorick Peterse's avatar Yorick Peterse
Browse files

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
......@@ -58,11 +58,37 @@ def authorized_snippets
.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
# 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 = Project.public_or_visible_to_user(current_user, use_where_in: false) do |part|
projects = projects_for_user do |part|
part.with_feature_available_for_user(:snippets, current_user)
end.select(:id)
......
......@@ -317,42 +317,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
......@@ -371,14 +342,11 @@ def self.with_feature_available_for_user(feature, user)
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
......
......@@ -601,6 +601,15 @@ def authorized_project?(project, min_access_level = nil)
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.
#
......
---
title: Revert Project.public_or_visible_to_user changes and only apply to snippets
merge_request:
author:
type: fixed
......@@ -1635,6 +1635,32 @@
end
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
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment