Commit 94fd4de7 authored by Sean McGivern's avatar Sean McGivern

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
parents 73579e66 6489d1ad
......@@ -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
......
---
title: Prevent accidental deletion of protected MR source branch by repeating checks
before actual deletion
merge_request:
author:
......@@ -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
......@@ -133,18 +133,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
......
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