Merge branch 'developer_can_push_to_protected_branches_option' into 'master'

Developer can push to protected branches option

Adds option to enable option to enable push to protected branches for developers, per protected branch. Off by default, developers cannot push to protected branches.

If turned on, developers can push to protected branches but still cannot force push or remove the branch.

Allows more control over protected branches.

See merge request !1353

app/assets/javascripts/protected_branches.js.coffee

+$ ->
+  $(":checkbox").change ->
+    name = $(this).attr("name")
+    if name == "developers_can_push"
+      id = $(this).val()
+      checked = $(this).is(":checked")
+      url = $(this).data("url")
+      $.ajax
+        type: "PUT"
+        url: url
+        dataType: "json"
+        data:
+          id: id
+          developers_can_push: checked
+
+        success: ->
+          new Flash("Branch updated.", "notice")
+          location.reload true
+
+        error: ->
+          new Flash("Failed to update branch!", "alert")

app/assets/stylesheets/sections/projects.scss

@@ -308,3 +308,10 @@ ul.nav.nav-projects-tabs {
     display: none;
   }
 }
+
+
+table.table.protected-branches-list tr.no-border {
+  th, td {
+    border: 0;
+  }
+}

app/controllers/projects/protected_branches_controller.rb

@@ -15,6 +15,24 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
     redirect_to project_protected_branches_path(@project)
   end
 
+  def update
+    protected_branch = @project.protected_branches.find(params[:id])
+
+    if protected_branch &&
+       protected_branch.update_attributes(
+        developers_can_push: params[:developers_can_push]
+       )
+
+      respond_to do |format|
+        format.json { render :json => protected_branch, status: :ok }
+      end
+    else
+      respond_to do |format|
+        format.json { render json: protected_branch.errors, status: :unprocessable_entity }
+      end
+    end
+  end
+
   def destroy
     @project.protected_branches.find(params[:id]).destroy
 
@@ -27,6 +45,6 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
   private
 
   def protected_branch_params
-    params.require(:protected_branch).permit(:name)
+    params.require(:protected_branch).permit(:name, :developers_can_push)
   end
 end

app/models/project.rb

@@ -467,6 +467,10 @@ class Project < ActiveRecord::Base
     protected_branches_names.include?(branch_name)
   end
 
+  def developers_can_push_to_protected_branch?(branch_name)
+    protected_branches.any? { |pb| pb.name == branch_name && pb.developers_can_push }
+  end
+
   def forked?
     !(forked_project_link.nil? || forked_project_link.forked_from_project.nil?)
   end

app/views/projects/protected_branches/_branches_list.html.haml

+- unless @branches.empty?
+  %h5 Already Protected:
+  %table.table.protected-branches-list
+    %thead
+      %tr.no-border
+        %th Branch
+        %th Developers can push
+        %th
+
+    %tbody
+      - @branches.each do |branch|
+        - @url = project_protected_branch_path(@project, branch)
+        %tr
+          %td
+            = link_to project_commits_path(@project, branch.name) do
+              %strong= branch.name
+              - if @project.root_ref?(branch.name)
+                %span.label.label-info default
+            %td
+              = check_box_tag "developers_can_push", branch.id, branch.developers_can_push, "data-url" => @url
+            %td
+              .pull-right
+                - if can? current_user, :admin_project, @project
+                  = link_to 'Unprotect', [@project, branch], data: { confirm: 'Branch will be writable for developers. Are you sure?' }, method: :delete, class: "btn btn-remove btn-small"
+        %tr.no-border
+          %td
+            - if commit = branch.commit
+              = link_to project_commit_path(@project, commit.id), class: 'commit_short_id' do
+                = commit.short_id
+              %span.light
+                = gfm escape_once(truncate(commit.title, length: 40))
+              #{time_ago_with_tooltip(commit.committed_date)}
+            - else
+              (branch was removed from repository)
+          %td
+          %td

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

@@ -22,29 +22,14 @@
       = f.label :name, "Branch", class: 'control-label'
       .col-sm-10
         = f.select(:name, @project.open_branches.map { |br| [br.name, br.name] } , {include_blank: "Select branch"}, {class: "select2"})
+    .form-group
+      = f.label :developers_can_push, class: 'control-label' do
+        Developers can push
+      .col-sm-10
+        .checkbox
+          = f.check_box :developers_can_push
+          %span.descr Allow developers to push to this branch
     .form-actions
       = f.submit 'Protect', class: "btn-create btn"
