-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Security Solution][Endpoint Exceptions] Wildcard warning with IS operator for endpoint exceptions creation/editing #182903
[Security Solution][Endpoint Exceptions] Wildcard warning with IS operator for endpoint exceptions creation/editing #182903
Conversation
…tions-is-wildcard
…tions-is-wildcard
…tions-is-wildcard
…tions-is-wildcard
Pinging @elastic/security-defend-workflows (Team:Defend Workflows) |
*/ | ||
export const getHasWrongOperator = (items: ExceptionsBuilderReturnExceptionItem[]): boolean => { | ||
let hasWrongOperator = false; | ||
items[0]?.entries.forEach((e) => { |
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.
Would you consider using array.some() instead of foreach? :)
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.
this is actually a great idea
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.
Hey, thanks for the changes! I went through the code and left some suggestions, that could help improve the readability and maintainability of this functionality 👍
Is this functionality something that you could invest more time into refactoring a bit, or you prefer to make as little changes as possible just to meet the required criteria of the task?
If the latter, I totally understand 👍 but this cumbersome component could use a little refresh - that could help you continue building on it in the future.
@@ -137,9 +137,10 @@ export const BuilderEntryItem: React.FC<EntryItemProps> = ({ | |||
([newOperator]: OperatorOption[]): void => { | |||
const { updatedEntry, index } = getEntryOnOperatorChange(entry, newOperator); | |||
handleError(false); | |||
handleWarning(false); |
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.
I was wondering if you could think of a different way to manage these errors/warning? Just updating multiple states at once seems odd, and also having wrappers just for calling prop function seems redundant.
Would you have some time to refactor this component a bit?
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.
I think refactoring would probably have to be at a later stage if that's OK
@@ -56,7 +56,7 @@ interface BuilderExceptionListItemProps { | |||
onDeleteExceptionItem: (item: ExceptionsBuilderExceptionItem, index: number) => void; | |||
onChangeExceptionItem: (item: ExceptionsBuilderExceptionItem, index: number) => void; | |||
setErrorsExist: (arg: EntryFieldError) => void; | |||
setWarningsExist: (arg: boolean) => void; | |||
setWarningsExist: (arg: boolean, arg2: string) => void; |
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.
Could you rename the args so they make more sense to other developers? :)
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.
Sorry, this additional arg actually isn't needed anymore
@@ -451,6 +462,14 @@ export const AddExceptionFlyout = memo(function AddExceptionFlyout({ | |||
expireTime, | |||
]); | |||
|
|||
const handleOnSubmit = useCallback(() => { | |||
if (wildcardWarningExists) { |
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 that adding this if statement to the original handleOnSubmit makes sense? We could avoid having another layer of abstraction then
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.
I don't think it would make sense in this case since I needed to separate the original submit handler for the confirmation modal. We allows the user to add the exception without addressing the warning in the confirmation modal so I wanted to have that logic on its own for the confirmation modal submit handler.
@@ -193,6 +200,10 @@ export const AddExceptionFlyout = memo(function AddExceptionFlyout({ | |||
|
|||
const setExceptionItemsToAdd = useCallback( | |||
(items: ExceptionsBuilderReturnExceptionItem[]): void => { | |||
dispatch({ | |||
type: 'setWildcardWithWrongOperator', | |||
warningExists: getHasWrongOperator(items), |
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.
Consider renaming constants/fields/functions to one (or at least similar name) - now there's too many variations for warnings
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.
Talked with @tomsonpl offline about the need for descriptive names since we have several different functions dealing with wildcards that all do different things.
@@ -335,6 +348,14 @@ const EditExceptionFlyoutComponent: React.FC<EditExceptionFlyoutProps> = ({ | |||
expireTime, | |||
]); | |||
|
|||
const handleOnSubmit = useCallback(() => { |
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.
Same as above 👍
But also - this seems to be a duplicated code, can we do something about the reusability here?
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.
I'm not sure that makes sense here since it's using variables from the individual reducers.
@@ -367,6 +388,24 @@ const EditExceptionFlyoutComponent: React.FC<EditExceptionFlyoutProps> = ({ | |||
prefix: 'exceptionFlyoutTitle', | |||
}); | |||
|
|||
const confirmModal = useMemo(() => { |
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.
Can this be moved out of this component and reused in add_exception_flyout.tsx ?
) { | ||
hasWrongOperator = true; | ||
} | ||
}) === true |
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.
Thanks for the change 👍 I think you do not need to compare this to === true, since validateHasWildcardWithWrongOperator already returns a boolean.
What do you think about the below:
export const hasInvalidOperator = (items: ExceptionsBuilderReturnExceptionItem[]): boolean => {
return items[0]?.entries.some((e) =>
validateHasWildcardWithWrongOperator({
operator: (e as ExceptionsBuilderReturnExceptionItem[number]).type,
value: (e as ExceptionsBuilderReturnExceptionItem[number]).value,
})
);
};
return items[0]?.entries.some((e) => { | ||
return ( | ||
validateHasWildcardWithWrongOperator({ | ||
operator: (e as ExceptionsBuilderReturnExceptionItem[number]).type, |
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.
this line throws type error
proc [tsc] packages/kbn-securitysolution-list-utils/src/helpers/index.ts:1036:62 - error TS2537: Type 'ExceptionsBuilderReturnExceptionItem' has no matching index signature for type 'number'.
--
| proc [tsc]
| proc [tsc] 1036 operator: (e as ExceptionsBuilderReturnExceptionItem[number]).type,
| proc [tsc] ~~~~~~
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.
can casting be avoided here?
@@ -367,6 +388,24 @@ const EditExceptionFlyoutComponent: React.FC<EditExceptionFlyoutProps> = ({ | |||
prefix: 'exceptionFlyoutTitle', | |||
}); | |||
|
|||
const confirmModal = useMemo(() => { | |||
const { title, body, confirmButton, cancelButton } = CONFIRM_WARNING_MODAL_LABELS( | |||
listType === ExceptionListTypeEnum.ENDPOINT ? ENDPOINT_EXCEPTION : RULE_EXCEPTION |
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.
So, I can see this PR affects not only endpoint exceptions, but rule exceptions too.
We already had help text, that points wildcard is used with a wrong operator.
Now, we would have additional waring and confirm modal. Three different UI elements highlighting the same possible misusing of wildcard.
cc: @ARWNightingale , @approksiu - are we good with such behaviour for rule exceptions?
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.
The additional warning callout and confirm modal were requests from product to make sure users were addressing the issue in the value. We had several sdhs where users were having trouble diagnosing why their entries were not effective and the smaller warning underneath the value is easy to miss. I feel like rule exceptions could also benefit from this behavior too since it's using the same component, but maybe I'm mistaken?
@caitlinbetz did you have more to add on the design decision?
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.
We should think about how to de-duplicate warnings. I thnk confirmation popup is good, but we should keep only one of the warnings - either under the value or under all conditions. cc @ARWNightingale @caitlinbetz
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.
Thanks @approksiu . We currently have two because 1. we wanted the user to be aware of the potential issue on the form, as it was being created, and 2. wanted the user to confirm the exception if we notice a potential configuration error. So - I think they do both currently serve a purpose.
I think the "third" message, under the value, we can remove (as long as I'm not missing anything @parkiino.) I believe it was added to maintain consistency across some other forms, but again @parkiino let me know your thoughts on removing this one.
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.
@caitlinbetz i think we probably want to keep the one under the value since they show the error per individual condition if there are multiple stringed together with the "AND" or "OR". It seems like @approksiu was suggesting we take out the single callout under all the conditions, but then that would make the design inconsistent with trusted apps and event filters. I'm not sure if it makes sense to only have it for endpoint exceptions and not rule exceptions too, if we tried to split it up that way. If it makes sense to you, maybe we just remove the callout in all the forms, if we do want to reduce one of the warning areas?
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.
@parkiino after discussing with @approksiu - we are okay to leave this functionality as is for the time being. Thank you.
…tions-is-wildcard
…tions-is-wildcard
…tions-is-wildcard
…tions-is-wildcard
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.
@parkiino
Detection engine code looks good.
I left one comment on function name.
I also leave with @approksiu decision on how to proceed with multiple warning and confirmation modal.
What do we decide to do? Should it be merged in a current state?
/** | ||
* Given an exceptions list, determine if any entries have an "IS" operator with a wildcard value | ||
*/ | ||
export const getHasWrongOperator = (items: ExceptionsBuilderReturnExceptionItem[]): boolean => { |
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.
I would rename this function to reflect it is related to wildcars, like hasWrongOperatorWithWildcard
.
Otherwise it difficult to say, what this function is doing.
…tions-is-wildcard
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.
thanks for the changes! 🙌 🚀
everything looks good, there's one minor request, so i'm requesting changes to avoid auto merge : )
@@ -202,7 +202,7 @@ export const CONFIRM_WARNING_MODAL_LABELS = (entryType: string) => { | |||
}), | |||
body: i18n.translate('xpack.securitySolution.artifacts.confirmWarningModal.body', { | |||
defaultMessage: | |||
'Using a "*" or a "?" in the value with the "IS" operator can make the entry ineffective. Change the operator to ‘matches’ to ensure wildcards run properly. Select “cancel” to revise your entry, or "add" to continue with the entry in its current state.', | |||
'Using a "*" or a "?" in the value with the "is" operator can make the entry ineffective. Change the operator to "matches" to ensure wildcards run properly. Select “Cancel” to revise your entry, or "Add" to continue with the entry in its current state.', |
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.
awesome, thanks for changing these as well!
packages/kbn-securitysolution-utils/src/path_validations/index.ts
Outdated
Show resolved
Hide resolved
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.
awesome, thanks for all the changes! 🚢 🚀
…tions-is-wildcard
@elasticmachine merge upstream |
@elasticmachine merge upstream |
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.
Discussed with @ARWNightingale and @caitlinbetz. I am OK to proceed as it is implemented now.
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Async chunks
History
To update your PR or re-run it, just comment with: |
Summary
Screenshots