Security form fix (#132834)

**Addresses:** #130767, #130770

Related to [this PR](#130825).

@banderror described the reason for the problems in his PR. There was [ask ](#130825 (review)) to not return the `useEffect` to the form lib.

The fix of the form clears the values [in this commit](df0b7bb)

Then I cherry-pick commits from @banderror PR to fix form stripping undefined values and also fixed some tests

### Checklist

Delete any items that are not applicable to this PR.

- [ ] [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

x-pack/plugins/security_solution/cypress/integration/detection_rules/custom_query_rule.spec.ts

@@ -26,12 +26,12 @@ import {
   SHOWING_RULES_TEXT,
 } from '../../screens/alerts_detection_rules';
 import {
-  // ABOUT_CONTINUE_BTN,
-  // ABOUT_EDIT_BUTTON,
+  ABOUT_CONTINUE_BTN,
+  ABOUT_EDIT_BUTTON,
   ACTIONS_THROTTLE_INPUT,
   CUSTOM_QUERY_INPUT,
-  // DEFINE_CONTINUE_BUTTON,
-  // DEFINE_EDIT_BUTTON,
+  DEFINE_CONTINUE_BUTTON,
+  DEFINE_EDIT_BUTTON,
   DEFINE_INDEX_INPUT,
   DEFAULT_RISK_SCORE_INPUT,
   RULE_DESCRIPTION_INPUT,
@@ -134,7 +134,6 @@ describe('Custom query rules', () => {
       fillAboutRuleAndContinue(this.rule);
       fillScheduleRuleAndContinue(this.rule);
 
-      /* Commenting this piece of code due to the following issue: https://github.com/elastic/kibana/issues/130767
       // expect define step to repopulate
       cy.get(DEFINE_EDIT_BUTTON).click();
       cy.get(CUSTOM_QUERY_INPUT).should('have.value', this.rule.customQuery);
@@ -145,7 +144,7 @@ describe('Custom query rules', () => {
       cy.get(ABOUT_EDIT_BUTTON).click();
       cy.get(RULE_NAME_INPUT).invoke('val').should('eql', this.rule.name);
       cy.get(ABOUT_CONTINUE_BTN).should('exist').click({ force: true });
-      cy.get(ABOUT_CONTINUE_BTN).should('not.exist');  */
+      cy.get(ABOUT_CONTINUE_BTN).should('not.exist');
 
       createAndEnableRule();
 

x-pack/plugins/security_solution/cypress/integration/detection_rules/indicator_match_rule.spec.ts

@@ -178,13 +178,11 @@ describe('indicator match', () => {
           selectIndicatorMatchType();
         });
 
-        // Unskip once https://github.com/elastic/kibana/issues/130770 is fixed
-        it.skip('Has a default set of *:*', () => {
+        it('Has a default set of *:*', () => {
           getCustomQueryInput().should('have.text', '*:*');
         });
 
-        // Unskip once https://github.com/elastic/kibana/issues/1307707 is fixed
-        it.skip('Shows invalidation text if text is removed', () => {
+        it('Shows invalidation text if text is removed', () => {
           getCustomQueryInput().type('{selectall}{del}');
           getCustomQueryInvalidationText().should('exist');
         });
@@ -401,8 +399,7 @@ describe('indicator match', () => {
       });
 
       describe('Schedule', () => {
-        // Unskip once https://github.com/elastic/kibana/issues/1307707 is fixed
-        it.skip('IM rule has 1h time interval and lookback by default', () => {
+        it('IM rule has 1h time interval and lookback by default', () => {
           visitWithoutDateRange(RULE_CREATION);
           selectIndicatorMatchType();
           fillDefineIndicatorMatchRuleAndContinue(getNewThreatIndicatorRule());
@@ -421,8 +418,7 @@ describe('indicator match', () => {
         deleteAlertsAndRules();
       });
 
-      // Unskip once https://github.com/elastic/kibana/issues/1307707 is fixed
-      it.skip('Creates and enables a new Indicator Match rule', () => {
+      it('Creates and enables a new Indicator Match rule', () => {
         visitWithoutDateRange(RULE_CREATION);
         selectIndicatorMatchType();
         fillDefineIndicatorMatchRuleAndContinue(getNewThreatIndicatorRule());

x-pack/plugins/security_solution/public/detections/components/rules/eql_query_bar/eql_query_bar.test.tsx

@@ -48,6 +48,7 @@ describe('EqlQueryBar', () => {
         query: 'newQuery',
         language: 'eql',
       },
+      saved_id: null,
     };
 
     expect(mockField.setValue).toHaveBeenCalledWith(expected);

x-pack/plugins/security_solution/public/detections/components/rules/eql_query_bar/eql_query_bar.tsx

@@ -96,6 +96,7 @@ export const EqlQueryBar: FC<EqlQueryBarProps> = ({
           query: newQuery,
           language: 'eql',
         },
+        saved_id: null,
       });
     },
     [setValue, onValiditingChange]

x-pack/plugins/security_solution/public/detections/components/rules/query_bar/index.tsx

@@ -29,7 +29,7 @@ import * as i18n from './translations';
 export interface FieldValueQueryBar {
   filters: Filter[];
   query: Query;
-  saved_id?: string;
+  saved_id: string | null;
 }
 interface QueryBarDefineRuleProps {
   browserFields: BrowserFields;
@@ -176,7 +176,7 @@ export const QueryBarDefineRule = ({
               query: '',
               language: 'kuery',
             },
-            saved_id: undefined,
+            saved_id: null,
           });
         }
       }
@@ -210,7 +210,7 @@ export const QueryBarDefineRule = ({
             ? [...newFilters, getDataProviderFilter(dataProvidersDsl)]
             : newFilters,
         query: newQuery,
-        saved_id: undefined,
+        saved_id: null,
       });
     },
     [browserFields, indexPattern, setFieldValue]

x-pack/plugins/security_solution/public/detections/components/rules/rule_preview/helpers.test.ts

@@ -75,7 +75,7 @@ describe('query_preview/helpers', () => {
           { entries: [{ field: 'test-field', value: 'test-value', type: 'mapping' }] },
         ],
         machineLearningJobId: ['test-ml-job-id'],
-        queryBar: { filters: [], query: { query: '', language: 'testlang' } },
+        queryBar: { filters: [], query: { query: '', language: 'testlang' }, saved_id: null },
       });
       expect(isDisabled).toEqual(true);
     });
@@ -91,7 +91,7 @@ describe('query_preview/helpers', () => {
           { entries: [{ field: 'test-field', value: 'test-value', type: 'mapping' }] },
         ],
         machineLearningJobId: ['test-ml-job-id'],
-        queryBar: { filters: [], query: { query: '', language: 'testlang' } },
+        queryBar: { filters: [], query: { query: '', language: 'testlang' }, saved_id: null },
       });
       expect(isDisabled).toEqual(true);
     });
@@ -107,7 +107,7 @@ describe('query_preview/helpers', () => {
           { entries: [{ field: 'test-field', value: 'test-value', type: 'mapping' }] },
         ],
         machineLearningJobId: ['test-ml-job-id'],
-        queryBar: { filters: [], query: { query: '', language: 'testlang' } },
+        queryBar: { filters: [], query: { query: '', language: 'testlang' }, saved_id: null },
       });
       expect(isDisabled).toEqual(true);
     });
@@ -123,7 +123,7 @@ describe('query_preview/helpers', () => {
           { entries: [{ field: 'test-field', value: 'test-value', type: 'mapping' }] },
         ],
         machineLearningJobId: ['test-ml-job-id'],
-        queryBar: { filters: [], query: { query: '', language: 'testlang' } },
+        queryBar: { filters: [], query: { query: '', language: 'testlang' }, saved_id: null },
       });
       expect(isDisabled).toEqual(true);
     });
@@ -137,7 +137,7 @@ describe('query_preview/helpers', () => {
         threatIndex: ['threat-*'],
         threatMapping: [],
         machineLearningJobId: ['test-ml-job-id'],
-        queryBar: { filters: [], query: { query: '', language: 'testlang' } },
+        queryBar: { filters: [], query: { query: '', language: 'testlang' }, saved_id: null },
       });
       expect(isDisabled).toEqual(true);
     });
@@ -151,7 +151,7 @@ describe('query_preview/helpers', () => {
         threatIndex: ['threat-*'],
         threatMapping: [],
         machineLearningJobId: [],
-        queryBar: { filters: [], query: { query: '', language: 'testlang' } },
+        queryBar: { filters: [], query: { query: '', language: 'testlang' }, saved_id: null },
       });
       expect(isDisabled).toEqual(true);
     });
