Merge branch 'ce-21513-fix-branch-protection-api' into 'master'

Fix branch protection API

- EE-specific (spec) changes related to gitlab-org/gitlab-ce!6215.
- Related to gitlab-org/gitlab-ce#21513.

See merge request !718

CHANGELOG.md

@@ -355,6 +355,7 @@ Please view this file on the master branch, on stable branches it's out of date.
   - Fix inconsistent checkbox alignment (ClemMakesApps)
   - Use the default branch for displaying the project icon instead of master !5792 (Hannes Rosenögger)
   - Adds response mime type to transaction metric action when it's not HTML
+  - Fix branch protection API !6215
   - Fix hover leading space bug in pipeline graph !5980
   - Avoid conflict with admin labels when importing GitHub labels
   - User can edit closed MR with deleted fork (Katarzyna Kobierska Ula Budziszewska) !5496

app/models/concerns/protected_branch_access.rb

@@ -8,6 +8,9 @@ module ProtectedBranchAccess
                             scope: :protected_branch,
                             unless: Proc.new { |access_level| access_level.user_id? || access_level.group_id? },
                             conditions: -> { where(user_id: nil, group_id: nil) }
+
+    scope :master, -> { where(access_level: Gitlab::Access::MASTER) }
+    scope :developer, -> { where(access_level: Gitlab::Access::DEVELOPER) }
   end
 
   def type

app/services/protected_branches/api_create_service.rb

+# The protected branches API still uses the `developers_can_push` and `developers_can_merge`
+# flags for backward compatibility, and so performs translation between that format and the
+# internal data model (separate access levels). The translation code is non-trivial, and so
+# lives in this service.
+module ProtectedBranches
+  class ApiCreateService < BaseService
+    def execute
+      push_access_level =
+        if params.delete(:developers_can_push)
+          Gitlab::Access::DEVELOPER
+        else
+          Gitlab::Access::MASTER
+        end
+
+      merge_access_level =
+        if params.delete(:developers_can_merge)
+          Gitlab::Access::DEVELOPER
+        else
+          Gitlab::Access::MASTER
+        end
+
+      @params.merge!(push_access_levels_attributes: [{ access_level: push_access_level }],
+                     merge_access_levels_attributes: [{ access_level: merge_access_level }])
+
+      service = ProtectedBranches::CreateService.new(@project, @current_user, @params)
+      service.execute
+    end
+  end
+end

app/services/protected_branches/api_update_service.rb

+# The protected branches API still uses the `developers_can_push` and `developers_can_merge`
+# flags for backward compatibility, and so performs translation between that format and the
+# internal data model (separate access levels). The translation code is non-trivial, and so
+# lives in this service.
+module ProtectedBranches
+  class ApiUpdateService < BaseService
+    def execute(protected_branch)
+      @developers_can_push = params.delete(:developers_can_push)
+      @developers_can_merge = params.delete(:developers_can_merge)
+
+      @protected_branch = protected_branch
+
+      protected_branch.transaction do
+        delete_redundant_ee_access_levels
+
+        case @developers_can_push
+        when true
+          params.merge!(push_access_levels_attributes: [{ access_level: Gitlab::Access::DEVELOPER }])
+        when false
+          params.merge!(push_access_levels_attributes: [{ access_level: Gitlab::Access::MASTER }])
+        end
+
+        case @developers_can_merge
+        when true
+          params.merge!(merge_access_levels_attributes: [{ access_level: Gitlab::Access::DEVELOPER }])
+        when false
+          params.merge!(merge_access_levels_attributes: [{ access_level: Gitlab::Access::MASTER }])
+        end
+
+        service = ProtectedBranches::UpdateService.new(@project, @current_user, @params)
+        service.execute(protected_branch)
+      end
+    end
+
+    private
+
+    def delete_redundant_access_levels
+      unless @developers_can_merge.nil?
+        @protected_branch.merge_access_levels.destroy_all
+      end
+
+      unless @developers_can_push.nil?
+        @protected_branch.push_access_levels.destroy_all
+      end
+    end
+
+    # If a protected branch can have more than one access level (EE), only
+    # remove the relevant access levels. If we don't do this, we'll have a
+    # failed validation.
+    def delete_redundant_ee_access_levels
+      case @developers_can_merge
+      when true
+        @protected_branch.merge_access_levels.developer.destroy_all
+      when false
+        @protected_branch.merge_access_levels.developer.destroy_all
+        @protected_branch.merge_access_levels.master.destroy_all
+      end
+
+      case @developers_can_push
+      when true
+        @protected_branch.push_access_levels.developer.destroy_all
+      when false
+        @protected_branch.push_access_levels.developer.destroy_all
+        @protected_branch.push_access_levels.master.destroy_all
+      end
+    end
+  end
+end

