-
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][Exceptions] - Exception Modal Part I #70639
Conversation
x-pack/plugins/lists/public/exceptions/hooks/persist_exception_item.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/common/components/exceptions/viewer/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/lists/common/schemas/request/update_exception_list_item_schema.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/lists/common/schemas/response/exception_list_item_schema.ts
Outdated
Show resolved
Hide resolved
@@ -13,6 +13,12 @@ export { useFindLists } from './lists/hooks/use_find_lists'; | |||
export { useImportList } from './lists/hooks/use_import_list'; | |||
export { useDeleteList } from './lists/hooks/use_delete_list'; | |||
export { useExportList } from './lists/hooks/use_export_list'; | |||
export { | |||
addExceptionListItem, | |||
updateExceptionListItem, |
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 we need to export these as they are all available in useApi
@@ -190,6 +205,21 @@ export const AlertsTableComponent: React.FC<AlertsTableComponentProps> = ({ | |||
[dispatchToaster] | |||
); | |||
|
|||
const openAddExceptionModalCallback = useCallback( | |||
({ ruleName, ruleId, exceptionListType, alertData }: AddExceptionOnClick) => { | |||
if (alertData !== null && alertData !== undefined) { |
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.
You can use !=
here to check for both null
and undefined
setAddExceptionModalState(addExceptionModalInitialState); | ||
}, [setShouldShowAddExceptionModal, setAddExceptionModalState]); | ||
|
||
const onAddExceptionCancel = 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.
Could onAddExceptionCancel
and onAddExceptionConfirm
be combined into one function like onCloseAddExceptionModal
?
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.
Will look into this in the follow up PR
start={from} | ||
utilityBar={utilityBarCallback} | ||
/> | ||
{shouldShowAddExceptionModal === true && addExceptionModalState.alertData !== null && ( |
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 here as before, using != null
will catch both undefined
and null
@@ -0,0 +1,139 @@ | |||
/* |
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.
You should just be able to use the usePersistExceptionItem
hook from the lists plugin. It determines when to use the post vs put within it. Maybe you can update that existing hook to accept a list of items instead of just 1?
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.
Will look into this in the follow up PR. This hook will eventually need to handle bulk close as well as current alert close also.
@@ -258,6 +258,21 @@ export const RuleDetailsPageComponent: FC<PropsFromRedux> = ({ | |||
return null; | |||
} | |||
|
|||
// TODO: remove this once namespace is finalized as camecased or snakecase | |||
const exceptionListsMeta = 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.
Working to get that PR in! :) Hope to have it cleaned up EOD.
<EuiFlexGroup gutterSize={'none'}> | ||
<EuiFlexItem grow={false}> | ||
<MyAvatar | ||
name={currentUser !== null ? currentUser.username.toUpperCase() ?? '' : ''} // TODO: Still need to standardize this |
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.
Is there a case where there wouldn't be a username accessible here? If there isn't one does that mean they don't have certain access rights and would we at that point not let them add an exception at all?
export interface AddExceptionOnClick { | ||
ruleName: string; | ||
ruleId: string; | ||
ruleExceptionLists?: ExceptionListSchema[]; |
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 might be explicit here like ruleExceptionLists: ExceptionListSchema[] | undefined;
Because we're using it as a tell for whether the user is creating a new list vs. editing right?
onConfirm(); | ||
}, [dispatchToaster, onConfirm]); | ||
|
||
const [{ isLoading: addExceptionIsLoading }, addOrUpdateExceptionItems] = useAddOrUpdateException( |
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.
Mentioned it above, but I think you can just use the existing persist_exception_item.ts
hook in the lists plugin.
[setShouldBulkCloseAlert] | ||
); | ||
|
||
const enrichExceptionItems = 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.
I might move some of these functions out into a helpers.ts
file so you can unit test them easily.
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'll look into moving the entire enrichExceptionItems
function to the helpers file in the follow up PR.
<EuiText>{i18n.EXCEPTION_BUILDER_INFO}</EuiText> | ||
<EuiSpacer /> | ||
<ExceptionBuilder | ||
exceptionListItems={exceptionItemsToAdd} |
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 I might avoid passing this in, since the ExceptionBuilder
manages its own state, it just takes an initial list of exception list items to start with. Might be easier to discuss this one offline to see if I missed something.
Pinging @elastic/siem (Team:SIEM) |
return Promise.resolve(newExceptionList); | ||
} catch (error) { | ||
// TODO: properly handle 409 conflict if exception list already exists | ||
return Promise.reject(error); |
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'll need to handle conflict errors in Part || if the exception list already exists due to stale rule references or race conditions.
_tags: undefined, | ||
tags: undefined, | ||
// TODO: Make this a constant that's shared throughout the app | ||
list_id: exceptionListType === 'endpoint' ? 'endpoint_list' : undefined, |
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.
In Part II, we'll create a constant and share it with the endpoint exceptions artifact builder code.
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 you need to pass a list_id here. It will be generated by the server.
@@ -90,6 +101,10 @@ export const AlertsTableComponent: React.FC<AlertsTableComponentProps> = ({ | |||
|
|||
const [showClearSelectionAction, setShowClearSelectionAction] = useState(false); | |||
const [filterGroup, setFilterGroup] = useState<Status>(FILTER_OPEN); | |||
const [shouldShowAddExceptionModal, setShouldShowAddExceptionModal] = useState(false); | |||
const [addExceptionModalState, setAddExceptionModalState] = useState( |
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.
Nit: I might add the typing here like - const [addExceptionModalState, setAddExceptionModalState] = useState<AddExceptionOnClick>(addExceptionModalInitialState);
const setIsModalOpen = useCallback( | ||
(isOpen: boolean): void => { | ||
const setCurrentModal = useCallback( | ||
(modalName: string | null): 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.
Nit: Can we make this an enum for the different modal types?
/> | ||
)} | ||
|
||
{currentModal === 'addModal' && exceptionListTypeToEdit !== null && ( |
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.
Here I would use the != null
to catch null
and undefined
.
ruleId, | ||
exceptionListType, | ||
onError, | ||
onSuccess, |
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 need the onSuccess
or can that be inferred from the return value?
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.
Don't need it. Removed.
): Promise<ExceptionListSchema> { | ||
const newExceptionList = await createExceptionList(ruleResponse); | ||
|
||
let newExceptionListReferences: ListArray; |
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 one pattern I've noticed is that we try to avoid using let
s and casting. I think here it could be something like:
const newExceptonListReference = {
id: newExceptionList.id,
namespace_type: newExceptionList.namespace_type,
};
const newExceptionListReferences: ListArray = [...(ruleResponse.exceptions_list ?? []), newExceptionListReference];
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.
Good call. Updated.
x-pack/plugins/security_solution/public/common/components/exceptions/builder/index.tsx
Show resolved
Hide resolved
@@ -161,6 +167,10 @@ export const getOperatingSystems = (tags: string[]): string => { | |||
return osMatches; | |||
}; | |||
|
|||
export const getOsTagValues = (tags: string[]): string[] => { |
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.
Is this functionally the same as getOperatingSystems
?
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 so. @dplumlee is going to look into dropping the additional formatting from getOperatingSystems
in the next PR.
@@ -184,7 +194,7 @@ export const getDescriptionListContent = ( | |||
const details = [ | |||
{ | |||
title: i18n.OPERATING_SYSTEM, | |||
value: getOperatingSystems(exceptionItem._tags), | |||
value: getOperatingSystems(exceptionItem._tags ?? []), |
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.
In kibana/x-pack/plugins/lists/common/schemas/request/create_exception_list_item_schema.ts
it looks like _tags
gets defaulted to an empty array on creation, so we should be good to remove the ?? []
.
return undefined; | ||
}; | ||
|
||
export const entryHasListType = ( |
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 you can use the helper export const isListType = (item: BuilderEntry): item is EmptyListEntry => item.type === OperatorTypeEnum.LIST;
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.
To be addressed in follow up PR
if (item != null && item.value != null) { | ||
return item.value; | ||
} | ||
return undefined; |
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 return an empty array here and then you don't have to do checks like the following and the return type is just string[]
: getMappedNonEcsValue({ data: alertData, fieldName: 'file.hash.sha1' }) ?? []
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.
To be addressed in follow up PR
931bc45
to
08a2f68
Compare
169cd10
to
052a80e
Compare
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.
Played around with the modal together while debugging a separate issue. Excited to see it all come together! LGTM - thanks for creating a ticket with the list of things to follow up on so they don't slip through the cracks. Some things I would make sure to follow up on is using != null
, that'll help avoid missing catching an undefined
or null
if we try to explicitly check for each separately. Also, following up on the need for list_id
on creation of exception lists. Thanks so much!
</ModalHeader> | ||
|
||
{fetchOrCreateListError === true && ( | ||
<EuiCallOut title={'Error'} color="danger" iconType="alert"> |
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 the title needs to be in i18n?
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.
Good catch. Done.
import { getFormattedComments } from './helpers'; | ||
|
||
interface AddExceptionCommentsProps { | ||
exceptionItemComments?: Comments[]; |
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.
Nit: I think we can get rid of the ?
here? Comments defaults to an empty array, so it should appear. Or maybe make it explicit Comments[] | undefined
if in a not yet created exception item it's not yet defined?
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.
My thinking was that this prop would be optional because we use this commend in the Add Modal also where there are no pre-existing comments.
<EuiSpacer /> | ||
<ExceptionBuilder | ||
exceptionListItems={initialExceptionItems} | ||
listType={exceptionListType as 'endpoint' | 'detection'} |
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 this needs to be casted here. The schema checks should validate that it's either endpoint | detection
.
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.
yup, done.
@@ -43,7 +43,7 @@ interface OnChangeProps { | |||
} | |||
|
|||
interface ExceptionBuilderProps { | |||
exceptionListItems: ExceptionListItemSchema[]; | |||
exceptionListItems: ExceptionsBuilderExceptionItem[]; | |||
listType: 'detection' | 'endpoint'; |
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 I missed this in my PR, but here we can update to use the enum from the lists plugin.
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 why I was having problems with listType
in the modal. I'll update the builder to use the enum.
<ModalBodySection className="builder-section"> | ||
<ExceptionBuilder | ||
exceptionListItems={[exceptionItem]} | ||
listType={exceptionListType as 'endpoint' | 'detection'} |
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 comment above, I think I missed updating the prop to be ExceptionListTypeEnum
(from lists plugin). That should remove any need to cast.
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.
done
@@ -292,7 +309,7 @@ export const getNewExceptionItem = ({ | |||
namespaceType, | |||
ruleName, | |||
}: { | |||
listType: 'detection' | 'endpoint'; | |||
listType: ExceptionListType; |
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 missed this in my PR. I would update to use ExceptionListTypeEnum
from lists common schema.
...endpointExceptionList, | ||
name: 'new endpoint exception list', | ||
}; | ||
const exceptionsListReferences: ListArray = [ |
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 we can use the getListMock
from x-pack/plugins/security_solution/common/detection_engine/schemas/types/lists.mock.ts
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.
Good call. Done.
_tags: undefined, | ||
tags: undefined, | ||
// TODO: Make this a constant that's shared throughout the app | ||
list_id: exceptionListType === 'endpoint' ? 'endpoint_list' : undefined, |
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 you need to pass a list_id here. It will be generated by the server.
} | ||
|
||
async function fetchRuleExceptionLists(ruleResponse: Rule): Promise<ExceptionListSchema[]> { | ||
const exceptionListReferences = ruleResponse.exceptions_list as ListArray; |
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.
Since the response gets validated, we shouldn't need to do any casting.
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.
Yup, this is not needed. It's leftover from when the types weren't finalized yet.
let exceptionListToUse: ExceptionListSchema; | ||
const matchingList = exceptionLists.find((list) => { | ||
if (exceptionListType === 'endpoint') { | ||
return list.type === exceptionListType && list.list_id === 'endpoint_list'; |
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 might check with @FrankHassanabad , but almost certain that we shouldn't create a list_id
in the client. We should let the server create it and then reference it here using the response from the creation.
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 need to think about the right behavior is here when there is no exception list present. We can iterate on it.
1fd2a48
to
0fe335b
Compare
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
History
To update your PR or re-run it, just comment with: |
…1035) * adds 2 menu items to alert page, progress on exception modal * adds enriching * remove unused useExceptionList() * implements some types * move add exception modal files * Exception builder changes to support latest schema * Changes to lists plugin schemas and fix api bug Needed to make the schemas more forgiving. Before this change they required name, description, etc for creation and update. The update item API was using the wrong url. * Adding and editing exceptions working - Modifies add_exception_modal component - Creates edit_exception_modal component - Creates shared comments component - Creates use_add_exception api hook for adding or editing exceptions - Updates viewer code to support adding and editing exceptions - Updates alerts table code to use updated version of add_exception_modal * fixes duplicate types * updates os tag input * fixes comment style * removes checkbox programatically * grahpql updates to expose exceptions_list * Add fetch_or_create_exception_list hook * fixes data population * refactor use_add_exception hook, add tests * fix rebase issues, pending updates to edit modal * fix edit modal and default endpoint exceptions * adds second checkbox * adds signal index stuff * switches boolean logic * fix some type errors * remove unnecesary code * fixes checkbox logic in edit modal * fixes recursive prop passing * addresses comments/fixes types * Revert schema type changes * type fixes * fixes regular exception modal * fix more type errors, remove console log * fix tests * move add exception hook, lint * close alert checkbox closes alert * address PR comments * add type to patch rule call, fix ts errors * fix lint * fix merge problems after conflict * Address PR comments * undo graphql type change Co-authored-by: Davis Plumlee <davis.plumlee@elastic.co> Co-authored-by: Davis Plumlee <davis.plumlee@elastic.co>
* master: (36 commits) fixed api url in example plugin (elastic#70934) [data.search.aggs]: Remove remaining client dependencies (elastic#70251) [Security Solution][Endpoint] Fix base64 download bug and adopt new user artifact/manifest format (elastic#70998) [Security Solution][Exceptions] - Exception Modal Part I (elastic#70639) [SIEM][Detection Engine][Lists] Adds additional data types to value based lists [SIEM][Detection Engine][Lists] Removes feature flag for lists [APM] Show license callout in ML settings (elastic#70959) Migrate service settings test to jest (elastic#70992) [APM] Add cloud attributes to data telemetry (elastic#71008) Fix breadcrumb on panels for visibility / round corners (elastic#71010) Improve search typescript (elastic#69333) [savedObjects field count] run in baseline job (elastic#70999) [Security Solution] [Timeline] Timeline manager tweaks (elastic#69988) [Endpoint] Support redirect from Policy Details to Ingest when user initiates Edit Policy from Datasource Edit page (elastic#70874) [APM] Add API tests (elastic#70740) [Security Solution][Exceptions] - Tie server and client code together (elastic#70918) [Audit Logging] Add AuditTrail service (elastic#69278) [Usage Collection] Ensure no type duplicates (elastic#70946) [Security Solution] [Timeline] Bugfix for timeline row actions disappear sometimes (elastic#70958) [CI] Add pipeline task queue framework and merge workers into one (elastic#64011) ...
Pinging @elastic/security-solution (Team: SecuritySolution) |
Summary
This PR adds modals for Adding and Editing exception items. These modals will be used in various pages in the app to enable the user to add Exceptions to their rules.
comments
before they're sent to the server. Endpoint exceptions are also enriched with an operating system_tag
.Coming in Part II
Usage Examples
Alerts page
Rule Details External Alerts tab
Rule Details Exceptions tab
Adding an exception from the exceptions viewer
Adding an exception from an alert event
Editing an exception
Exception comments
Testing
To turn on lists plugin - in kibana.dev.yml
Add Exception
orAdd Endpoint Exception
from the options in the alerts TimelineExceptions
tab, then selectAdd to endpoint list
orAdd to detection list
from the options in the Exception Viewer header.Checklist
Delete any items that are not applicable to this PR.
For maintainers