Limit queries to a user-branch combination
The query becomes a lot simpler if we can check the branch name as well instead of having to load all branch names.
This commit is contained in:
parent
9b27027619
commit
9aabd8fd5e
|
@ -1800,12 +1800,31 @@ class Project < ActiveRecord::Base
|
|||
Badge.where("id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection
|
||||
end
|
||||
|
||||
def branches_allowing_maintainer_access_to_user(user)
|
||||
@branches_allowing_maintainer_access_to_user ||= Hash.new do |result, user|
|
||||
result[user] = fetch_branches_allowing_maintainer_access_to_user(user)
|
||||
def merge_requests_allowing_push_to_user(user)
|
||||
return MergeRequest.none unless user
|
||||
|
||||
developer_access_exists = user.project_authorizations
|
||||
.where('access_level >= ? ', Gitlab::Access::DEVELOPER)
|
||||
.where('project_authorizations.project_id = merge_requests.target_project_id')
|
||||
.limit(1)
|
||||
.select(1)
|
||||
source_of_merge_requests.opened
|
||||
.where(allow_maintainer_to_push: true)
|
||||
.where('EXISTS (?)', developer_access_exists)
|
||||
end
|
||||
|
||||
@branches_allowing_maintainer_access_to_user[user]
|
||||
def branch_allows_maintainer_push?(user, branch_name)
|
||||
return false unless user
|
||||
|
||||
cache_key = "user:#{user.id}:#{branch_name}:branch_allows_push"
|
||||
|
||||
memoized_results = strong_memoize(:branch_allows_maintainer_push) do
|
||||
Hash.new do |result, cache_key|
|
||||
result[cache_key] = fetch_branch_allows_maintainer_push?(user, branch_name)
|
||||
end
|
||||
end
|
||||
|
||||
memoized_results[cache_key]
|
||||
end
|
||||
|
||||
private
|
||||
|
@ -1931,23 +1950,13 @@ class Project < ActiveRecord::Base
|
|||
raise ex
|
||||
end
|
||||
|
||||
def fetch_branches_allowing_maintainer_access_to_user(user)
|
||||
return [] unless user
|
||||
|
||||
projects_with_developer_access = user.project_authorizations
|
||||
.where('access_level >= ? ', Gitlab::Access::DEVELOPER)
|
||||
.select(:project_id)
|
||||
merge_requests_allowing_push = source_of_merge_requests.opened
|
||||
.where(allow_maintainer_to_push: true)
|
||||
.where(target_project_id: projects_with_developer_access)
|
||||
.select(:source_branch).uniq
|
||||
|
||||
def fetch_branch_allows_maintainer_push?(user, branch_name)
|
||||
if RequestStore.active?
|
||||
RequestStore.fetch("project-#{id}:user-#{user.id}:branches_allowing_maintainer_access") do
|
||||
merge_requests_allowing_push.pluck(:source_branch)
|
||||
RequestStore.fetch("project-#{id}:branch-#{branch_name}:user-#{user.id}:branch_allows_maintainer_push") do
|
||||
merge_requests_allowing_push_to_user(user).where(source_branch: branch_name).any?
|
||||
end
|
||||
else
|
||||
merge_requests_allowing_push.pluck(:source_branch)
|
||||
merge_requests_allowing_push_to_user(user).where(source_branch: branch_name).any?
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -61,9 +61,9 @@ class ProjectPolicy < BasePolicy
|
|||
desc "Project has request access enabled"
|
||||
condition(:request_access_enabled, scope: :subject) { project.request_access_enabled }
|
||||
|
||||
desc "The project has merge requests open that allow external users to push"
|
||||
condition(:merge_request_allows_push, scope: :subject) do
|
||||
project.branches_allowing_maintainer_access_to_user(@user).any?
|
||||
desc "Has merge requests allowing pushes to user"
|
||||
condition(:has_merge_requests_allowing_pushes, scope: :subject) do
|
||||
project.merge_requests_allowing_push_to_user(user).any?
|
||||
end
|
||||
|
||||
features = %w[
|
||||
|
@ -245,7 +245,6 @@ class ProjectPolicy < BasePolicy
|
|||
|
||||
rule { repository_disabled }.policy do
|
||||
prevent :push_code
|
||||
prevent :push_single_branch
|
||||
prevent :download_code
|
||||
prevent :fork_project
|
||||
prevent :read_commit_status
|
||||
|
@ -298,21 +297,14 @@ class ProjectPolicy < BasePolicy
|
|||
end
|
||||
|
||||
# These rules are included to allow maintainers of projects to push to certain
|
||||
# branches of forks.
|
||||
rule { can?(:public_access) & merge_request_allows_push }.policy do
|
||||
enable :push_single_branch
|
||||
# to run pipelines for the branches they have access to.
|
||||
rule { can?(:public_access) & has_merge_requests_allowing_pushes }.policy do
|
||||
enable :create_build
|
||||
enable :update_build
|
||||
enable :create_pipeline
|
||||
enable :update_pipeline
|
||||
end
|
||||
|
||||
# A wrapper around `push_code` and `push_single_branch` to avoid several
|
||||
# `push_code`: User can push everything to the repo
|
||||
# `push_single_brach`: User can push to a single branch in the repo
|
||||
# `push_to_repo`: User can push something to this repo.
|
||||
rule { can?(:push_code) | can?(:push_single_branch) }.enable :push_to_repo
|
||||
|
||||
private
|
||||
|
||||
def team_member?
|
||||
|
|
|
@ -47,7 +47,7 @@ module Gitlab
|
|||
protected
|
||||
|
||||
def push_checks
|
||||
if user_access.cannot_do_action?(:push_to_repo)
|
||||
unless can_push?
|
||||
raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:push_code]
|
||||
end
|
||||
end
|
||||
|
@ -183,6 +183,11 @@ module Gitlab
|
|||
def commits
|
||||
@commits ||= project.repository.new_commits(newrev)
|
||||
end
|
||||
|
||||
def can_push?
|
||||
user_access.can_do_action?(:push_code) ||
|
||||
user_access.can_push_to_branch?(branch_name)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -70,10 +70,8 @@ module Gitlab
|
|||
protected_branch_accessible_to?(ref, action: :push)
|
||||
elsif user.can?(:push_code, project)
|
||||
true
|
||||
elsif user.can?(:push_single_branch, project)
|
||||
project.branches_allowing_maintainer_access_to_user(user).include?(ref)
|
||||
else
|
||||
false
|
||||
project.branch_allows_maintainer_push?(user, ref)
|
||||
end
|
||||
end
|
||||
|
||||
|
|
|
@ -2,7 +2,7 @@ require 'spec_helper'
|
|||
|
||||
describe 'a maintainer edits files on a source-branch of an MR from a fork', :js do
|
||||
include ProjectForksHelper
|
||||
let(:user) { create(:user) }
|
||||
let(:user) { create(:user, username: 'the-maintainer') }
|
||||
let(:target_project) { create(:project, :public, :repository) }
|
||||
let(:author) { create(:user, username: 'mr-authoring-machine') }
|
||||
let(:source_project) { fork_project(target_project, author, repository: true) }
|
||||
|
|
|
@ -32,7 +32,8 @@ describe Gitlab::Checks::ChangeAccess do
|
|||
|
||||
context 'when the user is not allowed to push to the repo' do
|
||||
it 'raises an error' do
|
||||
expect(user_access).to receive(:can_do_action?).with(:push_to_repo).and_return(false)
|
||||
expect(user_access).to receive(:can_do_action?).with(:push_code).and_return(false)
|
||||
expect(user_access).to receive(:can_push_to_branch?).with('master').and_return(false)
|
||||
|
||||
expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to push code to this project.')
|
||||
end
|
||||
|
@ -42,7 +43,7 @@ describe Gitlab::Checks::ChangeAccess do
|
|||
let(:ref) { 'refs/tags/v1.0.0' }
|
||||
|
||||
it 'raises an error if the user is not allowed to update tags' do
|
||||
allow(user_access).to receive(:can_do_action?).with(:push_to_repo).and_return(true)
|
||||
allow(user_access).to receive(:can_do_action?).with(:push_code).and_return(true)
|
||||
expect(user_access).to receive(:can_do_action?).with(:admin_project).and_return(false)
|
||||
|
||||
expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to change existing tags on this project.')
|
||||
|
|
|
@ -3381,8 +3381,8 @@ describe Project do
|
|||
end
|
||||
end
|
||||
|
||||
describe '#branches_allowing_maintainer_access_to_user' do
|
||||
let(:maintainer) { create(:user) }
|
||||
context 'with cross project merge requests' do
|
||||
let(:user) { create(:user) }
|
||||
let(:target_project) { create(:project) }
|
||||
let(:project) { fork_project(target_project) }
|
||||
let!(:merge_request) do
|
||||
|
@ -3396,12 +3396,50 @@ describe Project do
|
|||
end
|
||||
|
||||
before do
|
||||
target_project.add_developer(maintainer)
|
||||
target_project.add_developer(user)
|
||||
end
|
||||
|
||||
describe '#merge_requests_allowing_push_to_user' do
|
||||
it 'returns open merge requests for which the user has developer access to the target project' do
|
||||
expect(project.merge_requests_allowing_push_to_user(user)).to include(merge_request)
|
||||
end
|
||||
|
||||
it 'does not include closed merge requests' do
|
||||
merge_request.close
|
||||
|
||||
expect(project.merge_requests_allowing_push_to_user(user)).to be_empty
|
||||
end
|
||||
|
||||
it 'does not include merge requests for guest users' do
|
||||
guest = create(:user)
|
||||
target_project.add_guest(guest)
|
||||
|
||||
expect(project.merge_requests_allowing_push_to_user(guest)).to be_empty
|
||||
end
|
||||
|
||||
it 'does not include the merge request for other users' do
|
||||
other_user = create(:user)
|
||||
|
||||
expect(project.merge_requests_allowing_push_to_user(other_user)).to be_empty
|
||||
end
|
||||
|
||||
it 'is empty when no user is passed' do
|
||||
expect(project.merge_requests_allowing_push_to_user(nil)).to be_empty
|
||||
end
|
||||
end
|
||||
|
||||
describe '#branch_allows_maintainer_push?' do
|
||||
it 'includes branch names for merge requests allowing maintainer access to a user' do
|
||||
expect(project.branches_allowing_maintainer_access_to_user(maintainer))
|
||||
.to include('awesome-feature-1')
|
||||
expect(project.branch_allows_maintainer_push?(user, 'awesome-feature-1'))
|
||||
.to be_truthy
|
||||
end
|
||||
|
||||
it 'does not allow guest users access' do
|
||||
guest = create(:user)
|
||||
target_project.add_guest(guest)
|
||||
|
||||
expect(project.branch_allows_maintainer_push?(guest, 'awesome-feature-1'))
|
||||
.to be_falsy
|
||||
end
|
||||
|
||||
it 'does not include branches for closed MRs' do
|
||||
|
@ -3411,12 +3449,12 @@ describe Project do
|
|||
source_branch: 'rejected-feature-1',
|
||||
allow_maintainer_to_push: true)
|
||||
|
||||
expect(project.branches_allowing_maintainer_access_to_user(maintainer))
|
||||
.not_to include('rejected-feature-1')
|
||||
expect(project.branch_allows_maintainer_push?(user, 'rejected-feature-1'))
|
||||
.to be_falsy
|
||||
end
|
||||
|
||||
it 'only queries once per user' do
|
||||
expect { 3.times { project.branches_allowing_maintainer_access_to_user(maintainer) } }
|
||||
expect { 3.times { project.branch_allows_maintainer_push?(user, 'awesome-feature-1') } }
|
||||
.not_to exceed_query_limit(1)
|
||||
end
|
||||
|
||||
|
@ -3425,9 +3463,10 @@ describe Project do
|
|||
# limiting to 3 queries:
|
||||
# 2 times loading the project
|
||||
# once loading the accessible branches
|
||||
expect { 2.times { described_class.find(project.id).branches_allowing_maintainer_access_to_user(maintainer) } }
|
||||
expect { 2.times { described_class.find(project.id).branch_allows_maintainer_push?(user, 'awesome-feature-1') } }
|
||||
.not_to exceed_query_limit(3)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -323,7 +323,7 @@ describe ProjectPolicy do
|
|||
)
|
||||
end
|
||||
let(:maintainer_abilities) do
|
||||
%w(push_single_branch create_build update_build create_pipeline update_pipeline)
|
||||
%w(create_build update_build create_pipeline update_pipeline)
|
||||
end
|
||||
|
||||
subject { described_class.new(user, project) }
|
||||
|
|
Loading…
Reference in New Issue