From b271eb42861c8067fc640a83a957742184d1221c Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Wed, 25 Sep 2019 18:25:40 +0200 Subject: Only assign merge params when allowed When a user updates a merge request coming from a fork, they should not be able to set `force_remove_source_branch` if they cannot push code to the source project. Otherwise developers of the target project could remove the source branch of the source project by setting this flag through the API. --- spec/requests/api/merge_requests_spec.rb | 32 ++++++++++ spec/services/auto_merge/base_service_spec.rb | 3 +- .../merge_when_pipeline_succeeds_service_spec.rb | 7 ++- .../merge_requests/assigns_merge_params_spec.rb | 70 ++++++++++++++++++++++ spec/services/merge_requests/build_service_spec.rb | 3 +- .../services/merge_requests/update_service_spec.rb | 24 ++++++++ 6 files changed, 133 insertions(+), 6 deletions(-) create mode 100644 spec/services/concerns/merge_requests/assigns_merge_params_spec.rb (limited to 'spec') diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 8179da2f97c..05160a33e61 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -1737,6 +1737,38 @@ describe API::MergeRequests do expect(json_response['state']).to eq('closed') expect(json_response['force_remove_source_branch']).to be_truthy end + + context 'with a merge request across forks' do + let(:fork_owner) { create(:user) } + let(:source_project) { fork_project(project, fork_owner) } + let(:target_project) { project } + + let(:merge_request) do + create(:merge_request, + source_project: source_project, + target_project: target_project, + source_branch: 'fixes', + merge_params: { 'force_remove_source_branch' => false }) + end + + it 'is true for an authorized user' do + put api("/projects/#{target_project.id}/merge_requests/#{merge_request.iid}", fork_owner), params: { state_event: 'close', remove_source_branch: true } + + expect(response).to have_gitlab_http_status(200) + expect(json_response['state']).to eq('closed') + expect(json_response['force_remove_source_branch']).to be true + end + + it 'is false for an unauthorized user' do + expect do + put api("/projects/#{target_project.id}/merge_requests/#{merge_request.iid}", target_project.owner), params: { state_event: 'close', remove_source_branch: true } + end.not_to change { merge_request.reload.merge_params } + + expect(response).to have_gitlab_http_status(200) + expect(json_response['state']).to eq('closed') + expect(json_response['force_remove_source_branch']).to be false + end + end end context "to close a MR" do diff --git a/spec/services/auto_merge/base_service_spec.rb b/spec/services/auto_merge/base_service_spec.rb index a409f21a7e4..0a6bcb1badc 100644 --- a/spec/services/auto_merge/base_service_spec.rb +++ b/spec/services/auto_merge/base_service_spec.rb @@ -51,7 +51,7 @@ describe AutoMerge::BaseService do expect(merge_request.merge_params['commit_message']).to eq("Merge branch 'patch-12' into 'master'") expect(merge_request.merge_params['sha']).to eq('200fcc9c260f7219eaf0daba87d818f0922c5b18') expect(merge_request.merge_params['should_remove_source_branch']).to eq(false) - expect(merge_request.merge_params['squash']).to eq(false) + expect(merge_request.squash).to eq(false) expect(merge_request.merge_params['squash_commit_message']).to eq('Update README.md') end end @@ -108,7 +108,6 @@ describe AutoMerge::BaseService do 'commit_message' => "Merge branch 'patch-12' into 'master'", 'sha' => "200fcc9c260f7219eaf0daba87d818f0922c5b18", 'should_remove_source_branch' => false, - 'squash' => false, 'squash_commit_message' => "Update README.md" } end diff --git a/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb b/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb index 931b52470c4..ccbb4e7c30d 100644 --- a/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb +++ b/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb @@ -59,8 +59,9 @@ describe AutoMerge::MergeWhenPipelineSucceedsService do it 'sets the params, merge_user, and flag' do expect(merge_request).to be_valid expect(merge_request.merge_when_pipeline_succeeds).to be_truthy - expect(merge_request.merge_params).to include commit_message: 'Awesome message' + expect(merge_request.merge_params).to include 'commit_message' => 'Awesome message' expect(merge_request.merge_user).to be user + expect(merge_request.auto_merge_strategy).to eq AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS end it 'creates a system note' do @@ -73,7 +74,7 @@ describe AutoMerge::MergeWhenPipelineSucceedsService do end context 'already approved' do - let(:service) { described_class.new(project, user, new_key: true) } + let(:service) { described_class.new(project, user, should_remove_source_branch: true) } let(:build) { create(:ci_build, ref: mr_merge_if_green_enabled.source_branch) } before do @@ -90,7 +91,7 @@ describe AutoMerge::MergeWhenPipelineSucceedsService do expect(SystemNoteService).not_to receive(:merge_when_pipeline_succeeds) service.execute(mr_merge_if_green_enabled) - expect(mr_merge_if_green_enabled.merge_params).to have_key(:new_key) + expect(mr_merge_if_green_enabled.merge_params).to have_key('should_remove_source_branch') end end end diff --git a/spec/services/concerns/merge_requests/assigns_merge_params_spec.rb b/spec/services/concerns/merge_requests/assigns_merge_params_spec.rb new file mode 100644 index 00000000000..5b653aa331c --- /dev/null +++ b/spec/services/concerns/merge_requests/assigns_merge_params_spec.rb @@ -0,0 +1,70 @@ +require 'spec_helper' + +describe MergeRequests::AssignsMergeParams do + it 'raises an error when used from an instance that does not respond to #current_user' do + define_class = -> { Class.new { include MergeRequests::AssignsMergeParams }.new } + + expect { define_class.call }.to raise_error %r{can not be included in (.*) without implementing #current_user} + end + + describe '#assign_allowed_merge_params' do + let(:merge_request) { build(:merge_request) } + + let(:params) do + { commit_message: 'Commit Message', + 'should_remove_source_branch' => true, + unknown_symbol: 'Unknown symbol', + 'unknown_string' => 'Unknown String' } + end + + subject(:merge_request_service) do + Class.new do + attr_accessor :current_user + + include MergeRequests::AssignsMergeParams + + def initialize(user) + @current_user = user + end + end + end + + it 'only assigns known parameters to the merge request' do + service = merge_request_service.new(merge_request.author) + + service.assign_allowed_merge_params(merge_request, params) + + expect(merge_request.merge_params).to eq('commit_message' => 'Commit Message', 'should_remove_source_branch' => true) + end + + it 'returns a hash without the known merge params' do + service = merge_request_service.new(merge_request.author) + + result = service.assign_allowed_merge_params(merge_request, params) + + expect(result).to eq({ 'unknown_symbol' => 'Unknown symbol', 'unknown_string' => 'Unknown String' }) + end + + context 'the force_remove_source_branch param' do + let(:params) { { force_remove_source_branch: true } } + + it 'assigns the param if the user is allowed to do that' do + service = merge_request_service.new(merge_request.author) + + result = service.assign_allowed_merge_params(merge_request, params) + + expect(merge_request.force_remove_source_branch?).to be true + expect(result).to be_empty + end + + it 'only removes the param if the user is not allowed to do that' do + service = merge_request_service.new(build(:user)) + + result = service.assign_allowed_merge_params(merge_request, params) + + expect(merge_request.force_remove_source_branch?).to be_falsy + expect(result).to be_empty + end + end + end +end diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index d546a092680..68e53553043 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -83,8 +83,9 @@ describe MergeRequests::BuildService do expect(merge_request.force_remove_source_branch?).to be_falsey end - context 'with force_remove_source_branch parameter' do + context 'with force_remove_source_branch parameter when the user is authorized' do let(:mr_params) { params.merge(force_remove_source_branch: '1') } + let(:source_project) { fork_project(project, user) } let(:merge_request) { described_class.new(project, user, mr_params).execute } it 'assigns force_remove_source_branch' do diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index d3c4c436901..d31f5dc0176 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -646,5 +646,29 @@ describe MergeRequests::UpdateService, :mailer do expect(merge_request.allow_collaboration).to be_truthy end end + + context 'updating `force_remove_source_branch`' do + let(:target_project) { create(:project, :repository, :public) } + let(:source_project) { fork_project(target_project, nil, repository: true) } + let(:user) { target_project.owner } + let(:merge_request) do + create(:merge_request, + source_project: source_project, + source_branch: 'fixes', + target_project: target_project) + end + + it "cannot be done by members of the target project when they don't have access" do + expect { update_merge_request(force_remove_source_branch: true) } + .not_to change { merge_request.reload.force_remove_source_branch? }.from(nil) + end + + it 'can be done by members of the target project if they can push to the source project' do + source_project.add_developer(user) + + expect { update_merge_request(force_remove_source_branch: true) } + .to change { merge_request.reload.force_remove_source_branch? }.from(nil).to(true) + end + end end end -- cgit v1.2.1