Merge branch '267511-update-delete-approval-rule-spi' into 'master'

Add update and destroy API endpoints to ExternalApprovalRules

See merge request gitlab-org/gitlab!55197

doc/api/merge_request_approvals.md

@@ -627,6 +627,35 @@ POST /projects/:id/external_approval_rules
 | `external_url`         | string         | yes      | URL of external approval resource                  |
 | `protected_branch_ids` | array<Integer> | no       | The ids of protected branches to scope the rule by |
 
+### Delete external approval rule **(ULTIMATE)**
+
+You can delete an external approval rule for a project using the following endpoint:
+
+```plaintext
+DELETE /projects/:id/external_approval_rules/:rule_id
+```
+
+| Attribute              | Type           | Required | Description                                        |
+|------------------------|----------------|----------|----------------------------------------------------|
+| `rule_id`              | integer        | yes      | The ID of an approval rule                         |
+| `id`                   | integer        | yes      | The ID of a project                                |
+
+### Update external approval rule **(ULTIMATE)**
+
+You can update an existing external approval rule for a project using the following endpoint:
+
+```plaintext
+PATCH /projects/:id/external_approval_rules/:rule_id
+```
+
+| Attribute              | Type           | Required | Description                                        |
+|------------------------|----------------|----------|----------------------------------------------------|
+| `id`                   | integer        | yes      | The ID of a project                                |
+| `rule_id`              | integer        | yes      | The ID of an external approval rule                |
+| `name`                 | string         | no       | Display name of approval rule                      |
+| `external_url`         | string         | no       | URL of external approval resource                  |
+| `protected_branch_ids` | array<Integer> | no       | The ids of protected branches to scope the rule by |
+
 ### Enable or disable External Project-level MR approvals **(ULTIMATE SELF)**
 
 Enable or disable External Project-level MR approvals is under development and not ready for production use. It is

ee/app/services/external_approval_rules/destroy_service.rb

+# frozen_string_literal: true
+
+module ExternalApprovalRules
+  class DestroyService < BaseContainerService
+    def execute(rule)
+      return unauthorized_error_response unless current_user.can?(:admin_project, container)
+
+      if rule.destroy
+        ServiceResponse.success
+      else
+        ServiceResponse.error(message: 'Failed to destroy rule',
+                              payload: { errors: rule.errors.full_messages },
+                              http_status: :unprocessable_entity)
+      end
+    end
+
+    private
+
+    def unauthorized_error_response
+      ServiceResponse.error(
+        message: 'Failed to destroy rule',
+        payload: { errors: ['Not allowed'] },
+        http_status: :unauthorized
+      )
+    end
+  end
+end

ee/app/services/external_approval_rules/update_service.rb

+# frozen_string_literal: true
+
+module ExternalApprovalRules
+  class UpdateService < BaseContainerService
+    def execute
+      return unauthorized_error_response unless current_user.can?(:admin_project, container)
+
+      if rule.update(resource_params)
+        ServiceResponse.success(payload: { rule: rule })
+      else
+        ServiceResponse.error(message: 'Failed to update rule',
+                              payload: { errors: rule.errors.full_messages },
+                              http_status: :unprocessable_entity)
+      end
+    end
+
+    private
+
+    def resource_params
+      params.slice(:name, :external_url, :protected_branch_ids)
+    end
+
+    def rule
+      container.external_approval_rules.find(params[:rule_id])
+    end
+
+    def unauthorized_error_response
+      ServiceResponse.error(
+        message: 'Failed to update rule',
+        payload: { errors: ['Not allowed'] },
+        http_status: :unauthorized
+      )
+    end
+  end
+end

ee/lib/api/external_approval_rules.rb

@@ -17,11 +17,10 @@ module API
       end
     end
 
-    resource :projects do
+    resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
       segment ':id/external_approval_rules' do
         params do
