summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorPhil Hughes <me@iamphill.com>2017-05-08 07:42:00 +0000
committerPhil Hughes <me@iamphill.com>2017-05-08 07:42:00 +0000
commit59c9ec5ce6ef5a59fe989a06f0b8fa94aaba31d7 (patch)
treeea175b4ef388f79d8550a424ac5b46a535555664
parent1a5e84febeb3f2d63c019981e80c777195c37dd2 (diff)
parent1ebd9dad8e4a3ade4fa829d1d5ac6cbc9825bf48 (diff)
downloadgitlab-ce-59c9ec5ce6ef5a59fe989a06f0b8fa94aaba31d7.tar.gz
Merge branch '21393-allow-deletion-of-protected-branches' into 'master'
Add confirm delete protected branch modal Closes #21393 See merge request !11000
-rw-r--r--app/assets/javascripts/branches/branches_delete_modal.js36
-rw-r--r--app/assets/javascripts/dispatcher.js2
-rw-r--r--app/assets/stylesheets/framework/lists.scss1
-rw-r--r--app/controllers/projects/branches_controller.rb12
-rw-r--r--app/controllers/projects/tags_controller.rb4
-rw-r--r--app/helpers/branches_helper.rb10
-rw-r--r--app/policies/project_policy.rb6
-rw-r--r--app/services/delete_branch_service.rb16
-rw-r--r--app/views/projects/branches/_branch.html.haml35
-rw-r--r--app/views/projects/branches/_delete_protected_modal.html.haml34
-rw-r--r--app/views/projects/branches/index.html.haml2
-rw-r--r--lib/gitlab/checks/change_access.rb68
-rw-r--r--lib/gitlab/user_access.rb10
-rw-r--r--spec/features/projects/branches_spec.rb85
-rw-r--r--spec/lib/gitlab/checks/change_access_spec.rb87
-rw-r--r--spec/lib/gitlab/git_access_spec.rb2
-rw-r--r--spec/lib/gitlab/user_access_spec.rb48
-rw-r--r--spec/policies/project_policy_spec.rb2
-rw-r--r--spec/requests/api/branches_spec.rb13
-rw-r--r--spec/requests/api/v3/branches_spec.rb13
-rw-r--r--spec/services/delete_merged_branches_service_spec.rb31
21 files changed, 375 insertions, 142 deletions
diff --git a/app/assets/javascripts/branches/branches_delete_modal.js b/app/assets/javascripts/branches/branches_delete_modal.js
new file mode 100644
index 00000000000..af8bcdc1794
--- /dev/null
+++ b/app/assets/javascripts/branches/branches_delete_modal.js
@@ -0,0 +1,36 @@
+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;
diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js
index d27d89cf91d..bf802056d36 100644
--- a/app/assets/javascripts/dispatcher.js
+++ b/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':
diff --git a/app/assets/stylesheets/framework/lists.scss b/app/assets/stylesheets/framework/lists.scss
index c9a25946ffd..d76053fe72a 100644
--- a/app/assets/stylesheets/framework/lists.scss
+++ b/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;
diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb
index f0f031303d8..59247280559 100644
--- a/app/controllers/projects/branches_controller.rb
+++ b/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
diff --git a/app/controllers/projects/tags_controller.rb b/app/controllers/projects/tags_controller.rb
index e13f0bde315..750c3ec486a 100644
--- a/app/controllers/projects/tags_controller.rb
+++ b/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
diff --git a/app/helpers/branches_helper.rb b/app/helpers/branches_helper.rb
index b7a28b1b4a7..59519c1335b 100644
--- a/app/helpers/branches_helper.rb
+++ b/app/helpers/branches_helper.rb
@@ -1,14 +1,4 @@
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],
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index 8f25ac30a22..3959b895f44 100644
--- a/app/policies/project_policy.rb
+++ b/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
diff --git a/app/services/delete_branch_service.rb b/app/services/delete_branch_service.rb
index 38a113caec7..64b3c0118fb 100644
--- a/app/services/delete_branch_service.rb
+++ b/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
diff --git a/app/views/projects/branches/_branch.html.haml b/app/views/projects/branches/_branch.html.haml
index 0f9ef3eded3..8b35a037c55 100644
--- a/app/views/projects/branches/_branch.html.haml
+++ b/app/views/projects/branches/_branch.html.haml
@@ -30,13 +30,34 @@
= render 'projects/buttons/download', project: @project, ref: branch.name, pipeline: @refs_pipelines[branch.name]
- if can?(current_user, :push_code, @project)
- = 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'}",
- method: :delete,
- data: { confirm: "Deleting the '#{branch.name}' branch cannot be undone. Are you sure?" },
- remote: true,
- "aria-label" => "Delete branch" do
- = icon("trash-o")
+ - 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 has-tooltip",
+ title: "Delete branch",
+ method: :delete,
+ data: { confirm: "Deleting the '#{branch.name}' branch cannot be undone. Are you sure?" },
+ remote: true,
+ "aria-label" => "Delete branch" do
+ = icon("trash-o")
- if branch.name != @repository.root_ref
.divergence-graph{ title: "#{number_commits_ahead} commits ahead, #{number_commits_behind} commits behind #{@repository.root_ref}" }
diff --git a/app/views/projects/branches/_delete_protected_modal.html.haml b/app/views/projects/branches/_delete_protected_modal.html.haml
new file mode 100644
index 00000000000..c5888afa54d
--- /dev/null
+++ b/app/views/projects/branches/_delete_protected_modal.html.haml
@@ -0,0 +1,34 @@
+#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"
diff --git a/app/views/projects/branches/index.html.haml b/app/views/projects/branches/index.html.haml
index 91b86280e4c..4bade77a077 100644
--- a/app/views/projects/branches/index.html.haml
+++ b/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'
diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb
index 8793b20aa35..c984eb20606 100644
--- a/lib/gitlab/checks/change_access.rb
+++ b/lib/gitlab/checks/change_access.rb
@@ -1,7 +1,6 @@
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 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?
- if user_access.can_merge_to_branch?(@branch_name) || user_access.can_push_to_branch?(@branch_name)
- return
- else
+ 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?
diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb
index e46ff313654..3b922da7ced 100644
--- a/lib/gitlab/user_access.rb
+++ b/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?
diff --git a/spec/features/projects/branches_spec.rb b/spec/features/projects/branches_spec.rb
index 8e0306ce83b..7668ce5f8be 100644
--- a/spec/features/projects/branches_spec.rb
+++ b/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
diff --git a/spec/lib/gitlab/checks/change_access_spec.rb b/spec/lib/gitlab/checks/change_access_spec.rb
index 959ae02c222..8d81ed5856e 100644
--- a/spec/lib/gitlab/checks/change_access_spec.rb
+++ b/spec/lib/gitlab/checks/change_access_spec.rb
@@ -96,40 +96,77 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
end
end
- context 'protected branches check' do
- before do
- allow(ProtectedBranch).to receive(:protected?).with(project, 'master').and_return(true)
- end
-
- it 'returns an error if the user is not allowed to do forced pushes to protected branches' do
- expect(Gitlab::Checks::ForcePush).to receive(:force_push?).and_return(true)
+ context 'branches check' do
+ context 'trying to delete the default branch' do
+ let(:newrev) { '0000000000000000000000000000000000000000' }
+ let(:ref) { 'refs/heads/master' }
- expect(subject.status).to be(false)
- expect(subject.message).to eq('You are not allowed to force push code to a protected branch on this project.')
+ 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
- it 'returns an error if the user is not allowed to merge to protected branches' do
- expect_any_instance_of(Gitlab::Checks::MatchingMergeRequest).to receive(:match?).and_return(true)
- expect(user_access).to receive(:can_merge_to_branch?).and_return(false)
- expect(user_access).to receive(:can_push_to_branch?).and_return(false)
+ 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
- expect(subject.status).to be(false)
- expect(subject.message).to eq('You are not allowed to merge code into protected branches on this project.')
- end
+ it 'returns an error if the user is not allowed to do forced pushes to protected branches' do
+ expect(Gitlab::Checks::ForcePush).to receive(:force_push?).and_return(true)
- it 'returns an error if the user is not allowed to push to protected branches' do
- expect(user_access).to receive(:can_push_to_branch?).and_return(false)
+ expect(subject.status).to be(false)
+ expect(subject.message).to eq('You are not allowed to force push code to a protected branch on this project.')
+ end
- expect(subject.status).to be(false)
- expect(subject.message).to eq('You are not allowed to push code to protected branches on this project.')
- end
+ it 'returns an error if the user is not allowed to merge to protected branches' do
+ expect_any_instance_of(Gitlab::Checks::MatchingMergeRequest).to receive(:match?).and_return(true)
+ expect(user_access).to receive(:can_merge_to_branch?).and_return(false)
+ expect(user_access).to receive(:can_push_to_branch?).and_return(false)
- context 'branch deletion' do
- let(:newrev) { '0000000000000000000000000000000000000000' }
+ expect(subject.status).to be(false)
+ expect(subject.message).to eq('You are not allowed to merge code into protected branches on this project.')
+ end
+
+ it 'returns an error if the user is not allowed to push to protected branches' do
+ expect(user_access).to receive(:can_push_to_branch?).and_return(false)
- it 'returns an error if the user is not allowed to delete protected branches' 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 push code to protected branches on this project.')
+ end
+
+ context 'branch deletion' do
+ let(:newrev) { '0000000000000000000000000000000000000000' }
+ let(:ref) { 'refs/heads/feature' }
+
+ 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. 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
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index d8b72615fab..25769977f24 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -1,7 +1,7 @@
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 }
diff --git a/spec/lib/gitlab/user_access_spec.rb b/spec/lib/gitlab/user_access_spec.rb
index 2b27ff66c09..0d87cf25dbb 100644
--- a/spec/lib/gitlab/user_access_spec.rb
+++ b/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
diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb
index 064847ee3dc..0d3af1f4499 100644
--- a/spec/policies/project_policy_spec.rb
+++ b/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
diff --git a/spec/requests/api/branches_spec.rb b/spec/requests/api/branches_spec.rb
index 7eaa89837c8..c64499fc8c0 100644
--- a/spec/requests/api/branches_spec.rb
+++ b/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
diff --git a/spec/requests/api/v3/branches_spec.rb b/spec/requests/api/v3/branches_spec.rb
index 72f8fbe71fb..c88f7788697 100644
--- a/spec/requests/api/v3/branches_spec.rb
+++ b/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
diff --git a/spec/services/delete_merged_branches_service_spec.rb b/spec/services/delete_merged_branches_service_spec.rb
index 7b921f606f8..cae74df9c90 100644
--- a/spec/services/delete_merged_branches_service_spec.rb
+++ b/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
- service.execute
- end
+ it 'deletes a branch that was merged' do
+ service.execute
- it 'deletes a branch that was merged' do
- expect(project.repository.branch_names).not_to include('improve/awesome')
- end
+ expect(project.repository.branch_names).not_to include('improve/awesome')
+ end
- it 'keeps branch that is unmerged' do
- expect(project.repository.branch_names).to include('feature')
- end
+ it 'keeps branch that is unmerged' do
+ service.execute
- it 'keeps "master"' do
- expect(project.repository.branch_names).to include('master')
- end
+ expect(project.repository.branch_names).to include('feature')
end
- context 'protected branches' do
- before do
- create(:protected_branch, name: 'improve/awesome', project: project)
- service.execute
- end
+ it 'keeps "master"' do
+ service.execute
- 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