[Security Solution] [Exceptions] Fix Exception Auto-populate from Alert actions #159908
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ | |
| * 2.0. | ||
| */ | ||
|
|
||
| import React, { useCallback, useEffect, useMemo, useReducer, useState } from 'react'; | ||
| import { EuiFlexGroup, EuiFlexItem, EuiSpacer } from '@elastic/eui'; | ||
| import styled from 'styled-components'; | ||
| import { HttpStart } from '@kbn/core/public'; | ||
|
|
@@ -34,14 +34,14 @@ import { | |
| } from '@kbn/securitysolution-list-utils'; | ||
| import { DataViewBase } from '@kbn/es-query'; | ||
| import type { AutocompleteStart } from '@kbn/unified-search-plugin/public'; | ||
| import deepEqual from 'fast-deep-equal'; | ||
|
|
||
| import { AndOrBadge } from '../and_or_badge'; | ||
|
|
||
| import { BuilderExceptionListItemComponent } from './exception_item_renderer'; | ||
| import { BuilderLogicButtons } from './logic_buttons'; | ||
| import { getTotalErrorExist } from './selectors'; | ||
| import { EntryFieldError, State, exceptionsBuilderReducer } from './reducer'; | ||
| const MyInvisibleAndBadge = styled(EuiFlexItem)` | ||
| visibility: hidden; | ||
| `; | ||
|
|
@@ -131,6 +131,7 @@ export const ExceptionBuilderComponent = ({ | |
| disableNested: isNestedDisabled, | ||
| disableOr: isOrDisabled, | ||
| }); | ||
| const [areAllEntriesDeleted, setAreAllEntriesDeleted] = useState<boolean>(false); | ||
|
|
||
| const { | ||
| addNested, | ||
|
|
@@ -252,6 +253,7 @@ export const ExceptionBuilderComponent = ({ | |
| // just add a default entry to it | ||
| if (updatedExceptions.length === 0) { | ||
| setDefaultExceptions(item); | ||
| setAreAllEntriesDeleted(true); | ||
| } else if (updatedExceptions.length > 0 && exceptionListItemSchema.is(item)) { | ||
| setUpdateExceptionsToDelete([...exceptionsToDelete, item]); | ||
| } else { | ||
|
|
@@ -394,12 +396,36 @@ export const ExceptionBuilderComponent = ({ | |
| } | ||
| }, [exceptions, handleAddNewExceptionItem]); | ||
|
|
||
| /** | ||
| * This component relies on the "exceptionListItems" to pre-fill its entries, | ||
| * but any subsequent updates to the entries are not reflected back to | ||
| * the "exceptionListItems". To ensure correct behavior, we need to only | ||
| * fill the entries from the "exceptionListItems" during initialization. | ||
| * | ||
| * In the initialization phase, if there are "exceptionListItems" with | ||
| * pre-filled entries, the exceptions array will be empty. However, | ||
| * there are cases where the "exceptionListItems" may not be sent | ||
| * correctly during initialization, leading to the exceptions | ||
| * array being filled with empty entries. Therefore, we need to | ||
| * check if the exception is correctly populated with a valid | ||
| * "field" when the "exceptionListItems" has entries. that's why | ||
| * "exceptionsEntriesPopulated" is used | ||
| * | ||
| * It's important to differentiate this case from when the user | ||
| * deletes all the entries and the "exceptionListItems" has pre-filled values. | ||
| * that's why "allEntriesDeleted" is used | ||
| * | ||
| * deepEqual(exceptionListItems, exceptions) to handle the exceptionListItems in | ||
| * the EventFiltersFlyout | ||
| */ | ||
| useEffect(() => { | ||
| if (!exceptionListItems.length || deepEqual(exceptionListItems, exceptions)) return; | ||
| const exceptionsEntriesPopulated = exceptions.some((exception) => | ||
| exception.entries.some((entry) => entry.field) | ||
| ); | ||
| if (!exceptionsEntriesPopulated && !areAllEntriesDeleted) | ||
| setUpdateExceptions(exceptionListItems); | ||
| }, [areAllEntriesDeleted, exceptionListItems, exceptions, setUpdateExceptions]); | ||
|
Comment on lines
+422
to
+428
There was a problem hiding this comment. I'd avoid such a custom logic which needs a long explanation in comment, if possible if I understand correctly, the problem is that the content of const hasInitializedExceptionsRef = useRef<boolean>(false);
useEffect(() => {
if (!hasInitializedExceptionsRef.current && exceptionListItems.length > 0) {
setUpdateExceptions(exceptionListItems);
hasInitializedExceptionsRef.current = true;
}
}, [exceptionListItems, setUpdateExceptions]);we can go even further, and modify the type of another way would be to solve it on the outside of the component, i.e. we say that the contract of x-pack/plugins/security_solution/public/detection_engine/rule_exceptions/components/flyout_components/item_conditions/index.tsx:
{exceptionListItems.length > 0 &&
getExceptionBuilderComponentLazy({
[...props come here...]
})}what do you think? There was a problem hiding this comment. Thank you, @gergoabraham, for your review. I appreciate your valid points, and I agree with you. Regarding this component, we @yctercero have a plan to refactor it soon because any change currently leads to inconsistent behavior. I added a lengthy comment to explain why we have to handle each case individually in this unideal way. I considered your solution of using So what we are trying to achieve here is to fix the issue with not causing any others, until we totally replace this component. There was a problem hiding this comment. if this works and you're already planning to replace the component, I'm okay with this. 👍 regarding the There was a problem hiding this comment. @gergoabraham definitely agree with the points you made. I'd chatted with @WafaaNasr and she'd noted there should be a more thorough fix, but given we're in post-feature freeze zone, we're trying to stick to minimal changes that address the bug and follow up with a fix to address the underlying issue 8.10. @WafaaNasr do we know if there's any existing ticket to describe some of the issues with this component? Maybe we should add the 8.10 candidate tag to it, and if not, create one to track for 8.10. There was a problem hiding this comment. Thank you, @gergoabraham! Regarding the flashing issue, it occurred when I deleted all the pre-filled fields on a Rule Exception and added new fields along with a name. I tried to capture it on video, but unfortunately, I couldn't catch it again. @yctercero, I thought that would be part of this ticket, as part of the design, let me know if not I will create a new one. There was a problem hiding this comment. thanks for the explanation @yctercero @WafaaNasr, totally understandable! it's great that you already have a refactor ticket 👏 |
||
|
|
||
| return ( | ||
| <EuiFlexGroup gutterSize="s" direction="column" data-test-subj="exceptionsBuilderWrapper"> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think we can rename
exceptionListItemstoinitialExceptionsor similar as it's only used as an initial value?