[Security Solution][Case][Bug] Only update alert status in its specific index #92530

jonathan-buttner · 2021-02-23T22:14:19Z

This PR fixes a bug where updating the status of an alert could change multiple alerts if they have the same _id in separate indices. To fix this we'll update and retrieve alerts using the Elasticsearch bulk and mget APIs. The alert information is currently stored in two arrays (alertIds and index) within the saved object. Before we interact with the Elasticsearch APIs we'll transform them into an array of objects so we tie each alert ID to a specific index. This avoids accidentally updating an alert with a specific _id across multiple indices at the same time.

Issue: #91936

elasticmachine · 2021-02-23T22:14:21Z

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

…up-id

jonathan-buttner · 2021-03-01T14:12:33Z

@elasticmachine merge upstream

michaelolo24 · 2021-03-01T21:13:30Z

x-pack/plugins/case/server/client/alerts/update_status.test.ts

-      ids: ['alert-id-1'],
-      status: CaseStatuses.closed,
-      indices: new Set<string>(['.siem-signals']),
+      alerts: [{ id: 'alert-id-1', index: '.siem-signals', status: CaseStatuses.closed }],


So each alert will only have one index? Just curious if index should still be a Set()

michaelolo24 · 2021-03-01T21:17:50Z

x-pack/plugins/case/server/client/cases/update.ts

