Added "developers can merge" setting to protected branches

- Cherry-picked from `mvestergaard:branch-protection-dev-merge`
- https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4220

15 files changed, 88 insertions, 14 deletions

diff --git a/CHANGELOG b/CHANGELOG
index f1dcc40a273..e144ba8513f 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -497,6 +497,7 @@ v 8.7.7
   - Prevent unauthorized access to other projects build traces
   - Forbid scripting for wiki files
   - Only show notes through JSON on confidential issues that the user has access to
+  - Added protected branch setting that allows Developers to accept merge requests while push is still disallowed. !4220 (Mathias Vestergaard)
 
 v 8.7.6
   - Fix links on wiki pages for relative url setups. !4131 (Artem Sidorenko)
diff --git a/app/assets/javascripts/protected_branches.js.coffee b/app/assets/javascripts/protected_branches.js.coffee
index 79c2306e4d2..ce2fc883620 100644
--- a/app/assets/javascripts/protected_branches.js.coffee
+++ b/app/assets/javascripts/protected_branches.js.coffee
@@ -1,9 +1,11 @@
 $ ->
   $(".protected-branches-list :checkbox").change (e) ->
     name = $(this).attr("name")
-    if name == "developers_can_push"
+    row = $(this).parents("tr")
+    if name == "developers_can_push" || name == "developers_can_merge"
       id = $(this).val()
-      checked = $(this).is(":checked")
+      can_push = row.find("input[name=developers_can_push]").is(":checked")
+      can_merge = row.find("input[name=developers_can_merge]").is(":checked")
       url = $(this).data("url")
       $.ajax
         type: "PUT"
@@ -12,7 +14,8 @@ $ ->
         data:
           id: id
           protected_branch:
-            developers_can_push: checked
+            developers_can_push: can_push
+            developers_can_merge: can_merge
 
         success: ->
           row = $(e.target)
diff --git a/app/controllers/projects/protected_branches_controller.rb b/app/controllers/projects/protected_branches_controller.rb
index 80dad758afa..10dca47fded 100644
--- a/app/controllers/projects/protected_branches_controller.rb
+++ b/app/controllers/projects/protected_branches_controller.rb
@@ -50,6 +50,6 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
   end
 
   def protected_branch_params
-    params.require(:protected_branch).permit(:name, :developers_can_push)
+    params.require(:protected_branch).permit(:name, :developers_can_push, :developers_can_merge)
   end
 end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 157901378d3..23d68706527 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -552,7 +552,8 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def can_be_merged_by?(user)
-    ::Gitlab::GitAccess.new(user, project, 'web').can_push_to_branch?(target_branch)
+    access = ::Gitlab::GitAccess.new(user, project, 'web')
+    access.can_push_to_branch?(target_branch) || access.can_merge_to_branch?(target_branch)
   end
 
   def mergeable_ci_state?
diff --git a/app/models/project.rb b/app/models/project.rb
index a66b750cd48..2bd97fc48f1 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -832,6 +832,10 @@ class Project < ActiveRecord::Base
     protected_branches.matching(branch_name).any?(&:developers_can_push)
   end
 
+  def developers_can_merge_to_protected_branch?(branch_name)
+    protected_branches.matching(branch_name).any?(&:developers_can_merge)
+  end
+
   def forked?
     !(forked_project_link.nil? || forked_project_link.forked_from_project.nil?)
   end
diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb
index a886f35981f..e02b50ff9a2 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -89,7 +89,8 @@ class GitPushService < BaseService
     # Set protection on the default branch if configured
     if current_application_settings.default_branch_protection != PROTECTION_NONE
       developers_can_push = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? true : false
-      @project.protected_branches.create({ name: @project.default_branch, developers_can_push: developers_can_push })
+      developers_can_merge = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_MERGE ? true : false
+      @project.protected_branches.create({ name: @project.default_branch, developers_can_push: developers_can_push, developers_can_merge: developers_can_merge })
     end
   end
 
diff --git a/app/views/projects/protected_branches/_branches_list.html.haml b/app/views/projects/protected_branches/_branches_list.html.haml
index 97cb1a9052b..181d1a27c73 100644
--- a/app/views/projects/protected_branches/_branches_list.html.haml
+++ b/app/views/projects/protected_branches/_branches_list.html.haml
@@ -8,6 +8,7 @@
   .table-responsive
     %table.table.protected-branches-list
       %colgroup
+        %col{ width: "27%" }
         %col{ width: "30%" }
         %col{ width: "25%" }
         %col{ width: "25%" }
@@ -18,6 +19,7 @@
           %th Protected Branch
           %th Commit
           %th Developers Can Push
+          %th Developers can Merge
           - if can_admin_project
             %th
       %tbody
diff --git a/app/views/projects/protected_branches/_protected_branch.html.haml b/app/views/projects/protected_branches/_protected_branch.html.haml
index 474aec3a97c..7fda7f96047 100644
--- a/app/views/projects/protected_branches/_protected_branch.html.haml
+++ b/app/views/projects/protected_branches/_protected_branch.html.haml
@@ -16,6 +16,8 @@
         (branch was removed from repository)
   %td
     = check_box_tag("developers_can_push", protected_branch.id, protected_branch.developers_can_push, data: { url: url })
+  %td
+    = check_box_tag("developers_can_merge", protected_branch.id, protected_branch.developers_can_merge, data: { url: url })
   - if can_admin_project
     %td
       = link_to 'Unprotect', [@project.namespace.becomes(Namespace), @project, protected_branch], data: { confirm: 'Branch will be writable for developers. Are you sure?' }, method: :delete, class: "btn btn-warning btn-sm pull-right"
