Merge branch 'id-move-approvals-endpoints-to-ce' into 'master'

Move approvals endpoints to FOSS version

See merge request gitlab-org/gitlab!36237

changelogs/unreleased/id-move-approvals-endpoints-to-ce.yml

+---
+title: Move approvals endpoints to FOSS version
+merge_request: 36237
+author:
+type: added

ee/lib/ee/api/api.rb

@@ -51,7 +51,6 @@ module EE
         mount ::API::VulnerabilityFindings
         mount ::API::VulnerabilityIssueLinks
         mount ::API::VulnerabilityExports
-        mount ::API::MergeRequestApprovals
         mount ::API::MergeRequestApprovalRules
         mount ::API::ProjectAliases
         mount ::API::Dependencies

ee/lib/ee/api/helpers.rb

@@ -68,12 +68,6 @@ module EE
         end
       end
 
-      def check_sha_param!(params, merge_request)
-        if params[:sha] && merge_request.diff_head_sha != params[:sha]
-          render_api_error!("SHA does not match HEAD of source branch: #{merge_request.diff_head_sha}", 409)
-        end
-      end
-
       # Normally, only admin users should have access to see LDAP
       # groups. However, due to the "Allow group owners to manage LDAP-related
       # group settings" setting, any group owner can sync LDAP groups with

ee/lib/ee/api/merge_request_approvals.rb

+# frozen_string_literal: true
+
+module EE
+  module API
+    module MergeRequestApprovals
+      extend ActiveSupport::Concern
+
+      prepended do
+        before { authenticate_non_get! }
+
+        helpers do
+          params :ee_approval_params do
+            optional :approval_password, type: String, desc: 'Current user\'s password if project is set to require explicit auth on approval'
+          end
+
+          def present_approval(merge_request)
+            present merge_request.approval_state, with: ::EE::API::Entities::ApprovalState, current_user: current_user
+          end
+
+          def handle_merge_request_errors!(errors)
+            if errors.has_key? :project_access
+              error!(errors[:project_access], 422)
+            elsif errors.has_key? :branch_conflict
+              error!(errors[:branch_conflict], 422)
+            elsif errors.has_key? :validate_fork
+              error!(errors[:validate_fork], 422)
+            elsif errors.has_key? :validate_branches
+              conflict!(errors[:validate_branches])
+            elsif errors.has_key? :base
+              error!(errors[:base], 422)
+            end
+
+            render_api_error!(errors, 400)
+          end
+
+          def present_merge_request_approval_state(presenter:, target_branch: nil)
+            merge_request = find_merge_request_with_access(params[:merge_request_iid])
+
+            present(
+              merge_request.approval_state(target_branch: target_branch),
+              with: presenter,
+              current_user: current_user
+            )
+          end
+        end
+
+        params do
+          requires :id, type: String, desc: 'The ID of a project'
+          requires :merge_request_iid, type: Integer, desc: 'The IID of a merge request'
+        end
+        resource :projects, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
+          segment ':id/merge_requests/:merge_request_iid' do
+            desc 'List approval rules for merge request', {
+              success: ::EE::API::Entities::MergeRequestApprovalSettings,
+              hidden: true
+            }
+            params do
+              optional :target_branch, type: String, desc: 'Branch that scoped approval rules apply to'
+            end
+            get 'approval_settings' do
+              present_merge_request_approval_state(
+                presenter: ::EE::API::Entities::MergeRequestApprovalSettings,
+                target_branch: declared_params[:target_branch]
+              )
+            end
+
+            desc 'Get approval state of merge request' do
+              success ::EE::API::Entities::MergeRequestApprovalState
+            end
+            get 'approval_state' do
+              present_merge_request_approval_state(presenter: ::EE::API::Entities::MergeRequestApprovalState)
+            end
+
+            desc 'Change approval-related configuration' do
+              detail 'This feature was introduced in 10.6'
+              success ::EE::API::Entities::ApprovalState
+            end
+            params do
+              requires :approvals_required, type: Integer, desc: 'The amount of approvals required. Must be higher than the project approvals'
+            end
+            post 'approvals' do
+              merge_request = find_merge_request_with_access(params[:merge_request_iid], :update_merge_request)
+
+              error!('Overriding approvals is disabled', 422) if merge_request.project.disable_overriding_approvers_per_merge_request
+
+              merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, approvals_before_merge: params[:approvals_required]).execute(merge_request)
+
+              if merge_request.valid?
+                present_approval(merge_request)
+              else
+                handle_merge_request_errors! merge_request.errors
+              end
+            end
+
+            desc 'Update approvers and approver groups' do
+              detail 'This feature was introduced in 10.6'
+              success ::EE::API::Entities::ApprovalState
+            end
+            params do
+              requires :approver_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce,
+                desc: 'Array of User IDs to set as approvers.'
+              requires :approver_group_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce,
+                desc: 'Array of Group IDs to set as approvers.'
+            end
+            put 'approvers' do
+              ::Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab/issues/8883')
+
+              merge_request = find_merge_request_with_access(params[:merge_request_iid], :update_approvers)
+
+              merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, declared(params, include_parent_namespaces: false).merge(remove_old_approvers: true)).execute(merge_request)
+
+              if merge_request.valid?
+                present_approval(merge_request)
+              else
+                handle_merge_request_errors! merge_request.errors
+              end
+            end
+          end
+        end
+      end
+    end
+  end
+end

