[Security Solution][Detections] Handle conflicts on alert status upda…

…te (#75492) * Proceed on conflict when updating alert status * Handle conflicts * Don't let the user retry * Tweak error messages * Fix route * Update add exception modal * Reapply changes after fixing conflicts * Type errors * types * Fix remaining conflicts * Fix tests * More test fixes * Simplify onConflict evaluation * Add callback return types * Update translation paths * Add missing import Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
elastic · Sep 4, 2020 · a7326e6 · a7326e6
1 parent c778646
commit a7326e6
Show file tree

Hide file tree

Showing 15 changed files with 145 additions and 60 deletions.
diff --git a/x-pack/plugins/security_solution/common/detection_engine/schemas/common/schemas.ts b/x-pack/plugins/security_solution/common/detection_engine/schemas/common/schemas.ts
@@ -283,6 +283,9 @@ export type Status = t.TypeOf<typeof status>;
 export const job_status = t.keyof({ succeeded: null, failed: null, 'going to run': null });
 export type JobStatus = t.TypeOf<typeof job_status>;
 
+export const conflicts = t.keyof({ abort: null, proceed: null });
+export type Conflicts = t.TypeOf<typeof conflicts>;
+
 // TODO: Create a regular expression type or custom date math part type here
 export const to = t.string;
 export type To = t.TypeOf<typeof to>;

diff --git a/...ins/security_solution/common/detection_engine/schemas/request/set_signal_status_schema.ts b/...ins/security_solution/common/detection_engine/schemas/request/set_signal_status_schema.ts
@@ -6,13 +6,14 @@
 
 import * as t from 'io-ts';
 
-import { signal_ids, signal_status_query, status } from '../common/schemas';
+import { conflicts, signal_ids, signal_status_query, status } from '../common/schemas';
 
 export const setSignalsStatusSchema = t.intersection([
   t.type({
     status,
   }),
   t.partial({
+    conflicts,
     signal_ids,
     query: signal_status_query,
   }),

diff --git a/...ugins/security_solution/public/common/components/exceptions/add_exception_modal/index.tsx b/...ugins/security_solution/public/common/components/exceptions/add_exception_modal/index.tsx
@@ -26,6 +26,7 @@ import {
   CreateExceptionListItemSchema,
   ExceptionListType,
 } from '../../../../../public/lists_plugin_deps';
+import * as i18nCommon from '../../../translations';
 import * as i18n from './translations';
 import * as sharedI18n from '../translations';
 import { TimelineNonEcsData, Ecs } from '../../../../graphql/types';
@@ -49,6 +50,7 @@ import {
 } from '../helpers';
 import { ErrorInfo, ErrorCallout } from '../error_callout';
 import { useFetchIndexPatterns } from '../../../../detections/containers/detection_engine/rules';
+import { ExceptionsBuilderExceptionItem } from '../types';
 
 export interface AddExceptionModalBaseProps {
   ruleName: string;
@@ -117,7 +119,7 @@ export const AddExceptionModal = memo(function AddExceptionModal({
     Array<ExceptionListItemSchema | CreateExceptionListItemSchema>
   >([]);
   const [fetchOrCreateListError, setFetchOrCreateListError] = useState<ErrorInfo | null>(null);
-  const { addError, addSuccess } = useAppToasts();
+  const { addError, addSuccess, addWarning } = useAppToasts();
   const { loading: isSignalIndexLoading, signalIndexName } = useSignalIndex();
   const [
     { isLoading: isSignalIndexPatternLoading, indexPatterns: signalIndexPatterns },
@@ -129,16 +131,26 @@ export const AddExceptionModal = memo(function AddExceptionModal({
   );
 
   const onError = useCallback(
-    (error: Error) => {
+    (error: Error): void => {
       addError(error, { title: i18n.ADD_EXCEPTION_ERROR });
       onCancel();
     },
     [addError, onCancel]
   );
-  const onSuccess = useCallback(() => {
-    addSuccess(i18n.ADD_EXCEPTION_SUCCESS);
-    onConfirm(shouldCloseAlert, shouldBulkCloseAlert);
-  }, [addSuccess, onConfirm, shouldBulkCloseAlert, shouldCloseAlert]);
+
+  const onSuccess = useCallback(
+    (updated: number, conflicts: number): void => {
+      addSuccess(i18n.ADD_EXCEPTION_SUCCESS);
+      onConfirm(shouldCloseAlert, shouldBulkCloseAlert);
+      if (conflicts > 0) {
+        addWarning({
+          title: i18nCommon.UPDATE_ALERT_STATUS_FAILED(conflicts),
+          text: i18nCommon.UPDATE_ALERT_STATUS_FAILED_DETAILED(updated, conflicts),
+        });
+      }
+    },
+    [addSuccess, addWarning, onConfirm, shouldBulkCloseAlert, shouldCloseAlert]
+  );
 
   const [{ isLoading: addExceptionIsLoading }, addOrUpdateExceptionItems] = useAddOrUpdateException(
     {
@@ -153,7 +165,7 @@ export const AddExceptionModal = memo(function AddExceptionModal({
       exceptionItems,
     }: {
       exceptionItems: Array<ExceptionListItemSchema | CreateExceptionListItemSchema>;
-    }) => {
+    }): void => {
       setExceptionItemsToAdd(exceptionItems);
     },
     [setExceptionItemsToAdd]
@@ -186,7 +198,7 @@ export const AddExceptionModal = memo(function AddExceptionModal({
   );
 
   const handleFetchOrCreateExceptionListError = useCallback(
-    (error: Error, statusCode: number | null, message: string | null) => {
+    (error: Error, statusCode: number | null, message: string | null): void => {
       setFetchOrCreateListError({
         reason: error.message,
         code: statusCode,
@@ -205,7 +217,7 @@ export const AddExceptionModal = memo(function AddExceptionModal({
     onSuccess: handleRuleChange,
   });
 
-  const initialExceptionItems = useMemo(() => {
+  const initialExceptionItems = useMemo((): ExceptionsBuilderExceptionItem[] => {
     if (exceptionListType === 'endpoint' && alertData !== undefined && ruleExceptionList) {
       return defaultEndpointExceptionItems(
         exceptionListType,
@@ -218,7 +230,7 @@ export const AddExceptionModal = memo(function AddExceptionModal({
     }
   }, [alertData, exceptionListType, ruleExceptionList, ruleName]);
 
-  useEffect(() => {
+  useEffect((): void => {
     if (isSignalIndexPatternLoading === false && isSignalIndexLoading === false) {
       setShouldDisableBulkClose(
         entryHasListType(exceptionItemsToAdd) ||
@@ -234,34 +246,34 @@ export const AddExceptionModal = memo(function AddExceptionModal({
     signalIndexPatterns,
   ]);
 
-  useEffect(() => {
+  useEffect((): void => {
     if (shouldDisableBulkClose === true) {
       setShouldBulkCloseAlert(false);
     }
   }, [shouldDisableBulkClose]);
 
   const onCommentChange = useCallback(
-    (value: string) => {
+    (value: string): void => {
       setComment(value);
     },
     [setComment]
   );
 
   const onCloseAlertCheckboxChange = useCallback(
-    (event: React.ChangeEvent<HTMLInputElement>) => {
+    (event: React.ChangeEvent<HTMLInputElement>): void => {
       setShouldCloseAlert(event.currentTarget.checked);
     },
     [setShouldCloseAlert]
   );
 
   const onBulkCloseAlertCheckboxChange = useCallback(
-    (event: React.ChangeEvent<HTMLInputElement>) => {
+    (event: React.ChangeEvent<HTMLInputElement>): void => {
       setShouldBulkCloseAlert(event.currentTarget.checked);
     },
     [setShouldBulkCloseAlert]
   );
 
-  const retrieveAlertOsTypes = useCallback(() => {
+  const retrieveAlertOsTypes = useCallback((): string[] => {
     const osDefaults = ['windows', 'macos'];
     if (alertData) {
       const osTypes = getMappedNonEcsValue({
@@ -276,7 +288,9 @@ export const AddExceptionModal = memo(function AddExceptionModal({
     return osDefaults;
   }, [alertData]);
 
-  const enrichExceptionItems = useCallback(() => {
+  const enrichExceptionItems = useCallback((): Array<
+    ExceptionListItemSchema | CreateExceptionListItemSchema
+  > => {
     let enriched: Array<ExceptionListItemSchema | CreateExceptionListItemSchema> = [];
     enriched =
       comment !== ''
@@ -289,7 +303,7 @@ export const AddExceptionModal = memo(function AddExceptionModal({
     return enriched;
   }, [comment, exceptionItemsToAdd, exceptionListType, retrieveAlertOsTypes]);
 
-  const onAddExceptionConfirm = useCallback(() => {
+  const onAddExceptionConfirm = useCallback((): void => {
     if (addOrUpdateExceptionItems !== null) {
       const alertIdToClose = shouldCloseAlert && alertData ? alertData.ecsData._id : undefined;
       const bulkCloseIndex =
@@ -306,7 +320,7 @@ export const AddExceptionModal = memo(function AddExceptionModal({
   ]);
 
   const isSubmitButtonDisabled = useMemo(
-    () =>
+    (): boolean =>
       fetchOrCreateListError != null ||
       exceptionItemsToAdd.every((item) => item.entries.length === 0),
     [fetchOrCreateListError, exceptionItemsToAdd]

diff --git a/.../plugins/security_solution/public/common/components/exceptions/use_add_exception.test.tsx b/.../plugins/security_solution/public/common/components/exceptions/use_add_exception.test.tsx
@@ -49,7 +49,7 @@ describe('useAddOrUpdateException', () => {
   const onError = jest.fn();
   const onSuccess = jest.fn();
   const alertIdToClose = 'idToClose';
-  const bulkCloseIndex = ['.signals'];
+  const bulkCloseIndex = ['.custom'];
   const itemsToAdd: CreateExceptionListItemSchema[] = [
     {
       ...getCreateExceptionListItemSchemaMock(),

diff --git a/x-pack/plugins/security_solution/public/common/components/exceptions/use_add_exception.tsx b/x-pack/plugins/security_solution/public/common/components/exceptions/use_add_exception.tsx
@@ -5,6 +5,7 @@
  */
 
 import { useEffect, useRef, useState, useCallback } from 'react';
+import { UpdateDocumentByQueryResponse } from 'elasticsearch';
 import { HttpStart } from '../../../../../../../src/core/public';
 
 import {
@@ -43,7 +44,7 @@ export type ReturnUseAddOrUpdateException = [
 export interface UseAddOrUpdateExceptionProps {
   http: HttpStart;
   onError: (arg: Error, code: number | null, message: string | null) => void;
-  onSuccess: () => void;
+  onSuccess: (updated: number, conficts: number) => void;
 }
 
 /**
@@ -122,8 +123,10 @@ export const useAddOrUpdateException = ({
     ) => {
       try {
         setIsLoading(true);
-        if (alertIdToClose !== null && alertIdToClose !== undefined) {
-          await updateAlertStatus({
+        let alertIdResponse: UpdateDocumentByQueryResponse | undefined;
+        let bulkResponse: UpdateDocumentByQueryResponse | undefined;
+        if (alertIdToClose != null) {
+          alertIdResponse = await updateAlertStatus({
             query: getUpdateAlertsQuery([alertIdToClose]),
             status: 'closed',
             signal: abortCtrl.signal,
@@ -139,7 +142,8 @@ export const useAddOrUpdateException = ({
             prepareExceptionItemsForBulkClose(exceptionItemsToAddOrUpdate),
             false
           );
-          await updateAlertStatus({
+
+          bulkResponse = await updateAlertStatus({
             query: {
               query: filter,
             },
@@ -150,9 +154,18 @@ export const useAddOrUpdateException = ({
 
         await addOrUpdateItems(exceptionItemsToAddOrUpdate);
 
+        // NOTE: there could be some overlap here... it's possible that the first response had conflicts
+        // but that the alert was closed in the second call. In this case, a conflict will be reported even
+        // though it was already resolved. I'm not sure that there's an easy way to solve this, but it should
+        // have minimal impact on the user... they'd see a warning that indicates a possible conflict, but the
+        // state of the alerts and their representation in the UI would be consistent.
+        const updated = (alertIdResponse?.updated ?? 0) + (bulkResponse?.updated ?? 0);
+        const conflicts =
+          alertIdResponse?.version_conflicts ?? 0 + (bulkResponse?.version_conflicts ?? 0);
+
         if (isSubscribed) {
           setIsLoading(false);
-          onSuccess();
+          onSuccess(updated, conflicts);
         }
       } catch (error) {
         if (isSubscribed) {

diff --git a/x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.test.ts b/x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.test.ts
@@ -14,13 +14,16 @@ jest.mock('../lib/kibana');
 describe('useDeleteList', () => {
   let addErrorMock: jest.Mock;
   let addSuccessMock: jest.Mock;
+  let addWarningMock: jest.Mock;
 
   beforeEach(() => {
     addErrorMock = jest.fn();
     addSuccessMock = jest.fn();
+    addWarningMock = jest.fn();
     (useToasts as jest.Mock).mockImplementation(() => ({
       addError: addErrorMock,
       addSuccess: addSuccessMock,
+      addWarning: addWarningMock,
     }));
   });
 

diff --git a/x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.ts b/x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.ts
@@ -10,7 +10,7 @@ import { ErrorToastOptions, ToastsStart, Toast } from '../../../../../../src/cor
 import { useToasts } from '../lib/kibana';
 import { isAppError, AppError } from '../utils/api';
 
-export type UseAppToasts = Pick<ToastsStart, 'addSuccess'> & {
+export type UseAppToasts = Pick<ToastsStart, 'addSuccess' | 'addWarning'> & {
   api: ToastsStart;
   addError: (error: unknown, options: ErrorToastOptions) => Toast;
 };
@@ -19,6 +19,7 @@ export const useAppToasts = (): UseAppToasts => {
   const toasts = useToasts();
   const addError = useRef(toasts.addError.bind(toasts)).current;
   const addSuccess = useRef(toasts.addSuccess.bind(toasts)).current;
+  const addWarning = useRef(toasts.addWarning.bind(toasts)).current;
 
   const addAppError = useCallback(
     (error: AppError, options: ErrorToastOptions) =>
@@ -44,5 +45,5 @@ export const useAppToasts = (): UseAppToasts => {
     [addAppError, addError]
   );
 
-  return { api: toasts, addError: _addError, addSuccess };
+  return { api: toasts, addError: _addError, addSuccess, addWarning };
 };
diff --git a/x-pack/plugins/security_solution/public/common/translations.ts b/x-pack/plugins/security_solution/public/common/translations.ts
@@ -61,3 +61,17 @@ export const EMPTY_ACTION_ENDPOINT_DESCRIPTION = i18n.translate(
       'Protect your hosts with threat prevention, detection, and deep security data visibility.',
   }
 );
+
+export const UPDATE_ALERT_STATUS_FAILED = (conflicts: number) =>
+  i18n.translate('xpack.securitySolution.pages.common.updateAlertStatusFailed', {
+    values: { conflicts },
+    defaultMessage:
+      'Failed to update { conflicts } {conflicts, plural, =1 {alert} other {alerts}}.',
+  });
+
+export const UPDATE_ALERT_STATUS_FAILED_DETAILED = (updated: number, conflicts: number) =>
+  i18n.translate('xpack.securitySolution.pages.common.updateAlertStatusFailedDetailed', {
+    values: { updated, conflicts },
+    defaultMessage: `{ updated } {updated, plural, =1 {alert was} other {alerts were}} updated successfully, but { conflicts } failed to update
+         because { conflicts, plural, =1 {it was} other {they were}} already being modified.`,
+  });
diff --git a/x-pack/plugins/security_solution/public/detections/components/alerts_table/actions.tsx b/x-pack/plugins/security_solution/public/detections/components/alerts_table/actions.tsx
@@ -9,6 +9,7 @@
 import dateMath from '@elastic/datemath';
 import { get, getOr, isEmpty, find } from 'lodash/fp';
 import moment from 'moment';
+import { i18n } from '@kbn/i18n';
 
 import { TimelineId } from '../../../../common/types/timeline';
 import { updateAlertStatus } from '../../containers/detection_engine/alerts/api';
@@ -83,7 +84,18 @@ export const updateAlertStatusAction = async ({
     // TODO: Only delete those that were successfully updated from updatedRules
     setEventsDeleted({ eventIds: alertIds, isDeleted: true });
 
-    onAlertStatusUpdateSuccess(response.updated, selectedStatus);
+    if (response.version_conflicts > 0 && alertIds.length === 1) {
+      throw new Error(
+        i18n.translate(
+          'xpack.securitySolution.detectionEngine.alerts.updateAlertStatusFailedSingleAlert',
+          {
+            defaultMessage: 'Failed to update alert because it was already being modified.',
+          }
+        )
+      );
+    }
+
+    onAlertStatusUpdateSuccess(response.updated, response.version_conflicts, selectedStatus);
   } catch (error) {
     onAlertStatusUpdateFailure(selectedStatus, error);
   } finally {