[Security Solution][Exceptions] Prevents value list entries from co-e…

…xisting with non value list entries (#72995) (#73217) ## Summary Fixes validation issue where value list exception entries could be added alongside non value list exception entries. Once a value list operator (`is in list` or `is not in list`) is selected the `nested` button will disable, and subsequent `and`/ `or`'s will only have the value list operators available to them: <p align="center"> <img width="400" src="https://user-images.githubusercontent.com/2946766/88323026-d36a8180-ccde-11ea-97f0-16490bf9a671.gif" /> </p> If a value list is not selected in the first exception entry, all subsequent entries will no longer have the value list operators: <p align="center"> <img width="400" src="https://user-images.githubusercontent.com/2946766/88250505-08d28900-cc65-11ea-9a6c-5b869e3d7f7f.gif" /> </p> Adds validation for empty case to prevent network error when submitted no entries. Add modal: <p align="center"> <img width="400" src="https://user-images.githubusercontent.com/2946766/88299601-c5593880-ccbf-11ea-9468-30796a0f9748.gif" /> </p> Edit modal: <p align="center"> <img width="400" src="https://user-images.githubusercontent.com/2946766/88299609-c7bb9280-ccbf-11ea-943c-836b641d7ae3.gif" /> </p> ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios
elastic · Jul 25, 2020 · 559bd87 · 559bd87
1 parent faba7fe
commit 559bd87
Show file tree

Hide file tree

Showing 10 changed files with 113 additions and 38 deletions.
diff --git a/x-pack/plugins/security_solution/public/common/components/autocomplete/operators.ts b/x-pack/plugins/security_solution/public/common/components/autocomplete/operators.ts
@@ -90,3 +90,17 @@ export const EXCEPTION_OPERATORS: OperatorOption[] = [
   isInListOperator,
   isNotInListOperator,
 ];
+
+export const EXCEPTION_OPERATORS_SANS_LISTS: OperatorOption[] = [
+  isOperator,
+  isNotOperator,
+  isOneOfOperator,
+  isNotOneOfOperator,
+  existsOperator,
+  doesNotExistOperator,
+];
+
+export const EXCEPTION_OPERATORS_ONLY_LISTS: OperatorOption[] = [
+  isInListOperator,
+  isNotInListOperator,
+];
diff --git a/...ugins/security_solution/public/common/components/exceptions/add_exception_modal/index.tsx b/...ugins/security_solution/public/common/components/exceptions/add_exception_modal/index.tsx
@@ -276,8 +276,8 @@ export const AddExceptionModal = memo(function AddExceptionModal({
     signalIndexName,
   ]);
 
-  const isSubmitButtonDisabled = useCallback(
-    () => fetchOrCreateListError || exceptionItemsToAdd.length === 0,
+  const isSubmitButtonDisabled = useMemo(
+    () => fetchOrCreateListError || exceptionItemsToAdd.every((item) => item.entries.length === 0),
     [fetchOrCreateListError, exceptionItemsToAdd]
   );
 
@@ -377,7 +377,7 @@ export const AddExceptionModal = memo(function AddExceptionModal({
           <EuiButton
             onClick={onAddExceptionConfirm}
             isLoading={addExceptionIsLoading}
-            isDisabled={isSubmitButtonDisabled()}
+            isDisabled={isSubmitButtonDisabled}
             fill
           >
             {i18n.ADD_EXCEPTION}

diff --git a/...security_solution/public/common/components/exceptions/builder/builder_entry_item.test.tsx b/...security_solution/public/common/components/exceptions/builder/builder_entry_item.test.tsx
@@ -213,7 +213,7 @@ describe('BuilderEntryItem', () => {
           title: 'logstash-*',
           fields,
         }}
-        showLabel={false}
+        showLabel={true}
         listType="detection"
         addNested={false}
         onChange={jest.fn()}
@@ -245,7 +245,7 @@ describe('BuilderEntryItem', () => {
           title: 'logstash-*',
           fields,
         }}
-        showLabel={false}
+        showLabel={true}
         listType="detection"
         addNested={false}
         onChange={jest.fn()}

diff --git a/...gins/security_solution/public/common/components/exceptions/builder/builder_entry_item.tsx b/...gins/security_solution/public/common/components/exceptions/builder/builder_entry_item.tsx
@@ -27,6 +27,7 @@ import {
   getEntryOnMatchAnyChange,
   getEntryOnListChange,
 } from './helpers';
+import { EXCEPTION_OPERATORS_ONLY_LISTS } from '../../autocomplete/operators';
 
 interface EntryItemProps {
   entry: FormattedBuilderEntry;
@@ -35,6 +36,7 @@ interface EntryItemProps {
   listType: ExceptionListType;
   addNested: boolean;
   onChange: (arg: BuilderEntry, i: number) => void;
+  onlyShowListOperators?: boolean;
 }
 
 export const BuilderEntryItem: React.FC<EntryItemProps> = ({
@@ -44,6 +46,7 @@ export const BuilderEntryItem: React.FC<EntryItemProps> = ({
   addNested,
   showLabel,
   onChange,
+  onlyShowListOperators = false,
 }): JSX.Element => {
   const handleFieldChange = useCallback(
     ([newField]: IFieldType[]): void => {
@@ -124,11 +127,14 @@ export const BuilderEntryItem: React.FC<EntryItemProps> = ({
   );
 
   const renderOperatorInput = (isFirst: boolean): JSX.Element => {
-    const operatorOptions = getOperatorOptions(
-      entry,
-      listType,
-      entry.field != null && entry.field.type === 'boolean'
-    );
+    const operatorOptions = onlyShowListOperators
+      ? EXCEPTION_OPERATORS_ONLY_LISTS
+      : getOperatorOptions(
+          entry,
+          listType,
+          entry.field != null && entry.field.type === 'boolean',
+          isFirst
+        );
     const comboBox = (
       <OperatorComponent
         placeholder={i18n.EXCEPTION_OPERATOR_PLACEHOLDER}

diff --git a/.../security_solution/public/common/components/exceptions/builder/builder_exception_item.tsx b/.../security_solution/public/common/components/exceptions/builder/builder_exception_item.tsx
@@ -44,6 +44,7 @@ interface ExceptionListItemProps {
   addNested: boolean;
   onDeleteExceptionItem: (item: ExceptionsBuilderExceptionItem, index: number) => void;
   onChangeExceptionItem: (item: ExceptionsBuilderExceptionItem, index: number) => void;
+  onlyShowListOperators?: boolean;
 }
 
 export const ExceptionListItemComponent = React.memo<ExceptionListItemProps>(
@@ -58,6 +59,7 @@ export const ExceptionListItemComponent = React.memo<ExceptionListItemProps>(
     andLogicIncluded,
     onDeleteExceptionItem,
     onChangeExceptionItem,
+    onlyShowListOperators = false,
   }) => {
     const handleEntryChange = useCallback(
       (entry: BuilderEntry, entryIndex: number): void => {
@@ -169,6 +171,7 @@ export const ExceptionListItemComponent = React.memo<ExceptionListItemProps>(
                           exceptionItemIndex === 0 && index === 0 && item.nested !== 'child'
                         }
                         onChange={handleEntryChange}
+                        onlyShowListOperators={onlyShowListOperators}
                       />
                     </EuiFlexItem>
                     {getDeleteButton(

diff --git a/...ck/plugins/security_solution/public/common/components/exceptions/builder/helpers.test.tsx b/...ck/plugins/security_solution/public/common/components/exceptions/builder/helpers.test.tsx
@@ -14,32 +14,33 @@ import { getEntryExistsMock } from '../../../../../../lists/common/schemas/types
 import { getExceptionListItemSchemaMock } from '../../../../../../lists/common/schemas/response/exception_list_item_schema.mock';
 import { getListResponseMock } from '../../../../../../lists/common/schemas/response/list_schema.mock';
 import {
-  isOperator,
-  isOneOfOperator,
-  isNotOperator,
-  isNotOneOfOperator,
-  existsOperator,
   doesNotExistOperator,
-  isInListOperator,
   EXCEPTION_OPERATORS,
+  EXCEPTION_OPERATORS_SANS_LISTS,
+  existsOperator,
+  isInListOperator,
+  isNotOneOfOperator,
+  isNotOperator,
+  isOneOfOperator,
+  isOperator,
 } from '../../autocomplete/operators';
-import { FormattedBuilderEntry, BuilderEntry, ExceptionsBuilderExceptionItem } from '../types';
-import { IIndexPattern, IFieldType } from '../../../../../../../../src/plugins/data/common';
-import { EntryNested, Entry } from '../../../../lists_plugin_deps';
+import { BuilderEntry, ExceptionsBuilderExceptionItem, FormattedBuilderEntry } from '../types';
+import { IFieldType, IIndexPattern } from '../../../../../../../../src/plugins/data/common';
+import { Entry, EntryNested } from '../../../../lists_plugin_deps';
 
 import {
-  getFilteredIndexPatterns,
-  getFormattedBuilderEntry,
-  isEntryNested,
-  getFormattedBuilderEntries,
-  getUpdatedEntriesOnDelete,
   getEntryFromOperator,
-  getOperatorOptions,
   getEntryOnFieldChange,
-  getEntryOnOperatorChange,
-  getEntryOnMatchChange,
-  getEntryOnMatchAnyChange,
   getEntryOnListChange,
+  getEntryOnMatchAnyChange,
+  getEntryOnMatchChange,
+  getEntryOnOperatorChange,
+  getFilteredIndexPatterns,
+  getFormattedBuilderEntries,
+  getFormattedBuilderEntry,
+  getOperatorOptions,
+  getUpdatedEntriesOnDelete,
+  isEntryNested,
 } from './helpers';
 import { OperatorOption } from '../../autocomplete/types';
 
@@ -672,6 +673,18 @@ describe('Exception builder helpers', () => {
       const expected: OperatorOption[] = [isOperator, existsOperator];
       expect(output).toEqual(expected);
     });
+
+    test('it returns list operators if specified to', () => {
+      const payloadItem: FormattedBuilderEntry = getMockBuilderEntry();
+      const output = getOperatorOptions(payloadItem, 'detection', false, true);
+      expect(output).toEqual(EXCEPTION_OPERATORS);
+    });
+
+    test('it does not return list operators if specified not to', () => {
+      const payloadItem: FormattedBuilderEntry = getMockBuilderEntry();
+      const output = getOperatorOptions(payloadItem, 'detection', false, false);
+      expect(output).toEqual(EXCEPTION_OPERATORS_SANS_LISTS);
+    });
   });
 
   describe('#getEntryOnFieldChange', () => {

diff --git a/x-pack/plugins/security_solution/public/common/components/exceptions/builder/helpers.tsx b/x-pack/plugins/security_solution/public/common/components/exceptions/builder/helpers.tsx
@@ -22,6 +22,7 @@ import {
   existsOperator,
   isOneOfOperator,
   EXCEPTION_OPERATORS,
+  EXCEPTION_OPERATORS_SANS_LISTS,
 } from '../../autocomplete/operators';
 import { OperatorOption } from '../../autocomplete/types';
 import {
@@ -40,7 +41,6 @@ import { getEntryValue, getExceptionOperatorSelect } from '../helpers';
  *
  * @param patterns IIndexPattern containing available fields on rule index
  * @param item exception item entry
- * @param addNested boolean noting whether or not UI is currently
  * set to add a nested field
  */
 export const getFilteredIndexPatterns = (
@@ -295,12 +295,14 @@ export const getEntryFromOperator = (
  *
  * @param item
  * @param listType
- *
+ * @param isBoolean
+ * @param includeValueListOperators whether or not to include the 'is in list' and 'is not in list' operators
  */
 export const getOperatorOptions = (
   item: FormattedBuilderEntry,
   listType: ExceptionListType,
-  isBoolean: boolean
+  isBoolean: boolean,
+  includeValueListOperators = true
 ): OperatorOption[] => {
   if (item.nested === 'parent' || item.field == null) {
     return [isOperator];
@@ -309,7 +311,11 @@ export const getOperatorOptions = (
   } else if (item.nested != null && listType === 'detection') {
     return isBoolean ? [isOperator, existsOperator] : [isOperator, isOneOfOperator, existsOperator];
   } else {
-    return isBoolean ? [isOperator, existsOperator] : EXCEPTION_OPERATORS;
+    return isBoolean
+      ? [isOperator, existsOperator]
+      : includeValueListOperators
+      ? EXCEPTION_OPERATORS
+      : EXCEPTION_OPERATORS_SANS_LISTS;
   }
 };
 
@@ -547,3 +553,6 @@ export const getDefaultNestedEmptyEntry = (): EmptyNestedEntry => ({
   type: OperatorTypeEnum.NESTED,
   entries: [],
 });
+
+export const containsValueListEntry = (items: ExceptionsBuilderExceptionItem[]): boolean =>
+  items.some((item) => item.entries.some((entry) => entry.type === OperatorTypeEnum.LIST));
diff --git a/x-pack/plugins/security_solution/public/common/components/exceptions/builder/index.tsx b/x-pack/plugins/security_solution/public/common/components/exceptions/builder/index.tsx
@@ -24,7 +24,11 @@ import { BuilderButtonOptions } from './builder_button_options';
 import { getNewExceptionItem, filterExceptionItems } from '../helpers';
 import { ExceptionsBuilderExceptionItem, CreateExceptionListItemBuilderSchema } from '../types';
 import { State, exceptionsBuilderReducer } from './reducer';
-import { getDefaultEmptyEntry, getDefaultNestedEmptyEntry } from './helpers';
+import {
+  containsValueListEntry,
+  getDefaultEmptyEntry,
+  getDefaultNestedEmptyEntry,
+} from './helpers';
 // eslint-disable-next-line @kbn/eslint/no-restricted-paths
 import exceptionableFields from '../exceptionable_fields.json';
 
@@ -44,6 +48,7 @@ const MyButtonsContainer = styled(EuiFlexItem)`
 
 const initialState: State = {
   disableAnd: false,
+  disableNested: false,
   disableOr: false,
   andLogicIncluded: false,
   addNested: false,
@@ -82,12 +87,21 @@ export const ExceptionBuilder = ({
   onChange,
 }: ExceptionBuilderProps) => {
   const [
-    { exceptions, exceptionsToDelete, andLogicIncluded, disableAnd, disableOr, addNested },
+    {
+      exceptions,
+      exceptionsToDelete,
+      andLogicIncluded,
+      disableAnd,
+      disableNested,
+      disableOr,
+      addNested,
+    },
     dispatch,
   ] = useReducer(exceptionsBuilderReducer(), {
     ...initialState,
     disableAnd: isAndDisabled,
     disableOr: isOrDisabled,
+    disableNested: isNestedDisabled,
   });
 
   const setUpdateExceptions = useCallback(
@@ -362,6 +376,7 @@ export const ExceptionBuilder = ({
                 isOnlyItem={exceptions.length === 1}
                 onDeleteExceptionItem={handleDeleteExceptionItem}
                 onChangeExceptionItem={handleExceptionItemChange}
+                onlyShowListOperators={containsValueListEntry(exceptions)}
               />
             </EuiFlexItem>
           </EuiFlexGroup>
@@ -379,7 +394,7 @@ export const ExceptionBuilder = ({
             <BuilderButtonOptions
               isOrDisabled={disableOr}
               isAndDisabled={disableAnd}
-              isNestedDisabled={isNestedDisabled}
+              isNestedDisabled={disableNested}
               isNested={addNested}
               showNestedButton
               onOrClicked={handleAddNewExceptionItem}

diff --git a/x-pack/plugins/security_solution/public/common/components/exceptions/builder/reducer.ts b/x-pack/plugins/security_solution/public/common/components/exceptions/builder/reducer.ts
@@ -4,13 +4,14 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 import { ExceptionsBuilderExceptionItem } from '../types';
-import { ExceptionListItemSchema } from '../../../../../public/lists_plugin_deps';
+import { ExceptionListItemSchema, OperatorTypeEnum } from '../../../../../public/lists_plugin_deps';
 import { getDefaultEmptyEntry } from './helpers';
 
 export type ViewerModalName = 'addModal' | 'editModal' | null;
 
 export interface State {
   disableAnd: boolean;
+  disableNested: boolean;
   disableOr: boolean;
   andLogicIncluded: boolean;
   addNested: boolean;
@@ -59,6 +60,9 @@ export const exceptionsBuilderReducer = () => (state: State, action: Action): St
       const isAndDisabled =
         lastEntry != null && lastEntry.type === 'nested' && lastEntry.entries.length === 0;
       const isOrDisabled = lastEntry != null && lastEntry.type === 'nested';
+      const containsValueList = action.exceptions.some(
+        ({ entries }) => entries.filter(({ type }) => type === OperatorTypeEnum.LIST).length > 0
+      );
 
       return {
         ...state,
@@ -67,6 +71,7 @@ export const exceptionsBuilderReducer = () => (state: State, action: Action): St
         addNested: isAddNested,
         disableAnd: isAndDisabled,
         disableOr: isOrDisabled,
+        disableNested: containsValueList,
       };
     }
     case 'setDefault': {

diff --git a/...gins/security_solution/public/common/components/exceptions/edit_exception_modal/index.tsx b/...gins/security_solution/public/common/components/exceptions/edit_exception_modal/index.tsx
@@ -4,7 +4,7 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import React, { memo, useState, useCallback, useEffect } from 'react';
+import React, { memo, useState, useCallback, useEffect, useMemo } from 'react';
 import styled, { css } from 'styled-components';
 import {
   EuiModal,
@@ -146,6 +146,11 @@ export const EditExceptionModal = memo(function EditExceptionModal({
     }
   }, [shouldDisableBulkClose]);
 
+  const isSubmitButtonDisabled = useMemo(
+    () => exceptionItemsToAdd.every((item) => item.entries.length === 0),
+    [exceptionItemsToAdd]
+  );
+
   const handleBuilderOnChange = useCallback(
     ({
       exceptionItems,
@@ -261,7 +266,12 @@ export const EditExceptionModal = memo(function EditExceptionModal({
         <EuiModalFooter>
           <EuiButtonEmpty onClick={onCancel}>{i18n.CANCEL}</EuiButtonEmpty>
 
-          <EuiButton onClick={onEditExceptionConfirm} isLoading={addExceptionIsLoading} fill>
+          <EuiButton
+            onClick={onEditExceptionConfirm}
+            isLoading={addExceptionIsLoading}
+            isDisabled={isSubmitButtonDisabled}
+            fill
+          >
             {i18n.EDIT_EXCEPTION_SAVE_BUTTON}
           </EuiButton>
         </EuiModalFooter>