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 (closed) (thanks, @mvestergaard!) ; I'm wrapping it up.

What are the relevant issue numbers?

#18193 (closed) Closes #967 (closed)

Screenshots

TODO

• #18193 (closed) !4892 (merged) Add "developers can merge" as an option for protected branches
  • Review existing code
  • Fix build
  • Implementation / refactoring
    • Clean up GitAccess
    • Clean up protected_branches.js.coffee
    • 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
      • Try detecting merge commits and allowing those
    • A push with regular commits and merge commits should fail
      • Figure out a solution
      • Extensive tests for MergeCommitCheck
  • Add tests
    • Untested parts of original MR
  • 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
    • Wait for replies on my proposal
    • Pick a strategy
    • Implementation
      • Save in_progress_merge_commit_sha
      • Check in_progress_merge_commit_sha
      • Clear in_progress_merge_commit_sha
    • Test / refactor
  • Merge conflicts
  • Verify workflows
    • Developer with 'developer can merge' on:
      • Can merge an MR from the Web UI
      • Error message for conflicts in the Web UI
      • Cannot merge an MR from the command line (HTTP)
      • Cannot merge an MR from the command line (SSH)
      • Cannot modify the branch otherwise
    • Developer with 'developer can merge' off:
      • Cannot merge an MR from the Web UI
      • Error message for conflicts in the Web UI
      • Cannot merge an MR from the command line (HTTP)
      • Cannot merge an MR from the command line (SSH)
      • Cannot modify the branch otherwise
    • New projects created could have have "Developers can merge" turned on automatically for the default branch
  • CHANGELOG
  • Fix build
  • Wait for build to pass
  • Screenshots
  • Assign to endboss
  • Respond to @dbalexandre's comments
    • Duplicated line, this is equals to line 26.
    • We aren't using any of these helpers in this migration, we can remove the include.
    • What do you think to add a default value for this column to avoid the Three-state Boolean Problem?
    • group all checks under Gitlab::Checks
    • You have a default value for developers_can_merge column, but your migration doesn't add it.
    • What do you think to rename Partially protected to anything else?
  • Fix conflicts
  • Make sure build passes
  • Wait for merge

mentioned in merge request !4220 (closed)

Added 3 commits:

• b7cac23e - Added "developers can merge" setting to protected branches
• 30020da1 - Enforce "developers can merge" during pre-receive
• a79f5e16 - Refactor Gitlab::GitAccess

Added 1 commit:

• 33613c5d - Clean up protected_branches.js.coffee

Marked the task Clean up protected_branches.js.coffee as completed

Marked the task Figure out authorization issue as completed

Marked the task Try detecting merge commits and allowing those as completed

Marked the task Implementation / refactoring as completed

Marked the task Review existing code as completed

Added 2 commits:

• 45e30931 - Refactor Gitlab::GitAccess
• db227796 - Clean up protected_branches.js.coffee

Added 2 commits:

• ce8b6651 - Refactor Gitlab::GitAccess
• 4dde25de - Clean up protected_branches.js.coffee

@timothyandrew, I'm a bit bothered by the "Can merge an MR from the command line" part on your implementation plan.

For us, at least, it's very important that what's merged is exactly what has been actually reviewed in the MR, that's the whole point of disallowing pushing, but allowing merging MRs (not merges in general!). If one only allows merging through the web interface, the problem is moot, but if one also allows merging MRs from the command line, one has to be very careful.

On the command line, you can merge in myriad of different ways, resolve conflicts and/or simply pack new code into the branch, so the fact that the last commit is a merge commit doesn't really tell you anything at all. If you only check this (I might have misunderstood your code, though!), then you're basically opening door to merging anything, whether this has to do with any MR or not. I think this is no good at all, because it makes it possible to circumvent the whole process, intentionally or not. One could just as well leave the push access enabled then.

My understanding is that the safe way of allowing command-line merges would be to check that:

1. The merge only contains exactly the commits that were in the latest version of some open MR
2. The last commit on top of this stack is the merge commit
3. This merge commit doesn't resolve any conflicts (otherwise, as I said, one can introduce arbitrary unreviewed code)

Only then you can be reasonably safe about command-line merging.

Is it really worth it, I don't know, so I would say, just disallow merging on the command line, and if the MR can't be merged automatically without conflict, display a special message asking for the developer to merge/rebase and resolve conflicts so that MR can be automatically merged.

Your thoughts?

@zyv

The path I was planning to follow was something like: A user can push (from the command-line) into a protected branch, only if the push is entirely composed of merge commits.
Your concern is absolutely valid, and makes a lot of sense. If we allow any merge commit, that's akin to having push access, just more convoluted.

Unfortunately, it seems like restricting this to the web UI is not as straightforward as it may seem. All access controls are checked in the /api/v3/internal/allowed endpoint, which gitlab-shell calls. All entry points (HTTP, SSH, Web UI) call out to gitlab-shell. There's no way (as of now) for the API endpoint to check which entry point a request came from.

I think that the strategy in this MR is a good first step, and we can improve it in the way that you suggest after this goes in. Nothing from the MR will need to be removed when command-line support is turned off, so this seems like a logical first step. Once we have a way for /allowed to know where a request came from, it can pick a strategy based on the entry point. There is some related discussion around changing the way that access controls are handled for the web UI here, which is probably relevant: https://gitlab.com/gitlab-org/gitlab-ce/issues/18771#note_12597272.

One other potential solution (which I don't really like) is to turn off the /allowed check (edit: only) when a merge request is merged in. This feels somewhat hacky to me — I think the right way to do this is to have the logic in /allowed modified so it can detect (and allow) a merge to go through.

@DouweM, @JobV @stanhu @jacobvosmaer-gitlab: Do you have any thoughts on this?

Marked the task Implementation / refactoring as incomplete

Marked the task Implementation / refactoring as completed

Marked the task A push with regular commits and merge commits should fail as completed

Marked the task Figure out a solution as completed

Marked the task Extensive tests for MergeCommitCheck as completed