Skip to content

BE: allow for both "scan_result_policy" and "approval_policy" policy types

Why are we doing this work

We plan to move from "Scan result policy" to a new name "Merge request approval policy". Because of this, we should adapt backend to support both types until %17.0. Afterwards, only approval_policy will be supported. We should also use approval_policy in the URL.

Relevant links

  • See epic

Non-functional requirements

  • Documentation: needs to be updated

Implementation plan

  1. Update JSON schema to support the new type

    Patch 
    diff --git a/ee/app/validators/json_schemas/security_orchestration_policy.json b/ee/app/validators/json_schemas/security_orchestration_policy.json
index 478660895a59..d5cf893a26fe 100644
--- a/ee/app/validators/json_schemas/security_orchestration_policy.json
+++ b/ee/app/validators/json_schemas/security_orchestration_policy.json
@@ -13,6 +13,11 @@
       "required": [
         "scan_result_policy"
       ]
+    },
+    {
+      "required": [
+        "approval_policy"
+      ]
     }
   ],
   "properties": {
@@ -389,6 +394,15 @@
       }
     },
     "scan_result_policy": {
+      "$ref": "#/$defs/approval_policy"
+    },
+    "approval_policy": {
+      "$ref": "#/$defs/approval_policy"
+    }
+  },
+  "additionalProperties": false,
+  "$defs": {
+    "approval_policy": {
       "type": "array",
       "description": "Declares actions to be enforced based on scan results.",
       "additionalItems": false,
@@ -907,6 +921,5 @@
         "additionalProperties": false
       }
     }
-  },
-  "additionalProperties": false
+  }
 }
diff --git a/ee/spec/models/security/orchestration_policy_configuration_spec.rb b/ee/spec/models/security/orchestration_policy_configuration_spec.rb
index fdba3a9d2bab..06753a236337 100644
--- a/ee/spec/models/security/orchestration_policy_configuration_spec.rb
+++ b/ee/spec/models/security/orchestration_policy_configuration_spec.rb
@@ -461,7 +461,8 @@

       specify do
         expect(errors).to contain_exactly("root is missing required keys: scan_execution_policy",
-          "root is missing required keys: scan_result_policy")
+          "root is missing required keys: scan_result_policy",
+          "root is missing required keys: approval_policy")
       end
     end

@@ -738,7 +739,7 @@
       it_behaves_like "policy_scope"
     end

-    describe "scan result policies" do
+    shared_examples_for "scan result policy validations" do |type|
       let(:scan_execution_policy) { nil }
       let(:scan_result_policy) { build(:scan_result_policy, rules: rules, actions: actions, policy_scope: policy_scope) }
       let(:rules) { [rule].compact }
@@ -754,7 +755,7 @@
             end

             specify do
-              expect(errors).to include("property '/scan_result_policy/0' is missing required keys: #{key}")
+              expect(errors).to include("property '/#{type}/0' is missing required keys: #{key}")
             end
           end
         end
@@ -767,7 +768,7 @@

             specify do
               expect(errors).to contain_exactly(
-                "property '/scan_result_policy/0/rules/0' is missing required keys: #{key}")
+                "property '/#{type}/0/rules/0' is missing required keys: #{key}")
             end
           end
         end
@@ -779,7 +780,7 @@
             end

             specify do
-              expect(errors).to contain_exactly("property '/scan_result_policy/0/name' is invalid: error_type=minLength")
+              expect(errors).to contain_exactly("property '/#{type}/0/name' is invalid: error_type=minLength")
             end
           end

@@ -789,7 +790,7 @@
             end

             specify do
-              expect(errors).to contain_exactly("property '/scan_result_policy/0/name' is invalid: error_type=maxLength")
+              expect(errors).to contain_exactly("property '/#{type}/0/name' is invalid: error_type=maxLength")
             end
           end
         end
@@ -802,7 +803,7 @@

             specify do
               expect(errors.count).to be(1)
-              expect(errors.first).to match("property '/scan_result_policy/0/rules/0/type' is not one of")
+              expect(errors.first).to match("property '/#{type}/0/rules/0/type' is not one of")
             end
           end
         end
@@ -877,7 +878,7 @@

             specify do
               expect(errors).to include(
-                "property '/scan_result_policy/0/actions/0/approvals_required' is invalid: error_type=maximum")
+                "property '/#{type}/0/actions/0/approvals_required' is invalid: error_type=maximum")
             end
           end

@@ -901,7 +902,7 @@

               specify do
                 expect(errors).to contain_exactly(
-                  "property '/scan_result_policy/0/actions/0/user_approvers' is invalid: error_type=minItems")
+                  "property '/#{type}/0/actions/0/user_approvers' is invalid: error_type=minItems")
               end
             end
           end
