Add policy name to ScanExecutionPolicyCommit mutation

doc/api/graphql/reference/index.md

@@ -4042,6 +4042,7 @@ Input type: `ScanExecutionPolicyCommitInput`
 | Name | Type | Description |
 | ---- | ---- | ----------- |
 | <a id="mutationscanexecutionpolicycommitclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. |
+| <a id="mutationscanexecutionpolicycommitname"></a>`name` | [`String`](#string) | Name of the policy. If the name is null, the `name` field from `policy_yaml` is used. |
 | <a id="mutationscanexecutionpolicycommitoperationmode"></a>`operationMode` | [`MutationOperationMode!`](#mutationoperationmode) | Changes the operation mode. |
 | <a id="mutationscanexecutionpolicycommitpolicyyaml"></a>`policyYaml` | [`String!`](#string) | YAML snippet of the policy. |
 | <a id="mutationscanexecutionpolicycommitprojectpath"></a>`projectPath` | [`ID!`](#id) | Full path of the project. |

ee/app/graphql/mutations/security_policy/commit_scan_execution_policy.rb

@@ -24,6 +24,10 @@ module Mutations
                required: true,
                description: 'Changes the operation mode.'
 
+      argument :name, GraphQL::Types::String,
+               required: false,
+               description: 'Name of the policy. If the name is null, the `name` field from `policy_yaml` is used.'
+
       field :branch,
             GraphQL::Types::String,
             null: true,
@@ -32,7 +36,7 @@ module Mutations
       def resolve(args)
         project = authorized_find!(args[:project_path])
 
-        result = commit_policy(project, args[:policy_yaml], args[:operation_mode])
+        result = commit_policy(project, args)
         error_message = result[:status] == :error ? result[:message] : nil
 
         {
@@ -43,9 +47,13 @@ module Mutations
 
       private
 
-      def commit_policy(project, policy_yaml, operation_mode)
+      def commit_policy(project, args)
         ::Security::SecurityOrchestrationPolicies::PolicyCommitService
-          .new(project: project, current_user: current_user, params: { policy_yaml: policy_yaml, operation: Types::MutationOperationModeEnum.enum.key(operation_mode).to_sym })
+          .new(project: project, current_user: current_user, params: {
+            name: args[:name],
+            policy_yaml: args[:policy_yaml],
+            operation: Types::MutationOperationModeEnum.enum.key(args[:operation_mode]).to_sym
+          })
           .execute
       end
     end

ee/app/services/security/security_orchestration_policies/policy_commit_service.rb

@@ -23,7 +23,12 @@ module Security
         policy = Gitlab::Config::Loader::Yaml.new(params[:policy_yaml]).load!
         updated_policy = ProcessPolicyService.new(
           policy_configuration: policy_configuration,
-          params: { operation: params[:operation], policy: policy, type: policy.delete(:type)&.to_sym }
+          params: {
+            operation: params[:operation],
+            name: params[:name],
+            policy: policy,
+            type: policy.delete(:type)&.to_sym
+          }
         ).execute
 
         YAML.dump(updated_policy.deep_stringify_keys)

ee/app/services/security/security_orchestration_policies/process_policy_service.rb

@@ -11,14 +11,17 @@ module Security
       def execute
         policy = params[:policy]
         type = params[:type]
+        name = params[:name]
+        operation = params[:operation]
 
         raise StandardError, "Invalid policy type" unless Security::OrchestrationPolicyConfiguration::AVAILABLE_POLICY_TYPES.include?(type)
+        raise StandardError, "Name should be same as the policy name" if name && operation != :replace && policy[:name] != name
 
         policy_hash = policy_configuration.policy_hash.dup || {}
 
-        case params[:operation]
+        case operation
         when :append then append_to_policy_hash(policy_hash, policy, type)
-        when :replace then replace_in_policy_hash(policy_hash, policy, type)
+        when :replace then replace_in_policy_hash(policy_hash, name, policy, type)
         when :remove then remove_from_policy_hash(policy_hash, policy, type)
         end
 
@@ -35,30 +38,32 @@ module Security
           return
         end
 
-        raise StandardError, "Policy already exists with same name" if policy_exists?(policy_hash, policy, type)
+        raise StandardError, "Policy already exists with same name" if policy_exists?(policy_hash, policy[:name], type)
 
         policy_hash[type] += [policy]
       end
 
-      def replace_in_policy_hash(policy_hash, policy, type)
-        existing_policy_index = check_if_policy_exists!(policy_hash, policy, type)
+      def replace_in_policy_hash(policy_hash, name, policy, type)
+        raise StandardError, "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)
         policy_hash[type][existing_policy_index] = policy
       end
 
       def remove_from_policy_hash(policy_hash, policy, type)
-        check_if_policy_exists!(policy_hash, policy, 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, type)
-        existing_policy_index = policy_exists?(policy_hash, policy, type)
+      def check_if_policy_exists!(policy_hash, policy_name, type)
+        existing_policy_index = policy_exists?(policy_hash, policy_name, type)
         raise StandardError, "Policy does not exist" if existing_policy_index.nil?
 
         existing_policy_index
       end
 
-      def policy_exists?(policy_hash, policy, type)
-        policy_hash[type].find_index { |p| p[:name] == policy[:name] }
+      def policy_exists?(policy_hash, policy_name, type)
+        policy_hash[type].find_index { |p| p[:name] == policy_name }
       end
 
       attr_reader :policy_configuration, :params

ee/spec/graphql/mutations/security_policy/commit_scan_execution_policy_spec.rb

@@ -10,9 +10,10 @@ RSpec.describe Mutations::SecurityPolicy::CommitScanExecutionPolicy do
     let_it_be(:policy_management_project) { create(:project, :repository, namespace: user.namespace) }
     let_it_be(:policy_configuration) { create(:security_orchestration_policy_configuration, security_policy_management_project: policy_management_project, project: project) }
     let_it_be(:operation_mode) { Types::MutationOperationModeEnum.enum[:append] }
-    let_it_be(:policy_yaml) { build(:scan_execution_policy).merge(type: 'scan_execution_policy').to_yaml }
+    let_it_be(:policy_name) { 'Test Policy' }
+    let_it_be(:policy_yaml) { build(:scan_execution_policy, name: policy_name).merge(type: 'scan_execution_policy').to_yaml }
 
-    subject { mutation.resolve(project_path: project.full_path, policy_yaml: policy_yaml, operation_mode: operation_mode) }
+    subject { mutation.resolve(project_path: project.full_path, name: policy_name, policy_yaml: policy_yaml, operation_mode: operation_mode) }
 
     context 'when permission is set for user' do
       before do

ee/spec/requests/api/graphql/mutations/security_policy/commit_scan_execution_policy_spec.rb

@@ -7,10 +7,11 @@ RSpec.describe 'Create scan execution policy for a project' do
 
   let_it_be(:current_user) { create(:user) }
   let_it_be(:project) { create(:project, :repository, namespace: current_user.namespace) }
-  let_it_be(:policy_yaml) { build(:scan_execution_policy).merge(type: 'scan_execution_policy').to_yaml }
+  let_it_be(:policy_name) { 'Test Policy' }
+  let_it_be(:policy_yaml) { build(:scan_execution_policy, name: policy_name).merge(type: 'scan_execution_policy').to_yaml }
 
   def mutation
-    variables = { project_path: project.full_path, policy_yaml: policy_yaml, operation_mode: 'APPEND' }
+    variables = { project_path: project.full_path, name: policy_name, policy_yaml: policy_yaml, operation_mode: 'APPEND' }
 
     graphql_mutation(:scan_execution_policy_commit, variables) do
       <<-QL.strip_heredoc

ee/spec/services/security/security_orchestration_policies/policy_commit_service_spec.rb

@@ -3,26 +3,28 @@
 require 'spec_helper'
 
 RSpec.describe Security::SecurityOrchestrationPolicies::PolicyCommitService do
+  include RepoHelpers
+
   describe '#execute' do
     let_it_be(:project) { create(:project) }
     let_it_be(:current_user) { project.owner }
-    let_it_be(:policy_configuration) { create(:security_orchestration_policy_configuration, project: project) }
+    let_it_be(:policy_management_project) { create(:project, :repository, creator: current_user) }
+    let_it_be(:policy_configuration) { create(:security_orchestration_policy_configuration, security_policy_management_project: policy_management_project, project: project) }
 
     let(:policy_hash) { build(:scan_execution_policy, name: 'Test Policy') }
     let(:input_policy_yaml) { policy_hash.merge(type: 'scan_execution_policy').to_yaml }
     let(:policy_yaml) { build(:orchestration_policy_yaml, scan_execution_policy: [policy_hash])}
+    let(:policy_name) { policy_hash[:name] }
 
     let(:operation) { :append }
-    let(:params) { { policy_yaml: input_policy_yaml, operation: operation } }
+    let(:params) { { policy_yaml: input_policy_yaml, name: policy_name, operation: operation } }
 
     subject(:service) do
       described_class.new(project: project, current_user: current_user, params: params)
     end
 
-    before do
-      allow_next_instance_of(Repository) do |repository|
-        allow(repository).to receive(:blob_data_at).and_return(policy_yaml)
-      end
+    around do |example|
+      Timecop.scale(60) { example.run }
     end
 
     context 'when policy_yaml is invalid' do
@@ -56,11 +58,16 @@ RSpec.describe Security::SecurityOrchestrationPolicies::PolicyCommitService do
 
     context 'when policy already exists in policy project' do
       before do
-        allow_next_instance_of(::Files::UpdateService) do |instance|
-          allow(instance).to receive(:execute).and_return({ status: :success })
-        end
-
+        create_file_in_repo(
+          policy_management_project,
+          policy_management_project.default_branch_or_main,
+          policy_management_project.default_branch_or_main,
+          Security::OrchestrationPolicyConfiguration::POLICY_PATH,
+          policy_yaml
+        )
         policy_configuration.security_policy_management_project.add_developer(current_user)
+        policy_configuration.clear_memoization(:policy_hash)
+        policy_configuration.clear_memoization(:policy_blob)
       end
 
       context 'append' do
@@ -74,23 +81,33 @@ RSpec.describe Security::SecurityOrchestrationPolicies::PolicyCommitService do
 
       context 'replace' do
         let(:operation) { :replace }
+        let(:input_policy_yaml) { build(:scan_execution_policy, name: 'Updated Policy').merge(type: 'scan_execution_policy').to_yaml }
+        let(:policy_name) { 'Test Policy' }
 
-        it 'creates branch' do
+        it 'creates branch with updated policy' do
           response = service.execute
 
           expect(response[:status]).to eq(:success)
           expect(response[:branch]).not_to be_nil
+
+          updated_policy_blob = policy_management_project.repository.blob_data_at(response[:branch], Security::OrchestrationPolicyConfiguration::POLICY_PATH)
+          updated_policy_yaml = Gitlab::Config::Loader::Yaml.new(updated_policy_blob).load!
+          expect(updated_policy_yaml[:scan_execution_policy][0][:name]).to eq('Updated Policy')
         end
       end
 
       context 'remove' do
         let(:operation) { :remove }
 
-        it 'creates branch' do
+        it 'creates branch with removed policy' do
           response = service.execute
 
           expect(response[:status]).to eq(:success)
           expect(response[:branch]).not_to be_nil
+
+          updated_policy_blob = policy_management_project.repository.blob_data_at(response[:branch], Security::OrchestrationPolicyConfiguration::POLICY_PATH)
+          updated_policy_yaml = Gitlab::Config::Loader::Yaml.new(updated_policy_blob).load!
+          expect(updated_policy_yaml[:scan_execution_policy]).to be_empty
         end
       end
     end

ee/spec/services/security/security_orchestration_policies/process_policy_service_spec.rb

@@ -11,6 +11,7 @@ RSpec.describe Security::SecurityOrchestrationPolicies::ProcessPolicyService do
     let(:policy_yaml) { Gitlab::Config::Loader::Yaml.new(policy.to_yaml).load! }
     let(:type) { :scan_execution_policy }
     let(:operation) { :append }
+    let(:policy_name) { policy[:name] }
 
     let(:repository_with_existing_policy_yaml) do
       pipeline_policy = build(:scan_execution_policy, name: 'Test Policy')
@@ -22,18 +23,27 @@ RSpec.describe Security::SecurityOrchestrationPolicies::ProcessPolicyService do
       build(:orchestration_policy_yaml, scan_execution_policy: [pipeline_policy, scheduled_policy])
     end
 
-    subject(:service) { described_class.new(policy_configuration: policy_configuration, params: { policy: policy_yaml, operation: operation, type: type }) }
+    subject(:service) { described_class.new(policy_configuration: policy_configuration, params: { policy: policy_yaml, name: policy_name, operation: operation, type: type }) }
 
     context 'when policy is invalid' do
-      let(:policy) { { invalid_name: 'invalid' } }
+      let(:policy_name) { 'invalid' }
+      let(:policy) { { name: 'invalid', invalid_field: 'invalid' } }
 
       it 'raises StandardError' do
         expect { service.execute }.to raise_error(StandardError, 'Invalid policy yaml')
       end
     end
 
+    context 'when policy name is not same as in policy' do
+      let(:policy_name) { 'invalid' }
+
+      it 'raises StandardError' do
+        expect { service.execute }.to raise_error(StandardError, 'Name should be same as the policy name')
+      end
+    end
+
     context 'when type is invalid' do
-      let(:type) { :invalid_type}
+      let(:type) { :invalid_type }
 
       it 'raises StandardError' do
         expect { service.execute }.to raise_error(StandardError, 'Invalid policy type')
@@ -79,6 +89,10 @@ RSpec.describe Security::SecurityOrchestrationPolicies::ProcessPolicyService do
     context 'replace policy' do
       let(:operation) { :replace }
 
+      before do
+        allow(policy_configuration).to receive(:policy_hash).and_return(Gitlab::Config::Loader::Yaml.new(repository_with_existing_policy_yaml).load!)
+      end
+
       context 'when policy is not present in repository' do
         before do
           allow(policy_configuration).to receive(:policy_hash).and_return(Gitlab::Config::Loader::Yaml.new(repository_policy_yaml).load!)
@@ -89,17 +103,45 @@ RSpec.describe Security::SecurityOrchestrationPolicies::ProcessPolicyService do
         end
       end
 
-      context 'when policy with same name already exists in repository' do
-        before do
-          allow(policy_configuration).to receive(:policy_hash).and_return(Gitlab::Config::Loader::Yaml.new(repository_with_existing_policy_yaml).load!)
+      context 'when policy name is empty' do
+        let(:policy_name) { nil }
+
+        it 'does not modify the policy name' do
+          result = service.execute
+
+          expect(result[:scan_execution_policy].first).to eq(policy_yaml)
         end
+      end
 
+      context 'when policy with same name already exists in repository' do
         it 'replaces the policy' do
           result = service.execute
 
           expect(result[:scan_execution_policy].first[:enabled]).to be_falsey
         end
       end
+
+      context 'when policy name is not same as in policy' do
+        let(:policy_yaml) do
+          Gitlab::Config::Loader::Yaml.new(build(:scan_execution_policy, name: 'Updated Policy', enabled: false).to_yaml).load!
+        end
+
+        it 'updates the policy name' do
+          result = service.execute
+
+          expect(result[:scan_execution_policy].first[:name]).to eq('Updated Policy')
+        end
+      end
+
+      context 'when name of the policy to be updated already exists' do
+        let(:policy_yaml) do
+          Gitlab::Config::Loader::Yaml.new(build(:scan_execution_policy, name: 'Scheduled DAST', enabled: false).to_yaml).load!
+        end
+
+        it 'raises StandardError' do
+          expect { service.execute }.to raise_error(StandardError, 'Policy already exists with same name')
+        end
+      end
     end
 
     context 'remove policy' do