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

7 files changed, 224 insertions, 144 deletions

diff --git a/spec/lib/gitlab/diff/position_tracer_spec.rb b/spec/lib/gitlab/diff/position_tracer_spec.rb
index 08312e60f4a..c268f84c759 100644
--- a/spec/lib/gitlab/diff/position_tracer_spec.rb
+++ b/spec/lib/gitlab/diff/position_tracer_spec.rb
@@ -1639,7 +1639,8 @@ describe Gitlab::Diff::PositionTracer, lib: true do
           committer: committer
         }
 
-        repository.merge(current_user, second_create_file_commit.sha, branch_name, options)
+        merge_request = create(:merge_request, source_branch: second_create_file_commit.sha, target_branch: branch_name, source_project: project)
+        repository.merge(current_user, merge_request, options)
         project.commit(branch_name)
       end
 
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index c79ba11f782..db33c7a22bb 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -6,67 +6,6 @@ describe Gitlab::GitAccess, lib: true do
   let(:user) { create(:user) }
   let(:actor) { user }
 
-  describe 'can_push_to_branch?' do
-    describe 'push to none protected branch' do
-      it "returns true if user is a master" do
-        project.team << [user, :master]
-        expect(access.can_push_to_branch?("random_branch")).to be_truthy
-      end
-
-      it "returns true if user is a developer" do
-        project.team << [user, :developer]
-        expect(access.can_push_to_branch?("random_branch")).to be_truthy
-      end
-
-      it "returns false if user is a reporter" do
-        project.team << [user, :reporter]
-        expect(access.can_push_to_branch?("random_branch")).to be_falsey
-      end
-    end
-
-    describe 'push to protected branch' do
-      before do
-        @branch = create :protected_branch, project: project
-      end
-      
-      it "returns true if user is a master" do
-        project.team << [user, :master]
-        expect(access.can_push_to_branch?(@branch.name)).to be_truthy
-      end
-
-      it "returns false if user is a developer" do
-        project.team << [user, :developer]
-        expect(access.can_push_to_branch?(@branch.name)).to be_falsey
-      end
-
-      it "returns false if user is a reporter" do
-        project.team << [user, :reporter]
-        expect(access.can_push_to_branch?(@branch.name)).to be_falsey
-      end
-    end
-
-    describe 'push to protected branch if allowed for developers' do
-      before do
-        @branch = create :protected_branch, project: project, developers_can_push: true
-      end
-      
-      it "returns true if user is a master" do
-        project.team << [user, :master]
-        expect(access.can_push_to_branch?(@branch.name)).to be_truthy
-      end
-
-      it "returns true if user is a developer" do
-        project.team << [user, :developer]
-        expect(access.can_push_to_branch?(@branch.name)).to be_truthy
-      end
-
-      it "returns false if user is a reporter" do
-        project.team << [user, :reporter]
-        expect(access.can_push_to_branch?(@branch.name)).to be_falsey
-      end
-    end
-  end
-
   describe '#check with single protocols allowed' do
     def disable_protocol(protocol)
       settings = ::ApplicationSetting.create_from_defaults
@@ -160,96 +99,46 @@ describe Gitlab::GitAccess, lib: true do
   end
 
   describe 'push_access_check' do
-    def protect_feature_branch
-      create(:protected_branch, name: 'feature', project: project)
-    end
+    before { merge_into_protected_branch }
+    let(:unprotected_branch) { FFaker::Internet.user_name }
 
-    def changes
-      {
-        push_new_branch: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow",
+    let(:changes) do
+      { push_new_branch: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow",
         push_master: '6f6d7e7ed 570e7b2ab refs/heads/master',
         push_protected_branch: '6f6d7e7ed 570e7b2ab refs/heads/feature',
         push_remove_protected_branch: "570e7b2ab #{Gitlab::Git::BLANK_SHA} "\
                                       'refs/heads/feature',
         push_tag: '6f6d7e7ed 570e7b2ab refs/tags/v1.0.0',
         push_new_tag: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/tags/v7.8.9",
-        push_all: ['6f6d7e7ed 570e7b2ab refs/heads/master', '6f6d7e7ed 570e7b2ab refs/heads/feature']
-      }
+        push_all: ['6f6d7e7ed 570e7b2ab refs/heads/master', '6f6d7e7ed 570e7b2ab refs/heads/feature'],
+        merge_into_protected_branch: "0b4bc9a #{merge_into_protected_branch} refs/heads/feature" }
     end
 
