Read about our upcoming Code of Conduct on this issue

Commit 8751b337 authored by Bob Van Landuyt's avatar Bob Van Landuyt
Browse files

Only allow users that can merge to push to source

We only allow users that can merge the merge request to push to the
fork.
parent f057930deaef
......@@ -865,7 +865,7 @@ def broken?
def can_be_merged_by?(user)
access = ::Gitlab::UserAccess.new(user, project: project)
access.can_push_to_branch?(target_branch) || access.can_merge_to_branch?(target_branch)
access.can_update_branch?(target_branch)
end
def can_be_merged_via_command_line_by?(user)
......
......@@ -1951,12 +1951,20 @@ def handle_update_attribute_error(ex, value)
end
def fetch_branch_allows_maintainer_push?(user, branch_name)
check_access = -> do
merge_request = source_of_merge_requests.opened
.where(allow_maintainer_to_push: true)
.find_by(source_branch: branch_name)
merge_request&.can_be_merged_by?(user)
end
if RequestStore.active?
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?
check_access.call
end
else
merge_requests_allowing_push_to_user(user).where(source_branch: branch_name).any?
check_access.call
end
end
end
......@@ -18,7 +18,7 @@
end
before do
target_project.add_developer(user)
target_project.add_master(user)
sign_in(user)
visit project_merge_request_path(target_project, merge_request)
......
......@@ -3383,12 +3383,13 @@ def domain_variable
context 'with cross project merge requests' do
let(:user) { create(:user) }
let(:target_project) { create(:project) }
let(:project) { fork_project(target_project) }
let(:target_project) { create(:project, :repository) }
let(:project) { fork_project(target_project, nil, repository: true) }
let!(:merge_request) do
create(
:merge_request,
target_project: target_project,
target_branch: 'target-branch',
source_project: project,
source_branch: 'awesome-feature-1',
allow_maintainer_to_push: true
......@@ -3429,7 +3430,7 @@ def domain_variable
end
describe '#branch_allows_maintainer_push?' do
it 'includes branch names for merge requests allowing maintainer access to a user' do
it 'allows access if the user can merge the merge request' do
expect(project.branch_allows_maintainer_push?(user, 'awesome-feature-1'))
.to be_truthy
end
......@@ -3442,9 +3443,10 @@ def domain_variable
.to be_falsy
end
it 'does not include branches for closed MRs' do
it 'does not allow access to branches for which the merge request was closed' do
create(:merge_request, :closed,
target_project: target_project,
target_branch: 'target-branch',
source_project: project,
source_branch: 'rejected-feature-1',
allow_maintainer_to_push: true)
......@@ -3453,18 +3455,26 @@ def domain_variable
.to be_falsy
end
it 'only queries once per user' do
it 'does not allow access if the user cannot merge the merge request' do
create(:protected_branch, :masters_can_push, project: target_project, name: 'target-branch')
expect(project.branch_allows_maintainer_push?(user, 'awesome-feature-1'))
.to be_falsy
end
it 'caches the result' do
control = ActiveRecord::QueryRecorder.new { project.branch_allows_maintainer_push?(user, 'awesome-feature-1') }
expect { 3.times { project.branch_allows_maintainer_push?(user, 'awesome-feature-1') } }
.not_to exceed_query_limit(1)
.not_to exceed_query_limit(control)
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
it 'only queries per project across instances' do
control = ActiveRecord::QueryRecorder.new { project.branch_allows_maintainer_push?(user, 'awesome-feature-1') }
expect { 2.times { described_class.find(project.id).branch_allows_maintainer_push?(user, 'awesome-feature-1') } }
.not_to exceed_query_limit(3)
.not_to exceed_query_limit(control).with_threshold(2)
end
end
end
......
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