+  // create an array of requests that indicate the id, index, and status to update an alert
+  const alertsToUpdate = totalAlerts.saved_objects.reduce(
+    (acc: UpdateAlertRequest[], alertComment) => {
+      if (isCommentRequestTypeAlertOrGenAlert(alertComment.attributes)) {


Not necessary for this PR, but just thinking it may be better to have functions that only care about one status, i.e. isCommentRequestType(attributes) || isGenAlert(attributes). I just imagine other forms of alerts in the near or far future appearing and this becoming more unwieldy.

michaelolo24 · 2021-03-01T21:21:38Z

x-pack/plugins/case/server/client/client.ts

@@ -219,8 +219,8 @@ export class CaseClientHandler implements CaseClient {
    } catch (error) {
      throw createCaseError({
        message: `Failed to get alerts using client ids: ${JSON.stringify(
-          args.ids
-        )} \nindices: ${JSON.stringify([...args.indices])}: ${error}`,
+          args.alertsInfo


~~is it alerts or alertsInfo...Looking at your other change in this file on line 172 (should they be the same or different?)~~ (saw your client/types file). Also, forgot to change the "ids" to "alerts" right before the stringify in this change

Oh thank you. Yeah alertsInfo vs alerts is kind of confusing. They're slightly different structures but I didn't know how to name them haha. Any ideas?

michaelolo24 · 2021-03-01T21:33:52Z

x-pack/plugins/case/server/routes/api/utils.ts

+ * id existing in an index at each position in the array. This is not ideal. Ideally an alert comment request would
+ * accept an array of objects like this: Array<{id: string; index: string; ruleName: string ruleID: string}> instead.
+ *
+ * To reformat the alert comment request requires a migration and a breaking API change.


Can you create an issue for this when you have time, so we can discuss it there? It sounds like it's worth making the change for more stability. And the changed API seems much easier to potentially work with

Yeah good idea. I'll create the issue 👍

https://github.com/elastic/security-team/issues/889

…up-id

michaelolo24

Reviewed here and also virtually with @jonathan-buttner

kibanamachine · 2021-03-02T22:23:06Z

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id	before	after	diff
`triggersActionsUi`	1.6MB	1.5MB	-23.9KB

Page load bundle

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

id	before	after	diff
`triggersActionsUi`	104.1KB	104.2KB	+82.0B

Unknown metric groups

async chunk count

id	before	after	diff
`triggersActionsUi`	41	42	+1

History

💚 Build #109905 succeeded ea924a6
💚 Build #109739 succeeded 3e03b8d
💚 Build #109722 succeeded 881adc8
💚 Build #109675 succeeded e2011d0
💔 Build #109631 failed 491e972

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

…ic index (elastic#92530) * Writing failing test for duplicate ids * Test is correctly failing prior to bug fix * Working jest tests * Adding more jest tests * Fixing jest tests * Adding await and gzip * Fixing type errors * Updating log message Co-authored-by: Kibana Machine <[email protected]>

…ic index (#92530) (#93484) * Writing failing test for duplicate ids * Test is correctly failing prior to bug fix * Working jest tests * Adding more jest tests * Fixing jest tests * Adding await and gzip * Fixing type errors * Updating log message Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

…ic index (#92530) (#93509) * Writing failing test for duplicate ids * Test is correctly failing prior to bug fix * Working jest tests * Adding more jest tests * Fixing jest tests * Adding await and gzip * Fixing type errors * Updating log message Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

jonathan-buttner added 2 commits February 23, 2021 13:33

Writing failing test for duplicate ids

880f019

Test is correctly failing prior to bug fix

b231725

jonathan-buttner added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.12.0 Team:Threat Hunting Security Solution Threat Hunting Team Feature:Cases Cases feature v7.13.0 labels Feb 23, 2021

jonathan-buttner requested a review from a team as a code owner February 23, 2021 22:14

jonathan-buttner added 5 commits February 24, 2021 11:29

Merge branch 'master' of github.com:elastic/kibana into sync-alerts-d…

3dc29c9

…up-id

Working jest tests

b852ec2

Merge branch 'master' of github.com:elastic/kibana into sync-alerts-d…

036d1ad

…up-id

Adding more jest tests

491e972

Fixing jest tests

60aa819

jonathan-buttner requested review from XavierM, cnasikas and michaelolo24 February 26, 2021 02:51

jonathan-buttner added 4 commits February 25, 2021 23:02

Adding await and gzip

e2011d0

Merge branch 'master' of github.com:elastic/kibana into sync-alerts-d…

881adc8

…up-id

Merge branch 'master' of github.com:elastic/kibana into sync-alerts-d…

48139f1

…up-id

Fixing type errors

3e03b8d

Merge branch 'master' into sync-alerts-dup-id

ea924a6

michaelolo24 reviewed Mar 1, 2021

View reviewed changes

jonathan-buttner added 2 commits March 2, 2021 15:22

Merge branch 'master' of github.com:elastic/kibana into sync-alerts-d…

0e43a58

…up-id

Updating log message

f29539f

michaelolo24 approved these changes Mar 2, 2021

View reviewed changes

jonathan-buttner removed the v7.12.0 label Mar 3, 2021

jonathan-buttner merged commit 9dd395b into elastic:master Mar 3, 2021

jonathan-buttner deleted the sync-alerts-dup-id branch March 3, 2021 18:29

jonathan-buttner mentioned this pull request Mar 3, 2021

[7.x] [Security Solution][Case][Bug] Only update alert status in its specific index (#92530) #93484

Merged

jonathan-buttner mentioned this pull request Mar 3, 2021

[7.12] [Security Solution][Case][Bug] Only update alert status in its specific index (#92530) #93509

Merged

ghost mentioned this pull request Mar 15, 2021

[Security Solution][Case][Bug] Multiple alerts updated for single _id #91936

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Case][Bug] Only update alert status in its specific index #92530

[Security Solution][Case][Bug] Only update alert status in its specific index #92530

jonathan-buttner commented Feb 23, 2021 •

edited

Loading

elasticmachine commented Feb 23, 2021

jonathan-buttner commented Mar 1, 2021

michaelolo24 Mar 1, 2021

michaelolo24 Mar 1, 2021

michaelolo24 Mar 1, 2021 •

edited

Loading

jonathan-buttner Mar 2, 2021

michaelolo24 Mar 1, 2021

jonathan-buttner Mar 2, 2021

jonathan-buttner Mar 2, 2021

michaelolo24 Mar 2, 2021

michaelolo24 left a comment

kibanamachine commented Mar 2, 2021

async chunk count

[Security Solution][Case][Bug] Only update alert status in its specific index #92530

[Security Solution][Case][Bug] Only update alert status in its specific index #92530

Conversation

jonathan-buttner commented Feb 23, 2021 • edited Loading

elasticmachine commented Feb 23, 2021

jonathan-buttner commented Mar 1, 2021

michaelolo24 Mar 1, 2021

Choose a reason for hiding this comment

michaelolo24 Mar 1, 2021

Choose a reason for hiding this comment

michaelolo24 Mar 1, 2021 • edited Loading

Choose a reason for hiding this comment

jonathan-buttner Mar 2, 2021

Choose a reason for hiding this comment

michaelolo24 Mar 1, 2021

Choose a reason for hiding this comment

jonathan-buttner Mar 2, 2021

Choose a reason for hiding this comment

jonathan-buttner Mar 2, 2021

Choose a reason for hiding this comment

michaelolo24 Mar 2, 2021

Choose a reason for hiding this comment

michaelolo24 left a comment

Choose a reason for hiding this comment

kibanamachine commented Mar 2, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

async chunk count

History

jonathan-buttner commented Feb 23, 2021 •

edited

Loading

michaelolo24 Mar 1, 2021 •

edited

Loading