Merge branch 'generalize_empty_array_for_all_scanners' into 'master'

Generalize empty array for all scanners

See merge request gitlab-org/gitlab!82644

ee/app/assets/javascripts/approvals/components/rule_form.vue

@@ -23,6 +23,8 @@ const DEFAULT_NAME = 'Default';
 
 export const READONLY_NAMES = [LICENSE_CHECK_NAME, VULNERABILITY_CHECK_NAME, COVERAGE_CHECK_NAME];
 
+const REPORT_TYPES_KEYS = Object.keys(REPORT_TYPES_DEFAULT);
+
 function mapServerResponseToValidationErrors(messages) {
   return Object.entries(messages).flatMap(([key, msgs]) => msgs.map((msg) => `${key} ${msg}`));
 }
@@ -73,7 +75,6 @@ export default {
       severityLevels: [],
       vulnerabilityStates: [],
       approvalVulnerabilityStatesKeys: Object.keys(APPROVAL_VULNERABILITY_STATES),
-      reportTypesKeys: Object.keys(REPORT_TYPES_DEFAULT),
       severityLevelsKeys: Object.keys(SEVERITY_LEVELS),
       ...this.getInitialData(),
     };
@@ -146,6 +147,7 @@ export default {
 
       return '';
     },
+    // A Vulnerability-Check approval rule requires at least one scanner.
     invalidScanners() {
       return this.scanners.length <= 0;
     },
@@ -242,7 +244,8 @@ export default {
         groupRecords: this.groups,
         removeHiddenGroups: this.removeHiddenGroups,
         protectedBranchIds: this.branches.map((x) => x.id),
-        scanners: this.scanners,
+        // No scanners specified in a vulnerability approval rule means all scanners will be used.
+        scanners: this.areAllScannersSelected ? [] : this.scanners,
         severityLevels: this.severityLevels,
         vulnerabilityStates: this.vulnerabilityStates,
       };
@@ -254,19 +257,19 @@ export default {
       return VULNERABILITY_CHECK_NAME === this.name;
     },
     areAllScannersSelected() {
-      return this.scanners.length === this.reportTypesKeys.length;
+      return this.scanners?.length === REPORT_TYPES_KEYS.length;
     },
     scannersText() {
       switch (this.scanners.length) {
-        case this.reportTypesKeys.length:
+        case REPORT_TYPES_KEYS.length:
           return APPROVAL_DIALOG_I18N.form.allScannersSelectedLabel;
         case 0:
           return APPROVAL_DIALOG_I18N.form.scannersSelectLabel;
         case 1:
-          return REPORT_TYPES_DEFAULT[this.scanners[0]];
+          return this.$options.REPORT_TYPES_DEFAULT[this.scanners[0]];
         default:
           return sprintf(APPROVAL_DIALOG_I18N.form.multipleSelectedLabel, {
-            firstLabel: REPORT_TYPES_DEFAULT[this.scanners[0]],
+            firstLabel: this.$options.REPORT_TYPES_DEFAULT[this.scanners[0]],
             numberOfAdditionalLabels: this.scanners.length - 1,
           });
       }
@@ -411,6 +414,10 @@ export default {
       const groups = this.initRule.groups.map((x) => ({ ...x, type: TYPE_GROUP }));
       const branches = this.initRule.protectedBranches || [];
 
+      const scanners =
+        this.initRule.scanners?.length === 0
+          ? [...REPORT_TYPES_KEYS]
+          : this.initRule.scanners || [];
       return {
         name: this.initRule.name || '',
         approvalsRequired: this.initRule.approvalsRequired || 0,
@@ -422,14 +429,14 @@ export default {
             containsHiddenGroups && !removeHiddenGroups ? [{ type: TYPE_HIDDEN_GROUPS }] : [],
           ),
         branches,
-        scanners: this.initRule.scanners || [],
+        scanners,
         vulnerabilitiesAllowed: this.initRule.vulnerabilitiesAllowed || 0,
         severityLevels: this.initRule.severityLevels || [],
         vulnerabilityStates: this.initRule.vulnerabilityStates || [],
       };
     },
     setAllSelectedScanners() {
-      this.scanners = this.areAllScannersSelected ? [] : this.reportTypesKeys;
+      this.scanners = this.areAllScannersSelected ? [] : [...REPORT_TYPES_KEYS];
     },
     isScannerSelected(scanner) {
       return this.scanners.includes(scanner);

ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/lib/humanize.js

@@ -86,6 +86,27 @@ const humanizeBranches = (branches) => {
   });
 };
 
