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

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) }

app/helpers/branches_helper.rb

+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

app/services/delete_branch_service.rb

+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

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

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

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

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