@@ -165,7 +165,7 @@ describe('query_preview/helpers', () => {
         threatIndex: ['threat-*'],
         threatMapping: [],
         machineLearningJobId: [],
-        queryBar: { filters: [], query: { query: '', language: 'testlang' } },
+        queryBar: { filters: [], query: { query: '', language: 'testlang' }, saved_id: null },
       });
       expect(isDisabled).toEqual(true);
     });
@@ -181,7 +181,7 @@ describe('query_preview/helpers', () => {
           { entries: [{ field: 'test-field', value: 'test-value', type: 'mapping' }] },
         ],
         machineLearningJobId: ['test-ml-job-id'],
-        queryBar: { filters: [], query: { query: '', language: 'testlang' } },
+        queryBar: { filters: [], query: { query: '', language: 'testlang' }, saved_id: null },
       });
       expect(isDisabled).toEqual(false);
     });
@@ -195,7 +195,11 @@ describe('query_preview/helpers', () => {
         threatIndex: ['threat-*'],
         threatMapping: [],
         machineLearningJobId: [],
-        queryBar: { filters: [], query: { query: 'any where true', language: 'testlang' } },
+        queryBar: {
+          filters: [],
+          query: { query: 'any where true', language: 'testlang' },
+          saved_id: null,
+        },
       });
       expect(isDisabled).toEqual(false);
     });