+/**
+ * Create a human-readable version of the scanners
+ * @param {Array} scanners
+ * @returns {String}
+ */
+const humanizeScanners = (scanners) => {
+  const hasEmptyScanners = scanners.length === 0;
+
+  if (hasEmptyScanners) {
+    return s__('SecurityOrchestration|All scanners find');
+  }
+
+  return sprintf(s__('SecurityOrchestration|%{scanners}'), {
+    scanners: humanizeItems({
+      items: scanners,
+      singular: s__('SecurityOrchestration|scanner finds'),
+      plural: s__('SecurityOrchestration|scanners find'),
+    }),
+  });
+};
+
 /**
  * Create a human-readable string, adding the necessary punctuation and conjunctions
  * @param {Object} action containing or not arrays of string and integers representing approvers
@@ -168,14 +189,10 @@ export const humanizeAction = (action) => {
 const humanizeRule = (rule) => {
   return sprintf(
     s__(
-      'SecurityOrchestration|The %{scanners} %{severities} in an open merge request targeting %{branches}.',
+      'SecurityOrchestration|%{scanners} %{severities} in an open merge request targeting %{branches}.',
     ),
     {
-      scanners: humanizeItems({
-        items: convertScannersToTitleCase(rule.scanners),
-        singular: s__('SecurityOrchestration|scanner finds'),
-        plural: s__('SecurityOrchestration|scanners find'),
-      }),
+      scanners: humanizeScanners(convertScannersToTitleCase(rule.scanners)),
       severities: humanizeItems({
         items: rule.severity_levels,
         singular: s__('SecurityOrchestration|vulnerability'),

ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/lib/index.js

 export { fromYaml } from './from_yaml';
 export { toYaml } from './to_yaml';
-export { buildRule } from './rules';
+export { buildRule, invalidScanners } from './rules';
 export { approversOutOfSync } from './actions';
 export * from './humanize';
 

ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/lib/rules.js

+import { REPORT_TYPES_DEFAULT } from 'ee/security_dashboard/store/constants';
+
+const REPORT_TYPES_KEYS = Object.keys(REPORT_TYPES_DEFAULT);
+
 /*
   Construct a new rule object.
 */
@@ -11,3 +15,14 @@ export function buildRule() {
     vulnerability_states: [],
   };
 }
+
+/*
+  Check if scanners are valid for each rule.
+*/
+export function invalidScanners(rules) {
+  return (
+    rules
+      ?.flatMap((rule) => rule.scanners)
+      .some((scanner) => !REPORT_TYPES_KEYS.includes(scanner)) || false
+  );
+}

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