lib/api/api.rb

@@ -168,6 +168,7 @@ module API
       mount ::API::Members
       mount ::API::MergeRequestDiffs
       mount ::API::MergeRequests
+      mount ::API::MergeRequestApprovals
       mount ::API::Metrics::Dashboard::Annotations
       mount ::API::Metrics::UserStarredDashboards
       mount ::API::Namespaces

lib/api/entities/merge_request_approvals.rb

+# frozen_string_literal: true
+
+module API
+  module Entities
+    class MergeRequestApprovals < Grape::Entity
+    end
+  end
+end

lib/api/helpers.rb

@@ -368,6 +368,12 @@ module API
       render_api_error!(message.join(' '), 404)
     end
 
+    def check_sha_param!(params, merge_request)
+      if params[:sha] && merge_request.diff_head_sha != params[:sha]
+        render_api_error!("SHA does not match HEAD of source branch: #{merge_request.diff_head_sha}", 409)
+      end
+    end
+
     def unauthorized!
       render_api_error!('401 Unauthorized', 401)
     end

ee/lib/api/merge_request_approvals.rb → lib/api/merge_request_approvals.rb

@@ -5,41 +5,14 @@ module API
     before { authenticate_non_get! }
 
     helpers do
-      def present_approval(merge_request)
-        present merge_request.approval_state, with: ::EE::API::Entities::ApprovalState, current_user: current_user
-      end
-
-      def handle_merge_request_errors!(errors)
-        if errors.has_key? :project_access
-          error!(errors[:project_access], 422)
-        elsif errors.has_key? :branch_conflict
-          error!(errors[:branch_conflict], 422)
-        elsif errors.has_key? :validate_fork
-          error!(errors[:validate_fork], 422)
-        elsif errors.has_key? :validate_branches
-          conflict!(errors[:validate_branches])
-        elsif errors.has_key? :base
-          error!(errors[:base], 422)
+      params :ee_approval_params do
       end
 
-        render_api_error!(errors, 400)
-      end
-
-      def present_merge_request_approval_state(presenter:, target_branch: nil)
-        merge_request = find_merge_request_with_access(params[:merge_request_iid])
-
-        present(
-          merge_request.approval_state(target_branch: target_branch),
-          with: presenter,
-          current_user: current_user
-        )
+      def present_approval(merge_request)
+        present merge_request, with: ::API::Entities::MergeRequestApprovals, current_user: current_user
       end
     end
 
