[Security Solution][Detection Engine] Fixes indicator matches mapping…

… UI where invalid list values can cause overwrites of other values (#89066) ## Summary This fixes the ReactJS keys to not use array indexes for the ReactJS keys which fixes #84893 as well as a few other bugs that I will show below. The fix for the ReactJS keys is to add a unique id version 4 `uuid.v4()` to the incoming threat_mapping and the entities. On save out to elastic I remove the id. This is considered [better practices for ReactJS keys](https://reactjs.org/docs/lists-and-keys.html) Down the road we might augment the arrays to have that id information but for now I add them when we get the data and then remove them as we save the data. This PR also: * Fixes tech debt around the hooks to remove the disabling of the `react-hooks/exhaustive-deps` in a few areas * Fixes one React Hook misnamed that would not have triggered React linter rules (_useRuleAsyn) * Adds 23 new Cypress e2e tests * Adds a new pattern of dealing with on button clicks for the Cypress tests that are make it less flakey ```ts cy.get(`button[title="${indexField}"]`) .should('be.visible') .then(([e]) => e.click()); ``` * Adds several new utilities to Cypress for testing rows for indicator matches and other Cypress utils to improve velocity and ergonomics ```ts fillIndicatorMatchRow getDefineContinueButton getIndicatorInvalidationText getIndicatorIndexComboField getIndicatorDeleteButton getIndicatorOrButton getIndicatorAndButton ``` ## Bug 1 Deleting row 1 can cause row 2 to be cleared out or only partial data to stick around. Before: ![im_bug_1](https://user-images.githubusercontent.com/1151048/105916137-c57b1d80-5fed-11eb-95b7-ad25b71cf4b8.gif) After: ![im_fix_1_1](https://user-images.githubusercontent.com/1151048/105917509-9fef1380-5fef-11eb-98eb-025c226f79fe.gif) ## Bug 2 Deleting row 2 in the middle of 3 rows did not shift the value up correctly Before: ![im_bug_2](https://user-images.githubusercontent.com/1151048/105917584-c01ed280-5fef-11eb-8c5b-fefb36f81008.gif) After: ![im_fix_2](https://user-images.githubusercontent.com/1151048/105917650-e0e72800-5fef-11eb-9fd3-020d52e4e3b1.gif) ## Bug 3 When using OR with values it does not shift up correctly similar to AND Before: ![im_bug_3](https://user-images.githubusercontent.com/1151048/105917691-f2303480-5fef-11eb-9368-b11d23159606.gif) After: ![im_fix_3](https://user-images.githubusercontent.com/1151048/105917714-f9574280-5fef-11eb-9be4-1f56c207525a.gif) ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers)
elastic · Jan 30, 2021 · 2f80e44 · 2f80e44
1 parent 2a913e4
commit 2f80e44
Show file tree

Hide file tree

Showing 25 changed files with 857 additions and 201 deletions.
diff --git a/...lugins/security_solution/cypress/integration/detection_rules/indicator_match_rule.spec.ts b/...lugins/security_solution/cypress/integration/detection_rules/indicator_match_rule.spec.ts
diff --git a/x-pack/plugins/security_solution/cypress/screens/create_new_rule.ts b/x-pack/plugins/security_solution/cypress/screens/create_new_rule.ts
@@ -38,6 +38,22 @@ export const CUSTOM_QUERY_INPUT = '[data-test-subj="queryInput"]';
 export const THREAT_MATCH_QUERY_INPUT =
   '[data-test-subj="detectionEngineStepDefineThreatRuleQueryBar"] [data-test-subj="queryInput"]';
 
+export const THREAT_MATCH_AND_BUTTON = '[data-test-subj="andButton"]';
+
+export const THREAT_ITEM_ENTRY_DELETE_BUTTON = '[data-test-subj="itemEntryDeleteButton"]';
+
+export const THREAT_MATCH_OR_BUTTON = '[data-test-subj="orButton"]';
+
+export const THREAT_COMBO_BOX_INPUT = '[data-test-subj="fieldAutocompleteComboBox"]';
+
+export const INVALID_MATCH_CONTENT = 'All matches require both a field and threat index field.';
+
+export const AT_LEAST_ONE_VALID_MATCH = 'At least one indicator match is required.';
+
+export const AT_LEAST_ONE_INDEX_PATTERN = 'A minimum of one index pattern is required.';
+
+export const CUSTOM_QUERY_REQUIRED = 'A custom query is required.';
+
 export const DEFINE_CONTINUE_BUTTON = '[data-test-subj="define-continue"]';
 
 export const DEFINE_EDIT_BUTTON = '[data-test-subj="edit-define-rule"]';

diff --git a/x-pack/plugins/security_solution/cypress/tasks/create_new_rule.ts b/x-pack/plugins/security_solution/cypress/tasks/create_new_rule.ts
@@ -63,13 +63,20 @@ import {
   EQL_QUERY_PREVIEW_HISTOGRAM,
   EQL_QUERY_VALIDATION_SPINNER,
   COMBO_BOX_CLEAR_BTN,
-  COMBO_BOX_RESULT,
   MITRE_ATTACK_TACTIC_DROPDOWN,
   MITRE_ATTACK_TECHNIQUE_DROPDOWN,
   MITRE_ATTACK_SUBTECHNIQUE_DROPDOWN,
   MITRE_ATTACK_ADD_TACTIC_BUTTON,
   MITRE_ATTACK_ADD_SUBTECHNIQUE_BUTTON,
   MITRE_ATTACK_ADD_TECHNIQUE_BUTTON,
+  THREAT_COMBO_BOX_INPUT,
+  THREAT_ITEM_ENTRY_DELETE_BUTTON,
+  THREAT_MATCH_AND_BUTTON,
+  INVALID_MATCH_CONTENT,
+  THREAT_MATCH_OR_BUTTON,
+  AT_LEAST_ONE_VALID_MATCH,
+  AT_LEAST_ONE_INDEX_PATTERN,
+  CUSTOM_QUERY_REQUIRED,
 } from '../screens/create_new_rule';
 import { TOAST_ERROR } from '../screens/shared';
 import { SERVER_SIDE_EVENT_COUNT } from '../screens/timeline';
@@ -144,7 +151,7 @@ export const fillAboutRuleAndContinue = (
   rule: CustomRule | MachineLearningRule | ThresholdRule | ThreatIndicatorRule
 ) => {
   fillAboutRule(rule);
-  cy.get(ABOUT_CONTINUE_BTN).should('exist').click({ force: true });
+  getAboutContinueButton().should('exist').click({ force: true });
 };
 
 export const fillAboutRuleWithOverrideAndContinue = (rule: OverrideRule) => {
@@ -222,7 +229,7 @@ export const fillAboutRuleWithOverrideAndContinue = (rule: OverrideRule) => {
     cy.get(COMBO_BOX_INPUT).type(`${rule.timestampOverride}{enter}`);
   });
 
-  cy.get(ABOUT_CONTINUE_BTN).should('exist').click({ force: true });
+  getAboutContinueButton().should('exist').click({ force: true });
 };
 
 export const fillDefineCustomRuleWithImportedQueryAndContinue = (
@@ -282,19 +289,132 @@ export const fillDefineEqlRuleAndContinue = (rule: CustomRule) => {
   cy.get(EQL_QUERY_INPUT).should('not.exist');
 };
 
+/**
+ * Fills in the indicator match rows for tests by giving it an optional rowNumber,
+ * a indexField, a indicatorIndexField, and an optional validRows which indicates
+ * which row is valid or not.
+ *
+ * There are special tricks below with Eui combo box:
+ * cy.get(`button[title="${indexField}"]`)
+ * .should('be.visible')
+ * .then(([e]) => e.click());
+ *
+ * To first ensure the button is there before clicking on the button. There are
+ * race conditions where if the Eui drop down button from the combo box is not
+ * visible then the click handler is not there either, and when we click on it
+ * that will cause the item to _not_ be selected. Using a {enter} with the combo
+ * box also does not select things from EuiCombo boxes either, so I have to click
+ * the actual contents of the EuiCombo box to select things.
+ */
+export const fillIndicatorMatchRow = ({
+  rowNumber,
+  indexField,
+  indicatorIndexField,
+  validColumns,
+}: {
+  rowNumber?: number; // default is 1
+  indexField: string;
+  indicatorIndexField: string;
+  validColumns?: 'indexField' | 'indicatorField' | 'both' | 'none'; // default is both are valid entries
+}) => {
+  const computedRowNumber = rowNumber == null ? 1 : rowNumber;
+  const computedValueRows = validColumns == null ? 'both' : validColumns;
+  const OFFSET = 2;
+  cy.get(COMBO_BOX_INPUT)
+    .eq(computedRowNumber * OFFSET + 1)
+    .type(indexField);
+  if (computedValueRows === 'indexField' || computedValueRows === 'both') {
+    cy.get(`button[title="${indexField}"]`)
+      .should('be.visible')
+      .then(([e]) => e.click());
+  }
+  cy.get(COMBO_BOX_INPUT)
+    .eq(computedRowNumber * OFFSET + 2)
+    .type(indicatorIndexField);
+
+  if (computedValueRows === 'indicatorField' || computedValueRows === 'both') {
+    cy.get(`button[title="${indicatorIndexField}"]`)
+      .should('be.visible')
+      .then(([e]) => e.click());
+  }
+};
+
+/**
+ * Fills in both the index pattern and the indicator match index pattern.
+ * @param indexPattern  The index pattern.
+ * @param indicatorIndex The indicator index pattern.
+ */
+export const fillIndexAndIndicatorIndexPattern = (
+  indexPattern?: string[],
+  indicatorIndex?: string[]
+) => {
+  getIndexPatternClearButton().click();
+  getIndicatorIndex().type(`${indexPattern}{enter}`);
+  getIndicatorIndicatorIndex().type(`${indicatorIndex}{enter}`);
+};
+
+/** Returns the indicator index drop down field. Pass in row number, default is 1 */
+export const getIndicatorIndexComboField = (row = 1) =>
+  cy.get(THREAT_COMBO_BOX_INPUT).eq(row * 2 - 2);
+
+/** Returns the indicator mapping drop down field. Pass in row number, default is 1 */
+export const getIndicatorMappingComboField = (row = 1) =>
+  cy.get(THREAT_COMBO_BOX_INPUT).eq(row * 2 - 1);
+
+/** Returns the indicator matches DELETE button for the mapping. Pass in row number, default is 1  */
+export const getIndicatorDeleteButton = (row = 1) =>
+  cy.get(THREAT_ITEM_ENTRY_DELETE_BUTTON).eq(row - 1);
+
+/** Returns the indicator matches AND button for the mapping */
+export const getIndicatorAndButton = () => cy.get(THREAT_MATCH_AND_BUTTON);
+
+/** Returns the indicator matches OR button for the mapping */
+export const getIndicatorOrButton = () => cy.get(THREAT_MATCH_OR_BUTTON);
+
+/** Returns the invalid match content. */
+export const getIndicatorInvalidationText = () => cy.contains(INVALID_MATCH_CONTENT);
+
+/** Returns that at least one valid match is required content */
+export const getIndicatorAtLeastOneInvalidationText = () => cy.contains(AT_LEAST_ONE_VALID_MATCH);
+
+/** Returns that at least one index pattern is required content */
+export const getIndexPatternInvalidationText = () => cy.contains(AT_LEAST_ONE_INDEX_PATTERN);
+
+/** Returns the continue button on the step of about */
+export const getAboutContinueButton = () => cy.get(ABOUT_CONTINUE_BTN);
+
+/** Returns the continue button on the step of define */
+export const getDefineContinueButton = () => cy.get(DEFINE_CONTINUE_BUTTON);
+
+/** Returns the indicator index pattern */
+export const getIndicatorIndex = () => cy.get(COMBO_BOX_INPUT).eq(0);
+
+/** Returns the indicator's indicator index */
+export const getIndicatorIndicatorIndex = () => cy.get(COMBO_BOX_INPUT).eq(2);
+
+/** Returns the index pattern's clear button  */
+export const getIndexPatternClearButton = () => cy.get(COMBO_BOX_CLEAR_BTN);
+
+/** Returns the custom query input */
+export const getCustomQueryInput = () => cy.get(CUSTOM_QUERY_INPUT).eq(0);
+
+/** Returns the custom query input */
+export const getCustomIndicatorQueryInput = () => cy.get(CUSTOM_QUERY_INPUT).eq(1);
+
+/** Returns custom query required content */
+export const getCustomQueryInvalidationText = () => cy.contains(CUSTOM_QUERY_REQUIRED);
+
+/**
+ * Fills in the define indicator match rules and then presses the continue button
+ * @param rule The rule to use to fill in everything
+ */
 export const fillDefineIndicatorMatchRuleAndContinue = (rule: ThreatIndicatorRule) => {
-  const INDEX_PATTERNS = 0;
-  const INDICATOR_INDEX_PATTERN = 2;
-  const INDICATOR_MAPPING = 3;
-  const INDICATOR_INDEX_FIELD = 4;
-
-  cy.get(COMBO_BOX_CLEAR_BTN).click();
-  cy.get(COMBO_BOX_INPUT).eq(INDEX_PATTERNS).type(`${rule.index}{enter}`);
-  cy.get(COMBO_BOX_INPUT).eq(INDICATOR_INDEX_PATTERN).type(`${rule.indicatorIndexPattern}{enter}`);
-  cy.get(COMBO_BOX_INPUT).eq(INDICATOR_MAPPING).type(`${rule.indicatorMapping}{enter}`);
-  cy.get(COMBO_BOX_RESULT).first().click();
-  cy.get(COMBO_BOX_INPUT).eq(INDICATOR_INDEX_FIELD).type(`${rule.indicatorIndexField}{enter}`);
-  cy.get(DEFINE_CONTINUE_BUTTON).should('exist').click({ force: true });
+  fillIndexAndIndicatorIndexPattern(rule.index, rule.indicatorIndexPattern);
+  fillIndicatorMatchRow({
+    indexField: rule.indicatorMapping,
+    indicatorIndexField: rule.indicatorIndexField,
+  });
+  getDefineContinueButton().should('exist').click({ force: true });
   cy.get(CUSTOM_QUERY_INPUT).should('not.exist');
 };
 
@@ -304,7 +424,7 @@ export const fillDefineMachineLearningRuleAndContinue = (rule: MachineLearningRu
   cy.get(ANOMALY_THRESHOLD_INPUT).type(`{selectall}${machineLearningRule.anomalyScoreThreshold}`, {
     force: true,
   });
-  cy.get(DEFINE_CONTINUE_BUTTON).should('exist').click({ force: true });
+  getDefineContinueButton().should('exist').click({ force: true });
 
   cy.get(MACHINE_LEARNING_DROPDOWN).should('not.exist');
 };

diff --git a/x-pack/plugins/security_solution/public/common/components/threat_match/entry_item.test.tsx b/x-pack/plugins/security_solution/public/common/components/threat_match/entry_item.test.tsx
@@ -22,6 +22,7 @@ describe('EntryItem', () => {
     const wrapper = mount(
       <EntryItem
         entry={{
+          id: '123',
           field: undefined,
           value: undefined,
           type: 'mapping',
@@ -54,6 +55,7 @@ describe('EntryItem', () => {
     const wrapper = mount(
       <EntryItem
         entry={{
+          id: '123',
           field: getField('ip'),
           type: 'mapping',
           value: getField('ip'),
@@ -84,6 +86,7 @@ describe('EntryItem', () => {
 
     expect(mockOnChange).toHaveBeenCalledWith(
       {
+        id: '123',
         field: 'machine.os',
         type: 'mapping',
         value: 'ip',
@@ -97,6 +100,7 @@ describe('EntryItem', () => {
     const wrapper = mount(
       <EntryItem
         entry={{
+          id: '123',
           field: getField('ip'),
           type: 'mapping',
           value: getField('ip'),
@@ -125,6 +129,9 @@ describe('EntryItem', () => {
       onChange: (a: EuiComboBoxOptionOption[]) => void;
     }).onChange([{ label: 'is not' }]);
 
-    expect(mockOnChange).toHaveBeenCalledWith({ field: 'ip', type: 'mapping', value: '' }, 0);
+    expect(mockOnChange).toHaveBeenCalledWith(
+      { id: '123', field: 'ip', type: 'mapping', value: '' },
+      0
+    );
   });
 });
diff --git a/x-pack/plugins/security_solution/public/common/components/threat_match/entry_item.tsx b/x-pack/plugins/security_solution/public/common/components/threat_match/entry_item.tsx
@@ -75,7 +75,11 @@ export const EntryItem: React.FC<EntryItemProps> = ({
         </EuiFormRow>
       );
     } else {
-      return comboBox;
+      return (
+        <EuiFormRow label={''} data-test-subj="entryItemFieldInputFormRow">
+          {comboBox}
+        </EuiFormRow>
+      );
     }
   }, [handleFieldChange, indexPattern, entry, showLabel]);
 
@@ -101,7 +105,11 @@ export const EntryItem: React.FC<EntryItemProps> = ({
         </EuiFormRow>
       );
     } else {
-      return comboBox;
+      return (
+        <EuiFormRow label={''} data-test-subj="threatFieldInputFormRow">
+          {comboBox}
+        </EuiFormRow>
+      );
     }
   }, [handleThreatFieldChange, threatIndexPatterns, entry, showLabel]);
 

diff --git a/x-pack/plugins/security_solution/public/common/components/threat_match/helpers.test.tsx b/x-pack/plugins/security_solution/public/common/components/threat_match/helpers.test.tsx
@@ -21,6 +21,10 @@ import {
 } from './helpers';
 import { ThreatMapEntry } from '../../../../common/detection_engine/schemas/types';
 
+jest.mock('uuid', () => ({
+  v4: jest.fn().mockReturnValue('123'),
+}));
+
 const getMockIndexPattern = (): IndexPattern =>
   ({
     id: '1234',
@@ -29,6 +33,7 @@ const getMockIndexPattern = (): IndexPattern =>
   } as IndexPattern);
 
 const getMockEntry = (): FormattedEntry => ({
+  id: '123',
   field: getField('ip'),
   value: getField('ip'),
   type: 'mapping',
@@ -42,6 +47,7 @@ describe('Helpers', () => {
 
   afterEach(() => {
     moment.tz.setDefault('Browser');
+    jest.clearAllMocks();
   });
 
   describe('#getFormattedEntry', () => {
@@ -70,6 +76,7 @@ describe('Helpers', () => {
       const output = getFormattedEntry(payloadIndexPattern, payloadIndexPattern, payloadItem, 0);
       const expected: FormattedEntry = {
         entryIndex: 0,
+        id: '123',
         field: {
           name: 'machine.os.raw.text',
           type: 'string',
@@ -94,6 +101,7 @@ describe('Helpers', () => {
       const output = getFormattedEntries(payloadIndexPattern, payloadIndexPattern, payloadItems);
       const expected: FormattedEntry[] = [
         {
+          id: '123',
           entryIndex: 0,
           field: undefined,
           value: undefined,
@@ -109,6 +117,7 @@ describe('Helpers', () => {
       const output = getFormattedEntries(payloadIndexPattern, payloadIndexPattern, payloadItems);
       const expected: FormattedEntry[] = [
         {
+          id: '123',
           entryIndex: 0,
           field: {
             name: 'machine.os',
@@ -134,6 +143,7 @@ describe('Helpers', () => {
       const output = getFormattedEntries(payloadIndexPattern, threatIndexPattern, payloadItems);
       const expected: FormattedEntry[] = [
         {
+          id: '123',
           entryIndex: 0,
           field: {
             name: 'machine.os',
@@ -170,6 +180,7 @@ describe('Helpers', () => {
       const output = getFormattedEntries(payloadIndexPattern, payloadIndexPattern, payloadItems);
       const expected: FormattedEntry[] = [
         {
+          id: '123',
           field: {
             name: 'machine.os',
             type: 'string',
@@ -194,6 +205,7 @@ describe('Helpers', () => {
           entryIndex: 0,
         },
         {
+          id: '123',
           field: {
             name: 'ip',
             type: 'ip',
@@ -249,9 +261,10 @@ describe('Helpers', () => {
       const payloadItem = getMockEntry();
       const payloadIFieldType = getField('ip');
       const output = getEntryOnFieldChange(payloadItem, payloadIFieldType);
-      const expected: { updatedEntry: Entry; index: number } = {
+      const expected: { updatedEntry: Entry & { id: string }; index: number } = {
         index: 0,
         updatedEntry: {
+          id: '123',
           field: 'ip',
           type: 'mapping',
           value: 'ip',