@@ -51,10 +51,12 @@ export default {
     },
     scannersToAdd: {
       get() {
-        return this.initRule.scanners;
+        return this.initRule.scanners.length === 0 ? this.reportTypesKeys : this.initRule.scanners;
       },
       set(values) {
-        this.triggerChanged({ scanners: values });
+        this.triggerChanged({
+          scanners: values.length === this.reportTypesKeys.length ? [] : values,
+        });
       },
     },
     vulnerabilityStates: {
@@ -107,6 +109,7 @@ export default {
             :item-type-name="$options.i18n.scanners"
             :items="$options.REPORT_TYPES_DEFAULT"
             data-testid="scanners-select"
+            :include-select-all="false"
           />
         </template>
 

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

@@ -22,7 +22,14 @@ import { assignSecurityPolicyProject, modifyPolicy } from '../utils';
 import DimDisableContainer from '../dim_disable_container.vue';
 import PolicyActionBuilder from './policy_action_builder.vue';
 import PolicyRuleBuilder from './policy_rule_builder.vue';
-import { DEFAULT_SCAN_RESULT_POLICY, fromYaml, toYaml, buildRule, approversOutOfSync } from './lib';
+import {
+  DEFAULT_SCAN_RESULT_POLICY,
+  fromYaml,
+  toYaml,
+  buildRule,
+  approversOutOfSync,
+  invalidScanners,
+} from './lib';
 
 export default {
   SECURITY_POLICY_ACTIONS,
@@ -209,7 +216,7 @@ export default {
       if (mode === EDITOR_MODE_YAML && !this.hasParsingError) {
         this.yamlEditorValue = toYaml(this.policy);
       } else if (mode === EDITOR_MODE_RULE && !this.hasParsingError) {
-        if (approversOutOfSync(this.policy.actions[0], this.existingApprovers)) {
+        if (this.invalidForRuleMode()) {
           this.yamlEditorError = new Error();
         }
       }
@@ -217,6 +224,12 @@ export default {
     updatePolicyApprovers(values) {
       this.existingApprovers = values;
     },
+    invalidForRuleMode() {
+      return (
+        approversOutOfSync(this.policy.actions[0], this.existingApprovers) ||
+        invalidScanners(this.policy.rules)
+      );
+    },
   },
 };
 </script>

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

@@ -27,6 +27,11 @@ export default {
       required: false,
       default: () => [],
     },
+    includeSelectAll: {
+      type: Boolean,
+      required: false,
+      default: () => true,
+    },
   },
   data() {
     return {
@@ -99,6 +104,7 @@ export default {
 <template>
   <gl-dropdown :text="text">
     <gl-dropdown-item
+      v-if="includeSelectAll"
       :key="$options.ALL_KEY"
       is-check-item
       :is-checked="areAllSelected"

ee/app/models/approval_project_rule.rb

@@ -34,7 +34,10 @@ class ApprovalProjectRule < ApplicationRecord
   validates :rule_type, uniqueness: { scope: :project_id, message: proc { _('any-approver for the project already exists') } }, if: :any_approver?
 
   validates :scanners, if: :scanners_changed?, inclusion: { in: SUPPORTED_SCANNERS }
-  default_value_for :scanners, allows_nil: false, value: SUPPORTED_SCANNERS
+
+  # No scanners specified in a vulnerability approval rule means all scanners will be used.
+  # Vulnerability-Check and scan result policy approval rules require at least one scanner.
+  default_value_for :scanners, allows_nil: false, value: []
 
   validates :vulnerabilities_allowed, numericality: { only_integer: true }
   default_value_for :vulnerabilities_allowed, allows_nil: false, value: 0

ee/spec/frontend/approvals/components/rule_form_spec.js

@@ -608,22 +608,6 @@ describe('EE Approvals RuleForm', () => {
     });
 
     describe(`with ${VULNERABILITY_CHECK_NAME}`, () => {
-      describe('and without any scanners selected', () => {
-        beforeEach(() => {
-          createComponent({
-            initRule: {
-              ...TEST_RULE_VULNERABILITY_CHECK,
-              scanners: [],
-            },
-          });
-          findForm().trigger('submit');
-        });
-
-        it('does not dispatch the action on submit', () => {
-          expect(actions.postRule).not.toHaveBeenCalled();
-        });
-      });
-
       describe('and with two scanners selected', () => {
         beforeEach(() => {
           createComponent({
@@ -649,6 +633,19 @@ describe('EE Approvals RuleForm', () => {
           findAllScannersSelected().trigger('click');
           findForm().trigger('submit');
         });
+
+        it('selects all scanners', () => {
+          const scannerDropdown = findScannersGroup().findComponent(GlDropdown);
+          expect(scannerDropdown.attributes().text).toBe('All scanners');
+        });
+
+        it('dispatches the action on submit with empty array', () => {
+          const expectedScanners = [];
+          expect(actions.postRule).toHaveBeenCalledWith(
+            expect.anything(),
+            expect.objectContaining({ scanners: expectedScanners }),
+          );
+        });
       });
 
       describe('with invalid number of vulnerabilities', () => {

ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/lib/humanize_spec.js

@@ -59,9 +59,18 @@ const mockRules = [
   },
 ];
 
+const ALL_SCANNERS_RULE = {
+  type: 'scan_finding',
+  branches: ['master', 'main'],
+  scanners: [],
+  vulnerabilities_allowed: 2,
+  severity_levels: ['info', 'critical'],
+  vulnerability_states: ['resolved'],
+};
+
 const mockRulesHumanized = [
-  'The Sast scanner finds a critical vulnerability in an open merge request targeting the main branch.',
-  'The Dast or Sast scanners find info or critical vulnerabilities in an open merge request targeting the master or main branches.',
+  'Sast scanner finds a critical vulnerability in an open merge request targeting the main branch.',
+  'Dast or Sast scanners find info or critical vulnerabilities in an open merge request targeting the master or main branches.',
 ];
 
 const mockRulesEmptyBranch = {
@@ -88,7 +97,13 @@ describe('humanizeRules', () => {
 
   it('returns a single rule as a human-readable string for all branches', () => {
     expect(humanizeRules([mockRulesEmptyBranch])).toStrictEqual([
-      'The Sast scanner finds a critical vulnerability in an open merge request targeting all branches.',
+      'Sast scanner finds a critical vulnerability in an open merge request targeting all branches.',
+    ]);
+  });
+
+  it('returns a single rule as a human-readable string for all scanners', () => {
+    expect(humanizeRules([ALL_SCANNERS_RULE])).toStrictEqual([
+      'All scanners find info or critical vulnerabilities in an open merge request targeting the master or main branches.',
     ]);
   });
 });

ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/lib/rules_spec.js

+import { invalidScanners } from 'ee/threat_monitoring/components/policy_editor/scan_result_policy/lib/rules';
+
+describe('invalidScanners', () => {
+  describe('with undefined rules', () => {
+    it('returns false', () => {
+      expect(invalidScanners(undefined)).toBe(false);
+    });
+  });
+
+  describe('with empty rules', () => {
+    it('returns false', () => {
+      expect(invalidScanners([])).toBe(false);
+    });
+  });
+
+  describe('with rules with valid scanners', () => {
+    it('returns false', () => {
+      expect(invalidScanners([{ scanners: ['sast'] }])).toBe(false);
+    });
+  });
+
+  describe('with rules without scanners', () => {
+    it('returns true', () => {
+      expect(invalidScanners([{ anotherKey: 'anotherValue' }])).toBe(true);
+    });
+  });
+
+  describe('with rules with invalid scanners', () => {
+    it('returns true', () => {
+      expect(invalidScanners([{ scanners: ['notValid'] }])).toBe(true);
+    });
+  });
+});

ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor_spec.js

@@ -73,11 +73,11 @@ describe('ScanResultPolicyEditor', () => {
     nextTick();
   };
 
-  const factoryWithExistingPolicy = () => {
+  const factoryWithExistingPolicy = (policy = {}) => {
     return factory({
       propsData: {
         assignedPolicyProject,
-        existingPolicy: mockScanResultObject,
+        existingPolicy: { ...mockScanResultObject, ...policy },
         isEditing: true,
       },
     });
@@ -336,5 +336,15 @@ describe('ScanResultPolicyEditor', () => {
       expect(findAlert().exists()).toBe(true);
       expect(findAlert().isVisible()).toBe(true);
     });
+
+    it('shows alert when policy scanners are invalid', async () => {
+      factoryWithExistingPolicy({ rules: [{ scanners: ['cluster_image_scanning'] }] });
+
+      expect(findAlert().exists()).toBe(false);
+
+      await findPolicyEditorLayout().vm.$emit('update-editor-mode', EDITOR_MODE_RULE);
+
+      expect(findAlert().exists()).toBe(true);
+    });
   });
 });

ee/spec/frontend/threat_monitoring/components/policy_rule_multi_select_spec.js

@@ -184,4 +184,14 @@ describe('Policy Rule Multi Select', () => {
     expect(dropdownItemsWithDone.filter((element) => element.props('isChecked'))).toHaveLength(0);
     expect(wrapper.emitted().input).toEqual([[[]]]);
   });
+
+  describe('with includeSelectAll set to false', () => {
+    beforeEach(() => {
+      createComponent({ includeSelectAll: false });
+    });
+
+    it('does not show select all option', () => {
+      expect(findAllSelectedItem().exists()).toBe(false);
+    });
+  });
 });

ee/spec/support/shared_examples/requests/api/project_approval_rules_api_shared_examples.rb

@@ -164,7 +164,7 @@ RSpec.shared_examples 'an API endpoint for updating project approval rule' do
       it 'returns 200 status' do
         expect do
           put api(url, current_user), params: { scanners: scanners }
-        end.to change { approval_rule.reload.scanners.count }.from(::ApprovalProjectRule::SUPPORTED_SCANNERS.count).to(scanners.count)
+        end.to change { approval_rule.reload.scanners.count }.from(0).to(scanners.count)
         expect(response).to have_gitlab_http_status(:ok)
       end
     end

locale/gitlab.pot

@@ -33084,6 +33084,12 @@ msgstr ""
 msgid "SecurityOrchestration|%{branches} and %{lastBranch} %{plural}"
 msgstr ""
 
+msgid "SecurityOrchestration|%{scanners}"
+msgstr ""
+
+msgid "SecurityOrchestration|%{scanners} %{severities} in an open merge request targeting %{branches}."
+msgstr ""
+
 msgid "SecurityOrchestration|.yaml preview"
 msgstr ""
 
@@ -33099,6 +33105,9 @@ msgstr ""
 msgid "SecurityOrchestration|All policies"
 msgstr ""
 
+msgid "SecurityOrchestration|All scanners find"
+msgstr ""
+
 msgid "SecurityOrchestration|Allow all inbound traffic to all pods from all pods on ports 443/TCP."
 msgstr ""
 
@@ -33294,9 +33303,6 @@ msgstr ""
 msgid "SecurityOrchestration|Summary"
 msgstr ""
 
-msgid "SecurityOrchestration|The %{scanners} %{severities} in an open merge request targeting %{branches}."
-msgstr ""
-
 msgid "SecurityOrchestration|There was a problem creating the new security policy"
 msgstr ""