-    def self.permissions_matrix
-      {
-        master: {
-          push_new_branch: true,
-          push_master: true,
-          push_protected_branch: true,
-          push_remove_protected_branch: false,
-          push_tag: true,
-          push_new_tag: true,
-          push_all: true,
-        },
-
-        developer: {
-          push_new_branch: true,
-          push_master: true,
-          push_protected_branch: false,
-          push_remove_protected_branch: false,
-          push_tag: false,
-          push_new_tag: true,
-          push_all: false,
-        },
-
-        reporter: {
-          push_new_branch: false,
-          push_master: false,
-          push_protected_branch: false,
-          push_remove_protected_branch: false,
-          push_tag: false,
-          push_new_tag: false,
-          push_all: false,
-        },
-
-        guest: {
-          push_new_branch: false,
-          push_master: false,
-          push_protected_branch: false,
-          push_remove_protected_branch: false,
-          push_tag: false,
-          push_new_tag: false,
-          push_all: false,
-        }
-      }
-    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
+    def stub_git_hooks
+      # Running the `pre-receive` hook is expensive, and not necessary for this test.
+      allow_any_instance_of(GitHooksService).to receive(:execute).and_yield
     end
 
-    permissions_matrix.keys.each do |role|
-      describe "#{role} access" do
-        before { protect_feature_branch }
-        before { project.team << [user, role] }
-
-        permissions_matrix[role].each do |action, allowed|
-          context action do
-            subject { access.push_access_check(changes[action]) }
+    def merge_into_protected_branch
+      @protected_branch_merge_commit ||= begin
+        stub_git_hooks
+        project.repository.add_branch(user, unprotected_branch, 'feature')
+        target_branch = project.repository.lookup('feature')
+        source_branch = project.repository.commit_file(user, FFaker::InternetSE.login_user_name, FFaker::HipsterIpsum.paragraph, FFaker::HipsterIpsum.sentence, unprotected_branch, false)
+        rugged = project.repository.rugged
+        author = { email: "email@example.com", time: Time.now, name: "Example Git User" }
 
-            it { expect(subject.allowed?).to allowed ? be_truthy : be_falsey }
-          end
-        end
+        merge_index = rugged.merge_commits(target_branch, source_branch)
+        Rugged::Commit.create(rugged, author: author, committer: author, message: "commit message", parents: [target_branch, source_branch], tree: merge_index.write_tree(rugged))
       end
     end
 
-    context "with enabled developers push to protected branches " do
-      updated_permissions_matrix.keys.each do |role|
+    def self.run_permission_checks(permissions_matrix)
+      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|
+          permissions_matrix[role].each do |action, allowed|
             context action do
               subject { access.push_access_check(changes[action]) }
 
@@ -259,5 +148,97 @@ describe Gitlab::GitAccess, lib: true do
         end
       end
     end
+
+    permissions_matrix = {
+      master: {
+        push_new_branch: true,
+        push_master: true,
+        push_protected_branch: true,
+        push_remove_protected_branch: false,
+        push_tag: true,
+        push_new_tag: true,
+        push_all: true,
+        merge_into_protected_branch: true
+      },
+
+      developer: {
+        push_new_branch: true,
+        push_master: true,
+        push_protected_branch: false,
+        push_remove_protected_branch: false,
+        push_tag: false,
+        push_new_tag: true,
+        push_all: false,
+        merge_into_protected_branch: false
+      },
+
+      reporter: {
+        push_new_branch: false,
+        push_master: false,
+        push_protected_branch: false,
+        push_remove_protected_branch: false,
+        push_tag: false,
+        push_new_tag: false,
+        push_all: false,
+        merge_into_protected_branch: false
+      },
+
+      guest: {
+        push_new_branch: false,
+        push_master: false,
+        push_protected_branch: false,
+        push_remove_protected_branch: false,
+        push_tag: false,
+        push_new_tag: false,
+        push_all: false,
+        merge_into_protected_branch: false
+      }
+    }
+
+    [['feature', 'exact'], ['feat*', 'wildcard']].each do |protected_branch_name, protected_branch_type|
+      context do
+        before { create(:protected_branch, name: protected_branch_name, project: project) }
+
+        run_permission_checks(permissions_matrix)
+      end
+
+      context "when 'developers can push' is turned on for the #{protected_branch_type} protected branch" do
+        before { create(:protected_branch, name: protected_branch_name, developers_can_push: true, project: project) }
+
+        run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true }))
+      end
+
+      context "when 'developers can merge' is turned on for the #{protected_branch_type} protected branch" do
+        before { create(:protected_branch, name: protected_branch_name, developers_can_merge: true, project: project) }
+
+        context "when a merge request exists for the given source/target branch" do
+          context "when the merge request is in progress" do
+            before do
+              create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', state: 'locked', in_progress_merge_commit_sha: merge_into_protected_branch)
+            end
+
+            run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: true }))
+          end
+
+          context "when the merge request is not in progress" do
+            before do
+              create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', in_progress_merge_commit_sha: nil)
+            end
+
+            run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false }))
+          end
+        end
+
+        context "when a merge request does not exist for the given source/target branch" do
+          run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false }))
+        end
+      end
+
+      context "when 'developers can merge' and 'developers can push' are turned on for the #{protected_branch_type} protected branch" do
+        before { create(:protected_branch, name: protected_branch_name, developers_can_merge: true, developers_can_push: true, project: project) }
+
+        run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true }))
+      end
+    end
   end
 end