-- unless @branches.empty?
-  %h5 Already Protected:
-  %ul.bordered-list.protected-branches-list
-    - @branches.each do |branch|
-      %li
-        %h4
-          = link_to project_commits_path(@project, branch.name) do
-            %strong= branch.name
-            - if @project.root_ref?(branch.name)
-              %span.label.label-info default
-            %span.label.label-success
-              %i.fa.fa-lock
-          .pull-right
-            - if can? current_user, :admin_project, @project
-              = link_to 'Unprotect', [@project, branch], data: { confirm: 'Branch will be writable for developers. Are you sure?' }, method: :delete, class: "btn btn-remove btn-small"
+= render 'branches_list'
 
-        - if commit = branch.commit
-          = link_to project_commit_path(@project, commit.id), class: 'commit_short_id' do
-            = commit.short_id
-          %span.light
-            = gfm escape_once(truncate(commit.title, length: 40))
-          #{time_ago_with_tooltip(commit.committed_date)}
-        - else
-          (branch was removed from repository)

config/routes.rb

@@ -263,7 +263,7 @@ Gitlab::Application.routes.draw do
 
       resources :branches, only: [:index, :new, :create, :destroy], constraints: { id: Gitlab::Regex.git_reference_regex }
       resources :tags, only: [:index, :new, :create, :destroy], constraints: { id: Gitlab::Regex.git_reference_regex }
-      resources :protected_branches, only: [:index, :create, :destroy], constraints: { id: Gitlab::Regex.git_reference_regex }
+      resources :protected_branches, only: [:index, :create, :update, :destroy], constraints: { id: Gitlab::Regex.git_reference_regex }
 
       resources :refs, only: [] do
         collection do

db/migrate/20141226080412_add_developers_can_push_to_protected_branches.rb

+class AddDevelopersCanPushToProtectedBranches < ActiveRecord::Migration
+  def change
+    add_column :protected_branches, :developers_can_push, :boolean, default: false, null: false
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20141217125223) do
+ActiveRecord::Schema.define(version: 20141226080412) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -322,10 +322,11 @@ ActiveRecord::Schema.define(version: 20141217125223) do
   add_index "projects", ["star_count"], name: "index_projects_on_star_count", using: :btree
 
   create_table "protected_branches", force: true do |t|
-    t.integer  "project_id", null: false
-    t.string   "name",       null: false
+    t.integer  "project_id",                          null: false
+    t.string   "name",                                null: false
     t.datetime "created_at"
     t.datetime "updated_at"
+    t.boolean  "developers_can_push", default: false, null: false
   end
 
   add_index "protected_branches", ["project_id"], name: "index_protected_branches_on_project_id", using: :btree
@@ -410,7 +411,6 @@ ActiveRecord::Schema.define(version: 20141217125223) do
     t.integer  "notification_level",       default: 1,     null: false
     t.datetime "password_expires_at"
     t.integer  "created_by_id"
-    t.datetime "last_credential_check_at"
     t.string   "avatar"
     t.string   "confirmation_token"
     t.datetime "confirmed_at"
@@ -418,6 +418,7 @@ ActiveRecord::Schema.define(version: 20141217125223) do
     t.string   "unconfirmed_email"
     t.boolean  "hide_no_ssh_key",          default: false
     t.string   "website_url",              default: "",    null: false
+    t.datetime "last_credential_check_at"
   end
 
   add_index "users", ["admin"], name: "index_users_on_admin", using: :btree

doc/permissions/permissions.md

@@ -28,6 +28,7 @@ If a user is a GitLab administrator they receive all permissions.
 | Add new team members                  |         |            |             | ✓        | ✓      |
 | Push to protected branches            |         |            |             | ✓        | ✓      |
 | Enable/disable branch protection      |         |            |             | ✓        | ✓      |
+| Turn on/off prot. branch push for devs|         |            |             | ✓        | ✓      |
 | Rewrite/remove git tags               |         |            |             | ✓        | ✓      |
 | Edit project                          |         |            |             | ✓        | ✓      |
 | Add deploy keys to project            |         |            |             | ✓        | ✓      |

doc/workflow/README.md

@@ -8,3 +8,4 @@
 - [GitLab Flow](gitlab_flow.md)
 - [Notifications](notifications.md)
 - [Migrating from SVN to GitLab](migrating_from_svn.md)
