From 0c8d5cf014487399ac3f9cfebac21373f6f51b4e Mon Sep 17 00:00:00 2001 From: Zamir Martins Filho Date: Mon, 7 Feb 2022 23:15:13 -0500 Subject: [PATCH 1/4] Add policy rule builder for scan result policy as an increment to the main body. It will be followed by a MR covering the policy action builder. EE: true --- .../scan_result_policy/lib/index.js | 1 + .../scan_result_policy/lib/rules.js | 13 ++ .../policy_rule_builder.vue | 156 ++++++++++++++++++ .../scan_result_policy_editor.vue | 24 ++- .../lib/policy_rule_builder_spec.js | 110 ++++++++++++ .../scan_result_policy_editor_spec.js | 61 ++++++- locale/gitlab.pot | 10 ++ 7 files changed, 371 insertions(+), 4 deletions(-) create mode 100644 ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/lib/rules.js create mode 100644 ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/policy_rule_builder.vue create mode 100644 ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/lib/policy_rule_builder_spec.js diff --git a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/lib/index.js b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/lib/index.js index df2c462e6aa1e8..a5a148d7b8749c 100644 --- a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/lib/index.js +++ b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/lib/index.js @@ -1,5 +1,6 @@ export { fromYaml } from './from_yaml'; export { toYaml } from './to_yaml'; +export { buildRule } from './rules'; export * from './humanize'; export const DEFAULT_SCAN_RESULT_POLICY = `type: scan_result_policy diff --git a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/lib/rules.js b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/lib/rules.js new file mode 100644 index 00000000000000..74452c3e1e7f29 --- /dev/null +++ b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/lib/rules.js @@ -0,0 +1,13 @@ +/* + Construct a new rule object. +*/ +export function buildRule() { + return { + type: 'scan_finding', + branches: [], + scanners: [], + vulnerabilities_allowed: 0, + severity_levels: [], + vulnerability_states: [], + }; +} diff --git a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/policy_rule_builder.vue b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/policy_rule_builder.vue new file mode 100644 index 00000000000000..d3e3648c2c7096 --- /dev/null +++ b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/policy_rule_builder.vue @@ -0,0 +1,156 @@ + + + diff --git a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor.vue b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor.vue index f11806712d019b..74f1cd9686477d 100644 --- a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor.vue +++ b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor.vue @@ -20,7 +20,8 @@ import PolicyEditorLayout from '../policy_editor_layout.vue'; import { assignSecurityPolicyProject, modifyPolicy } from '../utils'; import DimDisableContainer from '../dim_disable_container.vue'; import PolicyActionBuilder from './policy_action_builder.vue'; -import { DEFAULT_SCAN_RESULT_POLICY, fromYaml, toYaml } from './lib'; +import PolicyRuleBuilder from './policy_rule_builder.vue'; +import { DEFAULT_SCAN_RESULT_POLICY, fromYaml, toYaml, buildRule } from './lib'; export default { SECURITY_POLICY_ACTIONS, @@ -51,6 +52,7 @@ export default { GlFormTextarea, GlAlert, PolicyActionBuilder, + PolicyRuleBuilder, PolicyEditorLayout, DimDisableContainer, }, @@ -121,6 +123,15 @@ export default { updateAction(actionIndex, values) { this.policy.actions.splice(actionIndex, 1, values); }, + addRule() { + this.policy.rules.push(buildRule()); + }, + removeRule(ruleIndex) { + this.policy.rules.splice(ruleIndex, 1); + }, + updateRule(ruleIndex, values) { + this.policy.rules.splice(ruleIndex, 1, values); + }, handleError(error) { if (error.message.toLowerCase().includes('graphql')) { this.$emit('error', GRAPHQL_ERROR_MESSAGE); @@ -254,8 +265,17 @@ export default {
+ +
- + {{ $options.i18n.addRule }}
diff --git a/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/lib/policy_rule_builder_spec.js b/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/lib/policy_rule_builder_spec.js new file mode 100644 index 00000000000000..91a4da04e0e34a --- /dev/null +++ b/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/lib/policy_rule_builder_spec.js @@ -0,0 +1,110 @@ +// import { shallowMount } from '@vue/test-utils'; +import { mount } from '@vue/test-utils'; +import { GlButton } from '@gitlab/ui'; +import { nextTick } from 'vue'; +import Api from 'ee/api'; +import PolicyRuleBuilder from 'ee/threat_monitoring/components/policy_editor/scan_result_policy/policy_rule_builder.vue'; +import ProtectedBranchesSelector from 'ee/vue_shared/components/branches_selector/protected_branches_selector.vue'; + +describe('PolicyRuleBuilder', () => { + let wrapper; + + const PROTECTED_BRANCHES_MOCK = [{ id: 1, name: 'main' }]; + + const DEFAULT_RULE = { + type: 'scan_finding', + branches: [PROTECTED_BRANCHES_MOCK[0].name], + scanners: [], + vulnerabilities_allowed: 0, + severity_levels: [], + vulnerability_states: [], + }; + + const UPDATED_RULE = { + type: 'scan_finding', + branches: [PROTECTED_BRANCHES_MOCK[0].name], + scanners: ['dast'], + vulnerabilities_allowed: 1, + severity_levels: ['high'], + vulnerability_states: ['newly_detected'], + }; + + const factory = (propsData = {}) => { + wrapper = mount(PolicyRuleBuilder, { + propsData: { + initRule: DEFAULT_RULE, + ...propsData, + }, + provide: { + projectId: '1', + }, + }); + }; + + const findBranches = () => wrapper.findComponent(ProtectedBranchesSelector); + const findScanners = () => wrapper.find('[data-testid="scanners-select"]'); + const findSeverities = () => wrapper.find('[data-testid="severities-select"]'); + const findVulnStates = () => wrapper.find('[data-testid="vulnerability-states-select"]'); + const findVulnAllowed = () => wrapper.find('[data-testid="vulnerabilities-allowed-input"]'); + const findDeleteBtn = () => wrapper.findComponent(GlButton); + + beforeEach(() => { + jest + .spyOn(Api, 'projectProtectedBranches') + .mockReturnValue(Promise.resolve(PROTECTED_BRANCHES_MOCK)); + }); + + afterEach(() => { + wrapper.destroy(); + }); + + describe('initial rendering', () => { + it('renders one field for each attribute of the rule', async () => { + factory(); + await nextTick(); + + expect(findBranches().exists()).toBe(true); + expect(findScanners().exists()).toBe(true); + expect(findSeverities().exists()).toBe(true); + expect(findVulnStates().exists()).toBe(true); + expect(findVulnAllowed().exists()).toBe(true); + }); + + it('renders the delete buttom', async () => { + factory(); + await nextTick(); + + expect(findDeleteBtn().exists()).toBe(true); + }); + }); + + describe('when removing the rule', () => { + it('emits remove event', async () => { + factory(); + await nextTick(); + await findDeleteBtn().vm.$emit('click'); + + expect(wrapper.emitted().remove).toHaveLength(1); + }); + }); + + describe('when editing any attribute of the rule', () => { + it.each` + currentComponent | newValue | expected + ${findBranches} | ${PROTECTED_BRANCHES_MOCK[0]} | ${{ branches: UPDATED_RULE.branches }} + ${findScanners} | ${UPDATED_RULE.scanners} | ${{ scanners: UPDATED_RULE.scanners }} + ${findSeverities} | ${UPDATED_RULE.severity_levels} | ${{ severity_levels: UPDATED_RULE.severity_levels }} + ${findVulnStates} | ${UPDATED_RULE.vulnerability_states} | ${{ vulnerability_states: UPDATED_RULE.vulnerability_states }} + ${findVulnAllowed} | ${UPDATED_RULE.vulnerabilities_allowed} | ${{ vulnerabilities_allowed: UPDATED_RULE.vulnerabilities_allowed }} + `( + 'triggers a changed event (by $currentComponent) with the updated rule', + async ({ currentComponent, newValue, expected }) => { + factory(); + await nextTick(); + await currentComponent().vm.$emit('input', newValue); + + expect(wrapper.emitted().changed).toEqual([[expect.objectContaining(expected)]]); + }, + ); + }); +}); diff --git a/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor_spec.js b/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor_spec.js index 610f99bfc81f49..90531ddc16b6ba 100644 --- a/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor_spec.js +++ b/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor_spec.js @@ -22,6 +22,7 @@ import { } from 'ee/threat_monitoring/components/policy_editor/constants'; import DimDisableContainer from 'ee/threat_monitoring/components/policy_editor/dim_disable_container.vue'; import PolicyActionBuilder from 'ee/threat_monitoring/components/policy_editor/scan_result_policy/policy_action_builder.vue'; +import PolicyRuleBuilder from 'ee/threat_monitoring/components/policy_editor/scan_result_policy/policy_rule_builder.vue'; jest.mock('~/lib/utils/url_utility', () => ({ joinPaths: jest.requireActual('~/lib/utils/url_utility').joinPaths, @@ -92,6 +93,7 @@ describe('ScanResultPolicyEditor', () => { const findEnableToggle = () => wrapper.findComponent(GlToggle); const findAllDisabledComponents = () => wrapper.findAllComponents(DimDisableContainer); const findYamlPreview = () => wrapper.find('[data-testid="yaml-preview"]'); + const findAllRuleBuilders = () => wrapper.findAllComponents(PolicyRuleBuilder); afterEach(() => { wrapper.destroy(); @@ -111,10 +113,11 @@ describe('ScanResultPolicyEditor', () => { expect(findPolicyEditorLayout().attributes('yamleditorvalue')).toBe(newManifest); }); - it('disables add rule button until feature is merged', async () => { + it('displays the inital rule and add rule button', async () => { await factory(); - expect(findAddRuleButton().props('disabled')).toBe(true); + expect(findAllRuleBuilders().length).toBe(1); + expect(findAddRuleButton().exists()).toBe(true); }); it('displays alert for invalid yaml', async () => { @@ -198,6 +201,60 @@ describe('ScanResultPolicyEditor', () => { ); }, ); + + it('adds a new rule', async () => { + const rulesCount = 1; + factory(); + await nextTick(); + + expect(findAllRuleBuilders().length).toBe(rulesCount); + + await findAddRuleButton().vm.$emit('click'); + + expect(findAllRuleBuilders().length).toBe(rulesCount + 1); + }); + + it('hides add button when the limit of five rules has been reached', async () => { + const limit = 5; + factory(); + await nextTick(); + await findAddRuleButton().vm.$emit('click'); + await findAddRuleButton().vm.$emit('click'); + await findAddRuleButton().vm.$emit('click'); + await findAddRuleButton().vm.$emit('click'); + + expect(findAllRuleBuilders().length).toBe(limit); + expect(findAddRuleButton().exists()).toBe(false); + }); + + it('updates an existing rule', async () => { + const newValue = { + type: 'scan_finding', + branches: [], + scanners: [], + vulnerabilities_allowed: 1, + severity_levels: [], + vulnerability_states: [], + }; + factory(); + await nextTick(); + await findAllRuleBuilders().at(0).vm.$emit('changed', newValue); + + expect(wrapper.vm.policy.rules[0]).toEqual(newValue); + expect(findYamlPreview().html()).toMatch('vulnerabilities_allowed: 1'); + }); + + it('deletes the initial rule', async () => { + const initialRuleCount = 1; + factory(); + await nextTick(); + + expect(findAllRuleBuilders().length).toBe(initialRuleCount); + + await findAllRuleBuilders().at(0).vm.$emit('remove', 0); + + expect(findAllRuleBuilders().length).toBe(initialRuleCount - 1); + }); }); describe('when a user is not an owner of the project', () => { diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 37f8a3fd7b4539..d4cb4da3a67474 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -31845,6 +31845,16 @@ msgid "ScanResultPolicy|%{thenLabelStart}Then%{thenLabelEnd} Require approval fr msgstr "" msgid "ScanResultPolicy|add an approver" +msgid "ScanResultPolicy|%{ifLabelStart}if%{ifLabelEnd} %{scanners} scan in an open merge request targeting the %{branches} branch(es) finds %{vulnerabilitiesAllowed} or more %{severities} vulnerabilities that are %{vulnerabilityStates}" +msgstr "" + +msgid "ScanResultPolicy|scanners" +msgstr "" + +msgid "ScanResultPolicy|severity levels" +msgstr "" + +msgid "ScanResultPolicy|vulnerability states" msgstr "" msgid "Scanner" -- GitLab From 1c5c558970011660416c3f5e2de433cb8cc44065 Mon Sep 17 00:00:00 2001 From: Zamir Martins Filho Date: Wed, 9 Feb 2022 14:20:54 -0500 Subject: [PATCH 2/4] Address reviewer suggestions by renaming template variable to something more specific and remove unused code that was left commented. --- .../policy_editor/scan_result_policy/policy_rule_builder.vue | 4 ++-- .../scan_result_policy/lib/policy_rule_builder_spec.js | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/policy_rule_builder.vue b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/policy_rule_builder.vue index d3e3648c2c7096..207961b56f4448 100644 --- a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/policy_rule_builder.vue +++ b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/policy_rule_builder.vue @@ -37,7 +37,7 @@ export default { }; }, computed: { - sprintfTemplate() { + scanResultRuleCopy() { return s__( 'ScanResultPolicy|%{ifLabelStart}if%{ifLabelEnd} %{scanners} scan in an open merge request targeting the %{branches} branch(es) finds %{vulnerabilitiesAllowed} or more %{severities} vulnerabilities that are %{vulnerabilityStates}', ); @@ -87,7 +87,7 @@ export default { class="gl-bg-gray-10 gl-border-solid gl-border-1 gl-border-gray-100 gl-rounded-base px-3 pt-3 gl-relative gl-pb-4" > - + diff --git a/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/lib/policy_rule_builder_spec.js b/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/lib/policy_rule_builder_spec.js index 91a4da04e0e34a..d2dbe771eec25f 100644 --- a/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/lib/policy_rule_builder_spec.js +++ b/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/lib/policy_rule_builder_spec.js @@ -1,4 +1,3 @@ -// import { shallowMount } from '@vue/test-utils'; import { mount } from '@vue/test-utils'; import { GlButton } from '@gitlab/ui'; import { nextTick } from 'vue'; -- GitLab From bb808eda899c332fe76aa60188526b435e64ca38 Mon Sep 17 00:00:00 2001 From: Natalia Tepluhina Date: Fri, 11 Feb 2022 02:28:07 +0000 Subject: [PATCH 3/4] Apply maintainer suggestions --- .../scan_result_policy/scan_result_policy_editor_spec.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor_spec.js b/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor_spec.js index 90531ddc16b6ba..3fc086ab042c2c 100644 --- a/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor_spec.js +++ b/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor_spec.js @@ -211,7 +211,7 @@ describe('ScanResultPolicyEditor', () => { await findAddRuleButton().vm.$emit('click'); - expect(findAllRuleBuilders().length).toBe(rulesCount + 1); + expect(findAllRuleBuilders()).toHaveLength(rulesCount + 1); }); it('hides add button when the limit of five rules has been reached', async () => { @@ -223,7 +223,7 @@ describe('ScanResultPolicyEditor', () => { await findAddRuleButton().vm.$emit('click'); await findAddRuleButton().vm.$emit('click'); - expect(findAllRuleBuilders().length).toBe(limit); + expect(findAllRuleBuilders()).toHaveLength(limit); expect(findAddRuleButton().exists()).toBe(false); }); @@ -249,11 +249,11 @@ describe('ScanResultPolicyEditor', () => { factory(); await nextTick(); - expect(findAllRuleBuilders().length).toBe(initialRuleCount); + expect(findAllRuleBuilders()).toHaveLength(initialRuleCount); await findAllRuleBuilders().at(0).vm.$emit('remove', 0); - expect(findAllRuleBuilders().length).toBe(initialRuleCount - 1); + expect(findAllRuleBuilders()).toHaveLength(initialRuleCount - 1); }); }); -- GitLab From 341ad775678a729fea5bc2c53abb56e5e69f2530 Mon Sep 17 00:00:00 2001 From: Zamir Martins Filho Date: Fri, 11 Feb 2022 09:26:24 -0500 Subject: [PATCH 4/4] Address maintainer reviews by moving non reactive property from computed to options and replacing data/watch approach by computed get/set. --- .../policy_rule_builder.vue | 76 +++++++++++-------- locale/gitlab.pot | 4 +- 2 files changed, 46 insertions(+), 34 deletions(-) diff --git a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/policy_rule_builder.vue b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/policy_rule_builder.vue index 207961b56f4448..5f488745cb15f8 100644 --- a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/policy_rule_builder.vue +++ b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/policy_rule_builder.vue @@ -10,6 +10,9 @@ import PolicyRuleMultiSelect from 'ee/threat_monitoring/components/policy_rule_m import { APPROVAL_VULNERABILITY_STATES } from 'ee/approvals/constants'; export default { + scanResultRuleCopy: s__( + 'ScanResultPolicy|%{ifLabelStart}if%{ifLabelEnd} %{scanners} scan in an open merge request targeting the %{branches} branch(es) finds %{vulnerabilitiesAllowed} or more %{severities} vulnerabilities that are %{vulnerabilityStates}', + ), components: { GlSprintf, GlForm, @@ -28,47 +31,55 @@ export default { }, data() { return { - rule: { ...this.initRule }, reportTypesKeys: Object.keys(REPORT_TYPES_NO_CLUSTER_IMAGE), - branchesToAdd: this.initRule.branches, - severityLevelsToAdd: this.initRule.severity_levels, - scannersToAdd: this.initRule.scanners, - vulnerabilityStates: this.initRule.vulnerability_states, }; }, computed: { - scanResultRuleCopy() { - return s__( - 'ScanResultPolicy|%{ifLabelStart}if%{ifLabelEnd} %{scanners} scan in an open merge request targeting the %{branches} branch(es) finds %{vulnerabilitiesAllowed} or more %{severities} vulnerabilities that are %{vulnerabilityStates}', - ); - }, - }, - watch: { - branchesToAdd(value) { - this.rule.branches = value.id === null ? [] : [value.name]; - }, - severityLevelsToAdd(values) { - this.rule.severity_levels = values; + branchesToAdd: { + get() { + return this.initRule.branches; + }, + set(value) { + const branches = value.id === null ? [] : [value.name]; + this.triggerChanged({ branches }); + }, }, - scannersToAdd(values) { - this.rule.scanners = values; + severityLevelsToAdd: { + get() { + return this.initRule.severity_levels; + }, + set(values) { + this.triggerChanged({ severity_levels: values }); + }, }, - vulnerabilityStates(values) { - this.rule.vulnerability_states = values; + scannersToAdd: { + get() { + return this.initRule.scanners; + }, + set(values) { + this.triggerChanged({ scanners: values }); + }, }, - rule: { - handler(values) { - this.$emit('changed', values); + vulnerabilityStates: { + get() { + return this.initRule.vulnerability_states; + }, + set(values) { + this.triggerChanged({ vulnerability_states: values }); }, - deep: true, }, - initRule(values) { - this.rule = values; + vulnerabilitiesAllowed: { + get() { + return this.initRule.vulnerabilities_allowed; + }, + set(value) { + this.triggerChanged({ vulnerabilities_allowed: parseInt(value, 10) }); + }, }, }, methods: { - vulnerabilitiesAllowedChanged(value) { - this.rule.vulnerabilities_allowed = parseInt(value, 10); + triggerChanged(value) { + this.$emit('changed', { ...this.initRule, ...value }); }, }, REPORT_TYPES_NO_CLUSTER_IMAGE, @@ -87,7 +98,7 @@ export default { class="gl-bg-gray-10 gl-border-solid gl-border-1 gl-border-gray-100 gl-rounded-base px-3 pt-3 gl-relative gl-pb-4" > - + @@ -107,19 +118,18 @@ export default { diff --git a/locale/gitlab.pot b/locale/gitlab.pot index d4cb4da3a67474..8ee83196f8f8e4 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -31841,11 +31841,13 @@ msgstr "" msgid "Saving project." msgstr "" +msgid "ScanResultPolicy|%{ifLabelStart}if%{ifLabelEnd} %{scanners} scan in an open merge request targeting the %{branches} branch(es) finds %{vulnerabilitiesAllowed} or more %{severities} vulnerabilities that are %{vulnerabilityStates}" +msgstr "" + msgid "ScanResultPolicy|%{thenLabelStart}Then%{thenLabelEnd} Require approval from %{approvalsRequired} of the following approvers: %{approvers}" msgstr "" msgid "ScanResultPolicy|add an approver" -msgid "ScanResultPolicy|%{ifLabelStart}if%{ifLabelEnd} %{scanners} scan in an open merge request targeting the %{branches} branch(es) finds %{vulnerabilitiesAllowed} or more %{severities} vulnerabilities that are %{vulnerabilityStates}" msgstr "" msgid "ScanResultPolicy|scanners" -- GitLab