Merge branch '21393-allow-deletion-of-protected-branches' into 'master'

Add confirm delete protected branch modal

Closes #21393

See merge request !11000

app/assets/javascripts/branches/branches_delete_modal.js

+const MODAL_SELECTOR = '#modal-delete-branch';
+
+class DeleteModal {
+  constructor() {
+    this.$modal = $(MODAL_SELECTOR);
+    this.$toggleBtns = $(`[data-target="${MODAL_SELECTOR}"]`);
+    this.$branchName = $('.js-branch-name', this.$modal);
+    this.$confirmInput = $('.js-delete-branch-input', this.$modal);
+    this.$deleteBtn = $('.js-delete-branch', this.$modal);
+    this.bindEvents();
+  }
+
+  bindEvents() {
+    this.$toggleBtns.on('click', this.setModalData.bind(this));
+    this.$confirmInput.on('input', this.setDeleteDisabled.bind(this));
+  }
+
+  setModalData(e) {
+    this.branchName = e.currentTarget.dataset.branchName || '';
+    this.deletePath = e.currentTarget.dataset.deletePath || '';
+    this.updateModal();
+  }
+
+  setDeleteDisabled(e) {
+    this.$deleteBtn.attr('disabled', e.currentTarget.value !== this.branchName);
+  }
+
+  updateModal() {
+    this.$branchName.text(this.branchName);
+    this.$confirmInput.val('');
+    this.$deleteBtn.attr('href', this.deletePath);
+    this.$deleteBtn.attr('disabled', true);
+  }
+}
+
+export default DeleteModal;

app/assets/javascripts/dispatcher.js

@@ -38,6 +38,7 @@
 import Issue from './issue';
 
 import BindInOut from './behaviors/bind_in_out';
+import DeleteModal from './branches/branches_delete_modal';
 import Group from './group';
 import GroupName from './group_name';
 import GroupsList from './groups_list';
@@ -180,6 +181,7 @@ const ShortcutsBlob = require('./shortcuts_blob');
           break;
         case 'projects:branches:index':
           gl.AjaxLoadingSpinner.init();
+          new DeleteModal();
           break;
         case 'projects:issues:new':
         case 'projects:issues:edit':

app/assets/stylesheets/framework/lists.scss