diff --git a/app/views/projects/protected_branches/index.html.haml b/app/views/projects/protected_branches/index.html.haml
index 3fab95751e0..101b3f3b452 100644
--- a/app/views/projects/protected_branches/index.html.haml
+++ b/app/views/projects/protected_branches/index.html.haml
@@ -36,6 +36,14 @@
             = f.label :developers_can_push, "Developers can push", class: "label-light append-bottom-0"
             %p.light.append-bottom-0
               Allow developers to push to this branch
+
+        .form-group
+          = f.check_box :developers_can_merge, class: "pull-left"
+          .prepend-left-20
+            = f.label :developers_can_merge, "Developers can merge", class: "label-light append-bottom-0"
+            %p.light.append-bottom-0
+              Allow developers to accept merge requests to this branch
         = f.submit "Protect", class: "btn-create btn protect-branch-btn", disabled: true
+
     %hr
     = render "branches_list"
diff --git a/db/migrate/20160519203051_add_developers_can_merge_to_protected_branches.rb b/db/migrate/20160519203051_add_developers_can_merge_to_protected_branches.rb
new file mode 100644
index 00000000000..15ad8e8bcbb
--- /dev/null
+++ b/db/migrate/20160519203051_add_developers_can_merge_to_protected_branches.rb
@@ -0,0 +1,9 @@
+class AddDevelopersCanMergeToProtectedBranches < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  def change
+    add_column_with_default :protected_branches, :developers_can_merge, :boolean, default: false, allow_null: false
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index f24e47b85b2..4562e6bb0c3 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -860,11 +860,12 @@ ActiveRecord::Schema.define(version: 20160712171823) do
   add_index "projects", ["visibility_level"], name: "index_projects_on_visibility_level", using: :btree
 
   create_table "protected_branches", force: :cascade 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
+    t.boolean  "developers_can_push",  default: false, null: false
+    t.boolean  "developers_can_merge", default: false, null: false
   end
 
   add_index "protected_branches", ["project_id"], name: "index_protected_branches_on_project_id", using: :btree
diff --git a/lib/gitlab/access.rb b/lib/gitlab/access.rb
index 831f1e635ba..de41ea415a6 100644
--- a/lib/gitlab/access.rb
+++ b/lib/gitlab/access.rb
@@ -14,9 +14,10 @@ module Gitlab
     OWNER     = 50
 
     # Branch protection settings
-    PROTECTION_NONE         = 0
-    PROTECTION_DEV_CAN_PUSH = 1
-    PROTECTION_FULL         = 2
+    PROTECTION_NONE          = 0
+    PROTECTION_DEV_CAN_PUSH  = 1
+    PROTECTION_FULL          = 2
+    PROTECTION_DEV_CAN_MERGE = 3
 
     class << self
       def values
@@ -54,6 +55,7 @@ module Gitlab
       def protection_options
         {
           "Not protected: Both developers and masters can push new commits, force push, or delete the branch." => PROTECTION_NONE,
+          "Protected against pushes: Developers cannot push new commits, but are allowed to accept merge requests to the branch." => PROTECTION_DEV_CAN_MERGE,
           "Partially protected: Developers can push new commits, but cannot force push or delete the branch. Masters can do all of those." => PROTECTION_DEV_CAN_PUSH,
           "Fully protected: Developers cannot push new commits, force push, or delete the branch. Only masters can do any of those." => PROTECTION_FULL,
         }
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index 7679c7e4bb8..e20e3338262 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -39,6 +39,16 @@ module Gitlab
       end
     end
 
+    def can_merge_to_branch?(ref)
+      return false unless user
+
+      if project.protected_branch?(ref) && !project.developers_can_merge_to_protected_branch?(ref)
+        user.can?(:push_code_to_protected_branches, project)
+      else
+        user.can?(:push_code, project)
+      end
+    end
+
     def can_read_project?
       if user
         user.can?(:read_project, project)
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index c79ba11f782..b90d9c724f1 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -65,6 +65,27 @@ describe Gitlab::GitAccess, lib: true do
         expect(access.can_push_to_branch?(@branch.name)).to be_falsey
       end
     end
+
+    describe 'merge to protected branch if allowed for developers' do
+      before do
+        @branch = create :protected_branch, project: project, developers_can_merge: true
+      end
+
+      it "returns true if user is a master" do
+        project.team << [user, :master]
+        expect(access.can_merge_to_branch?(@branch.name)).to be_truthy
+      end
+
+      it "returns true if user is a developer" do
+        project.team << [user, :developer]
+        expect(access.can_merge_to_branch?(@branch.name)).to be_truthy
+      end
+
+      it "returns false if user is a reporter" do
+        project.team << [user, :reporter]
+        expect(access.can_merge_to_branch?(@branch.name)).to be_falsey
+      end
+    end
   end
 
   describe '#check with single protocols allowed' do
diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb
index afabeed4a80..0f30a84a2cf 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -224,7 +224,7 @@ describe GitPushService, services: true do
       it "when pushing a branch for the first time" do
         expect(project).to receive(:execute_hooks)
         expect(project.default_branch).to eq("master")
-        expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: false })
+        expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: false, developers_can_merge: false })
         execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
       end
 
@@ -242,7 +242,16 @@ describe GitPushService, services: true do
 
         expect(project).to receive(:execute_hooks)
         expect(project.default_branch).to eq("master")
-        expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: true })
+        expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: true, developers_can_merge: false })
+        execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
+      end
+
+      it "when pushing a branch for the first time with default branch protection set to 'developers can merge'" do
+        stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_MERGE)
+
+        expect(project).to receive(:execute_hooks)
+        expect(project.default_branch).to eq("master")
+        expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: false, developers_can_merge: true })
         execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
       end