Merge branch '18193-developers-can-merge' into 'master'

Allow developers to merge into a protected branch without having push access

## What does this MR do?

Adds a "Developers can merge" checkbox to protected branches much like the "Developers can push" checkbox. When the checkbox is enabled, a developer can merge MRs into that protected branch from the Web UI and from the command-line (any push that is entirely composed of merge commits is allowed).

## Are there points in the code the reviewer needs to double check?

- This MR refactors the `GitAccess` module, moving parts of it to `UserAccess` and the new `ChangeAccessCheck`.
- This MR refactors `GitAccessSpec`, which generates a "matrix" of tests.
- The main logic "developers can merge" should be straightforward enough.
- The commits are fairly atomic, and the commit messages are descriptive regarding the motivations behind every change.

## Why was this MR needed?

A significant portion of this feature was implemented in !4220 (thanks, @mvestergaard!) ; I'm wrapping it up.

## What are the relevant issue numbers?

#18193 
Closes #967 

## Screenshots

![1](/uploads/c636e88ba38628211754e7cf122b0dc4/1.png)
![2](/uploads/5ed1e7917e2f36853a479faa565b022a/2.png)
![3](/uploads/0d202ba42e8dc6aade7bc6ac8db41ee6/3.png)

## TODO

- [ ]  #18193 !4892 Add "developers can merge" as an option for protected branches
    - [x]  Review existing code
    - [x]  Fix build
    - [x]  Implementation / refactoring
        - [x]  Clean up `GitAccess`
        - [x]  Clean up `protected_branches.js.coffee`
        - [x]  Figure out authorization issue
            - If we try to merge code into a protected branch for a user who doesn't have access to that branch, an auth check will fail
            - We need to get around this, somehow
            - [x]  Try detecting merge commits and allowing those
        - [x]  A push with regular commits _and_ merge commits should fail
            - [x]  Figure out a solution
            - [x]  Extensive tests for `MergeCommitCheck`
    - [x]  Add tests
        - [x]  Untested parts of original MR
    - [x]  Improve the checks in `/allowed`
        -  @dzaporozhets's proposal:
            - commits in push == commits in merge request
            - branch to push == target branch of merge request
            - merge request has required amount of approves (ee only)
            - merge commit in push == merge commit we created when merged via UI
            - save merge commit sha in database and compare with `newrev`
        - my proposal
            - /allowed finds all open merge requests with the appropriate target branch
            - For each MR, compare the commit SHAs in the MR to the commit SHAs in the change set
            - If we have a match, compare the diff of the matching MR to the diff of the change set
            - If we still have a match, the merge is legit
        - [x]  Wait for replies on my proposal
        - [x]  Pick a strategy
        - [x]  Implementation
            - [x]  Save `in_progress_merge_commit_sha`
            - [x]  Check `in_progress_merge_commit_sha`
            - [x]  Clear `in_progress_merge_commit_sha`
        - [x]  Test / refactor
    - [x]  Merge conflicts
    - [x]  Verify workflows
        - [x]  Developer with 'developer can merge' on:
            - [x]  Can merge an MR from the Web UI
            - [x]  Error message for conflicts in the Web UI
            - [x]  Cannot merge an MR from the command line (HTTP)
            - [x]  Cannot merge an MR from the command line (SSH)
            - [x]  Cannot modify the branch otherwise
        - [x]  Developer with 'developer can merge' off:
            - [x]  Cannot merge an MR from the Web UI
            - [x]  Error message for conflicts in the Web UI
            - [x]  Cannot merge an MR from the command line (HTTP)
            - [x]  Cannot merge an MR from the command line (SSH)
            - [x]  Cannot modify the branch otherwise
        - [x]  New projects created could have have "Developers can merge" turned on automatically for the default branch
    - [x]  CHANGELOG
    - [x]  Fix build
    - [x]  Wait for [build](https://gitlab.com/gitlab-org/gitlab-ce/commit/42624e3d53754064186d4ae9048e310d1d3eed0b/builds) to pass
    - [x]  Screenshots
    - [x]  Assign to endboss
    - [x]  Respond to @dbalexandre's comments
        - [x]  Duplicated line, this is equals to line 26.
        - [x]  We aren't using any of these helpers in this migration, we can remove the include.
        - [x]  What do you think to add a default value for this column to avoid the Three-state Boolean Problem?
        - [x]  group all checks under Gitlab::Checks
        - [x]  You have a default value for developers_can_merge column, but your migration doesn't add it.
        - [x]  What do you think to rename Partially protected to anything else?
    - [x]  Fix conflicts
    - [x]  Make sure [build](https://gitlab.com/gitlab-org/gitlab-ce/commit/b1cfd42f20a78fd7f844288954e97cff32962e20/builds) passes
    - [ ]  Wait for merge

See merge request !4892

CHANGELOG

@@ -43,6 +43,7 @@ v 8.10.0 (unreleased)
   - Add "Enabled Git access protocols" to Application Settings
   - Diffs will create button/diff form on demand no on server side
   - Reduce size of HTML used by diff comment forms
+  - Protected branches have a "Developers can Merge" setting. !4892 (original implementation by Mathias Vestergaard)
   - Fix user creation with stronger minimum password requirements !4054 (nathan-pmt)
   - Only show New Snippet button to users that can create snippets.
   - PipelinesFinder uses git cache data

app/assets/javascripts/protected_branches.js.coffee

 $ ->
   $(".protected-branches-list :checkbox").change (e) ->
     name = $(this).attr("name")
-    if name == "developers_can_push"
+    if name == "developers_can_push" || name == "developers_can_merge"
       id = $(this).val()
-      checked = $(this).is(":checked")
+      can_push = $(this).is(":checked")
       url = $(this).data("url")
       $.ajax
-        type: "PUT"
+        type: "PATCH"
         url: url
         dataType: "json"
         data:
           id: id
           protected_branch:
-            developers_can_push: checked
+            "#{name}": can_push
         success: ->
           row = $(e.target)

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

app/helpers/branches_helper.rb

@@ -12,7 +12,7 @@ module BranchesHelper
   def can_push_branch?(project, branch_name)
     return false unless project.repository.branch_exists?(branch_name)
-    ::Gitlab::GitAccess.new(current_user, project, 'web').can_push_to_branch?(branch_name)
+    ::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(branch_name)
   end
   def project_branches

app/models/merge_request.rb

@@ -552,7 +552,13 @@ 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::UserAccess.new(user, project: project)
+    access.can_push_to_branch?(target_branch) || access.can_merge_to_branch?(target_branch)
+  end
+
+  def can_be_merged_via_command_line_by?(user)
+    access = ::Gitlab::UserAccess.new(user, project: project)
+    access.can_push_to_branch?(target_branch)
   end
   def mergeable_ci_state?

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

app/models/repository.rb

@@ -769,9 +769,9 @@ class Repository
     end
   end
-  def merge(user, source_sha, target_branch, options = {})
-    our_commit = rugged.branches[target_branch].target
-    their_commit = rugged.lookup(source_sha)
+  def merge(user, merge_request, options = {})
+    our_commit = rugged.branches[merge_request.target_branch].target
+    their_commit = rugged.lookup(merge_request.diff_head_sha)
     raise "Invalid merge target" if our_commit.nil?
     raise "Invalid merge source" if their_commit.nil?
@@ -779,14 +779,16 @@ class Repository
     merge_index = rugged.merge_commits(our_commit, their_commit)
     return false if merge_index.conflicts?
-    commit_with_hooks(user, target_branch) do |ref|
+    commit_with_hooks(user, merge_request.target_branch) do |tmp_ref|
       actual_options = options.merge(
         parents: [our_commit, their_commit],
         tree: merge_index.write_tree(rugged),
-        update_ref: ref
+        update_ref: tmp_ref
       )
-      Rugged::Commit.create(rugged, actual_options)
+      commit_id = Rugged::Commit.create(rugged, actual_options)
+      merge_request.update(in_progress_merge_commit_sha: commit_id)
+      commit_id
     end
   end

app/services/commits/change_service.rb

@@ -23,7 +23,7 @@ module Commits
     private
     def check_push_permissions
-      allowed = ::Gitlab::GitAccess.new(current_user, project, 'web').can_push_to_branch?(@target_branch)
+      allowed = ::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(@target_branch)
       unless allowed
         raise ValidationError.new('You are not allowed to push into this branch')
@@ -31,7 +31,7 @@ module Commits
       true
     end
-    
+
     def create_target_branch(new_branch)
       # Temporary branch exists and contains the change commit
       return success if repository.find_branch(new_branch)

app/services/files/base_service.rb

@@ -42,7 +42,7 @@ module Files
     end
     def validate
-      allowed = ::Gitlab::GitAccess.new(current_user, project, 'web').can_push_to_branch?(@target_branch)
+      allowed = ::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(@target_branch)
       unless allowed
         raise_error("You are not allowed to push into this branch")

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

app/services/merge_requests/merge_service.rb

@@ -34,7 +34,7 @@ module MergeRequests
         committer: committer
       }
-      commit_id = repository.merge(current_user, merge_request.diff_head_sha, merge_request.target_branch, options)
+      commit_id = repository.merge(current_user, merge_request, options)
       merge_request.update(merge_commit_sha: commit_id)
     rescue GitHooksService::PreReceiveError => e
       merge_request.update(merge_error: e.message)
@@ -43,6 +43,8 @@ module MergeRequests
       merge_request.update(merge_error: "Something went wrong during merge")
       Rails.logger.error(e.message)
       false
+    ensure
+      merge_request.update(in_progress_merge_commit_sha: nil)
     end
     def after_merge

app/services/merge_requests/refresh_service.rb

@@ -48,7 +48,7 @@ module MergeRequests
     end
     def force_push?
-      Gitlab::ForcePushCheck.force_push?(@project, @oldrev, @newrev)
+      Gitlab::Checks::ForcePush.force_push?(@project, @oldrev, @newrev)
     end
     # Refresh merge request diff if we push to source or target branch of merge request

app/views/projects/merge_requests/widget/open/_conflicts.html.haml

@@ -4,7 +4,7 @@
 %p
   Please resolve these conflicts or
-  - if @merge_request.can_be_merged_by?(current_user)
+  - if @merge_request.can_be_merged_via_command_line_by?(current_user)
     #{link_to "merge this request manually", "#modal_merge_info", class: "how_to_merge_link vlink", "data-toggle" => "modal"}.
   - else
     ask someone with write access to this repository to merge this request manually.

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

@@ -8,6 +8,7 @@
   .table-responsive
     %table.table.protected-branches-list
       %colgroup
+        %col{ width: "20%" }
         %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

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"

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"

db/migrate/20160519203051_add_developers_can_merge_to_protected_branches.rb

+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

db/migrate/20160629025435_add_column_in_progress_merge_commit_sha_to_merge_requests.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddColumnInProgressMergeCommitShaToMergeRequests < ActiveRecord::Migration
+  def change
+    add_column :merge_requests, :in_progress_merge_commit_sha, :string
+  end
+end

db/schema.rb

@@ -626,6 +626,7 @@ ActiveRecord::Schema.define(version: 20160712171823) do
     t.string   "merge_commit_sha"
     t.datetime "deleted_at"
     t.integer  "lock_version",              default: 0,     null: false
+    t.string   "in_progress_merge_commit_sha"
   end
   add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree
@@ -860,11 +861,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

lib/api/helpers.rb

@@ -17,7 +17,7 @@ module API
     def current_user
       @current_user ||= (find_user_by_private_token || doorkeeper_guard)
-      unless @current_user && Gitlab::UserAccess.allowed?(@current_user)
+      unless @current_user && Gitlab::UserAccess.new(@current_user).allowed?
         return nil
       end