@@ -152,6 +152,7 @@ ul.content-list {
         margin-top: 3px;
         margin-bottom: 4px;
 
+        &.has-tooltip,
         &:last-child {
           margin-right: 0;
 

app/controllers/projects/branches_controller.rb

@@ -73,13 +73,17 @@ class Projects::BranchesController < Projects::ApplicationController
 
   def destroy
     @branch_name = Addressable::URI.unescape(params[:id])
-    status = DeleteBranchService.new(project, current_user).execute(@branch_name)
+    result = DeleteBranchService.new(project, current_user).execute(@branch_name)
+
     respond_to do |format|
       format.html do
-        redirect_to namespace_project_branches_path(@project.namespace,
-                                                    @project), status: 303
+        flash_type = result[:status] == :error ? :alert : :notice
+        flash[flash_type] = result[:message]
+
+        redirect_to namespace_project_branches_path(@project.namespace, @project), status: 303
       end
-      format.js { render nothing: true, status: status[:return_code] }
+
+      format.js { render nothing: true, status: result[:return_code] }
     end
   end
 

app/controllers/projects/tags_controller.rb

@@ -48,7 +48,7 @@ class Projects::TagsController < Projects::ApplicationController
     respond_to do |format|
       if result[:status] == :success
         format.html do
-          redirect_to namespace_project_tags_path(@project.namespace, @project)
+          redirect_to namespace_project_tags_path(@project.namespace, @project), status: 303
         end
 
         format.js
@@ -57,7 +57,7 @@ class Projects::TagsController < Projects::ApplicationController
 
         format.html do
           redirect_to namespace_project_tags_path(@project.namespace, @project),
-            alert: @error
+            alert: @error, status: 303
         end
 
         format.js do

app/helpers/branches_helper.rb

 module BranchesHelper
-  def can_remove_branch?(project, branch_name)
-    if ProtectedBranch.protected?(project, branch_name)
-      false
-    elsif branch_name == project.repository.root_ref
-      false
-    else
-      can?(current_user, :push_code, project)
-    end
-  end
-
   def filter_branches_path(options = {})
     exist_opts = {
       search: params[:search],

app/policies/project_policy.rb

@@ -98,7 +98,7 @@ class ProjectPolicy < BasePolicy
   end
 
   def master_access!
-    can! :push_code_to_protected_branches
+    can! :delete_protected_branch
     can! :update_project_snippet
     can! :update_environment
     can! :update_deployment
@@ -173,7 +173,7 @@ class ProjectPolicy < BasePolicy
   def archived_access!
     cannot! :create_merge_request
     cannot! :push_code
-    cannot! :push_code_to_protected_branches
+    cannot! :delete_protected_branch
     cannot! :update_merge_request
     cannot! :admin_merge_request
   end
@@ -211,7 +211,7 @@ class ProjectPolicy < BasePolicy
 
     unless repository_enabled
       cannot! :push_code
-      cannot! :push_code_to_protected_branches
+      cannot! :delete_protected_branch
       cannot! :download_code
       cannot! :fork_project
       cannot! :read_commit_status

app/services/delete_branch_service.rb

@@ -3,22 +3,14 @@ class DeleteBranchService < BaseService
     repository = project.repository
     branch = repository.find_branch(branch_name)
 
-    unless branch
-      return error('No such branch', 404)
-    end
-
-    if branch_name == repository.root_ref
-      return error('Cannot remove HEAD branch', 405)
-    end
-
-    if ProtectedBranch.protected?(project, branch_name)
-      return error('Protected branch cant be removed', 405)
-    end
-
     unless current_user.can?(:push_code, project)
       return error('You dont have push access to repo', 405)
     end
 
+    unless branch
+      return error('No such branch', 404)
+    end
+
     if repository.rm_branch(current_user, branch_name)
       success('Branch was removed')
     else

app/views/projects/branches/_branch.html.haml

@@ -30,8 +30,29 @@
       = render 'projects/buttons/download', project: @project, ref: branch.name, pipeline: @refs_pipelines[branch.name]
 
       - if can?(current_user, :push_code, @project)
+        - if branch.name == @project.repository.root_ref
+          %button{ class: "btn btn-remove remove-row js-ajax-loading-spinner has-tooltip disabled",
+            disabled: true,
+            title: "The default branch cannot be deleted" }
+            = icon("trash-o")
+        - elsif protected_branch?(@project, branch)
+          - if can?(current_user, :delete_protected_branch, @project)
+            %button{ class: "btn btn-remove remove-row js-ajax-loading-spinner has-tooltip",
+              title: "Delete protected branch",
+              data: { toggle: "modal",
+                target: "#modal-delete-branch",
+                delete_path: namespace_project_branch_path(@project.namespace, @project, branch.name),
+                branch_name: branch.name } }
+              = icon("trash-o")
+          - else
+            %button{ class: "btn btn-remove remove-row js-ajax-loading-spinner has-tooltip disabled",
+              disabled: true,
+              title: "Only a project master or owner can delete a protected branch" }
+              = icon("trash-o")
+        - else
           = link_to namespace_project_branch_path(@project.namespace, @project, branch.name),
-          class: "btn btn-remove remove-row js-ajax-loading-spinner #{can_remove_branch?(@project, branch.name) ? '' : 'disabled'}",
+            class: "btn btn-remove remove-row js-ajax-loading-spinner has-tooltip",
+            title: "Delete branch",
             method: :delete,
             data: { confirm: "Deleting the '#{branch.name}' branch cannot be undone. Are you sure?" },
             remote: true,

app/views/projects/branches/_delete_protected_modal.html.haml

+#modal-delete-branch.modal{ tabindex: -1 }
+  .modal-dialog
+    .modal-content
+      .modal-header
+        %button.close{ data: { dismiss: 'modal' } } ×
+        %h3.page-title
+          Delete protected branch
+          = surround "'", "'?" do
+            %span.js-branch-name>[branch name]
+
+      .modal-body
+        %p
+          You’re about to permanently delete the protected branch
+          = succeed '.' do
+            %strong.js-branch-name [branch name]
+        %p
+          Once you confirm and press
+          = succeed ',' do
+            %strong Delete protected branch
+          it cannot be undone or recovered.
+        %p
+          %strong To confirm, type
+          %kbd.js-branch-name [branch name]
+
+        .form-group
+        = text_field_tag 'delete_branch_input', '', class: 'form-control js-delete-branch-input'
+
+      .modal-footer
+        %button.btn{ data: { dismiss: 'modal' } } Cancel
+        = link_to 'Delete protected branch', '',
+          class: "btn btn-danger js-delete-branch",
+          title: 'Delete branch',
+          method: :delete,
+          "aria-label" => "Delete"

app/views/projects/branches/index.html.haml

@@ -37,3 +37,5 @@
     = paginate @branches, theme: 'gitlab'
   - else
     .nothing-here-block No branches to show
+
+= render 'projects/branches/delete_protected_modal'

lib/gitlab/checks/change_access.rb

 module Gitlab
   module Checks
     class ChangeAccess
-      # protocol is currently used only in EE
       attr_reader :user_access, :project, :skip_authorization, :protocol
 
       def initialize(
@@ -18,7 +17,9 @@ module Gitlab
       end
 
       def exec
-        error = push_checks || tag_checks || protected_branch_checks
+        return GitAccessStatus.new(true) if skip_authorization
+
+        error = push_checks || branch_checks || tag_checks
 
         if error
           GitAccessStatus.new(false, error)
@@ -29,35 +30,59 @@ module Gitlab
 
       protected
 
-      def protected_branch_checks
-        return if skip_authorization
+      def push_checks
+        if user_access.cannot_do_action?(:push_code)
+          "You are not allowed to push code to this project."
+        end
+      end
+
+      def branch_checks
         return unless @branch_name
+
+        if deletion? && @branch_name == project.default_branch
+          return "The default branch of a project cannot be deleted."
+        end
+
+        protected_branch_checks
+      end
+
+      def protected_branch_checks
         return unless ProtectedBranch.protected?(project, @branch_name)
 
         if forced_push?
           return "You are not allowed to force push code to a protected branch on this project."
-        elsif deletion?
-          return "You are not allowed to delete protected branches from this project."
         end
 
-        if matching_merge_request?
-          if user_access.can_merge_to_branch?(@branch_name) || user_access.can_push_to_branch?(@branch_name)
-            return
+        if deletion?
+          protected_branch_deletion_checks
         else
+          protected_branch_push_checks
+        end
+      end
+
+      def protected_branch_deletion_checks
+        unless user_access.can_delete_branch?(@branch_name)
+          return 'You are not allowed to delete protected branches from this project. Only a project master or owner can delete a protected branch.'
+        end
+
+        unless protocol == 'web'
+          'You can only delete protected branches using the web interface.'
+        end
+      end
+
+      def protected_branch_push_checks
+        if matching_merge_request?
+          unless user_access.can_merge_to_branch?(@branch_name) || user_access.can_push_to_branch?(@branch_name)
             "You are not allowed to merge code into protected branches on this project."
           end
         else
-          if user_access.can_push_to_branch?(@branch_name)
-            return
-          else
+          unless user_access.can_push_to_branch?(@branch_name)
             "You are not allowed to push code to protected branches on this project."
           end
         end
       end
 
       def tag_checks
-        return if skip_authorization
-
         return unless @tag_name
 
         if tag_exists? && user_access.cannot_do_action?(:admin_project)
@@ -68,7 +93,8 @@ module Gitlab
       end
 
       def protected_tag_checks
-        return unless tag_protected?
+        return unless ProtectedTag.protected?(project, @tag_name)
+
         return "Protected tags cannot be updated." if update?
         return "Protected tags cannot be deleted." if deletion?
 
@@ -77,18 +103,6 @@ module Gitlab
         end
       end
 
-      def tag_protected?
-        ProtectedTag.protected?(project, @tag_name)
-      end
-
-      def push_checks
-        return if skip_authorization
-
-        if user_access.cannot_do_action?(:push_code)
-          "You are not allowed to push code to this project."
-        end
-      end
-
       private
 
       def tag_exists?

lib/gitlab/user_access.rb

@@ -38,6 +38,16 @@ module Gitlab
       end
     end
 
+    def can_delete_branch?(ref)
+      return false unless can_access_git?
+
+      if ProtectedBranch.protected?(project, ref)
+        user.can?(:delete_protected_branch, project)
+      else
+        user.can?(:push_code, project)
+      end
+    end
+
     def can_push_to_branch?(ref)
       return false unless can_access_git?
 

spec/features/projects/branches_spec.rb

@@ -4,7 +4,13 @@ describe 'Branches', feature: true do
   let(:project) { create(:project, :public) }
   let(:repository) { project.repository }
 
-  context 'logged in' do
+  def set_protected_branch_name(branch_name)
+    find(".js-protected-branch-select").click
+    find(".dropdown-input-field").set(branch_name)
+    click_on("Create wildcard #{branch_name}")
+  end
+
+  context 'logged in as developer' do
     before do
       login_as :user
       project.team << [@user, :developer]
@@ -38,6 +44,83 @@ describe 'Branches', feature: true do
         expect(find('.all-branches')).to have_selector('li', count: 1)
       end
     end
+
+    describe 'Delete unprotected branch' do
+      it 'removes branch after confirmation', js: true do
+        visit namespace_project_branches_path(project.namespace, project)
+
+        fill_in 'branch-search', with: 'fix'
+
+        find('#branch-search').native.send_keys(:enter)
+
+        expect(page).to have_content('fix')
+        expect(find('.all-branches')).to have_selector('li', count: 1)
+        find('.js-branch-fix .btn-remove').trigger(:click)
+
+        expect(page).not_to have_content('fix')
+        expect(find('.all-branches')).to have_selector('li', count: 0)
+      end
+    end
+
+    describe 'Delete protected branch' do
+      before do
+        project.add_user(@user, :master)
+        visit namespace_project_protected_branches_path(project.namespace, project)
+        set_protected_branch_name('fix')
+        click_on "Protect"
+
+        within(".protected-branches-list") { expect(page).to have_content('fix') }
+        expect(ProtectedBranch.count).to eq(1)
+        project.add_user(@user, :developer)
+      end
+
+      it 'does not allow devleoper to removes protected branch', js: true do
+        visit namespace_project_branches_path(project.namespace, project)
+
+        fill_in 'branch-search', with: 'fix'
+        find('#branch-search').native.send_keys(:enter)
+
+        expect(page).to have_css('.btn-remove.disabled')
+      end
+    end
+  end
+
+  context 'logged in as master' do
+    before do
+      login_as :user
+      project.team << [@user, :master]
+    end
+
+    describe 'Delete protected branch' do
+      before do
+        visit namespace_project_protected_branches_path(project.namespace, project)
+        set_protected_branch_name('fix')
+        click_on "Protect"
+
+        within(".protected-branches-list") { expect(page).to have_content('fix') }
+        expect(ProtectedBranch.count).to eq(1)
+      end
+
+      it 'removes branch after modal confirmation', js: true do
+        visit namespace_project_branches_path(project.namespace, project)
+
+        fill_in 'branch-search', with: 'fix'
+        find('#branch-search').native.send_keys(:enter)
+
+        expect(page).to have_content('fix')
+        expect(find('.all-branches')).to have_selector('li', count: 1)
+        page.find('[data-target="#modal-delete-branch"]').trigger(:click)
+
+        expect(page).to have_css('.js-delete-branch[disabled]')
+        fill_in 'delete_branch_input', with: 'fix'
+        click_link 'Delete protected branch'
+
+        fill_in 'branch-search', with: 'fix'
+        find('#branch-search').native.send_keys(:enter)
+
+        expect(page).to have_content('No branches to show')
+      end
+    end
   end
 
   context 'logged out' do

spec/lib/gitlab/checks/change_access_spec.rb

@@ -96,9 +96,21 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
       end
     end
 
+    context 'branches check' do
+      context 'trying to delete the default branch' do
+        let(:newrev) { '0000000000000000000000000000000000000000' }
+        let(:ref) { 'refs/heads/master' }
+
+        it 'returns an error' do
+          expect(subject.status).to be(false)
+          expect(subject.message).to eq('The default branch of a project cannot be deleted.')
+        end
+      end
+
       context 'protected branches check' do
         before do
           allow(ProtectedBranch).to receive(:protected?).with(project, 'master').and_return(true)
+          allow(ProtectedBranch).to receive(:protected?).with(project, 'feature').and_return(true)
         end
 
         it 'returns an error if the user is not allowed to do forced pushes to protected branches' do
@@ -126,10 +138,35 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
 
         context 'branch deletion' do
           let(:newrev) { '0000000000000000000000000000000000000000' }
+          let(:ref) { 'refs/heads/feature' }
 
-        it 'returns an error if the user is not allowed to delete protected branches' do
+          context 'if the user is not allowed to delete protected branches' do
+            it 'returns an error' do
               expect(subject.status).to be(false)
-          expect(subject.message).to eq('You are not allowed to delete protected branches from this project.')
+              expect(subject.message).to eq('You are not allowed to delete protected branches from this project. Only a project master or owner can delete a protected branch.')
+            end
+          end
+
+          context 'if the user is allowed to delete protected branches' do
+            before do
+              project.add_master(user)
+            end
+
+            context 'through the web interface' do
+              let(:protocol) { 'web' }
+
+              it 'allows branch deletion' do
+                expect(subject.status).to be(true)
+              end
+            end
+
+            context 'over SSH or HTTP' do
+              it 'returns an error' do
+                expect(subject.status).to be(false)
+                expect(subject.message).to eq('You can only delete protected branches using the web interface.')
+              end
+            end
+          end
         end
       end
     end

spec/lib/gitlab/git_access_spec.rb

 require 'spec_helper'
 
 describe Gitlab::GitAccess, lib: true do
-  let(:access) { Gitlab::GitAccess.new(actor, project, 'web', authentication_abilities: authentication_abilities) }
+  let(:access) { Gitlab::GitAccess.new(actor, project, 'ssh', authentication_abilities: authentication_abilities) }
   let(:project) { create(:project, :repository) }
   let(:user) { create(:user) }
   let(:actor) { user }

spec/lib/gitlab/user_access_spec.rb

@@ -5,7 +5,7 @@ describe Gitlab::UserAccess, lib: true do
   let(:project) { create(:project) }
   let(:user) { create(:user) }
 
-  describe 'can_push_to_branch?' do
+  describe '#can_push_to_branch?' do
     describe 'push to none protected branch' do
       it 'returns true if user is a master' do
         project.team << [user, :master]
@@ -143,7 +143,7 @@ describe Gitlab::UserAccess, lib: true do
     end
   end
 
-  describe 'can_create_tag?' do
+  describe '#can_create_tag?' do
     describe 'push to none protected tag' do
       it 'returns true if user is a master' do
         project.add_user(user, :master)
@@ -211,4 +211,48 @@ describe Gitlab::UserAccess, lib: true do
       end
     end
   end
+
+  describe '#can_delete_branch?' do
+    describe 'delete unprotected branch' do
+      it 'returns true if user is a master' do
+        project.add_user(user, :master)
+
+        expect(access.can_delete_branch?('random_branch')).to be_truthy
+      end
+
+      it 'returns true if user is a developer' do
+        project.add_user(user, :developer)
+
+        expect(access.can_delete_branch?('random_branch')).to be_truthy
+      end
+
+      it 'returns false if user is a reporter' do
+        project.add_user(user, :reporter)
+
+        expect(access.can_delete_branch?('random_branch')).to be_falsey
+      end
+    end
+
+    describe 'delete protected branch' do
+      let(:branch) { create(:protected_branch, project: project, name: "test") }
+
+      it 'returns true if user is a master' do
+        project.add_user(user, :master)
+
+        expect(access.can_delete_branch?(branch.name)).to be_truthy
+      end
+
+      it 'returns false if user is a developer' do
+        project.add_user(user, :developer)
+
+        expect(access.can_delete_branch?(branch.name)).to be_falsey
+      end
+
+      it 'returns false if user is a reporter' do
+        project.add_user(user, :reporter)
+
+        expect(access.can_delete_branch?(branch.name)).to be_falsey
+      end
+    end
+  end
 end

spec/policies/project_policy_spec.rb

@@ -43,7 +43,7 @@ describe ProjectPolicy, models: true do
 
   let(:master_permissions) do
     %i[
-      push_code_to_protected_branches update_project_snippet update_environment
+      delete_protected_branch update_project_snippet update_environment
       update_deployment admin_milestone admin_project_snippet
       admin_project_member admin_note admin_wiki admin_project
       admin_commit_status admin_build admin_container_image

spec/requests/api/branches_spec.rb

@@ -406,19 +406,6 @@ describe API::Branches do
       delete api("/projects/#{project.id}/repository/branches/foobar", user)
       expect(response).to have_http_status(404)
     end
-
-    it "removes protected branch" do
-      create(:protected_branch, project: project, name: branch_name)
-      delete api("/projects/#{project.id}/repository/branches/#{branch_name}", user)
-      expect(response).to have_http_status(405)
-      expect(json_response['message']).to eq('Protected branch cant be removed')
-    end
-
-    it "does not remove HEAD branch" do
-      delete api("/projects/#{project.id}/repository/branches/master", user)
-      expect(response).to have_http_status(405)
-      expect(json_response['message']).to eq('Cannot remove HEAD branch')
-    end
   end
 
   describe "DELETE /projects/:id/repository/merged_branches" do

spec/requests/api/v3/branches_spec.rb

@@ -47,19 +47,6 @@ describe API::V3::Branches do
       delete v3_api("/projects/#{project.id}/repository/branches/foobar", user)
       expect(response).to have_http_status(404)
     end
-
-    it "removes protected branch" do
-      create(:protected_branch, project: project, name: branch_name)
-      delete v3_api("/projects/#{project.id}/repository/branches/#{branch_name}", user)
-      expect(response).to have_http_status(405)
-      expect(json_response['message']).to eq('Protected branch cant be removed')
-    end
-
-    it "does not remove HEAD branch" do
-      delete v3_api("/projects/#{project.id}/repository/branches/master", user)
-      expect(response).to have_http_status(405)
-      expect(json_response['message']).to eq('Cannot remove HEAD branch')
-    end
   end
 
   describe "DELETE /projects/:id/repository/merged_branches" do

spec/services/delete_merged_branches_service_spec.rb

@@ -6,33 +6,22 @@ describe DeleteMergedBranchesService, services: true do
   let(:project) { create(:project, :repository) }
 
   context '#execute' do
-    context 'unprotected branches' do
-      before do
+    it 'deletes a branch that was merged' do
       service.execute
-      end
 
-      it 'deletes a branch that was merged' do
       expect(project.repository.branch_names).not_to include('improve/awesome')
     end
 
     it 'keeps branch that is unmerged' do
+      service.execute
+
       expect(project.repository.branch_names).to include('feature')
     end
 
     it 'keeps "master"' do
-        expect(project.repository.branch_names).to include('master')
-      end
-    end
-
-    context 'protected branches' do
-      before do
-        create(:protected_branch, name: 'improve/awesome', project: project)
       service.execute
-      end
 
-      it 'keeps protected branch' do
-        expect(project.repository.branch_names).to include('improve/awesome')
-      end
+      expect(project.repository.branch_names).to include('master')
     end
 
     context 'user without rights' do