BigW Consortium Gitlab

Commit cfa9d1ed by Bob Van Landuyt

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 558e9cd9
......@@ -865,7 +865,7 @@ class MergeRequest < ActiveRecord::Base
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 @@ class Project < ActiveRecord::Base
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 @@ describe 'a maintainer edits files on a source-branch of an MR from a fork', :js
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 @@ describe Project do
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 @@ describe Project do
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 @@ describe Project do
.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 @@ describe Project do
.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