-          requires :id, type: Integer, desc: 'The ID of the project to associate the rule with'
-          requires :name, type: String, desc: 'The approval rule\'s name'
+          requires :name, type: String, desc: 'The name of the rule'
           requires :external_url, type: String, desc: 'The URL to notify when MR receives new commits'
           optional :protected_branch_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The protected branch ids for this rule'
           use :pagination
@@ -30,7 +29,6 @@ module API
           success ::API::Entities::ExternalApprovalRule
           detail 'This feature is gated by the :ff_compliance_approval_gates feature flag.'
         end
-
         post do
           service = ::ExternalApprovalRules::CreateService.new(container: @project,
                                                                current_user: current_user,
@@ -51,6 +49,46 @@ module API
 
           present paginate(@project.external_approval_rules), with: ::API::Entities::ExternalApprovalRule
         end
+        segment ':rule_id' do
+          desc 'Delete an external approval rule' do
+            detail 'This feature is gated by the :ff_compliance_approval_gates feature flag.'
+          end
+          params do
+            requires :rule_id, type: Integer, desc: 'The approval rule ID'
+          end
+          delete do
+            external_approval_rule = user_project.external_approval_rules.find(params[:rule_id])
+
+            destroy_conditionally!(external_approval_rule) do |external_approval_rule|
+              ::ExternalApprovalRules::DestroyService.new(
+                container: @project,
+                current_user: current_user
+              ).execute(external_approval_rule)
+            end
+          end
+
+          desc 'Update new external approval rule' do
+            success ::API::Entities::ExternalApprovalRule
+            detail 'This feature is gated by the :ff_compliance_approval_gates feature flag.'
+          end
+          params do
+            requires :rule_id, type: Integer, desc: 'The approval rule ID'
+            optional :name, type: String, desc: 'The approval rule\'s name'
+            optional :external_url, type: String, desc: 'The URL to notify when MR receives new commits'
+            optional :protected_branch_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The protected branch ids for this rule'
+          end
+          put do
+            service = ::ExternalApprovalRules::UpdateService.new(container: @project,
+                                                                 current_user: current_user,
+                                                                 params: declared(params, include_missing: false)).execute
+
+            if service.success?
+              present service.payload[:rule], with: ::API::Entities::ExternalApprovalRule
+            else
+              render_api_error!(service.payload[:errors], service.http_status)
+            end
+          end
+        end
       end
     end
   end

ee/spec/factories/external_approval_rules.rb

@@ -3,7 +3,7 @@
 FactoryBot.define do
   factory :external_approval_rule, class: 'ApprovalRules::ExternalApprovalRule' do
     project
-    external_url { "https://testurl.example.test" }
+    external_url { FFaker::Internet.http_url }
 
     sequence :name do |i|
       "rule #{i}"

ee/spec/requests/api/external_approval_rules_spec.rb

@@ -6,25 +6,63 @@ RSpec.describe ::API::ExternalApprovalRules do
   using RSpec::Parameterized::TableSyntax
 
   let_it_be(:project) { create(:project) }
+  let_it_be(:rule) { create(:external_approval_rule, project: project, name: 'Rule 2', external_url: 'https://rule2.example') }
+  let(:collection_url) { "/projects/#{project.id}/external_approval_rules" }
+  let(:single_object_url) { "/projects/#{project.id}/external_approval_rules/#{rule.id}" }
+
+  describe 'DELETE projects/:id/external_approval_rules/:rule_id' do
+    before do
+      stub_licensed_features(compliance_approval_gates: true)
+    end
+
+    it 'deletes the specified rule' do
+      expect do
+        delete api(single_object_url, project.owner)
+      end.to change { ApprovalRules::ExternalApprovalRule.count }.by(-1)
+    end
+
+    context 'when feature is disabled, unlicensed or user has permission' do
+      where(:licensed, :flag, :project_owner, :status) do
+        false | false | false | :not_found
+        false | false | true  | :unauthorized
+        false | true  | true  | :unauthorized
+        false | true  | false | :not_found
+        true  | false | false | :not_found
+        true  | false | true  | :unauthorized
+        true  | true  | false | :not_found
+        true  | true  | true  | :success
+      end
+
+      with_them do
+        before do
+          stub_feature_flags(ff_compliance_approval_gates: flag)
+          stub_licensed_features(compliance_approval_gates: licensed)
+        end
+
+        it 'returns the correct status code' do
+          delete api(single_object_url, (project_owner ? project.owner : build(:user)))
+
+          expect(response).to have_gitlab_http_status(status)
+        end
+      end
+    end
+  end
 
   describe 'GET projects/:id/external_approval_rules' do
-    let_it_be(:rule) { create(:external_approval_rule, project: project) }
+    let_it_be(:rule) { create(:external_approval_rule, project: project, name: 'Rule 1', external_url: "http://rule1.example") }
     let_it_be(:protected_branches) { create_list(:protected_branch, 3, project: project) }
 
     before_all do
-      create(:external_approval_rule)
+      create(:external_approval_rule) # Creating an orphaned rule to make sure project scoping works as expected
     end
 
-    it 'responds with expected JSON' do
+    it 'responds with expected JSON', :aggregate_failures do
       stub_licensed_features(compliance_approval_gates: true)
-      get api("/projects/#{project.id}/external_approval_rules", project.owner)
-
-      first_result = json_response.dig(0)
+      get api(collection_url, project.owner)
 
-      expect(json_response.size).to eq(1)
-      expect(first_result['id']).not_to be_nil
-      expect(first_result['name']).to eq(rule.name)
-      expect(first_result['external_url']).to eq(rule.external_url)
+      expect(json_response.size).to eq(2)
+      expect(json_response.map { |r| r['name'] }).to contain_exactly('Rule 1', 'Rule 2')
+      expect(json_response.map { |r| r['external_url'] }).to contain_exactly('http://rule1.example', 'https://rule2.example')
     end
 
     context 'when feature is disabled, unlicensed or user has permission' do
@@ -46,7 +84,7 @@ RSpec.describe ::API::ExternalApprovalRules do
         end
 
         it 'returns the correct status code' do
-          get api("/projects/#{project.id}/external_approval_rules", (project_owner ? project.owner : build(:user)))
+          get api(collection_url, (project_owner ? project.owner : build(:user)))
 
           expect(response).to have_gitlab_http_status(status)
         end
@@ -128,4 +166,85 @@ RSpec.describe ::API::ExternalApprovalRules do
       end
     end
   end
+
+  describe 'PUT projects/:id/external_approval_rules/:rule_id' do
+    let(:params) { { external_url: 'http://newvalue.com', name: 'new name' } }
+
+    context 'successfully updating external approval rule' do
+      before do
+        stub_feature_flags(ff_compliance_approval_gates: true)
+        stub_licensed_features(compliance_approval_gates: true)
+      end
+
+      subject do
+        put api(single_object_url, project.owner), params: params
+      end
+
+      it 'updates an approval rule' do
+        expect { subject }.to change { rule.reload.external_url }.to eq('http://newvalue.com')
+      end
+
+      it 'responds with correct http status' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:success)
+      end
+
+      context 'with protected branches' do
+        let_it_be(:protected_branch) { create(:protected_branch, project: project) }
+        let(:params) do
+          { name: 'New rule', external_url: 'https://gitlab.com/test/example.json', protected_branch_ids: protected_branch.id }
+        end
+
+        subject do
+          put api(single_object_url, project.owner), params: params
+        end
+
+        it 'returns expected status code' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:success)
+        end
+
+        it 'creates protected branch records' do
+          expect { subject }.to change { ApprovalRules::ExternalApprovalRule.last.protected_branches.count }.by(1)
+        end
+
+        it 'responds with expected JSON', :aggregate_failures do
+          subject
+
+          expect(json_response['id']).not_to be_nil
+          expect(json_response['name']).to eq('New rule')
+          expect(json_response['external_url']).to eq('https://gitlab.com/test/example.json')
+          expect(json_response['protected_branches'].size).to eq(1)
+        end
+      end
+    end
+
+    context 'when feature is disabled, unlicensed or user has permission' do
+      where(:licensed, :flag, :project_owner, :status) do
+        false | false | false | :not_found
+        false | false | true  | :unauthorized
+        false | true  | true  | :unauthorized
+        false | true  | false | :not_found
+        true  | false | false | :not_found
+        true  | false | true  | :unauthorized
+        true  | true  | false | :not_found
+        true  | true  | true  | :success
+      end
+
+      with_them do
+        before do
+          stub_feature_flags(ff_compliance_approval_gates: flag)
+          stub_licensed_features(compliance_approval_gates: licensed)
+        end
+
+        it 'returns the correct status code' do
+          put api(single_object_url, (project_owner ? project.owner : build(:user))), params: attributes_for(:external_approval_rule)
+
+          expect(response).to have_gitlab_http_status(status)
+        end
+      end
+    end
+  end
 end