-    params do
-      requires :id, type: String, desc: 'The ID of a project'
-      requires :merge_request_iid, type: Integer, desc: 'The IID of a merge request'
-    end
     resource :projects, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
       segment ':id/merge_requests/:merge_request_iid' do
         # Get the status of the merge request's approvals
@@ -49,83 +22,13 @@ module API
         #   merge_request_iid (required)  - IID of MR
         # Examples:
         #   GET /projects/:id/merge_requests/:merge_request_iid/approvals
-        #
-        # @deprecated
-        desc 'List approvals for merge request' do
-          success ::EE::API::Entities::ApprovalState
-        end
+        desc 'List approvals for merge request'
         get 'approvals' do
           merge_request = find_merge_request_with_access(params[:merge_request_iid])
 
           present_approval(merge_request)
         end
 
-        desc 'List approval rules for merge request', {
-          success: ::EE::API::Entities::MergeRequestApprovalSettings,
-          hidden: true
-        }
-        params do
-          optional :target_branch, type: String, desc: 'Branch that scoped approval rules apply to'
-        end
-        get 'approval_settings' do
-          present_merge_request_approval_state(
-            presenter: ::EE::API::Entities::MergeRequestApprovalSettings,
-            target_branch: declared_params[:target_branch]
-          )
-        end
-
-        desc 'Get approval state of merge request' do
-          success ::EE::API::Entities::MergeRequestApprovalState
-        end
-        get 'approval_state' do
-          present_merge_request_approval_state(presenter: ::EE::API::Entities::MergeRequestApprovalState)
-        end
-
-        desc 'Change approval-related configuration' do
-          detail 'This feature was introduced in 10.6'
-          success ::EE::API::Entities::ApprovalState
-        end
-        params do
-          requires :approvals_required, type: Integer, desc: 'The amount of approvals required. Must be higher than the project approvals'
-        end
-        post 'approvals' do
-          merge_request = find_merge_request_with_access(params[:merge_request_iid], :update_merge_request)
-
-          error!('Overriding approvals is disabled', 422) if merge_request.project.disable_overriding_approvers_per_merge_request
-
-          merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, approvals_before_merge: params[:approvals_required]).execute(merge_request)
-
-          if merge_request.valid?
-            present_approval(merge_request)
-          else
-            handle_merge_request_errors! merge_request.errors
-          end
-        end
-
-        desc 'Update approvers and approver groups' do
-          detail 'This feature was introduced in 10.6'
-          success ::EE::API::Entities::ApprovalState
-        end
-        params do
-          requires :approver_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce,
-            desc: 'Array of User IDs to set as approvers.'
-          requires :approver_group_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce,
-            desc: 'Array of Group IDs to set as approvers.'
-        end
-        put 'approvers' do
-          Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab/issues/8883')
-
-          merge_request = find_merge_request_with_access(params[:merge_request_iid], :update_approvers)
-
-          merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, declared(params, include_parent_namespaces: false).merge(remove_old_approvers: true)).execute(merge_request)
-
-          if merge_request.valid?
-            present_approval(merge_request)
-          else
-            handle_merge_request_errors! merge_request.errors
-          end
-        end
-
         # Approve a merge request
         #
         # Parameters:
@@ -134,15 +37,14 @@ module API
         # Examples:
         #   POST /projects/:id/merge_requests/:merge_request_iid/approve
         #
-        desc 'Approve a merge request' do
-          success ::EE::API::Entities::ApprovalState
-        end
+        desc 'Approve a merge request'
         params do
           optional :sha, type: String, desc: 'When present, must have the HEAD SHA of the source branch'
-          optional :approval_password, type: String, desc: 'Current user\'s password if project is set to require explicit auth on approval'
+
+          use :ee_approval_params
         end
         post 'approve' do
