Merge branch '18193-no-one-can-push' into 'master'

Allow creating protected branches that can't be pushed to

## What does this MR do?

- Add "No one can push" as a setting to protected branches.
- This applies to Masters (as well as all other users)

## What are the relevant issue numbers?

Closes #18193 

## Does this need an EE merge request?

Yes. gitlab-org/gitlab-ee!569

## Screenshots

![Screen_Shot_2016-07-29_at_3.14.59_PM](/uploads/2e8774c311763bc6e570501a2e6cabf7/Screen_Shot_2016-07-29_at_3.14.59_PM.png)

## TODO

- [ ]  #18193 !5081 No one can push to protected branches
    - [x]  Implementation
        - [x]  Model changes
            - [x]  Remove "developers_can_merge" and "developers_can_push"
            - [x]  Replace with `ProtectedBranchPushAccess` and `ProtectedBranchMergeAccess`
                - [x]  Reversible migration
                - [x]  Raise error on failure
                - [x]  MySQL
        - [x]  Backend changes
            - [x]  Creating a protected branch creates access rows
            - [x]  Add `no_one` as an access level
            - [x]  Enforce "no one can push" 
            - [x]  Allow setting levels while creating protected branches?
        - [x]  Frontend
            - [x]  Replace checkboxes with `select`s
    - [x]  Add tests
    - [x]  `GitPushService` -> new projects' default branch protection
    - [x]  Fix existing tests
    - [x]  Refactor
    - [x]  Test workflows by hand
        - [x]  from the Web UI
            - [x]  When "Allowed to Push" is "No one"
                - [x]  Developers can't push
                - [x]  Masters can't push
            - [x]  When "Allowed to Push" is "Developers + Masters"
                - [x]  Developers can push
                - [x]  Masters can push
            - [x]  When "Allowed to Push" is "Masters"
                - [x]  Developers can't push
                - [x]  Masters can push
            - [x]  When "Allowed to Merge" is "Masters" and "Allowed to Push" is "Masters"
                - [x]  Developers can't push
                - [x]  Developers can't merge
                - [x]  Masters can merge
                - [x]  Masters can push
            - [x]  When "Allowed to Merge" is "Developers + Masters" and "Allowed to Push" is "Masters"
                - [x]  Developers can't push
                - [x]  Developers can merge
                - [x]  Masters can merge
                - [x]  Masters can push
            - [x]  When "Allowed to Merge" is "Developers + Masters" and "Allowed to Push" is "No one"
                - [x]  Developers can't push
                - [x]  Developers can merge
                - [x]  Masters can merge
                - [x]  Masters can't push
            - [x]  When "Allowed to Merge" is "Masters" and "Allowed to Push" is "No one"
                - [x]  Developers can't push
                - [x]  Developers can't merge
                - [x]  Masters can merge
                - [x]  Masters can't push
        - [x]  from CLI
            - [x]  When "Allowed to Push" is "No one"
                - [x]  Developers can't push
                - [x]  Masters can't push
            - [x]  When "Allowed to Push" is "Developers + Masters"
                - [x]  Developers can push
                - [x]  Masters can push
            - [x]  When "Allowed to Push" is "Masters"
                - [x]  Developers can't push
                - [x]  Masters can push
            - [x]  When "Allowed to Merge" is "Masters" and "Allowed to Push" is "Masters"
                - [x]  Developers can't push
                - [x]  Developers can't merge
                - [x]  Masters can merge
                - [x]  Masters can push
            - [x]  When "Allowed to Merge" is "Developers + Masters" and "Allowed to Push" is "Masters"
                - [x]  Developers can't push
                - [x]  Developers can't merge
                - [x]  Masters can merge
                - [x]  Masters can push
            - [x]  When "Allowed to Merge" is "Developers + Masters" and "Allowed to Push" is "No one"
                - [x]  Developers can't push
                - [x]  Developers can't merge
                - [x]  Masters can't merge
                - [x]  Masters can't push
            - [x]  When "Allowed to Merge" is "Masters" and "Allowed to Push" is "No one"
                - [x]  Developers can't push
                - [x]  Developers can't merge
                - [x]  Masters can't merge
                - [x]  Masters can't push
    - [x]  Add tests for owners and admins
    - [x]  CHANGELOG
    - [x]  Screenshots
    - [x]  Documentation
    - [x]  Wait for ~~!4665~~ to be merged in
    - [x]  Wait for ~~gitlab-org/gitlab-ce#19872~~ and ~~gitlab-org/gitlab-ee!564~~ to be closed
    - [x]  Rebase against master instead of !4892
    - [x]  Make sure [build](https://gitlab.com/gitlab-org/gitlab-ce/commit/a4ca206fd1cc0332d1e385ddbc0f2e4065c3ae73/builds) is green
    - [x]  Create EE MR
        - [x]  Cherry pick commits
        - [x]  Make sure [build](https://gitlab.com/gitlab-org/gitlab-ee/commit/4e17190d7dc546c1f977edcafd1cbcea4bdb4043/builds) is green
    - [x]  Address @axil's comments
    - [x]  Assign to endboss
    - [x]  Wait for @dbalexandre's review
    - [x]  Address @dbalexandre's comments
    - [x]  Address @axil's comments
        - [x]  Align dropdowns
        - [x]  No flash when protected branch is updated
    - [x]  Resolve conflicts
    - [x]  Implement protect/unprotect API
    - [x]  Address @dbalexandre's comments
    - [x]  Update EE MR
    - [x]  Address @rymai's comments
        - [x]  Create/Update service should return a `ProtectedBranch`
        - [x]  Successfuly protected branch creation shouldn't `load_protected_branches`
        - [x]  Rename `allowed_to_merge` as #minimum_access_level_for_merge
        - [x]  Rename `allowed_to_push` as #minimum_access_level_for_push
        - [x]  Use `inclusion` and `Gitlab::Access` instead of an `enum`
        - [x]  Modify `check_access` to work with `Gitlab::Access`
        - [x]  Pass `@protected_branch` to `#execute` in `UpdateService`
        - [x]  simplify with a nested field `protected_branch[push_access_level][access_level]`
        - [x]  `developers_can_{merge,push}` should be handled in the API
        - [x]  Use `can?(current_user, ...)` instead of `current_user.can?(...)`
        - [x]  Instantiate `ProtectedBranchesAccessSelect` in `dispatcher.js`
        - [x]  constants regarding downtime migrations
        - [x]  Explicit `#down` for columns with default
    - [x]  Update EE MR
    - [ ]  Wait for CE merge
    - [ ]  Wait for EE merge
    - [ ]  Create issue for UI changes proposed by @zyv


See merge request !5081

7 files changed, 165 insertions, 82 deletions

diff --git a/spec/factories/protected_branches.rb b/spec/factories/protected_branches.rb
index 28ed8078157..5575852c2d7 100644
--- a/spec/factories/protected_branches.rb
+++ b/spec/factories/protected_branches.rb
@@ -2,5 +2,28 @@ FactoryGirl.define do
   factory :protected_branch do
     name
     project
+
+    after(:create) do |protected_branch|
+      protected_branch.create_push_access_level!(access_level: Gitlab::Access::MASTER)
+      protected_branch.create_merge_access_level!(access_level: Gitlab::Access::MASTER)
+    end
+
+    trait :developers_can_push do
+      after(:create) do |protected_branch|
+        protected_branch.push_access_level.update!(access_level: Gitlab::Access::DEVELOPER)
+      end
+    end
+
+    trait :developers_can_merge do
+      after(:create) do |protected_branch|
+        protected_branch.merge_access_level.update!(access_level: Gitlab::Access::DEVELOPER)
+      end
+    end
+
+    trait :no_one_can_push do
+      after(:create) do |protected_branch|
+        protected_branch.push_access_level.update!(access_level: Gitlab::Access::NO_ACCESS)
+      end
+    end
   end
 end
diff --git a/spec/features/protected_branches_spec.rb b/spec/features/protected_branches_spec.rb
index d94dee0c797..57734b33a44 100644
--- a/spec/features/protected_branches_spec.rb
+++ b/spec/features/protected_branches_spec.rb
@@ -1,6 +1,8 @@
 require 'spec_helper'
 
 feature 'Projected Branches', feature: true, js: true do
+  include WaitForAjax
+
   let(:user) { create(:user, :admin) }
   let(:project) { create(:project) }
 
@@ -81,4 +83,68 @@ feature 'Projected Branches', feature: true, js: true do
       end
     end
   end
+
+  describe "access control" do
+    ProtectedBranch::PushAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)|
+      it "allows creating protected branches that #{access_type_name} can push to" do
+        visit namespace_project_protected_branches_path(project.namespace, project)
+        set_protected_branch_name('master')
+        within('.new_protected_branch') do
+          find(".allowed-to-push").click
+          within(".dropdown.open .dropdown-menu") { click_on access_type_name }
+        end
+        click_on "Protect"
+
+        expect(ProtectedBranch.count).to eq(1)
+        expect(ProtectedBranch.last.push_access_level.access_level).to eq(access_type_id)
+      end
+
+      it "allows updating protected branches so that #{access_type_name} can push to them" do
+        visit namespace_project_protected_branches_path(project.namespace, project)
+        set_protected_branch_name('master')
+        click_on "Protect"
+
+        expect(ProtectedBranch.count).to eq(1)
+
+        within(".protected-branches-list") do
+          find(".allowed-to-push").click
+          within('.dropdown-menu.push') { click_on access_type_name }
+        end
+
+        wait_for_ajax
+        expect(ProtectedBranch.last.push_access_level.access_level).to eq(access_type_id)
+      end
+    end
+
+    ProtectedBranch::MergeAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)|
+      it "allows creating protected branches that #{access_type_name} can merge to" do
+        visit namespace_project_protected_branches_path(project.namespace, project)
+        set_protected_branch_name('master')
+        within('.new_protected_branch') do
+          find(".allowed-to-merge").click
+          within(".dropdown.open .dropdown-menu") { click_on access_type_name }
+        end
+        click_on "Protect"
+
+        expect(ProtectedBranch.count).to eq(1)
+        expect(ProtectedBranch.last.merge_access_level.access_level).to eq(access_type_id)
+      end
+
+      it "allows updating protected branches so that #{access_type_name} can merge to them" do
+        visit namespace_project_protected_branches_path(project.namespace, project)
+        set_protected_branch_name('master')
+        click_on "Protect"
+
+        expect(ProtectedBranch.count).to eq(1)
+
+        within(".protected-branches-list") do
+          find(".allowed-to-merge").click
+          within('.dropdown-menu.merge') { click_on access_type_name }
+        end
+
+        wait_for_ajax
+        expect(ProtectedBranch.last.merge_access_level.access_level).to eq(access_type_id)
+      end
+    end
+  end
 end
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index ae064a878b0..8447305a316 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -151,7 +151,13 @@ describe Gitlab::GitAccess, lib: true do
     def self.run_permission_checks(permissions_matrix)
       permissions_matrix.keys.each do |role|
         describe "#{role} access" do