ee/spec/services/external_approval_rules/destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ExternalApprovalRules::DestroyService do
+  let_it_be(:project) { create(:project) }
+  let_it_be(:rule) { create(:external_approval_rule, project: project) }
+  let(:current_user) { project.owner }
+
+  subject { described_class.new(container: project, current_user: current_user).execute(rule) }
+
+  context 'when current user is project owner' do
+    it 'deletes an approval rule' do
+      expect { subject }.to change { ApprovalRules::ExternalApprovalRule.count }.by(-1)
+    end
+
+    it 'is successful' do
+      expect(subject.success?).to be true
+    end
+  end
+
+  context 'when current user is not a project owner' do
+    let_it_be(:current_user) { create(:user) }
+
+    it 'does not delete an approval rule' do
+      expect { subject }.not_to change { ApprovalRules::ExternalApprovalRule.count }
+    end
+
+    it 'is unsuccessful' do
+      expect(subject.error?).to be true
+    end
+
+    it 'returns an unauthorized status' do
+      expect(subject.http_status).to eq(:unauthorized)
+    end
+
+    it 'contains an appropriate message and error' do
+      expect(subject.message).to eq('Failed to destroy rule')
+      expect(subject.payload[:errors]).to contain_exactly('Not allowed')
+    end
+  end
+end

