[Security Solution][Case][Bug] Only update alert status in its specif…

…ic index (#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 <42973632+kibanamachine@users.noreply.github.com>
elastic · Mar 3, 2021 · 9dd395b · 9dd395b
1 parent 7cf2284
commit 9dd395b
Show file tree

Hide file tree

Showing 27 changed files with 7,282 additions and 312 deletions.
diff --git a/x-pack/plugins/case/server/client/alerts/get.ts b/x-pack/plugins/case/server/client/alerts/get.ts
@@ -6,34 +6,33 @@
  */
 
 import { ElasticsearchClient, Logger } from 'kibana/server';
+import { AlertInfo } from '../../common';
 import { AlertServiceContract } from '../../services';
 import { CaseClientGetAlertsResponse } from './types';
 
 interface GetParams {
   alertsService: AlertServiceContract;
-  ids: string[];
-  indices: Set<string>;
+  alertsInfo: AlertInfo[];
   scopedClusterClient: ElasticsearchClient;
   logger: Logger;
 }
 
 export const get = async ({
   alertsService,
-  ids,
-  indices,
+  alertsInfo,
   scopedClusterClient,
   logger,
 }: GetParams): Promise<CaseClientGetAlertsResponse> => {
-  if (ids.length === 0 || indices.size <= 0) {
+  if (alertsInfo.length === 0) {
     return [];
   }
 
-  const alerts = await alertsService.getAlerts({ ids, indices, scopedClusterClient, logger });
+  const alerts = await alertsService.getAlerts({ alertsInfo, scopedClusterClient, logger });
   if (!alerts) {
     return [];
   }
 
-  return alerts.hits.hits.map((alert) => ({
+  return alerts.docs.map((alert) => ({
     id: alert._id,
     index: alert._index,
     ...alert._source,

diff --git a/x-pack/plugins/case/server/client/alerts/update_status.test.ts b/x-pack/plugins/case/server/client/alerts/update_status.test.ts
@@ -15,17 +15,13 @@ describe('updateAlertsStatus', () => {
 
     const caseClient = await createCaseClientWithMockSavedObjectsClient({ savedObjectsClient });
     await caseClient.client.updateAlertsStatus({
-      ids: ['alert-id-1'],
-      status: CaseStatuses.closed,
-      indices: new Set<string>(['.siem-signals']),
+      alerts: [{ id: 'alert-id-1', index: '.siem-signals', status: CaseStatuses.closed }],
     });
 
     expect(caseClient.services.alertsService.updateAlertsStatus).toHaveBeenCalledWith({
-      scopedClusterClient: expect.anything(),
       logger: expect.anything(),
-      ids: ['alert-id-1'],
-      indices: new Set<string>(['.siem-signals']),
-      status: CaseStatuses.closed,
+      scopedClusterClient: expect.anything(),
+      alerts: [{ id: 'alert-id-1', index: '.siem-signals', status: CaseStatuses.closed }],
     });
   });
 });
diff --git a/x-pack/plugins/case/server/client/alerts/update_status.ts b/x-pack/plugins/case/server/client/alerts/update_status.ts
@@ -6,25 +6,21 @@
  */
 
 import { ElasticsearchClient, Logger } from 'src/core/server';
-import { CaseStatuses } from '../../../common/api';
 import { AlertServiceContract } from '../../services';
+import { UpdateAlertRequest } from '../types';
 
 interface UpdateAlertsStatusArgs {
   alertsService: AlertServiceContract;
-  ids: string[];
-  status: CaseStatuses;
-  indices: Set<string>;
+  alerts: UpdateAlertRequest[];
   scopedClusterClient: ElasticsearchClient;
   logger: Logger;
 }
 
 export const updateAlertsStatus = async ({
   alertsService,
-  ids,
-  status,
-  indices,
+  alerts,
   scopedClusterClient,
   logger,
 }: UpdateAlertsStatusArgs): Promise<void> => {
-  await alertsService.updateAlertsStatus({ ids, status, indices, scopedClusterClient, logger });
+  await alertsService.updateAlertsStatus({ alerts, scopedClusterClient, logger });
 };
diff --git a/x-pack/plugins/case/server/client/cases/push.ts b/x-pack/plugins/case/server/client/cases/push.ts
@@ -15,7 +15,7 @@ import {
   SavedObject,
 } from 'kibana/server';
 import { ActionResult, ActionsClient } from '../../../../actions/server';
-import { flattenCaseSavedObject, getAlertIndicesAndIDs } from '../../routes/api/utils';
+import { flattenCaseSavedObject, getAlertInfoFromComments } from '../../routes/api/utils';
 
 import {
   ActionConnector,
@@ -108,12 +108,11 @@ export const push = async ({
     );
   }
 
-  const { ids, indices } = getAlertIndicesAndIDs(theCase?.comments);
+  const alertsInfo = getAlertInfoFromComments(theCase?.comments);
 
   try {
     alerts = await caseClient.getAlerts({
-      ids,
-      indices,
+      alertsInfo,
     });
   } catch (e) {
     throw createCaseError({

diff --git a/x-pack/plugins/case/server/client/cases/update.test.ts b/x-pack/plugins/case/server/client/cases/update.test.ts
@@ -430,9 +430,13 @@ describe('update', () => {
       await caseClient.client.update(patchCases);
 
       expect(caseClient.client.updateAlertsStatus).toHaveBeenCalledWith({
-        ids: ['test-id'],
-        status: 'closed',
-        indices: new Set<string>(['test-index']),
+        alerts: [
+          {
+            id: 'test-id',
+            index: 'test-index',
+            status: 'closed',
+          },
+        ],
       });
     });
 
@@ -458,11 +462,10 @@ describe('update', () => {
       });
 
       const caseClient = await createCaseClientWithMockSavedObjectsClient({ savedObjectsClient });
-      caseClient.client.updateAlertsStatus = jest.fn();
 
       await caseClient.client.update(patchCases);
 
-      expect(caseClient.client.updateAlertsStatus).not.toHaveBeenCalled();
+      expect(caseClient.esClient.bulk).not.toHaveBeenCalled();
     });
 
     test('it updates alert status when syncAlerts is turned on', async () => {
@@ -492,9 +495,7 @@ describe('update', () => {
       await caseClient.client.update(patchCases);
 
       expect(caseClient.client.updateAlertsStatus).toHaveBeenCalledWith({
-        ids: ['test-id'],
-        status: 'open',
-        indices: new Set<string>(['test-index']),
+        alerts: [{ id: 'test-id', index: 'test-index', status: 'open' }],
       });
     });
 
@@ -515,11 +516,10 @@ describe('update', () => {
       });
 
       const caseClient = await createCaseClientWithMockSavedObjectsClient({ savedObjectsClient });
-      caseClient.client.updateAlertsStatus = jest.fn();
 
       await caseClient.client.update(patchCases);
 
-      expect(caseClient.client.updateAlertsStatus).not.toHaveBeenCalled();
+      expect(caseClient.esClient.bulk).not.toHaveBeenCalled();
     });
 
     test('it updates alert status for multiple cases', async () => {
@@ -576,22 +576,12 @@ describe('update', () => {
       caseClient.client.updateAlertsStatus = jest.fn();
 
       await caseClient.client.update(patchCases);
-      /**
-       * the update code will put each comment into a status bucket and then make at most 1 call
-       * to ES for each status bucket
-       * Now instead of doing a call per case to get the comments, it will do a single call with all the cases
-       * and sub cases and get all the comments in one go
-       */
-      expect(caseClient.client.updateAlertsStatus).toHaveBeenNthCalledWith(1, {
-        ids: ['test-id'],
-        status: 'open',
-        indices: new Set<string>(['test-index']),
-      });
 
-      expect(caseClient.client.updateAlertsStatus).toHaveBeenNthCalledWith(2, {
-        ids: ['test-id-2'],
-        status: 'closed',
-        indices: new Set<string>(['test-index-2']),
+      expect(caseClient.client.updateAlertsStatus).toHaveBeenCalledWith({
+        alerts: [
+          { id: 'test-id', index: 'test-index', status: 'open' },
+          { id: 'test-id-2', index: 'test-index-2', status: 'closed' },
+        ],
       });
     });
 
@@ -611,11 +601,10 @@ describe('update', () => {
       });
 
       const caseClient = await createCaseClientWithMockSavedObjectsClient({ savedObjectsClient });
-      caseClient.client.updateAlertsStatus = jest.fn();
 
       await caseClient.client.update(patchCases);
 
-      expect(caseClient.client.updateAlertsStatus).not.toHaveBeenCalled();
+      expect(caseClient.esClient.bulk).not.toHaveBeenCalled();
     });
   });
 

diff --git a/x-pack/plugins/case/server/client/cases/update.ts b/x-pack/plugins/case/server/client/cases/update.ts
@@ -18,7 +18,6 @@ import {
   Logger,
 } from 'kibana/server';
 import {
-  AlertInfo,
   flattenCaseSavedObject,
   isCommentRequestTypeAlertOrGenAlert,
 } from '../../routes/api/utils';
@@ -53,7 +52,8 @@ import {
   SUB_CASE_SAVED_OBJECT,
 } from '../../saved_object_types';
 import { CaseClientHandler } from '..';
-import { addAlertInfoToStatusMap } from '../../common';
+import { createAlertUpdateRequest } from '../../common';
+import { UpdateAlertRequest } from '../types';
 import { createCaseError } from '../../common/error';
 
 /**
@@ -291,33 +291,25 @@ async function updateAlerts({
   // get a map of sub case id to the sub case status
   const subCasesToStatus = await getSubCasesToStatus({ totalAlerts, client, caseService });
 
-  // create a map of the case statuses to the alert information that we need to update for that status
-  // This allows us to make at most 3 calls to ES, one for each status type that we need to update
-  // One potential improvement here is to do a tick (set timeout) to reduce the memory footprint if that becomes an issue
-  const alertsToUpdate = totalAlerts.saved_objects.reduce((acc, alertComment) => {
-    if (isCommentRequestTypeAlertOrGenAlert(alertComment.attributes)) {
-      const status = getSyncStatusForComment({
-        alertComment,
-        casesToSyncToStatus,
-        subCasesToStatus,
-      });
+  // 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)) {
+        const status = getSyncStatusForComment({
+          alertComment,
+          casesToSyncToStatus,
+          subCasesToStatus,
+        });
 
-      addAlertInfoToStatusMap({ comment: alertComment.attributes, statusMap: acc, status });
-    }
+        acc.push(...createAlertUpdateRequest({ comment: alertComment.attributes, status }));
+      }
 
-    return acc;
-  }, new Map<CaseStatuses, AlertInfo>());
-
-  // This does at most 3 calls to Elasticsearch to update the status of the alerts to either open, closed, or in-progress
-  for (const [status, alertInfo] of alertsToUpdate.entries()) {
-    if (alertInfo.ids.length > 0 && alertInfo.indices.size > 0) {
-      caseClient.updateAlertsStatus({
-        ids: alertInfo.ids,
-        status,
-        indices: alertInfo.indices,
-      });
-    }
-  }
+      return acc;
+    },
+    []
+  );
+
+  await caseClient.updateAlertsStatus({ alerts: alertsToUpdate });
 }
 
 interface UpdateArgs {

diff --git a/x-pack/plugins/case/server/client/client.ts b/x-pack/plugins/case/server/client/client.ts
@@ -169,9 +169,9 @@ export class CaseClientHandler implements CaseClient {
       });
     } catch (error) {
       throw createCaseError({
-        message: `Failed to update alerts status using client ids: ${JSON.stringify(
-          args.ids
-        )} \nindices: ${JSON.stringify([...args.indices])} \nstatus: ${args.status}: ${error}`,
+        message: `Failed to update alerts status using client alerts: ${JSON.stringify(
+          args.alerts
+        )}: ${error}`,
         error,
         logger: this.logger,
       });
@@ -218,9 +218,9 @@ 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}`,
+        message: `Failed to get alerts using client requested alerts: ${JSON.stringify(
+          args.alertsInfo
+        )}: ${error}`,
         error,
         logger: this.logger,
       });