@@ -234,6 +238,7 @@ describe('query_preview/helpers', () => {
         {
           query: { query: 'host.name:*', language: 'kuery' },
           filters: [{ meta: { alias: '', disabled: false, negate: false } }],
+          saved_id: null,
         },
         ['foo-*'],
         'query'
@@ -260,6 +265,7 @@ describe('query_preview/helpers', () => {
         {
           query: { query: 'host.name:*', language: 'kuery' },
           filters: [{ meta: { alias: '', disabled: false, negate: false } }],
+          saved_id: null,
         },
         ['foo-*'],
         'saved_query'
@@ -286,6 +292,7 @@ describe('query_preview/helpers', () => {
         {
           query: { query: 'host.name:*', language: 'kuery' },
           filters: [{ meta: { alias: '', disabled: false, negate: false } }],
+          saved_id: null,
         },
         ['foo-*'],
         'threshold'
@@ -312,6 +319,7 @@ describe('query_preview/helpers', () => {
         {
           query: { query: 'file where true', language: 'eql' },
           filters: [{ meta: { alias: '', disabled: false, negate: false } }],
+          saved_id: null,
         },
         ['foo-*'],
         'eql'
@@ -329,6 +337,7 @@ describe('query_preview/helpers', () => {
         {
           query: { query: 'host.name:', language: 'kuery' },
           filters: [{ meta: { alias: '', disabled: false, negate: false } }],
+          saved_id: null,
         },
         ['foo-*'],
         'threshold'

x-pack/plugins/security_solution/public/detections/components/rules/rule_preview/index.test.tsx

@@ -40,10 +40,12 @@ const defaultProps: RulePreviewProps = {
   query: {
     filters: [],
     query: { query: 'file.hash.md5:*', language: 'kuery' },
+    saved_id: null,
   },
   threatQuery: {
     filters: [],
     query: { query: 'threat.indicator.file.hash.md5:*', language: 'kuery' },
+    saved_id: null,
   },
   threshold: {
     field: ['agent.hostname'],

x-pack/plugins/security_solution/public/detections/components/rules/step_define_rule/index.tsx

@@ -10,6 +10,7 @@ import React, { FC, memo, useCallback, useMemo, useState, useEffect } from 'reac
 import styled from 'styled-components';
 import { isEqual, isEmpty } from 'lodash';
 import { FieldSpec } from '@kbn/data-views-plugin/common';
+import usePrevious from 'react-use/lib/usePrevious';
 
 import {
   DEFAULT_INDEX_KEY,
@@ -75,12 +76,12 @@ export const stepDefineDefaultValue: DefineStepRule = {
   queryBar: {
     query: { query: '', language: 'kuery' },
     filters: [],
-    saved_id: undefined,
+    saved_id: null,
   },
   threatQueryBar: {
     query: { query: DEFAULT_THREAT_MATCH_QUERY, language: 'kuery' },
     filters: [],
-    saved_id: undefined,
+    saved_id: null,
   },
   requiredFields: [],
   relatedIntegrations: [],
@@ -111,6 +112,11 @@ const threatQueryBarDefaultValue: DefineStepRule['queryBar'] = {
   query: { ...stepDefineDefaultValue.queryBar.query, query: '*:*' },
 };
 
+const defaultCustomQuery = {
+  forNormalRules: stepDefineDefaultValue.queryBar,
+  forThreatMatchRules: threatQueryBarDefaultValue,
+};
+
 export const MyLabelButton = styled(EuiButtonEmpty)`
   height: 18px;
   font-size: 12px;
@@ -195,6 +201,7 @@ const StepDefineRuleComponent: FC<StepDefineRuleProps> = ({
   const machineLearningJobId = formMachineLearningJobId ?? initialState.machineLearningJobId;
   const anomalyThreshold = formAnomalyThreshold ?? initialState.anomalyThreshold;
   const ruleType = formRuleType || initialState.ruleType;
+  const previousRuleType = usePrevious(ruleType);
   const [indexPatternsLoading, { browserFields, indexPatterns }] = useFetchIndex(index);
   const fields: Readonly<BrowserFields> = aggregatableFields(browserFields);
   const [optionsSelected, setOptionsSelected] = useState<EqlOptionsSelected>(
@@ -220,36 +227,55 @@ const StepDefineRuleComponent: FC<StepDefineRuleProps> = ({
   }, [threatIndex, threatIndicesConfig]);
 
   /**
-   * When a rule type is changed to or from a threat match this will modify the
-   * default query string to either:
-   *   * from the empty string '' to '*:*' if the rule type is "threatMatchRule"
-   *   * from '*:*' back to the empty string '' if the rule type is not "threatMatchRule"
-   * This calls queryBar.reset() in both cases to not trigger validation errors as
-   * the user has not entered data into those areas yet.
-   * If the user has entered data then through reference compares we can detect reliably if
-   * the user has changed data.
-   *   * queryBar.value === defaultQueryBar (Has the user changed the input of '' yet?)
-   *   * queryBar.value === threatQueryBarDefaultValue (Has the user changed the input of '*:*' yet?)
-   * This is a stronger guarantee than "isPristine" off of the forms as that value can be reset
-   * if you go to step 2) and then back to step 1) or the form is reset in another way. Using
-   * the reference compare we know factually if the data is changed as the references must change
-   * in the form libraries form the initial defaults.
+   * When the user changes rule type to or from "threat_match" this will modify the
+   * default "Custom query" string to either:
+   *   * from '' to '*:*' if the type is switched to "threat_match"
+   *   * from '*:*' back to '' if the type is switched back from "threat_match" to another one
    */
   useEffect(() => {
     const { queryBar } = getFields();
-    if (queryBar != null) {
-      const { queryBar: defaultQueryBar } = stepDefineDefaultValue;
-      if (isThreatMatchRule(ruleType) && queryBar.value === defaultQueryBar) {
+    if (queryBar == null) {
+      return;
+    }
+
+    // NOTE: Below this code does two things that are worth commenting.
+
+    // 1. If the user enters some text in the "Custom query" form field, we want
+    // to keep it even if the user switched to another rule type. So we want to
+    // be able to figure out if the field has been modified.
+    // - The forms library provides properties (isPristine, isModified, isDirty)
+    //   for that but they can't be used in our case: their values can be reset
+    //   if you go to step 2 and then back to step 1 or the form is reset in another way.
+    // - That's why we compare the actual value of the field with default ones.
+    //   NOTE: It's important to do a deep object comparison by value.
+    //   Don't do it by reference because the forms lib can change it internally.
+
+    // 2. We call queryBar.reset() in both cases to not trigger validation errors
+    // as the user has not entered data into those areas yet.
+
+    // If the user switched rule type to "threat_match" from any other one,
+    // but hasn't changed the custom query used for normal rules (''),
+    // we reset the custom query to the default used for "threat_match" rules ('*:*').
+    if (isThreatMatchRule(ruleType) && !isThreatMatchRule(previousRuleType)) {
+      if (isEqual(queryBar.value, defaultCustomQuery.forNormalRules)) {
         queryBar.reset({
-          defaultValue: threatQueryBarDefaultValue,
+          defaultValue: defaultCustomQuery.forThreatMatchRules,
         });
-      } else if (queryBar.value === threatQueryBarDefaultValue) {
+        return;
+      }
+    }
+
+    // If the user switched rule type from "threat_match" to any other one,
+    // but hasn't changed the custom query used for "threat_match" rules ('*:*'),
+    // we reset the custom query to another default value ('').
+    if (!isThreatMatchRule(ruleType) && isThreatMatchRule(previousRuleType)) {
+      if (isEqual(queryBar.value, defaultCustomQuery.forThreatMatchRules)) {
         queryBar.reset({
-          defaultValue: defaultQueryBar,
+          defaultValue: defaultCustomQuery.forNormalRules,
         });
       }
     }
-  }, [ruleType, getFields]);
+  }, [ruleType, previousRuleType, getFields]);
 
   const handleSubmit = useCallback(() => {
     if (onSubmit) {

x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/create/helpers.test.ts

@@ -364,6 +364,7 @@ describe('helpers', () => {
         threatQueryBar: {
           query: { language: 'kql', query: 'threat_host: *' },
           filters: threatFilters,
+          saved_id: null,
         },
         threatMapping,
       };