ee/spec/services/external_approval_rules/update_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ExternalApprovalRules::UpdateService do
+  let_it_be(:project) { create(:project) }
+  let_it_be(:rule) { create(:external_approval_rule, project: project) }
+  let_it_be(:protected_branch) { create(:protected_branch, project: project) }
+  let(:current_user) { project.owner }
+  let(:params) { { id: project.id, rule_id: rule.id, external_url: 'http://newvalue.com', name: 'new name', protected_branch_ids: [protected_branch.id] } }
+
+  subject { described_class.new(container: project, current_user: current_user, params: params).execute }
+
+  context 'when current user is project owner' do
+    it 'updates an approval rule' do
+      subject
+
+      rule.reload
+
+      expect(rule.external_url).to eq('http://newvalue.com')
+      expect(rule.name).to eq('new name')
+      expect(rule.protected_branches).to contain_exactly(protected_branch)
+    end
+
+    it 'is successful' do
+      expect(subject.success?).to be true
+    end
+  end
+
+  context 'when current user is not a project owner' do
+    let_it_be(:current_user) { create(:user) }
+
+    it 'does not change an approval rule' do
+      expect { subject }.not_to change { rule.name }
+    end
+
+    it 'is unsuccessful' do
+      expect(subject.error?).to be true
+    end
+
+    it 'returns an unauthorized status' do
+      expect(subject.http_status).to eq(:unauthorized)
+    end
+
+    it 'contains an appropriate message and error' do
+      expect(subject.message).to eq('Failed to update rule')
+      expect(subject.payload[:errors]).to contain_exactly('Not allowed')
+    end
+  end
+end