From 6489d1ad4ff40b6d5acf92280dde756d37bd2489 Mon Sep 17 00:00:00 2001
From: Douwe Maan <douwe@selenight.nl>
Date: Thu, 29 Jun 2017 16:48:56 -0500
Subject: Prevent accidental deletion of protected MR source branch by
 repeating checks before actual deletion

---
 app/services/merge_requests/merge_service.rb       |  8 ++-
 ...-always-verify-source-branch-can-be-deleted.yml |  5 ++
 spec/services/merge_requests/merge_service_spec.rb | 69 ++++++++++++++++++----
 3 files changed, 69 insertions(+), 13 deletions(-)
 create mode 100644 changelogs/unreleased/dm-always-verify-source-branch-can-be-deleted.yml

diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb
index b247cb89e5e..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 711059208c1..19d9e4049fe 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
@@ -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
 
-- 
cgit v1.2.1