[Security Solution][Detection Alerts] Alert tagging #157786

dplumlee · 2023-05-15T18:41:06Z

Summary

Epic (internal): https://github.com/elastic/security-team/issues/3273

Adds tagging feature to alerts generated by security solution detection rules. Users will now be able to tag individual alerts via a dropdown on the alerts page or via the take action menu in the alert details flyout. They will also be able to bulk manage tags within the bulk actions menu on the alerts table. Tag options are generated from a field in the advanced settings and will be defaulted to 3 options (Duplicate, False positive, Further investigation required).

From a code standpoint, alert tags will live in a new field: kibana.alert.workflow_tags in order to avoid merging with copied source event tags in existing schema fields

Screenshots

From alert context menu

From bulk actions

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)

For maintainers

This was checked for breaking API changes and was labeled appropriately

x-pack/plugins/security_solution/common/constants.ts

yctercero · 2023-05-17T20:43:31Z

x-pack/plugins/security_solution/common/detection_engine/schemas/common/schemas.ts

@@ -125,3 +127,10 @@ export const privilege = t.type({
 });

 export type Privilege = t.TypeOf<typeof privilege>;
+
+export const alert_tags = t.type({


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:

const BulkActionEditPayloadTags = t.type({ type: t.union([ t.literal(BulkActionEditType.add_tags), t.literal(BulkActionEditType.delete_tags), t.literal(BulkActionEditType.set_tags), ]), value: RuleTagArray, });

That way if we decide theres a new action type we want to introduce for tags, the schema updates are minimal.

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.

yctercero · 2023-05-17T20:55:55Z

.../plugins/security_solution/public/common/components/toolbar/bulk_actions/alert_bulk_tags.tsx

+
+  const { setAlertTags } = useSetAlertTags();
+  const initalTagsState = useMemo(() => {
+    const existingTags = alertIds.map(


Nit: can we extract this into a helper or util we can unit test?

Also, would using a set here simplify the number of iterations through the tags?

.../plugins/security_solution/public/common/components/toolbar/bulk_actions/alert_bulk_tags.tsx

yctercero · 2023-05-22T04:41:14Z

.../plugins/security_solution/public/common/components/toolbar/bulk_actions/alert_bulk_tags.tsx

+    } else if (changedOption.checked === 'on') {
+      tagsToAdd[changedOption.label] = true;
+      delete tagsToRemove[changedOption.label];
+    } else if (!changedOption.checked) {


Here are you checking if the checked property exists or if it's null or undefined?

.../plugins/security_solution/public/common/components/toolbar/bulk_actions/alert_bulk_tags.tsx

packages/kbn-securitysolution-ecs/src/index.ts

x-pack/plugins/security_solution/server/lib/detection_engine/routes/signals/helpers.ts

stephmilovic · 2023-06-20T15:31:50Z

.../security_solution/public/detections/components/host_isolation/use_host_isolation_action.tsx

-            >
-              {isolateHostTitle}
-            </EuiContextMenuItem>,
+            {


needs unit test

stephmilovic · 2023-06-20T15:33:26Z

x-pack/plugins/security_solution/public/detections/components/take_action_dropdown/index.tsx

        !isEvent && actionsData.ruleId
-          ? [...statusActionItems, ...exceptionActionItems]
+          ? [...statusActionItems, ...alertTagsItems, ...exceptionActionItems]
          : isEndpointEvent && canCreateEndpointEventFilters
          ? eventFilterActionItems
          : [],


please add unit tests for rendering correct alerts actions items

stephmilovic · 2023-06-20T15:36:16Z

...s/security_solution/public/detections/hooks/trigger_actions_alert_table/use_bulk_actions.tsx

@@ -88,5 +89,11 @@ export const getBulkActionHook =
      refetch: refetchGlobalQuery,
    });

-    return [...alertActions, timelineAction];
+    const { alertTagsItems, alertTagsPanels } = useBulkAlertTagsItems({


I know this file does not have any unit tests to add to, but it really should have them on the new and existing behavior

stephmilovic

Thanks for the new feature, super cool!

With Feature Freeze pending, I won't require the requested unit tests for approval, but I would request a Github issue be created noting what tests are missing.

One thing I do have to insist be done before merging is to add a mount check to any state updates that could happen after a component has dismounted (see here)

x-pack/plugins/security_solution/common/detection_engine/schemas/alerts/8.9.0/index.ts

stephmilovic · 2023-06-20T15:45:03Z

...lution/public/detections/components/alerts_table/timeline_actions/use_alert_tags_actions.tsx

+  refetch?: () => void;
+}
+
+export const useAlertTagsActions = ({ closePopover, ecsRowData, scopeId, refetch }: Props) => {


this hook needs unit tests

stephmilovic · 2023-06-20T15:46:12Z

x-pack/plugins/security_solution/server/lib/detection_engine/routes/signals/helpers.ts

+
+export const validateAlertTagsArrays = (tags: AlertTags) => {
+  const { tags_to_add: tagsToAdd, tags_to_remove: tagsToRemove } = tags;
+  const duplicates = tagsToAdd.filter((tag) => tagsToRemove.includes(tag));


needs a little test here as well

semd · 2023-06-20T15:47:17Z

.../plugins/security_solution/public/common/components/toolbar/bulk_actions/alert_bulk_tags.tsx

+      if (refetchQuery) refetchQuery();
+      if (refresh) refresh();
+      if (clearSelection) clearSelection();


Can we group all these props under a single onUpdateSuccess prop? And let the caller do whatever it needs when the update succeeded, outside this component.

stephmilovic · 2023-06-20T15:48:25Z

...gers_actions_ui/public/application/sections/alerts_table/bulk_actions/components/toolbar.tsx

-      }),
-    [alerts, isAllSelected, items, rowSelection, setIsBulkActionsLoading, clearSelection, refresh]
-  );
+  const bulkActionsPanels = useMemo(() => {


entire file needs unit tests, especially the logic here

stephmilovic · 2023-06-20T15:50:08Z

...ugins/triggers_actions_ui/public/application/sections/alerts_table/hooks/use_bulk_actions.ts

@@ -71,6 +72,23 @@ const getCaseAttachments = ({
  return groupAlertsByRule?.(filteredAlerts) ?? [];
 };

+const addItemsToInitalPanel = ({


Suggested change

const addItemsToInitalPanel = ({

const addItemsToInitialPanel = ({

Can you also pease ensure your tests hit each of these conditions, I believe only one is currently tested

stephmilovic · 2023-06-20T15:50:25Z

...ugins/triggers_actions_ui/public/application/sections/alerts_table/hooks/use_bulk_actions.ts


  const clearSelection = () => {
    updateBulkActionsState({ action: BulkActionsVerbs.clear });
  };
  const caseBulkActions = useBulkAddToCaseActions({ casesConfig, refresh, clearSelection });

-  const bulkActions = [...configBulkActions, ...caseBulkActions];
+  const bulkActions =
+    caseBulkActions.length !== 0


another condition we should test

yes this hook also probably runs too often as well, both the old way and new way.

...lugins/security_solution/public/common/components/toolbar/bulk_actions/use_set_alert_tags.ts

.../plugins/security_solution/public/common/components/toolbar/bulk_actions/alert_bulk_tags.tsx

...plugins/security_solution/server/lib/detection_engine/routes/signals/set_alert_tags_route.ts

kqualters-elastic · 2023-06-20T21:38:16Z

...ugins/security_solution/public/common/components/toolbar/bulk_actions/use_set_alert_tags.tsx

+      try {
+        setIsLoading(true);
+        setTableLoading(true);
+        const response = await http.fetch<estypes.UpdateByQueryResponse>(


why not react-query?

x-pack/plugins/security_solution/public/common/components/toolbar/bulk_actions/index.tsx

kqualters-elastic

new warning when I apply a tag:

Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in a useEffect cleanup function.
    at BulkAlertTagsPanelComponent (http://localhost:5601/9007199254740991/bundles/plugin/securitySolution/1.0.0/securitySolution.chunk.lazy_register_alerts_table_configuration.js:107579:3)
    at div
    at EuiResizeObserver (http://localhost:5601/9007199254740991/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:128128:81)
    at div
    at EuiContextMenuPanel (http://localhost:5601/9007199254740991/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:84005:81)
    at div
    at EuiContextMenu (http://localhost:5601/9007199254740991/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:83483:81)
    at div
    at EuiMutationObserver (http://localhost:5601/9007199254740991/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:127891:81)
    at div
    at http://localhost:5601/9007199254740991/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:166047:73
    at EuiPanel (http://localhost:5601/9007199254740991/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:131939:23)
    at http://localhost:5601/9007199254740991/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:166047:73
    at EuiPopoverPanel (http://localhost:5601/9007199254740991/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:133242:23)
    at div
    at http://localhost:5601/9007199254740991/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:366533:59
    at FocusLockUI (http://localhost:5601/9007199254740991/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:362836:71)
    at http://localhost:5601/9007199254740991/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:363739:60
    at EuiFocusTrap (http://localhost:5601/9007199254740991/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:105315:81)
    at EuiPortal (http://localhost:5601/9007199254740991/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:133710:81)
    at div
    at http://localhost:5601/9007199254740991/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:166047:73
    at EuiPopover (http://localhost:5601/9007199254740991/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:132478:81)
    at div
    at forwardRef (http://localhost:5601/9007199254740991/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:423669:12)
    at div
    at AlertContextMenuComponent (http://localhost:5601/9007199254740991/bundles/plugin/securitySolution/1.0.0/securitySolution.chunk.lazy_register_alerts_table_configuration.js:142978:3)
    at ConnectFunction (http://localhost:5601/9007199254740991/bundles/plugin/securitySolution/1.0.0/securitySolution.chunk.3.js:422:41)
    at div
    at forwardRef (http://localhost:5601/9007199254740991/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:423669:12)
    at ActionsComponent (http://localhost:5601/9007199254740991/bundles/plugin/securitySolution/1.0.0/securitySolution.chunk.lazy_register_alerts_table_configuration.js:95250:3)
    at RowActionComponent (http://localhost:5601/9007199254740991/bundles/plugin/securitySolution/1.0.0/securitySolution.chunk.lazy_register_alerts_table_configuration.js:85295:3)
    at div
    at http://localhost:5601/9007199254740991/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:166047:73
    at http://localhost:5601/9007199254740991/bundles/kbn-ui-shared-deps-npm/kbn-ui-shared-deps-npm.dll.js:104071:23
    at rowCellRender (http://localhost:5601/9007199254740991/bundles/plugin/triggersActionsUi/1.0.0/triggersActionsUi.chunk.24.js:1030:13)

kqualters-elastic

agree with needing additional unit tests, also should fix the warning(s) that appear, but overall feature works alright locally and any bugs/missing tests/warning fixes can be fixed post FF 👍

stephmilovic

Agree with @kqualters-elastic about the console warnings, that likely means we didn't catch the state updates on something dismounted. I strongly suggest updating the query to be managed either by useFetch or react-query to reduce risk of bad state updates. I added a comment to your unit test PR to address the console warnings as well. Thank you for the changes!

LGTM, please ping me directly for the follow up PR.

kibana-ci · 2023-06-21T02:01:18Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: baf4fde

Failed CI Steps

FTR Configs #17

Test Failures

[job] [logs] FTR Configs #17 / apis index_patterns scripted_fields_deprecations scripted field deprecations no scripted fields deprecations

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	4201	4206	+5

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/rule-data-utils`	103	104	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`apm`	3.6MB	3.6MB	+45.0B
`infra`	2.0MB	2.0MB	+45.0B
`observability`	1.0MB	1.0MB	+45.0B
`securitySolution`	11.0MB	11.0MB	+6.4KB
`triggersActionsUi`	1.4MB	1.4MB	+518.0B
total			+7.1KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`apm`	36.0KB	36.0KB	+60.0B
`infra`	119.7KB	119.7KB	+60.0B
`observability`	86.0KB	86.1KB	+60.0B
`securitySolution`	51.4KB	51.9KB	+475.0B
`triggersActionsUi`	85.7KB	85.7KB	+60.0B
total			+715.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/rule-data-utils`	106	107	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	13	15	+2
`securitySolution`	411	415	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	14	16	+2
`securitySolution`	494	498	+4
total			+6

History

💔 Build #136773 failed 9d8ae0c
💔 Build #136699 failed 0119e69
💚 Build #135917 succeeded 69cbf20
💛 Build #135893 was flaky 64b5077
💔 Build #135864 failed a352269

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @dplumlee

dplumlee added Team:Detections and Resp Security Detection Response Team Feature:Detection Alerts Security Solution Detection Alerts Feature release_note:feature Makes this part of the condensed release notes Team:Detection Alerts Security Detection Alerts Area Team v8.9.0 labels May 15, 2023

dplumlee self-assigned this May 15, 2023

working poc-types still broken

3ad0adf

dplumlee force-pushed the security-alert-tags branch from d514119 to 3ad0adf Compare May 15, 2023 18:50

yctercero reviewed May 17, 2023

View reviewed changes

x-pack/plugins/security_solution/common/constants.ts Show resolved Hide resolved

yctercero reviewed May 22, 2023

View reviewed changes

dplumlee added 10 commits May 22, 2023 17:39

adds translations

233586d

updates some tests

c5029b6

fixes some type errors

96658d8

fixes more tests

c77aa15

fixes some more types

d391a76

fixes tests

a06d3b5

fixes tests 2

1a5c13c

Merge remote-tracking branch 'upstream/main' into security-alert-tags

bbd7a89

fix telemetry check

938e805

uiSettings fix

3bcd05a

dplumlee marked this pull request as ready for review May 30, 2023 17:12

dplumlee requested review from a team as code owners May 30, 2023 17:12

stephmilovic reviewed Jun 20, 2023

View reviewed changes

stephmilovic requested changes Jun 20, 2023

View reviewed changes

stephmilovic reviewed Jun 20, 2023

View reviewed changes

x-pack/plugins/security_solution/common/detection_engine/schemas/alerts/8.9.0/index.ts Outdated Show resolved Hide resolved

stephmilovic reviewed Jun 20, 2023

View reviewed changes

semd reviewed Jun 20, 2023

View reviewed changes

stephmilovic reviewed Jun 20, 2023

View reviewed changes

semd reviewed Jun 20, 2023

View reviewed changes

...lugins/security_solution/public/common/components/toolbar/bulk_actions/use_set_alert_tags.ts Outdated Show resolved Hide resolved

semd reviewed Jun 20, 2023

View reviewed changes

.../plugins/security_solution/public/common/components/toolbar/bulk_actions/alert_bulk_tags.tsx Show resolved Hide resolved

machadoum reviewed Jun 20, 2023

View reviewed changes

...plugins/security_solution/server/lib/detection_engine/routes/signals/set_alert_tags_route.ts Outdated Show resolved Hide resolved

pgayvallet approved these changes Jun 20, 2023

View reviewed changes

dplumlee added 3 commits June 20, 2023 14:39

addresses comments

4a6a735

updates helper util

db9bc72

Merge remote-tracking branch 'upstream/main' into security-alert-tags

0119e69

kqualters-elastic reviewed Jun 20, 2023

View reviewed changes

x-pack/plugins/security_solution/public/common/components/toolbar/bulk_actions/index.tsx Outdated Show resolved Hide resolved

addresses comments and updates tests

9d8ae0c

kqualters-elastic reviewed Jun 20, 2023

View reviewed changes

dplumlee mentioned this pull request Jun 20, 2023

[Security Solution][Detection Alerts] Add follow-up unit tests from alert tagging PR #160086

Open

kqualters-elastic approved these changes Jun 20, 2023

View reviewed changes

stephmilovic approved these changes Jun 20, 2023

View reviewed changes

skips tests due to refresh change

baf4fde

dplumlee merged commit 0f57260 into elastic:main Jun 21, 2023

kibanamachine added the backport:skip This commit does not require backporting label Jun 21, 2023

dplumlee deleted the security-alert-tags branch July 4, 2023 05:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detection Alerts] Alert tagging #157786

[Security Solution][Detection Alerts] Alert tagging #157786

dplumlee commented May 15, 2023 •

edited

Loading

yctercero May 17, 2023

dplumlee May 22, 2023 •

edited

Loading

yctercero May 17, 2023

yctercero May 22, 2023

stephmilovic Jun 20, 2023

stephmilovic Jun 20, 2023

stephmilovic Jun 20, 2023

stephmilovic left a comment •

edited

Loading

stephmilovic Jun 20, 2023

stephmilovic Jun 20, 2023

semd Jun 20, 2023

stephmilovic Jun 20, 2023

stephmilovic Jun 20, 2023

stephmilovic Jun 20, 2023

kqualters-elastic Jun 20, 2023

kqualters-elastic Jun 20, 2023

kqualters-elastic left a comment

kqualters-elastic left a comment

stephmilovic left a comment

kibana-ci commented Jun 21, 2023

API count

ESLint disabled line counts

Total ESLint disabled count

	const addItemsToInitalPanel = ({
	const addItemsToInitialPanel = ({

[Security Solution][Detection Alerts] Alert tagging #157786

[Security Solution][Detection Alerts] Alert tagging #157786

Conversation

dplumlee commented May 15, 2023 • edited Loading

Summary

Screenshots

From alert context menu

From bulk actions

Checklist

For maintainers

Choose a reason for hiding this comment

dplumlee May 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephmilovic left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kqualters-elastic left a comment

Choose a reason for hiding this comment

kqualters-elastic left a comment

Choose a reason for hiding this comment

stephmilovic left a comment

Choose a reason for hiding this comment

kibana-ci commented Jun 21, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

dplumlee commented May 15, 2023 •

edited

Loading

dplumlee May 22, 2023 •

edited

Loading

stephmilovic left a comment •

edited

Loading