diff options
author | Sean McGivern <sean@mcgivern.me.uk> | 2017-07-04 18:51:33 +0000 |
---|---|---|
committer | James Edwards-Jones <jedwardsjones@gitlab.com> | 2017-07-04 20:17:59 +0100 |
commit | e1e3087c721f20fb0c5a09037c921cf630123e88 (patch) | |
tree | 45c161a93020239effc9dd4b1b9e376d2cf88d7d | |
parent | 3b59528d0b9a30dbf527df2e9b90653577efcf49 (diff) | |
download | gitlab-ce-e1e3087c721f20fb0c5a09037c921cf630123e88.tar.gz |
Merge branch 'dm-always-verify-source-branch-can-be-deleted' into 'master'
Prevent accidental deletion of protected MR source branch by repeating checks before actual deletion
Closes #34456
See merge request !12574
3 files changed, 69 insertions, 13 deletions
diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index fac3ac7a4c7..bc846e07f24 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -61,8 +61,12 @@ module MergeRequests MergeRequests::PostMergeService.new(project, current_user).execute(merge_request) if params[:should_remove_source_branch].present? || @merge_request.force_remove_source_branch? - DeleteBranchService.new(@merge_request.source_project, branch_deletion_user). - execute(merge_request.source_branch) + # Verify again that the source branch can be removed, since branch may be protected, + # or the source branch may have been updated. + if @merge_request.can_remove_source_branch?(branch_deletion_user) + DeleteBranchService.new(@merge_request.source_project, branch_deletion_user) + .execute(merge_request.source_branch) + end end end diff --git a/changelogs/unreleased/dm-always-verify-source-branch-can-be-deleted.yml b/changelogs/unreleased/dm-always-verify-source-branch-can-be-deleted.yml new file mode 100644 index 00000000000..f2e1f412502 --- /dev/null +++ b/changelogs/unreleased/dm-always-verify-source-branch-can-be-deleted.yml @@ -0,0 +1,5 @@ +--- +title: Prevent accidental deletion of protected MR source branch by repeating checks + before actual deletion +merge_request: +author: diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index d96f819e66a..b33a0ca3a32 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe MergeRequests::MergeService, services: true do let(:user) { create(:user) } let(:user2) { create(:user) } - let(:merge_request) { create(:merge_request, assignee: user2) } + let(:merge_request) { create(:merge_request, :simple, author: user2, assignee: user2) } let(:project) { merge_request.project } before do @@ -131,18 +131,65 @@ describe MergeRequests::MergeService, services: true do it { expect(todo).to be_done } end - context 'remove source branch by author' do - let(:service) do - merge_request.merge_params['force_remove_source_branch'] = '1' - merge_request.save! - MergeRequests::MergeService.new(project, user, commit_message: 'Awesome message') + context 'source branch removal' do + context 'when the source branch is protected' do + let(:service) do + MergeRequests::MergeService.new(project, user, should_remove_source_branch: '1') + end + + before do + create(:protected_branch, project: project, name: merge_request.source_branch) + end + + it 'does not delete the source branch' do + expect(DeleteBranchService).not_to receive(:new) + service.execute(merge_request) + end end - it 'removes the source branch' do - expect(DeleteBranchService).to receive(:new). - with(merge_request.source_project, merge_request.author). - and_call_original - service.execute(merge_request) + context 'when the source branch is the default branch' do + let(:service) do + MergeRequests::MergeService.new(project, user, should_remove_source_branch: '1') + end + + before do + allow(project).to receive(:root_ref?).with(merge_request.source_branch).and_return(true) + end + + it 'does not delete the source branch' do + expect(DeleteBranchService).not_to receive(:new) + service.execute(merge_request) + end + end + + context 'when the source branch can be removed' do + context 'when MR author set the source branch to be removed' do + let(:service) do + merge_request.merge_params['force_remove_source_branch'] = '1' + merge_request.save! + MergeRequests::MergeService.new(project, user, commit_message: 'Awesome message') + end + + it 'removes the source branch using the author user' do + expect(DeleteBranchService).to receive(:new) + .with(merge_request.source_project, merge_request.author) + .and_call_original + service.execute(merge_request) + end + end + + context 'when MR merger set the source branch to be removed' do + let(:service) do + MergeRequests::MergeService.new(project, user, commit_message: 'Awesome message', should_remove_source_branch: '1') + end + + it 'removes the source branch using the current user' do + expect(DeleteBranchService).to receive(:new) + .with(merge_request.source_project, user) + .and_call_original + service.execute(merge_request) + end + end end end |