lib/api/branches.rb

@@ -54,43 +54,25 @@ module API
         not_found!('Branch') unless @branch
         protected_branch = user_project.protected_branches.find_by(name: @branch.name)
 
-        developers_can_merge = to_boolean(params[:developers_can_merge])
-        developers_can_push = to_boolean(params[:developers_can_push])
-
         protected_branch_params = {
-          name: @branch.name
+          name: @branch.name,
+          developers_can_push: to_boolean(params[:developers_can_push]),
+          developers_can_merge: to_boolean(params[:developers_can_merge])
         }
 
-        # If `developers_can_merge` is switched off, _all_ `DEVELOPER`
-        # merge_access_levels need to be deleted.
-        if developers_can_merge == false
-          protected_branch.merge_access_levels.where(access_level: Gitlab::Access::DEVELOPER).destroy_all
-        end
-
-        # If `developers_can_push` is switched off, _all_ `DEVELOPER`
-        # push_access_levels need to be deleted.
-        if developers_can_push == false
-          protected_branch.push_access_levels.where(access_level: Gitlab::Access::DEVELOPER).destroy_all
-        end
+        service_args = [user_project, current_user, protected_branch_params]
 
-        protected_branch_params.merge!(
-          merge_access_levels_attributes: [{
-            access_level: developers_can_merge ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
-          }],
-          push_access_levels_attributes: [{
-            access_level: developers_can_push ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
-          }]
-        )
-
-        if protected_branch
-          service = ProtectedBranches::UpdateService.new(user_project, current_user, protected_branch_params)
-          service.execute(protected_branch)
+        protected_branch = if protected_branch
+                             ProtectedBranches::ApiUpdateService.new(*service_args).execute(protected_branch)
                            else
-          service = ProtectedBranches::CreateService.new(user_project, current_user, protected_branch_params)
-          service.execute
+                             ProtectedBranches::ApiCreateService.new(*service_args).execute
                            end
 
+        if protected_branch.valid?
           present @branch, with: Entities::RepoBranch, project: user_project
+        else
+          render_api_error!(protected_branch.errors.full_messages, 422)
+        end
       end
 
       # Unprotect a single branch

spec/requests/api/branches_spec.rb

@@ -48,6 +48,7 @@ describe API::API, api: true  do
   end
 
   describe 'PUT /projects/:id/repository/branches/:branch/protect' do
+    context "when a protected branch doesn't already exist" do
       it 'protects a single branch' do
         put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user)
 
@@ -106,34 +107,108 @@ describe API::API, api: true  do
         expect(json_response['developers_can_push']).to eq(false)
         expect(json_response['developers_can_merge']).to eq(false)
       end
+    end
 
-    context 'on a protected branch' do
-      let(:protected_branch) { 'foo' }
-
+    context 'for an existing protected branch' do
       before do
-        project.repository.add_branch(user, protected_branch, 'master')
-        create(:protected_branch, :developers_can_push, :developers_can_merge, project: project, name: protected_branch)
+        project.repository.add_branch(user, protected_branch.name, 'master')
       end
 
-      it 'updates that a developer can push' do
-        put api("/projects/#{project.id}/repository/branches/#{protected_branch}/protect", user),
+      context "when developers can push and merge" do
+        let(:protected_branch) { create(:protected_branch, :developers_can_push, :developers_can_merge, project: project, name: 'protected_branch') }
+
+        it 'updates that a developer cannot push or merge' do
+          put api("/projects/#{project.id}/repository/branches/#{protected_branch.name}/protect", user),
               developers_can_push: false, developers_can_merge: false
 
           expect(response).to have_http_status(200)