-          merge_request = find_project_merge_request(params[:merge_request_iid])
+          merge_request = find_merge_request_with_access(params[:merge_request_iid], :approve_merge_request)
 
           check_sha_param!(params, merge_request)
 
@@ -156,11 +58,9 @@ module API
           present_approval(merge_request)
         end
 
-        desc 'Remove an approval from a merge request' do
-          success ::EE::API::Entities::ApprovalState
-        end
+        desc 'Remove an approval from a merge request'
         post 'unapprove' do
-          merge_request = find_project_merge_request(params[:merge_request_iid])
+          merge_request = find_merge_request_with_access(params[:merge_request_iid], :approve_merge_request)
 
           success = ::MergeRequests::RemoveApprovalService
             .new(user_project, current_user)
@@ -174,3 +74,5 @@ module API
     end
   end
 end
+
+API::MergeRequestApprovals.prepend_if_ee('EE::API::MergeRequestApprovals')

lib/api/merge_requests.rb

@@ -68,12 +68,6 @@ module API
         mr.all_pipelines
       end
 
-      def check_sha_param!(params, merge_request)
-        if params[:sha] && merge_request.diff_head_sha != params[:sha]
-          render_api_error!("SHA does not match HEAD of source branch: #{merge_request.diff_head_sha}", 409)
-        end
-      end
-
       def automatically_mergeable?(merge_when_pipeline_succeeds, merge_request)
         pipeline_active = merge_request.head_pipeline_active? || merge_request.actual_head_pipeline_active?
         merge_when_pipeline_succeeds && merge_request.mergeable_state?(skip_ci_check: true) && pipeline_active

spec/requests/api/merge_request_approvals_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe API::MergeRequestApprovals do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project, :public, :repository, creator: user, namespace: user.namespace) }
+  let_it_be(:approver) { create :user }
+  let_it_be(:group) { create :group }
+
+  let(:merge_request) { create(:merge_request, :simple, author: user, source_project: project) }
+
+  describe 'GET :id/merge_requests/:merge_request_iid/approvals' do
+    it 'retrieves the approval status' do
+      project.add_developer(approver)
+      project.add_developer(create(:user))
+
+      create(:approval, user: approver, merge_request: merge_request)
+
+      get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", user)
+
+      expect(response).to have_gitlab_http_status(:ok)
+    end
+  end
+
+  describe 'POST :id/merge_requests/:merge_request_iid/approve' do
+    context 'as a valid approver' do
+      let_it_be(:approver) { create(:user) }
+
+      before do
+        project.add_developer(approver)
+        project.add_developer(create(:user))
+      end
+
+      def approve(extra_params = {})
+        post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approve", approver), params: extra_params
+      end
+
+      context 'when the sha param is not set' do
+        it 'approves the merge request' do
+          approve
+
+          expect(response).to have_gitlab_http_status(:created)
+        end
+      end
+
+      context 'when the sha param is correct' do
+        it 'approves the merge request' do
+          approve(sha: merge_request.diff_head_sha)
+
+          expect(response).to have_gitlab_http_status(:created)
+        end
+      end
+
+      context 'when the sha param is incorrect' do
+        it 'does not approve the merge request' do
+          approve(sha: merge_request.diff_head_sha.reverse)
+
+          expect(response).to have_gitlab_http_status(:conflict)
+          expect(merge_request.approvals).to be_empty
+        end
+      end
+    end
+  end
+
+  describe 'POST :id/merge_requests/:merge_request_iid/unapprove' do
+    context 'as a user who has approved the merge request' do
+      it 'unapproves the merge request' do
+        unapprover = create(:user)
+
+        project.add_developer(approver)
+        project.add_developer(unapprover)
+        project.add_developer(create(:user))
+
+        create(:approval, user: approver, merge_request: merge_request)
+        create(:approval, user: unapprover, merge_request: merge_request)
+
+        post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/unapprove", unapprover)
+
+        expect(response).to have_gitlab_http_status(:created)
+      end
+    end
+  end
+end