From 88781783dd425d1ba4ff5cacf9b4cc4c23a9a35e Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 22 May 2014 14:23:20 +0300 Subject: Delete branch service with permission checks Signed-off-by: Dmitriy Zaporozhets --- app/services/delete_branch_service.rb | 42 +++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 app/services/delete_branch_service.rb diff --git a/app/services/delete_branch_service.rb b/app/services/delete_branch_service.rb new file mode 100644 index 00000000000..9f48ab4b60d --- /dev/null +++ b/app/services/delete_branch_service.rb @@ -0,0 +1,42 @@ +class DeleteBranchService + def execute(project, branch_name, current_user) + repository = project.repository + branch = repository.find_branch(branch_name) + + # No such branch + unless branch + return error('No such branch') + end + + # Dont allow remove of protected branch + if project.protected_branch?(branch_name) + return error('Protected branch cant be removed') + end + + # Dont allow user to remove branch if he is not allowed to push + unless current_user.can?(:push_code, project) + return error('You dont have push access to repo') + end + + if repository.rm_branch(branch_name) + Event.create_ref_event(project, current_user, branch, 'rm') + success('Branch was removed') + else + return error('Failed to remove branch') + end + end + + def error(message) + { + message: message, + state: :error + } + end + + def success(message) + { + message: message, + state: :success + } + end +end -- cgit v1.2.1 From e089b11b7a4026acc7706e519682f4b7141b2bcd Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 22 May 2014 14:38:48 +0300 Subject: Add BranchesHelper Signed-off-by: Dmitriy Zaporozhets --- app/helpers/branches_helper.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 app/helpers/branches_helper.rb diff --git a/app/helpers/branches_helper.rb b/app/helpers/branches_helper.rb new file mode 100644 index 00000000000..e08ffccb94c --- /dev/null +++ b/app/helpers/branches_helper.rb @@ -0,0 +1,11 @@ +module BranchesHelper + def can_remove_branch?(project, branch_name) + if project.protected_branch? branch_name + false + elsif branch_name == project.repository.root_ref + false + else + can?(current_user, :push_code, project) + end + end +end -- cgit v1.2.1 From 0455391add2032dddc7353d1b0ae36d591818d3f Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 22 May 2014 14:39:09 +0300 Subject: Improve branch-removal logic Signed-off-by: Dmitriy Zaporozhets --- app/controllers/projects/branches_controller.rb | 9 ++------- app/services/delete_branch_service.rb | 4 ++++ app/views/projects/branches/_branch.html.haml | 4 ++-- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb index 6a6cbe48184..00811f17adb 100644 --- a/app/controllers/projects/branches_controller.rb +++ b/app/controllers/projects/branches_controller.rb @@ -4,8 +4,7 @@ class Projects::BranchesController < Projects::ApplicationController before_filter :require_non_empty_project before_filter :authorize_code_access! - before_filter :authorize_push!, only: [:create] - before_filter :authorize_admin_project!, only: [:destroy] + before_filter :authorize_push!, only: [:create, :destroy] def index @branches = Kaminari.paginate_array(@repository.branches).page(params[:page]).per(30) @@ -22,11 +21,7 @@ class Projects::BranchesController < Projects::ApplicationController end def destroy - branch = @repository.find_branch(params[:id]) - - if branch && @repository.rm_branch(branch.name) - Event.create_ref_event(@project, current_user, branch, 'rm') - end + DeleteBranchService.new.execute(project, params[:id], current_user) respond_to do |format| format.html { redirect_to project_branches_path(@project) } diff --git a/app/services/delete_branch_service.rb b/app/services/delete_branch_service.rb index 9f48ab4b60d..ce2d8093dff 100644 --- a/app/services/delete_branch_service.rb +++ b/app/services/delete_branch_service.rb @@ -8,6 +8,10 @@ class DeleteBranchService return error('No such branch') end + if branch_name == repository.root_ref + return error('Cannot remove HEAD branch') + end + # Dont allow remove of protected branch if project.protected_branch?(branch_name) return error('Protected branch cant be removed') diff --git a/app/views/projects/branches/_branch.html.haml b/app/views/projects/branches/_branch.html.haml index f0731977098..87f4dd88c27 100644 --- a/app/views/projects/branches/_branch.html.haml +++ b/app/views/projects/branches/_branch.html.haml @@ -16,8 +16,8 @@ %i.icon-copy Compare - - if can?(current_user, :admin_project, @project) && branch.name != @repository.root_ref - = link_to project_branch_path(@project, branch.name), class: 'btn btn-grouped btn-small remove-row', method: :delete, data: { confirm: 'Removed branch cannot be restored. Are you sure?'}, remote: true do + - if can_remove_branch?(@project, branch.name) + = link_to project_branch_path(@project, branch.name), class: 'btn btn-grouped btn-small btn-remove remove-row', method: :delete, data: { confirm: 'Removed branch cannot be restored. Are you sure?'}, remote: true do %i.icon-trash - if commit -- cgit v1.2.1 From a9d60b3b146acf5540b422919fa87fa931801062 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 22 May 2014 14:39:50 +0300 Subject: Api call to remove branch Signed-off-by: Dmitriy Zaporozhets --- lib/api/branches.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/lib/api/branches.rb b/lib/api/branches.rb index d54f9371fbe..32597eb94c4 100644 --- a/lib/api/branches.rb +++ b/lib/api/branches.rb @@ -84,6 +84,18 @@ module API present @branch, with: Entities::RepoObject, project: user_project end + + # Delete branch + # + # Parameters: + # id (required) - The ID of a project + # branch (required) - The name of the branch + # Example Request: + # DELETE /projects/:id/repository/branches/:branch + delete ":id/repository/branches/:branch" do + authorize_push_project + DeleteBranchService.new.execute(user_project, params[:branch], current_user) + end end end end -- cgit v1.2.1 From 36cac35b24d7b8318de40bd2e495b8807eef48fc Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 22 May 2014 15:38:47 +0300 Subject: Dont allow remove of protected branch Signed-off-by: Dmitriy Zaporozhets --- lib/gitlab/git_access.rb | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 4f49ca4189e..2f8b55aaca0 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -44,14 +44,18 @@ module Gitlab def push_allowed?(user, project, ref, oldrev, newrev, forced_push) if user && user_allowed?(user) action = if project.protected_branch?(ref) - if forced_push.to_s == 'true' - :force_push_code_to_protected_branches - else - :push_code_to_protected_branches - end - else - :push_code - end + # we dont allow force push to protected branch + if forced_push.to_s == 'true' + :force_push_code_to_protected_branches + # and we dont allow remove of protected branch + elsif newrev =~ /0000000/ + :remove_protected_branches + else + :push_code_to_protected_branches + end + else + :push_code + end user.can?(action, project) else false -- cgit v1.2.1 From b9d1fc2f3bdebe541795d6ef6e94da9e98b043d3 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 22 May 2014 18:31:22 +0300 Subject: Improve protected branch explanation Signed-off-by: Dmitriy Zaporozhets --- app/views/projects/protected_branches/index.html.haml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/views/projects/protected_branches/index.html.haml b/app/views/projects/protected_branches/index.html.haml index 4a6e8943a9f..e9f67b671bf 100644 --- a/app/views/projects/protected_branches/index.html.haml +++ b/app/views/projects/protected_branches/index.html.haml @@ -4,12 +4,12 @@ = render "projects/branches/filter" .col-md-9 .bs-callout.bs-callout-info - %p Protected branches designed to prevent push for all except #{link_to "masters", help_permissions_path, class: "vlink"}. - %p This ability allows: + %p Protected branches designed to %ul - %li keep stable branches secured - %li forced code review before merge to protected branches - %li prevents branch from force push + %li prevent push for all except #{link_to "masters", help_permissions_path, class: "vlink"}. + %li prevent branch from force push + %li prevent branch from removal + %p This ability allows to keep stable branches secured and force code review before merge to protected branches %p Read more about project permissions #{link_to "here", help_permissions_path, class: "underlined-link"} - if can? current_user, :admin_project, @project -- cgit v1.2.1