-          before { project.team << [user, role] }
+          before do
+            if role == :admin
+              user.update_attribute(:admin, true)
+            else
+              project.team << [user, role]
+            end
+          end
 
           permissions_matrix[role].each do |action, allowed|
             context action do
@@ -165,6 +171,17 @@ describe Gitlab::GitAccess, lib: true do
     end
 
     permissions_matrix = {
+      admin: {
+        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
+      },
+
       master: {
         push_new_branch: true,
         push_master: true,
@@ -217,19 +234,20 @@ describe Gitlab::GitAccess, lib: true do
         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) }
+      context "when developers are allowed to push into the #{protected_branch_type} protected branch" do
+        before { create(:protected_branch, :developers_can_push, name: protected_branch_name, 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 "developers are allowed to merge into the #{protected_branch_type} protected branch" do
+        before { create(:protected_branch, :developers_can_merge, name: protected_branch_name, 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)
+              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 }))
@@ -242,51 +260,59 @@ describe Gitlab::GitAccess, lib: true do
 
             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 }))
+          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
       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) }
+      context "when developers are allowed to push and merge into the #{protected_branch_type} protected branch" do
+        before { create(:protected_branch, :developers_can_merge, :developers_can_push, name: protected_branch_name, 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 no one is allowed to push to the #{protected_branch_name} protected branch" do
+        before { create(:protected_branch, :no_one_can_push, name: protected_branch_name, project: project) }
+
+        run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false },
+                                                            master: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false },
+                                                            admin: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false }))
+      end
     end