@@ -920,7 +921,7 @@

               specify do
                 expect(errors).to contain_exactly(
-                  "property '/scan_result_policy/0/actions/0/user_approvers_ids' is invalid: error_type=minItems")
+                  "property '/#{type}/0/actions/0/user_approvers_ids' is invalid: error_type=minItems")
               end
             end
           end
@@ -939,7 +940,7 @@

               specify do
                 expect(errors).to contain_exactly(
-                  "property '/scan_result_policy/0/actions/0/group_approvers' is invalid: error_type=minItems")
+                  "property '/#{type}/0/actions/0/group_approvers' is invalid: error_type=minItems")
               end
             end
           end
@@ -958,7 +959,7 @@

               specify do
                 expect(errors).to contain_exactly(
-                  "property '/scan_result_policy/0/actions/0/group_approvers_ids' is invalid: error_type=minItems")
+                  "property '/#{type}/0/actions/0/group_approvers_ids' is invalid: error_type=minItems")
               end
             end
           end
@@ -977,7 +978,7 @@

               specify do
                 expect(errors.count).to be(1)
-                expect(errors.first).to match("property '/scan_result_policy/0/actions/0/role_approvers/0' is not one of")
+                expect(errors.first).to match("property '/#{type}/0/actions/0/role_approvers/0' is not one of")
               end
             end
           end
@@ -992,8 +993,8 @@
           end

           specify do
-            expect(errors).to contain_exactly("property '/scan_result_policy/0' is missing required keys: actions",
-              "property '/scan_result_policy/0' is missing required keys: approval_settings")
+            expect(errors).to contain_exactly("property '/#{type}/0' is missing required keys: actions",
+              "property '/#{type}/0' is missing required keys: approval_settings")
           end
         end

@@ -1054,7 +1055,7 @@
             end

             specify do
-              expect(errors).to contain_exactly("property '/scan_result_policy/0/rules/0' is invalid: error_type=oneOf")
+              expect(errors).to contain_exactly("property '/#{type}/0/rules/0' is invalid: error_type=oneOf")
             end
           end
         end
@@ -1073,7 +1074,7 @@
             end

             specify do
-              expect(errors).to contain_exactly("property '/scan_result_policy/0/rules/0' is invalid: error_type=oneOf")
+              expect(errors).to contain_exactly("property '/#{type}/0/rules/0' is invalid: error_type=oneOf")
             end
           end
         end
@@ -1086,8 +1087,8 @@

           specify do
             expect(errors).to contain_exactly(
-              "property '/scan_result_policy/0/rules/0' is missing required keys: branch_type",
-              "property '/scan_result_policy/0/rules/0' is missing required keys: branches")
+              "property '/#{type}/0/rules/0' is missing required keys: branch_type",
+              "property '/#{type}/0/rules/0' is missing required keys: branches")
           end
         end
       end
@@ -1117,7 +1118,7 @@

           specify do
             expect(errors).to contain_exactly(
-              "property '/scan_result_policy/0/rules/0/scanners/0' is invalid: error_type=minLength")
+              "property '/#{type}/0/rules/0/scanners/0' is invalid: error_type=minLength")
           end
         end

@@ -1128,7 +1129,7 @@

           specify do
             expect(errors.count).to be(1)
-            expect(errors.first).to match("property '/scan_result_policy/0/rules/0/severity_levels/0' is not one of")
+            expect(errors.first).to match("property '/#{type}/0/rules/0/severity_levels/0' is not one of")
           end
         end

@@ -1140,7 +1141,7 @@
           specify do
             expect(errors.count).to be(1)
             expect(errors.first).to match(
-              "property '/scan_result_policy/0/rules/0/vulnerability_states/0' is not one of")
+              "property '/#{type}/0/rules/0/vulnerability_states/0' is not one of")
           end
         end

@@ -1168,7 +1169,7 @@
               specify do
                 expect(errors.count).to eq(1)
                 expect(errors.first).to(
-                  match "property '/scan_result_policy/0/rules/0/vulnerability_age' is missing required keys: #{key}"
+                  match "property '/#{type}/0/rules/0/vulnerability_age' is missing required keys: #{key}"
                 )
               end
             end
@@ -1180,7 +1181,7 @@
             specify do
               expect(errors.count).to eq(1)
               expect(errors.first).to(
-                match "property '/scan_result_policy/0/rules/0/vulnerability_age/additional' is invalid"
+                match "property '/#{type}/0/rules/0/vulnerability_age/additional' is invalid"
               )
             end
           end
@@ -1210,7 +1211,7 @@

           specify do
             expect(errors).to contain_exactly(
-              "property '/scan_result_policy/0/rules/0/license_types/0' is invalid: error_type=minLength")
+              "property '/#{type}/0/rules/0/license_types/0' is invalid: error_type=minLength")
           end
         end

