Prevent accidental deletion of protected MR source branch by repeating checks...

Prevent accidental deletion of protected MR source branch by repeating checks before actual deletion

app/services/merge_requests/merge_service.rb

@@ -61,10 +61,14 @@ module MergeRequests
       MergeRequests::PostMergeService.new(project, current_user).execute(merge_request)
 
       if params[:should_remove_source_branch].present? || @merge_request.force_remove_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
 
     def branch_deletion_user
       @merge_request.force_remove_source_branch? ? @merge_request.author : current_user

changelogs/unreleased/dm-always-verify-source-branch-can-be-deleted.yml

+---
+title: Prevent accidental deletion of protected MR source branch by repeating checks
+  before actual deletion
+merge_request:
+author:

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
@@ -133,14 +133,46 @@ describe MergeRequests::MergeService, services: true do
       it { expect(todo).to be_done }
     end
 
-    context 'remove source branch by author' do
+    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
+
+      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' do
+          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
@@ -148,6 +180,21 @@ describe MergeRequests::MergeService, services: true do
           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
+
     context "error handling" do
       let(:service) { MergeRequests::MergeService.new(project, user, commit_message: 'Awesome message') }