Merge branch 'api-remove-branch' into 'master'

New rules for protected branches This MR change permissions logic for branch removal. Changes listed below: Before * developer can remove branch with terminal but not in UI * masters can remove any branch with UI even protected one * force-push to protected branch is not allowed but remove is allowed After * none can force push or remove protected branches * developers and masters can remove normal branches with console and UI

Merge branch 'api-remove-branch' into 'master'
New rules for protected branches This MR change permissions logic for branch removal. Changes listed below: Before * developer can remove branch with terminal but not in UI * masters can remove any branch with UI even protected one * force-push to protected branch is not allowed but remove is allowed After * none can force push or remove protected branches * developers and masters can remove normal branches with console and UI
Dmitriy Zaporozhets
2 parents eee14a0c b9d1fc2f
Showing 7 changed files with 90 additions and 22 deletions Show diff stats
app/controllers/projects/branches_controller.rb
app/helpers/branches_helper.rb
app/services/delete_branch_service.rb
app/views/projects/branches/_branch.html.haml
app/views/projects/protected_branches/index.html.haml
lib/api/branches.rb
lib/gitlab/git_access.rb
@@ -4,8 +4,7 @@ class Projects::BranchesController &lt; 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 &lt; 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) }
@@ -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
@@ -0,0 +1,46 @@
+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
+
+    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')
+    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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -0,0 +1,11 @@		@@ -0,0 +1,11 @@
	1	+module BranchesHelper
	2	+ def can_remove_branch?(project, branch_name)
	3	+ if project.protected_branch? branch_name
	4	+ false
	5	+ elsif branch_name == project.repository.root_ref
	6	+ false
	7	+ else
	8	+ can?(current_user, :push_code, project)
	9	+ end
	10	+ end
	11	+end
@@ -0,0 +1,46 @@		@@ -0,0 +1,46 @@
	1	+class DeleteBranchService
	2	+ def execute(project, branch_name, current_user)
	3	+ repository = project.repository
	4	+ branch = repository.find_branch(branch_name)
	5	+
	6	+ # No such branch
	7	+ unless branch
	8	+ return error('No such branch')
	9	+ end
	10	+
	11	+ if branch_name == repository.root_ref
	12	+ return error('Cannot remove HEAD branch')
	13	+ end
	14	+
	15	+ # Dont allow remove of protected branch
	16	+ if project.protected_branch?(branch_name)
	17	+ return error('Protected branch cant be removed')
	18	+ end
	19	+
	20	+ # Dont allow user to remove branch if he is not allowed to push
	21	+ unless current_user.can?(:push_code, project)
	22	+ return error('You dont have push access to repo')
	23	+ end
	24	+
	25	+ if repository.rm_branch(branch_name)
	26	+ Event.create_ref_event(project, current_user, branch, 'rm')
	27	+ success('Branch was removed')
	28	+ else
	29	+ return error('Failed to remove branch')
	30	+ end
	31	+ end
	32	+
	33	+ def error(message)
	34	+ {
	35	+ message: message,
	36	+ state: :error
	37	+ }
	38	+ end
	39	+
	40	+ def success(message)
	41	+ {
	42	+ message: message,
	43	+ state: :success
	44	+ }
	45	+ end
	46	+end
	@@ -84,6 +84,18 @@ module API		@@ -84,6 +84,18 @@ module API
84		84
85	present @branch, with: Entities::RepoObject, project: user_project	85	present @branch, with: Entities::RepoObject, project: user_project
86	end	86	end
		87	+
		88	+ # Delete branch
		89	+ #
		90	+ # Parameters:
		91	+ # id (required) - The ID of a project
		92	+ # branch (required) - The name of the branch
		93	+ # Example Request:
		94	+ # DELETE /projects/:id/repository/branches/:branch
		95	+ delete ":id/repository/branches/:branch" do
		96	+ authorize_push_project
		97	+ DeleteBranchService.new.execute(user_project, params[:branch], current_user)
		98	+ end
87	end	99	end
88	end	100	end
89	end	101	end