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

9 files changed, 213 insertions, 136 deletions

diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 77e407b54c5..73557cf7db6 100644
--- a/lib/api/helpers.rb
+++ b/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
 
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/checks/change_access.rb b/lib/gitlab/checks/change_access.rb
new file mode 100644
index 00000000000..5551fac4b8b
--- /dev/null
+++ b/lib/gitlab/checks/change_access.rb
@@ -0,0 +1,96 @@
+module Gitlab
+  module Checks
+    class ChangeAccess
+      attr_reader :user_access, :project
+
+      def initialize(change, user_access:, project:)
+        @oldrev, @newrev, @ref = change.split(' ')
+        @branch_name = branch_name(@ref)
+        @user_access = user_access
+        @project = project
+      end
+
+      def exec
+        error = protected_branch_checks || tag_checks || push_checks
+
+        if error
+          GitAccessStatus.new(false, error)
+        else
+          GitAccessStatus.new(true)
+        end
+      end
+
+      protected
+
+      def protected_branch_checks
+        return unless project.protected_branch?(@branch_name)
+
+        if forced_push? && user_access.cannot_do_action?(:force_push_code_to_protected_branches)
+          return "You are not allowed to force push code to a protected branch on this project."
+        elsif Gitlab::Git.blank_ref?(@newrev) && user_access.cannot_do_action?(:remove_protected_branches)
+          return "You are not allowed to delete protected branches from this project."
+        end
+
+        if matching_merge_request?
+          if user_access.can_merge_to_branch?(@branch_name) || user_access.can_push_to_branch?(@branch_name)
+            return
+          else
+            "You are not allowed to merge code into protected branches on this project."
+          end
+        else
+          if user_access.can_push_to_branch?(@branch_name)
+            return
+          else
+            "You are not allowed to push code to protected branches on this project."
+          end
+        end
+      end
+
+      def tag_checks
+        tag_ref = tag_name(@ref)
+
+        if tag_ref && protected_tag?(tag_ref) && user_access.cannot_do_action?(:admin_project)
+          "You are not allowed to change existing tags on this project."
+        end
+      end
+
+      def push_checks
+        if user_access.cannot_do_action?(:push_code)
+          "You are not allowed to push code to this project."
+        end
+      end
+
+      private
+
+      def protected_tag?(tag_name)
+        project.repository.tag_exists?(tag_name)
+      end
+
+      def forced_push?
+        Gitlab::Checks::ForcePush.force_push?(@project, @oldrev, @newrev)
+      end
+
+      def matching_merge_request?
+        Checks::MatchingMergeRequest.new(@newrev, @branch_name, @project).match?
+      end
+
+      def branch_name(ref)
+        ref = @ref.to_s
+        if Gitlab::Git.branch_ref?(ref)
+          Gitlab::Git.ref_name(ref)
+        else
+          nil
+        end
+      end
+
+      def tag_name(ref)
+        ref = @ref.to_s
+        if Gitlab::Git.tag_ref?(ref)
+          Gitlab::Git.ref_name(ref)
+        else
+          nil
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/checks/force_push.rb b/lib/gitlab/checks/force_push.rb
new file mode 100644
index 00000000000..dfa83a0eab3
--- /dev/null
+++ b/lib/gitlab/checks/force_push.rb
@@ -0,0 +1,17 @@
+module Gitlab
+  module Checks
+    class ForcePush
+      def self.force_push?(project, oldrev, newrev)
+        return false if project.empty_repo?
+
+        # Created or deleted branch
+        if Gitlab::Git.blank_ref?(oldrev) || Gitlab::Git.blank_ref?(newrev)
+          false
+        else
+          missed_refs, _ = Gitlab::Popen.popen(%W(#{Gitlab.config.git.bin_path} --git-dir=#{project.repository.path_to_repo} rev-list #{oldrev} ^#{newrev}))
+          missed_refs.split("\n").size > 0
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/checks/matching_merge_request.rb b/lib/gitlab/checks/matching_merge_request.rb
new file mode 100644
index 00000000000..849848515da
--- /dev/null
+++ b/lib/gitlab/checks/matching_merge_request.rb
@@ -0,0 +1,18 @@
+module Gitlab
+  module Checks
+    class MatchingMergeRequest
+      def initialize(newrev, branch_name, project)
+        @newrev = newrev
+        @branch_name = branch_name
+        @project = project
+      end
+
+      def match?
+        @project.merge_requests
+          .with_state(:locked)
+          .where(in_progress_merge_commit_sha: @newrev, target_branch: @branch_name)
+          .exists?
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/force_push_check.rb b/lib/gitlab/force_push_check.rb
deleted file mode 100644
index 93c6a5bb7f5..00000000000
--- a/lib/gitlab/force_push_check.rb
+++ /dev/null
@@ -1,15 +0,0 @@
-module Gitlab
-  class ForcePushCheck
-    def self.force_push?(project, oldrev, newrev)
-      return false if project.empty_repo?
-
-      # Created or deleted branch
-      if Gitlab::Git.blank_ref?(oldrev) || Gitlab::Git.blank_ref?(newrev)
-        false
-      else
-        missed_refs, _ = Gitlab::Popen.popen(%W(#{Gitlab.config.git.bin_path} --git-dir=#{project.repository.path_to_repo} rev-list #{oldrev} ^#{newrev}))
-        missed_refs.split("\n").size > 0
-      end
-    end
-  end
-end
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index 7679c7e4bb8..308f23bc9bc 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -1,52 +1,17 @@
+# Check a user's access to perform a git action. All public methods in this
+# class return an instance of `GitlabAccessStatus`
 module Gitlab
   class GitAccess
     DOWNLOAD_COMMANDS = %w{ git-upload-pack git-upload-archive }
     PUSH_COMMANDS = %w{ git-receive-pack }
 
-    attr_reader :actor, :project, :protocol
+    attr_reader :actor, :project, :protocol, :user_access
 
     def initialize(actor, project, protocol)
       @actor    = actor
       @project  = project
       @protocol = protocol
-    end
-
-    def user
-      return @user if defined?(@user)
-
-      @user =
-        case actor
-        when User
-          actor
-        when DeployKey
-          nil
-        when Key
-          actor.user
-        end
-    end
-
-    def deploy_key
-      actor if actor.is_a?(DeployKey)
-    end
-
-    def can_push_to_branch?(ref)
-      return false unless user
-
-      if project.protected_branch?(ref) && !project.developers_can_push_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)
-      elsif deploy_key
-        deploy_key.projects.include?(project)
-      else
-        false
-      end
+      @user_access = UserAccess.new(user, project: project)
     end
 
     def check(cmd, changes = nil)
@@ -56,11 +21,11 @@ module Gitlab
         return build_status_object(false, "No user or key was provided.")
       end
 
-      if user && !user_allowed?
+      if user && !user_access.allowed?
         return build_status_object(false, "Your account has been blocked.")
       end
 
-      unless project && can_read_project?
+      unless project && (user_access.can_read_project? || deploy_key_can_read_project?)
         return build_status_object(false, 'The project you were looking for could not be found.')
       end
 
@@ -95,7 +60,7 @@ module Gitlab
     end
 
     def user_download_access_check
-      unless user.can?(:download_code, project)
+      unless user_access.can_do_action?(:download_code)
         return build_status_object(false, "You are not allowed to download code from this project.")
       end
 
@@ -125,46 +90,8 @@ module Gitlab
       build_status_object(true)
     end
 
-    def can_user_do_action?(action)
-      @permission_cache ||= {}
-      @permission_cache[action] ||= user.can?(action, project)
-    end
-
     def change_access_check(change)
-      oldrev, newrev, ref = change.split(' ')
-
-      action =
-        if project.protected_branch?(branch_name(ref))
-          protected_branch_action(oldrev, newrev, branch_name(ref))
-        elsif (tag_ref = tag_name(ref)) && protected_tag?(tag_ref)
-          # Prevent any changes to existing git tag unless user has permissions
-          :admin_project
-        else
-          :push_code
-        end
-
-      unless can_user_do_action?(action)
-        status =
-          case action
-          when :force_push_code_to_protected_branches
-            build_status_object(false, "You are not allowed to force push code to a protected branch on this project.")
-          when :remove_protected_branches
-            build_status_object(false, "You are not allowed to deleted protected branches from this project.")
-          when :push_code_to_protected_branches
-            build_status_object(false, "You are not allowed to push code to protected branches on this project.")
-          when :admin_project
-            build_status_object(false, "You are not allowed to change existing tags on this project.")
-          else # :push_code
-            build_status_object(false, "You are not allowed to push code to this project.")
-          end
-        return status
-      end
-
-      build_status_object(true)
-    end
-
-    def forced_push?(oldrev, newrev)
-      Gitlab::ForcePushCheck.force_push?(project, oldrev, newrev)
+      Checks::ChangeAccess.new(change, user_access: user_access, project: project).exec
     end
 
     def protocol_allowed?
@@ -173,48 +100,38 @@ module Gitlab
 
     private
 
-    def protected_branch_action(oldrev, newrev, branch_name)
-      # we dont allow force push to protected branch
-      if forced_push?(oldrev, newrev)
-        :force_push_code_to_protected_branches
-      elsif Gitlab::Git.blank_ref?(newrev)
-        # and we dont allow remove of protected branch
-        :remove_protected_branches
-      elsif project.developers_can_push_to_protected_branch?(branch_name)
-        :push_code
-      else
-        :push_code_to_protected_branches
-      end
+    def matching_merge_request?(newrev, branch_name)
+      Checks::MatchingMergeRequest.new(newrev, branch_name, project).match?
     end
 
-    def protected_tag?(tag_name)
-      project.repository.tag_exists?(tag_name)
-    end
-
-    def user_allowed?
-      Gitlab::UserAccess.allowed?(user)
-    end
-
-    def branch_name(ref)
-      ref = ref.to_s
-      if Gitlab::Git.branch_ref?(ref)
-        Gitlab::Git.ref_name(ref)
-      else
-        nil
-      end
+    def deploy_key
+      actor if actor.is_a?(DeployKey)
     end
 
-    def tag_name(ref)
-      ref = ref.to_s
-      if Gitlab::Git.tag_ref?(ref)
-        Gitlab::Git.ref_name(ref)
+    def deploy_key_can_read_project?
+      if deploy_key
+        deploy_key.projects.include?(project)
       else
-        nil
+        false
       end
     end
 
     protected
 
+    def user
+      return @user if defined?(@user)
+
+      @user =
+        case actor
+        when User
+          actor
+        when DeployKey
+          nil
+        when Key
+          actor.user
+        end
+    end
+
     def build_status_object(status, message = '')
       GitAccessStatus.new(status, message)
     end
diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb
index 8672cbc0ec4..f71d3575909 100644
--- a/lib/gitlab/git_access_wiki.rb
+++ b/lib/gitlab/git_access_wiki.rb
@@ -1,7 +1,7 @@
 module Gitlab
   class GitAccessWiki < GitAccess
     def change_access_check(change)
-      if user.can?(:create_wiki, project)
+      if user_access.can_do_action?(:create_wiki)
         build_status_object(true)
       else
         build_status_object(false, "You are not allowed to write to this project's wiki.")
diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb
index d1b42c1f9b9..c0f85e9b3a8 100644
--- a/lib/gitlab/user_access.rb
+++ b/lib/gitlab/user_access.rb
@@ -1,7 +1,23 @@
 module Gitlab
-  module UserAccess
-    def self.allowed?(user)
-      return false if user.blocked?
+  class UserAccess
+    attr_reader :user, :project
+
+    def initialize(user, project: nil)
+      @user = user
+      @project = project
+    end
+
+    def can_do_action?(action)
+      @permission_cache ||= {}
+      @permission_cache[action] ||= user.can?(action, project)
+    end
+
+    def cannot_do_action?(action)
+      !can_do_action?(action)
+    end
+
+    def allowed?
+      return false if user.blank? || user.blocked?
 
       if user.requires_ldap_check? && user.try_obtain_ldap_lease
         return false unless Gitlab::LDAP::Access.allowed?(user)
@@ -9,5 +25,31 @@ module Gitlab
 
       true
     end
+
+    def can_push_to_branch?(ref)
+      return false unless user
+
+      if project.protected_branch?(ref) && !project.developers_can_push_to_protected_branch?(ref)
+        user.can?(:push_code_to_protected_branches, project)
+      else
+        user.can?(:push_code, project)
+      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?
+      return false unless user
+
+      user.can?(:read_project, project)
+    end
   end
 end