From 9aabd8fd5ecb4090515db48692f3d064624aec0a Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 6 Mar 2018 23:30:47 +0100 Subject: [PATCH] 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. --- app/models/project.rb | 45 +++++---- app/policies/project_policy.rb | 18 +--- lib/gitlab/checks/change_access.rb | 7 +- lib/gitlab/user_access.rb | 4 +- .../maintainer_edits_fork_spec.rb | 2 +- spec/lib/gitlab/checks/change_access_spec.rb | 5 +- spec/models/project_spec.rb | 91 +++++++++++++------ spec/policies/project_policy_spec.rb | 2 +- 8 files changed, 109 insertions(+), 65 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 0128967e038..a2b2e0554b1 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -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 + + 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 - @branches_allowing_maintainer_access_to_user[user] + 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 diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index e143a32e350..57ab0c23dcd 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -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? diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb index 87e9e47b21a..51ba09aa129 100644 --- a/lib/gitlab/checks/change_access.rb +++ b/lib/gitlab/checks/change_access.rb @@ -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 diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index fa32776d9f8..fd68b9f2b48 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -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 diff --git a/spec/features/merge_request/maintainer_edits_fork_spec.rb b/spec/features/merge_request/maintainer_edits_fork_spec.rb index d11ccb46a26..c1f76202e60 100644 --- a/spec/features/merge_request/maintainer_edits_fork_spec.rb +++ b/spec/features/merge_request/maintainer_edits_fork_spec.rb @@ -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) } diff --git a/spec/lib/gitlab/checks/change_access_spec.rb b/spec/lib/gitlab/checks/change_access_spec.rb index 2f173d2e75c..48e9902027c 100644 --- a/spec/lib/gitlab/checks/change_access_spec.rb +++ b/spec/lib/gitlab/checks/change_access_spec.rb @@ -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.') diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index a95a4637234..3463cf2eeca 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -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,37 +3396,76 @@ describe Project do end before do - target_project.add_developer(maintainer) + target_project.add_developer(user) end - 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') + 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 - it 'does not include branches for closed MRs' do - create(:merge_request, :closed, - target_project: target_project, - source_project: project, - source_branch: 'rejected-feature-1', - allow_maintainer_to_push: true) + describe '#branch_allows_maintainer_push?' do + it 'includes branch names for merge requests allowing maintainer access to a user' do + expect(project.branch_allows_maintainer_push?(user, 'awesome-feature-1')) + .to be_truthy + end - expect(project.branches_allowing_maintainer_access_to_user(maintainer)) - .not_to include('rejected-feature-1') - end + it 'does not allow guest users access' do + guest = create(:user) + target_project.add_guest(guest) - it 'only queries once per user' do - expect { 3.times { project.branches_allowing_maintainer_access_to_user(maintainer) } } - .not_to exceed_query_limit(1) - end + expect(project.branch_allows_maintainer_push?(guest, 'awesome-feature-1')) + .to be_falsy + end - context 'when the requeststore is active', :request_store do - it 'only queries once per user accross project instances' 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) } } - .not_to exceed_query_limit(3) + it 'does not include branches for closed MRs' do + create(:merge_request, :closed, + target_project: target_project, + source_project: project, + source_branch: 'rejected-feature-1', + allow_maintainer_to_push: true) + + 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.branch_allows_maintainer_push?(user, 'awesome-feature-1') } } + .not_to exceed_query_limit(1) + end + + context 'when the requeststore is active', :request_store do + it 'only queries once per user accross project instances' do + # limiting to 3 queries: + # 2 times loading the project + # once loading the accessible branches + 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 diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index f449fbd6b52..ea76e604153 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -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) }