-        expect(json_response['name']).to eq(protected_branch)
+          expect(json_response['name']).to eq(protected_branch.name)
+          expect(json_response['protected']).to eq(true)
+          expect(json_response['developers_can_push']).to eq(false)
+          expect(json_response['developers_can_merge']).to eq(false)
+        end
+
+        it "doesn't result in 0 access levels when 'developers_can_push' is switched off" do
+          put api("/projects/#{project.id}/repository/branches/#{protected_branch.name}/protect", user),
+              developers_can_push: false
+
+          expect(response).to have_http_status(200)
+          expect(json_response['name']).to eq(protected_branch.name)
+          expect(protected_branch.reload.push_access_levels.first).to be_present
+          expect(protected_branch.reload.push_access_levels.first.access_level).to eq(Gitlab::Access::MASTER)
+        end
+
+        it "doesn't result in 0 access levels when 'developers_can_merge' is switched off" do
+          put api("/projects/#{project.id}/repository/branches/#{protected_branch.name}/protect", user),
+              developers_can_merge: false
+
+          expect(response).to have_http_status(200)
+          expect(json_response['name']).to eq(protected_branch.name)
+          expect(protected_branch.reload.merge_access_levels.first).to be_present
+          expect(protected_branch.reload.merge_access_levels.first.access_level).to eq(Gitlab::Access::MASTER)
+        end
+      end
+
+      context "when developers cannot push or merge" do
+        let(:protected_branch) { create(:protected_branch, project: project, name: 'protected_branch') }
+
+        it 'updates that a developer can push and merge' do
+          put api("/projects/#{project.id}/repository/branches/#{protected_branch.name}/protect", user),
+              developers_can_push: true, developers_can_merge: true
+
+          expect(response).to have_http_status(200)
+          expect(json_response['name']).to eq(protected_branch.name)
+          expect(json_response['protected']).to eq(true)
+          expect(json_response['developers_can_push']).to eq(true)
+          expect(json_response['developers_can_merge']).to eq(true)
+        end
+      end
+
+      context "when no one can push" do
+        let(:protected_branch) { create(:protected_branch, :no_one_can_push, project: project, name: 'protected_branch') }
+
+        it "updates 'developers_can_push' without removing the 'no_one' access level" do
+          put api("/projects/#{project.id}/repository/branches/#{protected_branch.name}/protect", user),
+              developers_can_push: true, developers_can_merge: true
+
+          expect(response).to have_http_status(200)
+          expect(json_response['name']).to eq(protected_branch.name)
+          expect(protected_branch.reload.push_access_levels.pluck(:access_level)).to include(Gitlab::Access::NO_ACCESS)
+        end
+      end
+    end
+
+    context "multiple API calls" do
+      it "returns success when `protect` is called twice" do
+        put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user)
+        put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user)
+
+        expect(response).to have_http_status(200)
+        expect(json_response['name']).to eq(branch_name)
         expect(json_response['protected']).to eq(true)
         expect(json_response['developers_can_push']).to eq(false)
         expect(json_response['developers_can_merge']).to eq(false)
       end
 
-      it 'does not update that a developer can push' do
-        put api("/projects/#{project.id}/repository/branches/#{protected_branch}/protect", user),
-          developers_can_push: 'foobar', developers_can_merge: 'foo'
+      it "returns success when `protect` is called twice with `developers_can_push` turned on" do
+        put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user), developers_can_push: true
+        put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user), developers_can_push: true
 
         expect(response).to have_http_status(200)
-        expect(json_response['name']).to eq(protected_branch)
+        expect(json_response['name']).to eq(branch_name)
         expect(json_response['protected']).to eq(true)
         expect(json_response['developers_can_push']).to eq(true)
+        expect(json_response['developers_can_merge']).to eq(false)
+      end
+
+      it "returns success when `protect` is called twice with `developers_can_merge` turned on" do
+        put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user), developers_can_merge: true
+        put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user), developers_can_merge: true
+
+        expect(response).to have_http_status(200)
+        expect(json_response['name']).to eq(branch_name)
+        expect(json_response['protected']).to eq(true)
+        expect(json_response['developers_can_push']).to eq(false)
         expect(json_response['developers_can_merge']).to eq(true)
       end
     end
@@ -147,12 +222,6 @@ describe API::API, api: true  do
       put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user2)
       expect(response).to have_http_status(403)
     end
-
-    it "returns success when protect branch again" do
-      put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user)
-      put api("/projects/#{project.id}/repository/branches/#{branch_name}/protect", user)
-      expect(response).to have_http_status(200)
-    end
   end
 
   describe "PUT /projects/:id/repository/branches/:branch/unprotect" do