From 8ad5ecef03768ebd28a6cd9336762bb6acb6a62e Mon Sep 17 00:00:00 2001 From: Yara Tercero Date: Wed, 8 Jul 2020 22:30:35 -0400 Subject: [PATCH] [Security Solution][Exceptions Builder] - Fixes operator selection bug (#71178) ### Summary This PR fixes two bugs in the exceptions builder. The first was that it was not allowing you to select any of the "excluded" operators. The second was that it was not adding the "and" badge when it should on initial render. It also adds unit tests for the EntryItemComponent. --- .../exceptions/builder/entry_item.test.tsx | 438 ++++++++++++++++++ .../exceptions/builder/entry_item.tsx | 35 +- .../exceptions/builder/exception_item.tsx | 13 +- .../components/exceptions/builder/index.tsx | 19 +- 4 files changed, 477 insertions(+), 28 deletions(-) create mode 100644 x-pack/plugins/security_solution/public/common/components/exceptions/builder/entry_item.test.tsx diff --git a/x-pack/plugins/security_solution/public/common/components/exceptions/builder/entry_item.test.tsx b/x-pack/plugins/security_solution/public/common/components/exceptions/builder/entry_item.test.tsx new file mode 100644 index 000000000000..791782b0f015 --- /dev/null +++ b/x-pack/plugins/security_solution/public/common/components/exceptions/builder/entry_item.test.tsx @@ -0,0 +1,438 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { mount } from 'enzyme'; +import React from 'react'; +import { EuiComboBox, EuiComboBoxOptionOption } from '@elastic/eui'; + +import { EntryItemComponent } from './entry_item'; +import { + isOperator, + isNotOperator, + isOneOfOperator, + isNotOneOfOperator, + isInListOperator, + isNotInListOperator, + existsOperator, + doesNotExistOperator, +} from '../../autocomplete/operators'; +import { + fields, + getField, +} from '../../../../../../../../src/plugins/data/common/index_patterns/fields/fields.mocks.ts'; +import { getFoundListSchemaMock } from '../../../../../../lists/common/schemas/response/found_list_schema.mock'; +import { getEmptyValue } from '../../empty_value'; + +// mock out lists hook +const mockStart = jest.fn(); +const mockResult = getFoundListSchemaMock(); +jest.mock('../../../../common/lib/kibana'); +jest.mock('../../../../lists_plugin_deps', () => { + const originalModule = jest.requireActual('../../../../lists_plugin_deps'); + + return { + ...originalModule, + useFindLists: () => ({ + loading: false, + start: mockStart.mockReturnValue(mockResult), + result: mockResult, + error: undefined, + }), + }; +}); + +describe('EntryItemComponent', () => { + test('it renders fields disabled if "isLoading" is "true"', () => { + const wrapper = mount( + + ); + + expect( + wrapper.find('[data-test-subj="exceptionBuilderEntryField"] input').props().disabled + ).toBeTruthy(); + expect( + wrapper.find('[data-test-subj="exceptionBuilderEntryOperator"] input').props().disabled + ).toBeTruthy(); + expect( + wrapper.find('[data-test-subj="exceptionBuilderEntryFieldMatch"] input').props().disabled + ).toBeTruthy(); + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryFieldFormRow"]')).toHaveLength(0); + }); + + test('it renders field labels if "showLabel" is "true"', () => { + const wrapper = mount( + + ); + + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryFieldFormRow"]')).not.toEqual(0); + }); + + test('it renders field values correctly when operator is "isOperator"', () => { + const wrapper = mount( + + ); + + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryField"]').text()).toEqual('ip'); + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryOperator"]').text()).toEqual('is'); + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryFieldMatch"]').text()).toEqual( + '1234' + ); + }); + + test('it renders field values correctly when operator is "isNotOperator"', () => { + const wrapper = mount( + + ); + + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryField"]').text()).toEqual('ip'); + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryOperator"]').text()).toEqual( + 'is not' + ); + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryFieldMatch"]').text()).toEqual( + '1234' + ); + }); + + test('it renders field values correctly when operator is "isOneOfOperator"', () => { + const wrapper = mount( + + ); + + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryField"]').text()).toEqual('ip'); + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryOperator"]').text()).toEqual( + 'is one of' + ); + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryFieldMatchAny"]').text()).toEqual( + '1234' + ); + }); + + test('it renders field values correctly when operator is "isNotOneOfOperator"', () => { + const wrapper = mount( + + ); + + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryField"]').text()).toEqual('ip'); + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryOperator"]').text()).toEqual( + 'is not one of' + ); + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryFieldMatchAny"]').text()).toEqual( + '1234' + ); + }); + + test('it renders field values correctly when operator is "isInListOperator"', () => { + const wrapper = mount( + + ); + + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryField"]').text()).toEqual('ip'); + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryOperator"]').text()).toEqual( + 'is in list' + ); + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryFieldList"]').text()).toEqual( + 'some name' + ); + }); + + test('it renders field values correctly when operator is "isNotInListOperator"', () => { + const wrapper = mount( + + ); + + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryField"]').text()).toEqual('ip'); + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryOperator"]').text()).toEqual( + 'is not in list' + ); + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryFieldList"]').text()).toEqual( + 'some name' + ); + }); + + test('it renders field values correctly when operator is "existsOperator"', () => { + const wrapper = mount( + + ); + + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryField"]').text()).toEqual('ip'); + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryOperator"]').text()).toEqual( + 'exists' + ); + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryFieldExists"]').text()).toEqual( + getEmptyValue() + ); + expect( + wrapper.find('[data-test-subj="exceptionBuilderEntryFieldExists"] input').props().disabled + ).toBeTruthy(); + }); + + test('it renders field values correctly when operator is "doesNotExistOperator"', () => { + const wrapper = mount( + + ); + + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryField"]').text()).toEqual('ip'); + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryOperator"]').text()).toEqual( + 'does not exist' + ); + expect(wrapper.find('[data-test-subj="exceptionBuilderEntryFieldExists"]').text()).toEqual( + getEmptyValue() + ); + expect( + wrapper.find('[data-test-subj="exceptionBuilderEntryFieldExists"] input').props().disabled + ).toBeTruthy(); + }); + + test('it invokes "onChange" when new field is selected and resets operator and value fields', () => { + const mockOnChange = jest.fn(); + const wrapper = mount( + + ); + + ((wrapper.find(EuiComboBox).at(0).props() as unknown) as { + onChange: (a: EuiComboBoxOptionOption[]) => void; + }).onChange([{ label: 'machine.os' }]); + + expect(mockOnChange).toHaveBeenCalledWith( + { field: 'machine.os', operator: 'included', type: 'match', value: undefined }, + 0 + ); + }); + + test('it invokes "onChange" when new operator is selected and resets value field', () => { + const mockOnChange = jest.fn(); + const wrapper = mount( + + ); + + ((wrapper.find(EuiComboBox).at(1).props() as unknown) as { + onChange: (a: EuiComboBoxOptionOption[]) => void; + }).onChange([{ label: 'is not' }]); + + expect(mockOnChange).toHaveBeenCalledWith( + { field: 'ip', operator: 'excluded', type: 'match', value: '' }, + 0 + ); + }); + + test('it invokes "onChange" when new value field is entered for match operator', () => { + const mockOnChange = jest.fn(); + const wrapper = mount( + + ); + + ((wrapper.find(EuiComboBox).at(2).props() as unknown) as { + onCreateOption: (a: string) => void; + }).onCreateOption('126.45.211.34'); + + expect(mockOnChange).toHaveBeenCalledWith( + { field: 'ip', operator: 'excluded', type: 'match', value: '126.45.211.34' }, + 0 + ); + }); + + test('it invokes "onChange" when new value field is entered for match_any operator', () => { + const mockOnChange = jest.fn(); + const wrapper = mount( + + ); + + ((wrapper.find(EuiComboBox).at(2).props() as unknown) as { + onCreateOption: (a: string) => void; + }).onCreateOption('126.45.211.34'); + + expect(mockOnChange).toHaveBeenCalledWith( + { field: 'ip', operator: 'included', type: 'match_any', value: ['126.45.211.34'] }, + 0 + ); + }); + + test('it invokes "onChange" when new value field is entered for list operator', () => { + const mockOnChange = jest.fn(); + const wrapper = mount( + + ); + + ((wrapper.find(EuiComboBox).at(2).props() as unknown) as { + onChange: (a: EuiComboBoxOptionOption[]) => void; + }).onChange([{ label: 'some name' }]); + + expect(mockOnChange).toHaveBeenCalledWith( + { + field: 'ip', + operator: 'excluded', + type: 'list', + list: { id: 'some-list-id', type: 'ip' }, + }, + 0 + ); + }); +}); diff --git a/x-pack/plugins/security_solution/public/common/components/exceptions/builder/entry_item.tsx b/x-pack/plugins/security_solution/public/common/components/exceptions/builder/entry_item.tsx index 39a1e1bdbad5..0f5000c8c0ab 100644 --- a/x-pack/plugins/security_solution/public/common/components/exceptions/builder/entry_item.tsx +++ b/x-pack/plugins/security_solution/public/common/components/exceptions/builder/entry_item.tsx @@ -67,13 +67,13 @@ export const EntryItemComponent: React.FC = ({ { field: entry.field != null ? entry.field.name : undefined, type: OperatorTypeEnum.MATCH, - operator: isOperator.operator, + operator: entry.operator.operator, value: newField, }, entryIndex ); }, - [onChange, entryIndex, entry.field] + [onChange, entryIndex, entry.field, entry.operator.operator] ); const handleFieldMatchAnyValueChange = useCallback( @@ -82,13 +82,13 @@ export const EntryItemComponent: React.FC = ({ { field: entry.field != null ? entry.field.name : undefined, type: OperatorTypeEnum.MATCH_ANY, - operator: isOperator.operator, + operator: entry.operator.operator, value: newField, }, entryIndex ); }, - [onChange, entryIndex, entry.field] + [onChange, entryIndex, entry.field, entry.operator.operator] ); const handleFieldListValueChange = useCallback( @@ -97,13 +97,13 @@ export const EntryItemComponent: React.FC = ({ { field: entry.field != null ? entry.field.name : undefined, type: OperatorTypeEnum.LIST, - operator: isOperator.operator, + operator: entry.operator.operator, list: { id: newField.id, type: newField.type }, }, entryIndex ); }, - [onChange, entryIndex, entry.field] + [onChange, entryIndex, entry.field, entry.operator.operator] ); const renderFieldInput = (isFirst: boolean): JSX.Element => { @@ -114,9 +114,9 @@ export const EntryItemComponent: React.FC = ({ selectedField={entry.field} isLoading={isLoading} isClearable={false} - isDisabled={indexPattern == null} + isDisabled={isLoading} onChange={handleFieldChange} - data-test-subj="filterFieldSuggestionList" + data-test-subj="exceptionBuilderEntryField" /> ); @@ -137,11 +137,11 @@ export const EntryItemComponent: React.FC = ({ placeholder={i18n.EXCEPTION_OPERATOR_PLACEHOLDER} selectedField={entry.field} operator={entry.operator} - isDisabled={false} + isDisabled={isLoading} isLoading={false} isClearable={false} onChange={handleOperatorChange} - data-test-subj="filterFieldSuggestionList" + data-test-subj="exceptionBuilderEntryOperator" /> ); @@ -165,12 +165,12 @@ export const EntryItemComponent: React.FC = ({ placeholder={i18n.EXCEPTION_FIELD_VALUE_PLACEHOLDER} selectedField={entry.field} selectedValue={value} - isDisabled={false} + isDisabled={isLoading} isLoading={isLoading} isClearable={false} indexPattern={indexPattern} onChange={handleFieldMatchValueChange} - data-test-subj="filterFieldSuggestionList" + data-test-subj="exceptionBuilderEntryFieldMatch" /> ); case OperatorTypeEnum.MATCH_ANY: @@ -180,12 +180,12 @@ export const EntryItemComponent: React.FC = ({ placeholder={i18n.EXCEPTION_FIELD_VALUE_PLACEHOLDER} selectedField={entry.field} selectedValue={values} - isDisabled={false} + isDisabled={isLoading} isLoading={isLoading} isClearable={false} indexPattern={indexPattern} onChange={handleFieldMatchAnyValueChange} - data-test-subj="filterFieldSuggestionList" + data-test-subj="exceptionBuilderEntryFieldMatchAny" /> ); case OperatorTypeEnum.LIST: @@ -195,17 +195,18 @@ export const EntryItemComponent: React.FC = ({ selectedField={entry.field} placeholder={i18n.EXCEPTION_FIELD_LISTS_PLACEHOLDER} selectedValue={id} - isLoading={false} - isDisabled={false} + isLoading={isLoading} + isDisabled={isLoading} isClearable={false} onChange={handleFieldListValueChange} + data-test-subj="exceptionBuilderEntryFieldList" /> ); case OperatorTypeEnum.EXISTS: return ( ); default: diff --git a/x-pack/plugins/security_solution/public/common/components/exceptions/builder/exception_item.tsx b/x-pack/plugins/security_solution/public/common/components/exceptions/builder/exception_item.tsx index 3afdf43ec7df..5e53ce3ba657 100644 --- a/x-pack/plugins/security_solution/public/common/components/exceptions/builder/exception_item.tsx +++ b/x-pack/plugins/security_solution/public/common/components/exceptions/builder/exception_item.tsx @@ -29,6 +29,7 @@ interface ExceptionListItemProps { isLoading: boolean; indexPattern: IIndexPattern; andLogicIncluded: boolean; + onCheckAndLogic: (item: ExceptionsBuilderExceptionItem[]) => void; onDeleteExceptionItem: (item: ExceptionsBuilderExceptionItem, index: number) => void; onExceptionItemChange: (item: ExceptionsBuilderExceptionItem, index: number) => void; } @@ -41,6 +42,7 @@ export const ExceptionListItemComponent = React.memo( indexPattern, isLoading, andLogicIncluded, + onCheckAndLogic, onDeleteExceptionItem, onExceptionItemChange, }) => { @@ -70,11 +72,12 @@ export const ExceptionListItemComponent = React.memo( onDeleteExceptionItem(updatedExceptionItem, exceptionItemIndex); }; - const entries = useMemo( - (): FormattedBuilderEntry[] => - indexPattern != null ? getFormattedBuilderEntries(indexPattern, exceptionItem.entries) : [], - [indexPattern, exceptionItem.entries] - ); + const entries = useMemo((): FormattedBuilderEntry[] => { + onCheckAndLogic([exceptionItem]); + return indexPattern != null + ? getFormattedBuilderEntries(indexPattern, exceptionItem.entries) + : []; + }, [indexPattern, exceptionItem, onCheckAndLogic]); const andBadge = useMemo((): JSX.Element => { const badge = ; 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 index c6376c34c768..dcf06eeeffaf 100644 --- 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 @@ -77,15 +77,21 @@ export const ExceptionBuilder = ({ indexPatternConfig ?? [] ); + const handleCheckAndLogic = (items: ExceptionsBuilderExceptionItem[]): void => { + setAndLogicIncluded((includesAnd: boolean): boolean => { + if (includesAnd) { + return true; + } else { + return items.filter(({ entries }) => entries.length > 1).length > 0; + } + }); + }; + // Bubble up changes to parent useEffect(() => { onChange({ exceptionItems: filterExceptionItems(exceptions), exceptionsToDelete }); }, [onChange, exceptionsToDelete, exceptions]); - const checkAndLogic = (items: ExceptionsBuilderExceptionItem[]): void => { - setAndLogicIncluded(items.filter(({ entries }) => entries.length > 1).length > 0); - }; - const handleDeleteExceptionItem = ( item: ExceptionsBuilderExceptionItem, itemIndex: number @@ -100,7 +106,7 @@ export const ExceptionBuilder = ({ ...existingExceptions.slice(0, itemIndex), ...existingExceptions.slice(itemIndex + 1), ]; - checkAndLogic(updatedExceptions); + handleCheckAndLogic(updatedExceptions); return updatedExceptions; }); @@ -118,7 +124,7 @@ export const ExceptionBuilder = ({ ...exceptions.slice(index + 1), ]; - checkAndLogic(updatedExceptions); + handleCheckAndLogic(updatedExceptions); setExceptions(updatedExceptions); }; @@ -214,6 +220,7 @@ export const ExceptionBuilder = ({ isLoading={indexPatternLoading} exceptionItemIndex={index} andLogicIncluded={andLogicIncluded} + onCheckAndLogic={handleCheckAndLogic} onDeleteExceptionItem={handleDeleteExceptionItem} onExceptionItemChange={handleExceptionItemChange} />