-
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][Detection Alerts] Alert tagging #157786
Changes from 21 commits
3ad0adf
233586d
c5029b6
96658d8
c77aa15
d391a76
a06d3b5
1a5c13c
bbd7a89
938e805
3bcd05a
3b9291b
d3fda33
488293a
aa26c0a
09cae98
49c39b8
f9f91f9
b24c6d0
10eaf14
7d3d600
a9cc608
8698128
5f291df
f877402
2df309a
982b81f
a352269
8339d9f
64b5077
69cbf20
4a6a735
db9bc72
0119e69
9d8ae0c
baf4fde
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ export interface UsageStats { | |
*/ | ||
'securitySolution:defaultIndex': string; | ||
'securitySolution:defaultThreatIndex': string; | ||
'securitySolution:alertTags': string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q: should this be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking that in the previous commit here, yeah, but I ran into a test failing with an error message about this type having to be a keyword and not an array, like the other |
||
'securitySolution:newsFeedUrl': string; | ||
'xpackReporting:customPdfLogo': string; | ||
'notifications:banner': string; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,6 +45,9 @@ export type SignalIds = t.TypeOf<typeof signal_ids>; | |
// TODO: Can this be more strict or is this is the set of all Elastic Queries? | ||
export const signal_status_query = t.object; | ||
|
||
export const alert_tag_query = t.record(t.string, t.unknown); | ||
export type AlertTagQuery = t.TypeOf<typeof alert_tag_query>; | ||
|
||
export const fields = t.array(t.string); | ||
export type Fields = t.TypeOf<typeof fields>; | ||
export const fieldsOrUndefined = t.union([fields, t.undefined]); | ||
|
@@ -125,3 +128,10 @@ export const privilege = t.type({ | |
}); | ||
|
||
export type Privilege = t.TypeOf<typeof privilege>; | ||
|
||
export const alert_tags = t.type({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think of following the rules bulk edit tags schema? It may make it easier to expand the feature by following a more reusable pattern like:
That way if we decide theres a new action type we want to introduce for tags, the schema updates are minimal. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd be open to more discussion on this pertaining to more reusable patterns and expandability, it's definitely a bit unusual, but the main reason we created it like this in the first place was because we have the intermediate state in the bulk actions tags dropdown, so we couldn't just handle setting all queried alerts to the same tags array. With the two different buckets of add and remove we can cover each of the possible use cases for each specific alert with one api call. |
||
tags_to_add: t.array(t.string), | ||
tags_to_remove: t.array(t.string), | ||
}); | ||
|
||
export type AlertTags = t.TypeOf<typeof alert_tags>; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
import type { SetAlertTagsSchema } from './set_alert_tags_schema'; | ||
|
||
export const getSetAlertTagsRequestMock = ( | ||
tagsToAdd: string[] = [], | ||
tagsToRemove: string[] = [] | ||
): SetAlertTagsSchema => ({ tags: { tags_to_add: tagsToAdd, tags_to_remove: tagsToRemove } }); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
import * as t from 'io-ts'; | ||
|
||
import { alert_tag_query, alert_tags } from '../common/schemas'; | ||
|
||
export const setAlertTagsSchema = t.intersection([ | ||
t.type({ | ||
tags: alert_tags, | ||
}), | ||
t.partial({ | ||
query: alert_tag_query, | ||
}), | ||
]); | ||
|
||
export type SetAlertTagsSchema = t.TypeOf<typeof setAlertTagsSchema>; | ||
export type SetAlertTagsSchemaDecoded = SetAlertTagsSchema; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
import { i18n } from '@kbn/i18n'; | ||
|
||
export const DUPLICATE = i18n.translate('xpack.securitySolution.defaultAlertTags.duplicate', { | ||
defaultMessage: 'Duplicate', | ||
}); | ||
|
||
export const FALSE_POSITIVE = i18n.translate( | ||
'xpack.securitySolution.defaultAlertTags.falsePositive', | ||
{ | ||
defaultMessage: 'False Positive', | ||
} | ||
); | ||
|
||
export const FURTHER_INVESTIGATION_REQUIRED = i18n.translate( | ||
'xpack.securitySolution.defaultAlertTags.furtherInvestigationRequired', | ||
{ | ||
defaultMessage: 'Further investigation required', | ||
} | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
import { render } from '@testing-library/react'; | ||
import React from 'react'; | ||
import { TestProviders } from '../../../mock'; | ||
import { useUiSetting$ } from '../../../lib/kibana'; | ||
|
||
import { BulkAlertTagsPanel } from './alert_bulk_tags'; | ||
import { ALERT_WORKFLOW_TAGS } from '@kbn/rule-data-utils'; | ||
import { useAppToasts } from '../../../hooks/use_app_toasts'; | ||
import { useSetAlertTags } from './use_set_alert_tags'; | ||
|
||
jest.mock('../../../lib/kibana'); | ||
jest.mock('../../../hooks/use_app_toasts'); | ||
jest.mock('./use_set_alert_tags'); | ||
|
||
const mockTagItems = [ | ||
{ | ||
_id: 'test-id', | ||
data: [{ field: ALERT_WORKFLOW_TAGS, value: ['tag-1', 'tag-2'] }], | ||
ecs: { _id: 'test-id' }, | ||
}, | ||
]; | ||
|
||
(useUiSetting$ as jest.Mock).mockReturnValue(['default-test-tag']); | ||
(useAppToasts as jest.Mock).mockReturnValue({ | ||
addError: jest.fn(), | ||
addSuccess: jest.fn(), | ||
addWarning: jest.fn(), | ||
}); | ||
(useSetAlertTags as jest.Mock).mockReturnValue({ | ||
setAlertTags: jest.fn(), | ||
}); | ||
|
||
describe('BulkAlertTagsPanel', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add more tests here! This component has a ton of logic, some of it being async. It is important to have unit test coverage on components like this |
||
test('it renders', () => { | ||
const wrapper = render( | ||
<TestProviders> | ||
<BulkAlertTagsPanel | ||
alertItems={mockTagItems} | ||
setIsLoading={() => {}} | ||
closePopoverMenu={() => {}} | ||
/> | ||
</TestProviders> | ||
); | ||
|
||
expect(wrapper.queryByTestId('alert-tags-selectable-menu')).toBeTruthy(); | ||
dplumlee marked this conversation as resolved.
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.
@elastic/actionable-observability we chose to create a new field here instead of using an existing one (
tags
was our original choice) because we found that, since we copy over source event tags into the top leveltags
field, it was possible to "merge" these event tags with our user based tags potentially leading to some confusing workflows or breaking them all together. We went withworkflow_tags
as the usage for the tags in this feature aligned fairly well with the existing other workflow fields, and we wouldn't have to worry about breaking existing workflows or effectively having source event data be mutable by usingtags