Merge branch '341418-add-pretty-error-messages-from-json-schema' into 'master'

Return pretty error validation messages for Security Policy

See merge request gitlab-org/gitlab!78916

ee/app/assets/javascripts/threat_monitoring/components/policy_editor/policy_editor.vue

@@ -34,6 +34,7 @@ export default {
   data() {
     return {
       error: '',
+      errorMessages: [],
       newPolicyType: POLICY_TYPE_COMPONENT_OPTIONS.container.value,
     };
   },
@@ -77,8 +78,8 @@ export default {
     },
   },
   methods: {
-    setError(error) {
-      this.error = error;
+    setError(errors) {
+      [this.error, ...this.errorMessages] = errors.split('\n');
     },
     handleNewPolicyType(type) {
       this.newPolicyType = type;
@@ -92,8 +93,18 @@ export default {
 
 <template>
   <section class="policy-editor">
-    <gl-alert v-if="error" dissmissable="true" variant="danger" @dismiss="setError('')">
-      {{ error }}
+    <gl-alert
+      v-if="error"
+      :title="error"
+      dissmissable="true"
+      variant="danger"
+      @dismiss="setError('')"
+    >
+      <ul v-if="errorMessages.length" class="gl-mb-0! gl-ml-5">
+        <li v-for="errorMessage in errorMessages" :key="errorMessage">
+          {{ errorMessage }}
+        </li>
+      </ul>
     </gl-alert>
     <header class="gl-pb-5">
       <h3>{{ $options.i18n.title }}</h3>

ee/app/assets/javascripts/threat_monitoring/components/policy_editor/utils.js

@@ -10,7 +10,7 @@ import { DEFAULT_MR_TITLE, SECURITY_POLICY_ACTIONS } from './constants';
  */
 const checkForErrors = ({ errors }) => {
   if (errors?.length) {
-    throw new Error(errors);
+    throw new Error(errors.join('\n'));
   }
 };
 

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

@@ -38,10 +38,11 @@ module Mutations
 
         result = commit_policy(project, args)
         error_message = result[:status] == :error ? result[:message] : nil
+        error_details = result[:status] == :error ? result[:details] : nil
 
         {
           branch: result[:branch],
-          errors: [error_message].compact
+          errors: [error_message, *error_details].compact
         }
       end
 

ee/app/models/security/orchestration_policy_configuration.rb

@@ -11,6 +11,7 @@ module Security
 
     POLICY_PATH = '.gitlab/security-policies/policy.yml'
     POLICY_SCHEMA_PATH = 'ee/app/validators/json_schemas/security_orchestration_policy.json'
+    POLICY_SCHEMA = JSONSchemer.schema(Rails.root.join(POLICY_SCHEMA_PATH))
     AVAILABLE_POLICY_TYPES = %i{scan_execution_policy scan_result_policy}.freeze
 
     belongs_to :project, inverse_of: :security_orchestration_policy_configuration
@@ -44,9 +45,13 @@ module Security
     end
 
     def policy_configuration_valid?(policy = policy_hash)
-      JSONSchemer
-        .schema(Rails.root.join(POLICY_SCHEMA_PATH))
-        .valid?(policy.to_h.deep_stringify_keys)
+      POLICY_SCHEMA.valid?(policy.to_h.deep_stringify_keys)
+    end
+
+    def policy_configuration_validation_errors(policy = policy_hash)
+      POLICY_SCHEMA
+        .validate(policy.to_h.deep_stringify_keys)
+        .map { |error| JSONSchemer::Errors.pretty(error) }
     end
 
     def policy_last_updated_by

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

@@ -8,8 +8,10 @@ module Security
 
         return error('Security Policy Project does not exist') unless policy_configuration.present?
 
-        result = commit_policy(process_policy_yaml)
+        process_policy_result = process_policy
+        return process_policy_result if process_policy_result[:status] != :success
 
+        result = commit_policy(process_policy_result[:policy_hash])
         return error(result[:message], :bad_request) if result[:status] != :success
 
         success({ branch: branch_name })
@@ -19,9 +21,8 @@ module Security
 
       private
 
-      def process_policy_yaml
-        policy = Gitlab::Config::Loader::Yaml.new(params[:policy_yaml]).load!
-        updated_policy = ProcessPolicyService.new(
+      def process_policy
+        ProcessPolicyService.new(
           policy_configuration: policy_configuration,
           params: {
             operation: params[:operation],
@@ -30,11 +31,11 @@ module Security
             type: policy.delete(:type)&.to_sym
           }
         ).execute
-
-        YAML.dump(updated_policy.deep_stringify_keys)
       end
 
-      def commit_policy(policy_yaml)
+      def commit_policy(policy_hash)
+        policy_yaml = YAML.dump(policy_hash.deep_stringify_keys)
+
         return create_commit(::Files::UpdateService, policy_yaml) if policy_configuration.policy_configuration_exists?
 
         create_commit(::Files::CreateService, policy_yaml)
@@ -68,6 +69,10 @@ module Security
         @branch_name ||= "update-policy-#{Time.now.to_i}"
       end
 
+      def policy
+        @policy ||= Gitlab::Config::Loader::Yaml.new(params[:policy_yaml]).load!
+      end
+
       attr_reader :project, :policy_configuration
     end
   end

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

@@ -3,6 +3,8 @@
 module Security
   module SecurityOrchestrationPolicies
     class ProcessPolicyService
+      include BaseServiceUtility
+
       def initialize(policy_configuration:, params:)
         @policy_configuration = policy_configuration
         @params = params
@@ -14,8 +16,8 @@ module Security
         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
+        return error("Invalid policy type", :bad_request) unless Security::OrchestrationPolicyConfiguration::AVAILABLE_POLICY_TYPES.include?(type)
+        return error("Name should be same as the policy name", :bad_request) if name && operation != :replace && policy[:name] != name
 
         policy_hash = policy_configuration.policy_hash.dup || {}
 
@@ -25,13 +27,17 @@ module Security
         when :remove then remove_from_policy_hash(policy_hash, policy, type)
         end
 
-        raise StandardError, "Invalid policy yaml" unless policy_configuration.policy_configuration_valid?(policy_hash)
+        return error('Invalid policy YAML', :bad_request, pass_back: { details: policy_configuration_validation_errors(policy_hash) }) unless policy_configuration_valid?(policy_hash)
 
-        policy_hash
+        success(policy_hash: policy_hash)
+      rescue StandardError => e
+        error(e.message)
       end
 
       private
 
+      delegate :policy_configuration_validation_errors, :policy_configuration_valid?, to: :policy_configuration
+
       def append_to_policy_hash(policy_hash, policy, type)
         if policy_hash[type].blank?
           policy_hash[type] = [policy]

ee/spec/frontend/threat_monitoring/components/policy_editor/policy_editor_spec.js

@@ -70,7 +70,17 @@ describe('PolicyEditor component', () => {
       await nextTick();
       const alert = findAlert();
       expect(alert.exists()).toBe(true);
-      expect(alert.text()).toBe(errorMessage);
+      expect(alert.props('title')).toBe(errorMessage);
+    });
+
+    it('shows an alert with details when multiline "error" is emitted from the component', async () => {
+      const errorMessages = 'title\ndetail1';
+      findNeworkPolicyEditor().vm.$emit('error', errorMessages);
+      await nextTick();
+      const alert = findAlert();
+      expect(alert.exists()).toBe(true);
+      expect(alert.props('title')).toBe('title');
+      expect(alert.text()).toBe('detail1');
     });
 
     it.each`

ee/spec/models/security/orchestration_policy_configuration_spec.rb

@@ -180,6 +180,46 @@ RSpec.describe Security::OrchestrationPolicyConfiguration do
     end
   end
 
+  describe '#policy_configuration_validation_errors' do
+    subject { security_orchestration_policy_configuration.policy_configuration_validation_errors }
+
+    context 'when file is invalid' do
+      let(:policy_yaml) do
+        build(:orchestration_policy_yaml, scan_execution_policy:
+        [build(:scan_execution_policy, rules: [{ type: 'pipeline', branches: 'production' }])])
+      end
+
+      it { is_expected.to eq(["property '/scan_execution_policy/0/rules/0/branches' is not of type: array"]) }
+    end
+
+    context 'when file is valid' do
+      it { is_expected.to eq([]) }
+    end
+
+    context 'when policy is passed as argument' do
+      let_it_be(:policy_yaml) { nil }
+      let_it_be(:policy) { { scan_execution_policy: [build(:scan_execution_policy)] } }
+
+      context 'when scan type is secret_detection' do
+        it 'returns false if extra fields are present' do
+          invalid_policy = policy.deep_dup
+          invalid_policy[:scan_execution_policy][0][:actions][0][:scan] = 'secret_detection'
+
+          expect(security_orchestration_policy_configuration.policy_configuration_validation_errors(invalid_policy)).to eq(
+            ["property '/scan_execution_policy/0/actions/0' is invalid: error_type=maxProperties"]
+          )
+        end
+
+        it 'returns true if extra fields are not present' do
+          valid_policy = policy.deep_dup
+          valid_policy[:scan_execution_policy][0][:actions][0] = { scan: 'secret_detection' }
+
+          expect(security_orchestration_policy_configuration.policy_configuration_validation_errors(valid_policy)).to eq([])
+        end
+      end
+    end
+  end
+
   describe '#active_scan_execution_policies' do
     let(:enforce_dast_yaml) { build(:orchestration_policy_yaml, scan_execution_policy: [build(:scan_execution_policy)]) }
     let(:policy_yaml) { fixture_file('security_orchestration.yml', dir: 'ee') }

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

@@ -46,5 +46,15 @@ RSpec.describe 'Create scan execution policy for a project' do
       expect(branch).not_to be_nil
       expect(commit.message).to eq('Add a new policy to .gitlab/security-policies/policy.yml')
     end
+
+    context 'when provided policy is invalid' do
+      let_it_be(:policy_yaml) { build(:scan_execution_policy, name: policy_name).merge(type: 'scan_execution_policy', rules: [{ type: 'invalid_type' }]).to_yaml }
+
+      it 'returns error with detailed information' do
+        post_graphql_mutation(mutation, current_user: current_user)
+
+        expect(mutation_response['errors']).to eq(['Invalid policy YAML', "property '/scan_execution_policy/0/rules/0/type' is not one of: [\"pipeline\", \"schedule\"]"])
+      end
+    end
   end
 end

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

@@ -41,7 +41,8 @@ RSpec.describe Security::SecurityOrchestrationPolicies::PolicyCommitService do
         response = service.execute
 
         expect(response[:status]).to eq(:error)
-        expect(response[:message]).to eq("Invalid policy yaml")
+        expect(response[:message]).to eq("Invalid policy YAML")
+        expect(response[:details]).to eq(["property '/scan_execution_policy/0' is missing required keys: name, enabled, rules, actions"])
       end
     end
 

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

@@ -29,24 +29,34 @@ RSpec.describe Security::SecurityOrchestrationPolicies::ProcessPolicyService do
       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')
+      it 'returns error' do
+        result = service.execute
+
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to eq('Invalid policy YAML')
+        expect(result[:details]).to eq(["property '/scan_execution_policy/0' is missing required keys: enabled, rules, actions"])
       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')
+      it 'returns error' do
+        result = service.execute
+
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to eq('Name should be same as the policy name')
       end
     end
 
     context 'when type is invalid' do
       let(:type) { :invalid_type }
 
-      it 'raises StandardError' do
-        expect { service.execute }.to raise_error(StandardError, 'Invalid policy type')
+      it 'returns error' do
+        result = service.execute
+
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to eq('Invalid policy type')
       end
     end
 
@@ -59,7 +69,8 @@ RSpec.describe Security::SecurityOrchestrationPolicies::ProcessPolicyService do
         it 'appends the new policy' do
           result = service.execute
 
-          expect(result[:scan_execution_policy].count).to eq(3)
+          expect(result[:status]).to eq(:success)
+          expect(result.dig(:policy_hash, :scan_execution_policy).count).to eq(3)
         end
       end
 
@@ -68,8 +79,11 @@ RSpec.describe Security::SecurityOrchestrationPolicies::ProcessPolicyService do
           allow(policy_configuration).to receive(:policy_hash).and_return(Gitlab::Config::Loader::Yaml.new(repository_with_existing_policy_yaml).load!)
         end
 
-        it 'raises StandardError' do
-          expect { service.execute }.to raise_error(StandardError, 'Policy already exists with same name')
+        it 'returns error' do
+          result = service.execute
+
+          expect(result[:status]).to eq(:error)
+          expect(result[:message]).to eq('Policy already exists with same name')
         end
       end
 
@@ -81,7 +95,8 @@ RSpec.describe Security::SecurityOrchestrationPolicies::ProcessPolicyService do
         it 'appends the new policy' do
           result = service.execute
 
-          expect(result[:scan_execution_policy].count).to eq(1)
+          expect(result[:status]).to eq(:success)
+          expect(result.dig(:policy_hash, :scan_execution_policy).count).to eq(1)
         end
       end
     end
@@ -98,8 +113,11 @@ RSpec.describe Security::SecurityOrchestrationPolicies::ProcessPolicyService do
           allow(policy_configuration).to receive(:policy_hash).and_return(Gitlab::Config::Loader::Yaml.new(repository_policy_yaml).load!)
         end
 
-        it 'raises StandardError' do
-          expect { service.execute }.to raise_error(StandardError, 'Policy does not exist')
+        it 'returns error' do
+          result = service.execute
+
+          expect(result[:status]).to eq(:error)
+          expect(result[:message]).to eq('Policy does not exist')
         end
       end
 
@@ -109,7 +127,7 @@ RSpec.describe Security::SecurityOrchestrationPolicies::ProcessPolicyService do
         it 'does not modify the policy name' do
           result = service.execute
 
-          expect(result[:scan_execution_policy].first).to eq(policy_yaml)
+          expect(result.dig(:policy_hash, :scan_execution_policy).first).to eq(policy_yaml)
         end
       end
 
@@ -117,7 +135,7 @@ RSpec.describe Security::SecurityOrchestrationPolicies::ProcessPolicyService do
         it 'replaces the policy' do
           result = service.execute
 
-          expect(result[:scan_execution_policy].first[:enabled]).to be_falsey
+          expect(result.dig(:policy_hash, :scan_execution_policy).first[:enabled]).to be_falsey
         end
       end
 
@@ -129,7 +147,7 @@ RSpec.describe Security::SecurityOrchestrationPolicies::ProcessPolicyService do
         it 'updates the policy name' do
           result = service.execute
 
-          expect(result[:scan_execution_policy].first[:name]).to eq('Updated Policy')
+          expect(result.dig(:policy_hash, :scan_execution_policy).first[:name]).to eq('Updated Policy')
         end
       end
 
@@ -138,8 +156,11 @@ RSpec.describe Security::SecurityOrchestrationPolicies::ProcessPolicyService 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')
+        it 'returns error' do
+          result = service.execute
+
+          expect(result[:status]).to eq(:error)
+          expect(result[:message]).to eq('Policy already exists with same name')
         end
       end
     end
@@ -152,8 +173,11 @@ RSpec.describe Security::SecurityOrchestrationPolicies::ProcessPolicyService do
           allow(policy_configuration).to receive(:policy_hash).and_return(Gitlab::Config::Loader::Yaml.new(repository_policy_yaml).load!)
         end
 
-        it 'raises StandardError' do
-          expect { service.execute }.to raise_error(StandardError, 'Policy does not exist')
+        it 'returns error' do
+          result = service.execute
+
+          expect(result[:status]).to eq(:error)
+          expect(result[:message]).to eq('Policy does not exist')
         end
       end
 
@@ -165,7 +189,8 @@ RSpec.describe Security::SecurityOrchestrationPolicies::ProcessPolicyService do
         it 'removes the policy' do
           result = service.execute
 
-          expect(result[:scan_execution_policy].count).to eq(1)
+          expect(result[:status]).to eq(:success)
+          expect(result.dig(:policy_hash, :scan_execution_policy).count).to eq(1)
         end
       end
     end