+  end
 
-    describe 'deploy key permissions' do
-      let(:key) { create(:deploy_key) }
-      let(:actor) { key }
+  describe 'deploy key permissions' do
+    let(:key) { create(:deploy_key) }
+    let(:actor) { key }
 
-      context 'push code' do
-        subject { access.check('git-receive-pack') }
+    context 'push code' do
+      subject { access.check('git-receive-pack') }
 
-        context 'when project is authorized' do
-          before { key.projects << project }
+      context 'when project is authorized' do
+        before { key.projects << project }
 
-          it { expect(subject).not_to be_allowed }
-        end
+        it { expect(subject).not_to be_allowed }
+      end
 
-        context 'when unauthorized' do
-          context 'to public project' do
-            let(:project) { create(:project, :public) }
+      context 'when unauthorized' do
+        context 'to public project' do
+          let(:project) { create(:project, :public) }
 
-            it { expect(subject).not_to be_allowed }
-          end
+          it { expect(subject).not_to be_allowed }
+        end
 
-          context 'to internal project' do
-            let(:project) { create(:project, :internal) }
+        context 'to internal project' do
+          let(:project) { create(:project, :internal) }
 
-            it { expect(subject).not_to be_allowed }
-          end
+          it { expect(subject).not_to be_allowed }
+        end
 
