summaryrefslogtreecommitdiff
path: root/spec
diff options
context:
space:
mode:
authorGitLab Release Tools Bot <robert+release-tools@gitlab.com>2019-10-24 20:26:26 +0000
committerGitLab Release Tools Bot <robert+release-tools@gitlab.com>2019-10-24 20:26:26 +0000
commit1581fb4cba8abf4439cea2ca138fd5f9818b0884 (patch)
treeeb0488d9a9c70df75774b07f417e8815ba0ced04 /spec
parenta2c31462f7fc013f41e7ca914a0b96869aa42c73 (diff)
parentb271eb42861c8067fc640a83a957742184d1221c (diff)
downloadgitlab-ce-1581fb4cba8abf4439cea2ca138fd5f9818b0884.tar.gz
Merge branch 'security-bvl-validate-force-remove-branch-on-mrs-12-4-ce' into '12-4-stable'
Only assign merge params when allowed See merge request gitlab/gitlabhq!3487
Diffstat (limited to 'spec')
-rw-r--r--spec/requests/api/merge_requests_spec.rb32
-rw-r--r--spec/services/auto_merge/base_service_spec.rb3
-rw-r--r--spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb7
-rw-r--r--spec/services/concerns/merge_requests/assigns_merge_params_spec.rb70
-rw-r--r--spec/services/merge_requests/build_service_spec.rb3
-rw-r--r--spec/services/merge_requests/update_service_spec.rb24
6 files changed, 133 insertions, 6 deletions
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