Merge branch '6096-approvers-api-form-encoded' into 'master'

Resolve "Approvers API does not work properly with form-encoded input"

Closes #6096

See merge request gitlab-org/gitlab-ee!5784

ee/changelogs/unreleased/6096-approvers-api-form-encoded.yml

+---
+title: Fix approvers API not accepting empty form-encoded params
+merge_request: 5784
+author:
+type: fixed

ee/lib/api/merge_request_approvals.rb

@@ -2,6 +2,8 @@ module API
   class MergeRequestApprovals < ::Grape::API
     before { authenticate_non_get! }
 
+    ARRAY_COERCION_LAMBDA = ->(val) { val.empty? ? [] : Array.wrap(val) }
+
     helpers do
       def handle_merge_request_errors!(errors)
         if errors.has_key? :project_access
@@ -69,8 +71,8 @@ module API
           success Entities::MergeRequestApprovals
         end
         params do
-          requires :approver_ids, type: Array[String], allow_blank: true, desc: 'Array of User IDs to set as approvers.'
-          requires :approver_group_ids, type: Array[String], allow_blank: true, desc: 'Array of Group IDs to set as approvers.'
+          requires :approver_ids, type: Array[String], coerce_with: ARRAY_COERCION_LAMBDA, desc: 'Array of User IDs to set as approvers.'
+          requires :approver_group_ids, type: Array[String], coerce_with: ARRAY_COERCION_LAMBDA, desc: 'Array of Group IDs to set as approvers.'
         end
         put 'approvers' do
           merge_request = find_merge_request_with_access(params[:merge_request_iid], :update_approvers)

ee/lib/api/project_approvals.rb

@@ -3,6 +3,8 @@ module API
     before { authenticate! }
     before { authorize! :update_approvers, user_project }
 
+    ARRAY_COERCION_LAMBDA = ->(val) { val.empty? ? [] : Array.wrap(val) }
+
     params do
       requires :id, type: String, desc: 'The ID of a project'
     end
@@ -44,8 +46,8 @@ module API
         success ::API::Entities::ApprovalSettings
       end
       params do
-        requires :approver_ids, type: Array[String], allow_blank: true, desc: 'Array of User IDs to set as approvers.'
-        requires :approver_group_ids, type: Array[String], allow_blank: true, desc: 'Array of Group IDs to set as approvers.'
+        requires :approver_ids, type: Array[String], coerce_with: ARRAY_COERCION_LAMBDA, desc: 'Array of User IDs to set as approvers.'
+        requires :approver_group_ids, type: Array[String], coerce_with: ARRAY_COERCION_LAMBDA, desc: 'Array of Group IDs to set as approvers.'
       end
       put ':id/approvers' do
         result = ::Projects::UpdateService.new(user_project, current_user, declared(params, include_parent_namespaces: false).merge(remove_old_approvers: true)).execute

ee/spec/requests/api/merge_request_approvals_spec.rb

@@ -165,6 +165,22 @@ describe API::MergeRequestApprovals do
           expect(response).to have_gitlab_http_status(200)
           expect(json_response['approvers']).to eq([])
         end
+
+        context 'when sending form-encoded data' do
+          it 'removes approvers not in the payload' do
+            merge_request.approvers.create(user: approver)
+            merge_request.approver_groups.create(group: approver_group)
+
+            expect do
+              put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvers", current_user),
+                approver_ids: '', approver_group_ids: ''
+            end.to change { merge_request.approvers.count }.from(1).to(0)
+              .and change { merge_request.approver_groups.count }.from(1).to(0)
+
+            expect(response).to have_gitlab_http_status(200)
+            expect(json_response['approvers']).to eq([])
+          end
+        end
       end
     end
 

ee/spec/requests/api/project_approvals_spec.rb

@@ -107,6 +107,20 @@ describe API::ProjectApprovals do
         expect(json_response['approver_groups']).to be_empty
       end
 
+      context 'when sending form-encoded data' do
+        it 'removes all approvers if no params are given' do
+          project.approvers.create(user: approver)
+
+          expect do
+            put api(url, current_user), approver_ids: '', approver_group_ids: ''
+          end.to change { project.approvers.count }.from(1).to(0)
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(json_response['approvers']).to be_empty
+          expect(json_response['approver_groups']).to be_empty
+        end
+      end
+
       it 'sets approvers and approver groups' do
         project.approvers.create(user: approver)