-          context 'to private project' do
-            let(:project) { create(:project, :internal) }
+        context 'to private project' do
+          let(:project) { create(:project, :internal) }
 
-            it { expect(subject).not_to be_allowed }
-          end
+          it { expect(subject).not_to be_allowed }
         end
       end
     end
diff --git a/spec/lib/gitlab/user_access_spec.rb b/spec/lib/gitlab/user_access_spec.rb
index aa9ec243498..5bb095366fa 100644
--- a/spec/lib/gitlab/user_access_spec.rb
+++ b/spec/lib/gitlab/user_access_spec.rb
@@ -44,7 +44,7 @@ describe Gitlab::UserAccess, lib: true do
 
     describe 'push to protected branch if allowed for developers' do
       before do
-        @branch = create :protected_branch, project: project, developers_can_push: true
+        @branch = create :protected_branch, :developers_can_push, project: project
       end
 
       it 'returns true if user is a master' do
@@ -65,7 +65,7 @@ describe Gitlab::UserAccess, lib: true do
 
     describe 'merge to protected branch if allowed for developers' do
       before do
-        @branch = create :protected_branch, project: project, developers_can_merge: true
+        @branch = create :protected_branch, :developers_can_merge, project: project
       end
 
       it 'returns true if user is a master' do
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 72b8a4e25bd..e365e4e98b2 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1095,46 +1095,6 @@ describe Project, models: true do
     end
   end
 