diff --git a/spec/lib/gitlab/user_access_spec.rb b/spec/lib/gitlab/user_access_spec.rb
new file mode 100644
index 00000000000..aa9ec243498
--- /dev/null
+++ b/spec/lib/gitlab/user_access_spec.rb
@@ -0,0 +1,88 @@
+require 'spec_helper'
+
+describe Gitlab::UserAccess, lib: true do
+  let(:access) { Gitlab::UserAccess.new(user, project: project) }
+  let(:project) { create(:project) }
+  let(:user) { create(:user) }
+
+  describe 'can_push_to_branch?' do
+    describe 'push to none protected branch' do
+      it 'returns true if user is a master' do
+        project.team << [user, :master]
+        expect(access.can_push_to_branch?('random_branch')).to be_truthy
+      end
+
+      it 'returns true if user is a developer' do
+        project.team << [user, :developer]
+        expect(access.can_push_to_branch?('random_branch')).to be_truthy
+      end
+
+      it 'returns false if user is a reporter' do
+        project.team << [user, :reporter]
+        expect(access.can_push_to_branch?('random_branch')).to be_falsey
+      end
+    end
+
+    describe 'push to protected branch' do
+      let(:branch) { create :protected_branch, project: project }
+
+      it 'returns true if user is a master' do
+        project.team << [user, :master]
+        expect(access.can_push_to_branch?(branch.name)).to be_truthy
+      end
+
+      it 'returns false if user is a developer' do
+        project.team << [user, :developer]
+        expect(access.can_push_to_branch?(branch.name)).to be_falsey
+      end
+
+      it 'returns false if user is a reporter' do
+        project.team << [user, :reporter]
+        expect(access.can_push_to_branch?(branch.name)).to be_falsey
+      end
+    end
+
+    describe 'push to protected branch if allowed for developers' do
+      before do
+        @branch = create :protected_branch, project: project, developers_can_push: true
+      end
+
+      it 'returns true if user is a master' do
+        project.team << [user, :master]
+        expect(access.can_push_to_branch?(@branch.name)).to be_truthy
+      end
+
+      it 'returns true if user is a developer' do
+        project.team << [user, :developer]
+        expect(access.can_push_to_branch?(@branch.name)).to be_truthy
+      end
+
+      it 'returns false if user is a reporter' do
+        project.team << [user, :reporter]
+        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
+end
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index b39b958450c..e14cec589fe 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -4,16 +4,17 @@ describe Repository, models: true do
   include RepoHelpers
   TestBlob = Struct.new(:name)
 
-  let(:repository) { create(:project).repository }
+  let(:project) { create(:project) }
+  let(:repository) { project.repository }
   let(:user) { create(:user) }
   let(:commit_options) do
     author = repository.user_to_committer(user)
     { message: 'Test message', committer: author, author: author }
   end
   let(:merge_commit) do
-    source_sha = repository.find_branch('feature').target
-    merge_commit_sha = repository.merge(user, source_sha, 'master', commit_options)
-    repository.commit(merge_commit_sha)
+    merge_request = create(:merge_request, source_branch: 'feature', target_branch: 'master', source_project: project)
+    merge_commit_id = repository.merge(user, merge_request, commit_options)
+    repository.commit(merge_commit_id)
   end
 
   describe '#branch_names_contains' do
diff --git a/spec/requests/api/api_helpers_spec.rb b/spec/requests/api/api_helpers_spec.rb
index 83025953889..3d5c19aeff3 100644
--- a/spec/requests/api/api_helpers_spec.rb
+++ b/spec/requests/api/api_helpers_spec.rb
@@ -49,7 +49,7 @@ describe API::Helpers, api: true do
 
       it "should return nil for a user without access" do
         env[API::Helpers::PRIVATE_TOKEN_HEADER] = user.private_token
-        allow(Gitlab::UserAccess).to receive(:allowed?).and_return(false)
+        allow_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(false)
         expect(current_user).to be_nil
       end
 
@@ -73,7 +73,7 @@ describe API::Helpers, api: true do
 
       it "should return nil for a user without access" do
         env[API::Helpers::PRIVATE_TOKEN_HEADER] = personal_access_token.token
-        allow(Gitlab::UserAccess).to receive(:allowed?).and_return(false)
+        allow_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(false)
         expect(current_user).to be_nil
       end
 
diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb
index afabeed4a80..47c0580e0f0 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,17 @@ 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
 
diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb
index 06f56d85aa8..ce643b3f860 100644
--- a/spec/services/merge_requests/refresh_service_spec.rb
+++ b/spec/services/merge_requests/refresh_service_spec.rb
@@ -88,8 +88,7 @@ describe MergeRequests::RefreshService, services: true do
         # Merge master -> feature branch
         author = { email: 'test@gitlab.com', time: Time.now, name: "Me" }
         commit_options = { message: 'Test message', committer: author, author: author }
-        master_commit = @project.repository.commit('master')
-        @project.repository.merge(@user, master_commit.id, 'feature', commit_options)
+        @project.repository.merge(@user, @merge_request, commit_options)
         commit = @project.repository.commit('feature')
         service.new(@project, @user).execute(@oldrev, commit.id, 'refs/heads/feature')
         reload_mrs