@@ -1222,7 +1223,7 @@

             specify do
               expect(errors).to contain_exactly(
-                "property '/scan_result_policy/0/rules/0/license_states' is invalid: error_type=minItems")
+                "property '/#{type}/0/rules/0/license_states' is invalid: error_type=minItems")
             end
           end

@@ -1234,7 +1235,7 @@
             specify do
               expect(errors.count).to be(1)
               expect(errors.first).to match(
-                "property '/scan_result_policy/0/rules/0/license_states/0' is not one of")
+                "property '/#{type}/0/rules/0/license_states/0' is not one of")
             end
           end
         end
@@ -1261,12 +1262,26 @@

           specify do
             expect(errors).to contain_exactly(
-              "property '/scan_result_policy/0/rules/0/commits' is not one of: [\"any\", \"unsigned\"]")
+              "property '/#{type}/0/rules/0/commits' is not one of: [\"any\", \"unsigned\"]")
           end
         end
       end
     end

+    describe "scan result policies" do
+      it_behaves_like 'scan result policy validations', 'scan_result_policy'
+    end
+
+    describe "approval policies" do
+      it_behaves_like 'scan result policy validations', 'approval_policy' do
+        let(:policy_yaml) do
+          {
+            approval_policy: [scan_result_policy].compact
+          }
+        end
+      end
+    end
+
     context 'when file is valid' do
       it { is_expected.to eq([]) }
     end

  2. Update processing to support both policy types

    Patch 
    diff --git a/ee/app/graphql/resolvers/concerns/construct_security_policies.rb b/ee/app/graphql/resolvers/concerns/construct_security_policies.rb