-  describe "#developers_can_push_to_protected_branch?" do
-    let(:project) { create(:empty_project) }
-
-    context "when the branch matches a protected branch via direct match" do
-      it "returns true if 'Developers can Push' is turned on" do
-        create(:protected_branch, name: "production", project: project, developers_can_push: true)
-
-        expect(project.developers_can_push_to_protected_branch?('production')).to be true
-      end
-
-      it "returns false if 'Developers can Push' is turned off" do
-        create(:protected_branch, name: "production", project: project, developers_can_push: false)
-
-        expect(project.developers_can_push_to_protected_branch?('production')).to be false
-      end
-    end
-
-    context "when the branch matches a protected branch via wilcard match" do
-      it "returns true if 'Developers can Push' is turned on" do
-        create(:protected_branch, name: "production/*", project: project, developers_can_push: true)
-
-        expect(project.developers_can_push_to_protected_branch?('production/some-branch')).to be true
-      end
-
-      it "returns false if 'Developers can Push' is turned off" do
-        create(:protected_branch, name: "production/*", project: project, developers_can_push: false)
-
-        expect(project.developers_can_push_to_protected_branch?('production/some-branch')).to be false
-      end
-    end
-
-    context "when the branch does not match a protected branch" do
-      it "returns false" do
-        create(:protected_branch, name: "production/*", project: project, developers_can_push: true)
-
-        expect(project.developers_can_push_to_protected_branch?('staging/some-branch')).to be false
-      end
-    end
-  end
-
   describe '#container_registry_path_with_namespace' do
     let(:project) { create(:empty_project, path: 'PROJECT') }
 
diff --git a/spec/requests/api/branches_spec.rb b/spec/requests/api/branches_spec.rb
index 719da27f919..e8fd697965f 100644
--- a/spec/requests/api/branches_spec.rb
+++ b/spec/requests/api/branches_spec.rb
@@ -112,7 +112,7 @@ describe API::API, api: true  do
 
       before do
         project.repository.add_branch(user, protected_branch, 'master')
-        create(:protected_branch, project: project, name: protected_branch, developers_can_push: true, developers_can_merge: true)
+        create(:protected_branch, :developers_can_push, :developers_can_merge, project: project, name: protected_branch)
       end
 
       it 'updates that a developer can push' do
diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb
index 47c0580e0f0..ffa998dffc3 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -7,6 +7,7 @@ describe GitPushService, services: true do
   let(:project)       { create :project }
 
   before do
+    project.team << [user, :master]
     @blankrev = Gitlab::Git::BLANK_SHA
     @oldrev = sample_commit.parent_id
     @newrev = sample_commit.id
@@ -172,7 +173,7 @@ describe GitPushService, services: true do
   describe "Push Event" do
     before do
       service = execute_service(project, user, @oldrev, @newrev, @ref )
-      @event = Event.last
+      @event = Event.find_by_action(Event::PUSHED)
       @push_data = service.push_data
     end
 
@@ -224,8 +225,10 @@ 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, developers_can_merge: false })
         execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
+        expect(project.protected_branches).not_to be_empty
+        expect(project.protected_branches.first.push_access_level.access_level).to eq(Gitlab::Access::MASTER)
+        expect(project.protected_branches.first.merge_access_level.access_level).to eq(Gitlab::Access::MASTER)
       end
 
       it "when pushing a branch for the first time with default branch protection disabled" do
@@ -233,8 +236,8 @@ describe GitPushService, services: true do
 
         expect(project).to receive(:execute_hooks)
         expect(project.default_branch).to eq("master")
-        expect(project.protected_branches).not_to receive(:create)
         execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
+        expect(project.protected_branches).to be_empty
       end
 
       it "when pushing a branch for the first time with default branch protection set to 'developers can push'" do
@@ -242,9 +245,12 @@ 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, developers_can_merge: false })
 
-        execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master')
+        execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
+
+        expect(project.protected_branches).not_to be_empty
+        expect(project.protected_branches.last.push_access_level.access_level).to eq(Gitlab::Access::DEVELOPER)
+        expect(project.protected_branches.last.merge_access_level.access_level).to eq(Gitlab::Access::MASTER)
       end
 
       it "when pushing a branch for the first time with default branch protection set to 'developers can merge'" do
@@ -252,8 +258,10 @@ 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: false, developers_can_merge: true })
         execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
+        expect(project.protected_branches).not_to be_empty
+        expect(project.protected_branches.first.push_access_level.access_level).to eq(Gitlab::Access::MASTER)
+        expect(project.protected_branches.first.merge_access_level.access_level).to eq(Gitlab::Access::DEVELOPER)
       end
 
       it "when pushing new commits to existing branch" do