+- [Protected branches](protected_branches.md)

doc/workflow/protected_branches.md

+# Protected branches
+
+Permission in GitLab are fundamentally defined around the idea of having read or write permission to the repository and branches.
+
+To prevent people from messing with history or pushing code without review, we've created protected branches.
+
+A protected branch does three simple things:
+
+* it prevents pushes from everybody except users with Master permission
+* it prevents anyone from force pushing to the branch
+* it prevents anyone from deleting the branch
+
+You can make any branch a protected branch. GitLab makes the master branch a protected branch by default.
+
+To protect a branch, user needs to have at least a Master permission level, see [permissions document](permissions/permissions.md).
+
+![protected branches page](protected_branches/protected_branches1.png)
+
+Navigate to project settings page and select `protected branches`. From the `Branch` dropdown menu select the branch you want to protect.
+
+Some workflows, like [GitLab workflow](gitlab_flow.md), require all users with write access to submit a Merge request in order to get the code into a protected branch.
+
+Since Masters and Owners can already push to protected branches, that means Developers cannot push to protected branch and need to submit a Merge request.
+
+However, there are workflows where that is not needed and only protecting from force pushes and branch removal is useful.
+
+For those workflows, you can allow everyone with write access to push to a protected branch by selecting `Developers can push` check box.
+
+On already protected branches you can also allow developers to push to the repository by selecting the `Developers can push` check box.
+
+![Developers can push](protected_branches/protected_branches2.png)
+
+

lib/gitlab/git_access.rb

@@ -79,16 +79,8 @@ module Gitlab
       oldrev, newrev, ref = change.split(' ')
 
       action = if project.protected_branch?(branch_name(ref))
-                 # we dont allow force push to protected branch
-                 if forced_push?(project, oldrev, newrev)
-                   :force_push_code_to_protected_branches
-                   # and we dont allow remove of protected branch
-                 elsif newrev == Gitlab::Git::BLANK_SHA
-                   :remove_protected_branches
-                 else
-                   :push_code_to_protected_branches
-                 end
-               elsif project.repository.tag_names.include?(tag_name(ref))
+                 protected_branch_action(project, oldrev, newrev, branch_name(ref))
+               elsif protected_tag?(project, tag_name(ref))
                  # Prevent any changes to existing git tag unless user has permissions
                  :admin_project
                else
@@ -108,6 +100,24 @@ module Gitlab
 
     private
 
+    def protected_branch_action(project, oldrev, newrev, branch_name)
+      # we dont allow force push to protected branch
+      if forced_push?(project, oldrev, newrev)
+       :force_push_code_to_protected_branches
+       # and we dont allow remove of protected branch
+      elsif newrev == Gitlab::Git::BLANK_SHA
+       :remove_protected_branches
+      elsif project.developers_can_push_to_protected_branch?(branch_name)
+       :push_code
+      else
+       :push_code_to_protected_branches
+      end
+    end
+
+    def protected_tag?(project, tag_name)
+      project.repository.tag_names.include?(tag_name)
+    end
+
     def user_allowed?(user)
       Gitlab::UserAccess.allowed?(user)
     end

spec/lib/gitlab/git_access_spec.rb

@@ -129,6 +129,13 @@ describe Gitlab::GitAccess do
       }
     end
 
+    def self.updated_permissions_matrix
+      updated_permissions_matrix = permissions_matrix.dup
+      updated_permissions_matrix[:developer][:push_protected_branch] = true
+      updated_permissions_matrix[:developer][:push_all] = true
+      updated_permissions_matrix
+    end
+
     permissions_matrix.keys.each do |role|
       describe "#{role} access" do
         before { protect_feature_branch }
@@ -143,5 +150,22 @@ describe Gitlab::GitAccess do
         end
       end
     end
+
+    context "with enabled developers push to protected branches " do
+      updated_permissions_matrix.keys.each do |role|
+        describe "#{role} access" do
+          before { create(:protected_branch, name: 'feature', developers_can_push: true, project: project) }
+          before { project.team << [user, role] }
+
+          updated_permissions_matrix[role].each do |action, allowed|
+            context action do
+              subject { access.push_access_check(user, project, changes[action]) }
+
+              it { subject.allowed?.should allowed ? be_true : be_false }
+            end
+          end
+        end
+      end
+    end
   end
 end