index 26640ca3c163..e144ccb0be86 100644
--- a/ee/app/graphql/resolvers/concerns/construct_security_policies.rb
+++ b/ee/app/graphql/resolvers/concerns/construct_security_policies.rb
@@ -29,7 +29,7 @@ def construct_scan_result_policies(policies)
       {
         name: policy[:name],
         description: policy[:description],
-        edit_path: edit_path(policy, :scan_result_policy),
+        edit_path: edit_path(policy, :approval_policy),
         enabled: policy[:enabled],
         yaml: YAML.dump(policy.slice(*POLICY_YAML_ATTRIBUTES).deep_stringify_keys),
         updated_at: policy[:config].policy_last_updated_at,
diff --git a/ee/app/models/concerns/security/scan_result_policy.rb b/ee/app/models/concerns/security/scan_result_policy.rb
index d1abd4d42ef8..664b0037ddef 100644
--- a/ee/app/models/concerns/security/scan_result_policy.rb
+++ b/ee/app/models/concerns/security/scan_result_policy.rb
@@ -13,6 +13,7 @@ module ScanResultPolicy
     LICENSE_SCANNING = 'license_scanning'
     LICENSE_FINDING = 'license_finding'
     ANY_MERGE_REQUEST = 'any_merge_request'
+    SCAN_RESULT_POLICY_TYPES = %i[scan_result_policy approval_policy].freeze

     REQUIRE_APPROVAL = 'require_approval'

@@ -75,7 +76,7 @@ def applicable_scan_result_policies_for_project(project)
       end

       def scan_result_policies
-        policy_by_type(:scan_result_policy)
+        policy_by_type(SCAN_RESULT_POLICY_TYPES)
       end

       def delete_in_batches(relation)
diff --git a/ee/app/models/security/orchestration_policy_configuration.rb b/ee/app/models/security/orchestration_policy_configuration.rb
index 13073948438a..cfc2a3f202b1 100644
--- a/ee/app/models/security/orchestration_policy_configuration.rb
+++ b/ee/app/models/security/orchestration_policy_configuration.rb
@@ -20,7 +20,7 @@ class OrchestrationPolicyConfiguration < ApplicationRecord
     # json_schemer computes an $id fallback property for schemas lacking one.
     # But this schema is kept anonymous on purpose, so the $id is stripped.
     POLICY_SCHEMA_JSON = POLICY_SCHEMA.as_json['root'].except('$id')
-    AVAILABLE_POLICY_TYPES = %i[scan_execution_policy scan_result_policy].freeze
+    AVAILABLE_POLICY_TYPES = (%i[scan_execution_policy] + Security::ScanResultPolicy::SCAN_RESULT_POLICY_TYPES).freeze
     JSON_SCHEMA_VALIDATION_TIMEOUT = 5.seconds

     belongs_to :project, inverse_of: :security_orchestration_policy_configuration, optional: true
@@ -94,10 +94,14 @@ def policy_last_updated_at
       end
     end

-    def policy_by_type(type)
+    def policy_by_type(type_or_types)
       return [] if policy_hash.blank?

-      policy_hash.fetch(type, [])
+      if type_or_types.is_a?(Array)
+        policy_hash.values_at(*type_or_types).flatten.compact || []
+      else
+        policy_hash.fetch(type_or_types, [])
+      end
     end

     def default_branch_or_main
diff --git a/ee/app/services/security/security_orchestration_policies/fetch_policy_service.rb b/ee/app/services/security/security_orchestration_policies/fetch_policy_service.rb
index dc530824f4d6..933183040944 100644
--- a/ee/app/services/security/security_orchestration_policies/fetch_policy_service.rb
+++ b/ee/app/services/security/security_orchestration_policies/fetch_policy_service.rb
@@ -8,7 +8,11 @@ class FetchPolicyService
       def initialize(policy_configuration:, name:, type:)
         @policy_configuration = policy_configuration
         @name = name
-        @type = type
+        @type = if Security::ScanResultPolicy::SCAN_RESULT_POLICY_TYPES.include?(type)
+                  Security::ScanResultPolicy::SCAN_RESULT_POLICY_TYPES
+                else
+                  type
+                end
       end

       def execute
diff --git a/ee/app/services/security/security_orchestration_policies/process_policy_service.rb b/ee/app/services/security/security_orchestration_policies/process_policy_service.rb
index a604b57da776..ad02a5978896 100644
--- a/ee/app/services/security/security_orchestration_policies/process_policy_service.rb
+++ b/ee/app/services/security/security_orchestration_policies/process_policy_service.rb
@@ -53,20 +53,33 @@ def append_to_policy_hash(policy_hash, policy, type)
       def replace_in_policy_hash(policy_hash, name, policy, type)
         raise Error, "Policy already exists with same name" if name && name != policy[:name] && policy_exists?(policy_hash, policy[:name], type)

-        existing_policy_index = check_if_policy_exists!(policy_hash, name || policy[:name], type)
+        existing_policy_index, type = check_if_policy_exists!(policy_hash, name || policy[:name], type)
         policy_hash[type][existing_policy_index] = policy
       end

       def remove_from_policy_hash(policy_hash, policy, type)
-        check_if_policy_exists!(policy_hash, policy[:name], type)
+        _index, type = check_if_policy_exists!(policy_hash, policy[:name], type)
         policy_hash[type].reject! { |p| p[:name] == policy[:name] }
       end

       def check_if_policy_exists!(policy_hash, policy_name, type)
-        existing_policy_index = policy_exists?(policy_hash, policy_name, type)
+        if type == :scan_execution_policy
+          existing_policy_index = policy_exists?(policy_hash, policy_name, type)
+        else
+          existing_policy_index, type = approval_policy_exists?(policy_hash, policy_name)
+        end
+
         raise Error, "Policy does not exist" if existing_policy_index.nil?

-        existing_policy_index
+        [existing_policy_index, type]
+      end
+
+      def approval_policy_exists?(policy_hash, policy_name)
+        existing_policy_index_scan_result = policy_exists?(policy_hash, policy_name, :scan_result_policy)
+        return [existing_policy_index_scan_result, :scan_result_policy] if existing_policy_index_scan_result.present?
+
+        existing_policy_index_approval = policy_exists?(policy_hash, policy_name, :approval_policy)
+        [existing_policy_index_approval, :approval_policy] if existing_policy_index_approval.present?
       end

       def policy_exists?(policy_hash, policy_name, type)

Verification steps

  1. Visit Secure -> Policies, create a new Merge request approval policy
  2. Use the following YAML. Notice type: approval_policy:
type: approval_policy
name: Test approval_policy
description: ''
enabled: true
rules:
  - type: any_merge_request
    branch_type: protected
    commits: any
  - type: scan_finding
    scanners: []
    vulnerabilities_allowed: 0
    severity_levels: []
    vulnerability_states: []
    branch_type: protected
actions:
  - type: require_approval
    approvals_required: 1
    role_approvers:
      - developer
  1. Configure with merge request and merge
  2. Go back to the policies list and verify that the new policy is visible

Update of approval_policy

  1. Edit the policy and verify it is loaded and type: approval_policy is visible in the YAML preview
  2. Update the description and click Configure with merge request
  3. Verify that the only change is in the description attribute and merge
  4. Go back to the policies list and verify that the new description has been persisted

Update of scan_result_policy

  1. Create a policy with the previous scan_result_policy type by specifying it manually in the .yaml mode
  2. Configure with merge request and merge
  3. Edit the policy and verify that it is loaded and type: approval_policy is shown
  4. Configure with merge request and merge
  5. Go to the security policy project, inspect .gitlab/security-policies/policy.yml file and verify that the policy has been migrated to type: approval_policy

Verify that the policy is enforced

  1. Update README in a new MR and verify that approvals for this policy are required
Edited by Martin Čavoj