[Security Solutions] Critical bug fix to make error messages about mi…

…ssing connections clearer for the end user. (elastic#116490) ## Summary Fixes issue see on this comment: elastic#116336 (comment) * Removes legacy toaster component * Adds newer toaster component * Removes issue with the deps array within ReactJS * Adds utility to give a better network error message to the end user. * This does effect the timeline component since it shares the same import common component. * Adds a count of how many rules/timeline items have failed imports * These error toasters mimic Kibana core's error toaster error message and UI/UX * Adds e2e tests for imports with actions and error messages for them. ## Rules import error messages now Before for small toaster: <img width="417" alt="Screen Shot 2021-10-26 at 6 03 25 PM" src="https://user-images.githubusercontent.com/1151048/139132586-3cf77c73-53ac-4066-b01f-2e91ef2da111.png"> After for small toaster for different error conditions: <img width="358" alt="Screen Shot 2021-10-26 at 6 00 24 PM" src="https://user-images.githubusercontent.com/1151048/139132679-2eeb1ed3-9f6e-4766-a8ed-8804ce3e6963.png"> <img width="396" alt="Screen Shot 2021-10-26 at 6 01 00 PM" src="https://user-images.githubusercontent.com/1151048/139132742-750cd937-f401-44e8-9a10-c21410073b5d.png"> <img width="379" alt="Screen Shot 2021-10-26 at 6 02 29 PM" src="https://user-images.githubusercontent.com/1151048/139132766-21b58bea-7f46-43a6-a0e9-f01632958eab.png"> Before for when you click "See the full error": <img width="817" alt="Screen Shot 2021-10-26 at 5 58 47 PM" src="https://user-images.githubusercontent.com/1151048/139132980-de1942d6-7b03-4c08-b34a-1fc4a22d5207.png"> After for when you click "See the full error": <img width="838" alt="Screen Shot 2021-10-27 at 1 48 16 PM" src="https://user-images.githubusercontent.com/1151048/139136581-af1e331e-ed77-4338-8fb0-c2457acd135f.png"> <img width="802" alt="Screen Shot 2021-10-27 at 1 26 31 PM" src="https://user-images.githubusercontent.com/1151048/139135083-9ca56940-30a8-4f83-9355-312307172834.png"> ## timeline Before: <img width="441" alt="Screen Shot 2021-10-27 at 1 19 00 PM" src="https://user-images.githubusercontent.com/1151048/139136614-8360d6a6-d182-413e-b5d9-b18e3d70dc24.png"> <img width="827" alt="Screen Shot 2021-10-27 at 1 19 08 PM" src="https://user-images.githubusercontent.com/1151048/139136637-f9203ac2-0eea-4a77-9c53-ac2c20ab32e0.png"> After: <img width="408" alt="Screen Shot 2021-10-27 at 1 49 45 PM" src="https://user-images.githubusercontent.com/1151048/139136758-7532a8ba-6d73-45e2-adbb-6756ee997289.png"> <img width="820" alt="Screen Shot 2021-10-27 at 1 49 50 PM" src="https://user-images.githubusercontent.com/1151048/139136774-26d4a8a2-caf0-4c6f-94d3-a6cd92b79f5f.png"> ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
kibanamachine · Oct 29, 2021 · 1d17cb5 · 1d17cb5
1 parent 69cb2d3
commit 1d17cb5
Show file tree

Hide file tree

Showing 17 changed files with 866 additions and 87 deletions.
diff --git a/x-pack/plugins/security_solution/public/common/components/import_data_modal/index.test.tsx b/x-pack/plugins/security_solution/public/common/components/import_data_modal/index.test.tsx
@@ -19,7 +19,7 @@ describe('ImportDataModal', () => {
         importComplete={jest.fn()}
         checkBoxLabel="checkBoxLabel"
         description="description"
-        errorMessage="errorMessage"
+        errorMessage={jest.fn()}
         failedDetailed={jest.fn()}
         importData={jest.fn()}
         showCheckBox={true}

diff --git a/x-pack/plugins/security_solution/public/common/components/import_data_modal/index.tsx b/x-pack/plugins/security_solution/public/common/components/import_data_modal/index.tsx
@@ -23,23 +23,16 @@ import React, { useCallback, useState } from 'react';
 import {
   ImportDataResponse,
   ImportDataProps,
-  ImportRulesResponseError,
-  ImportResponseError,
 } from '../../../detections/containers/detection_engine/rules';
-import {
-  displayErrorToast,
-  displaySuccessToast,
-  useStateToaster,
-  errorToToaster,
-} from '../toasters';
+import { useAppToasts } from '../../hooks/use_app_toasts';
 import * as i18n from './translations';
 
 interface ImportDataModalProps {
   checkBoxLabel: string;
   closeModal: () => void;
   description: string;
-  errorMessage: string;
-  failedDetailed: (id: string, statusCode: number, message: string) => string;
+  errorMessage: (totalCount: number) => string;
+  failedDetailed: (message: string) => string;
   importComplete: () => void;
   importData: (arg: ImportDataProps) => Promise<ImportDataResponse>;
   showCheckBox: boolean;
@@ -50,12 +43,6 @@ interface ImportDataModalProps {
   title: string;
 }
 
-const isImportRulesResponseError = (
-  error: ImportRulesResponseError | ImportResponseError
-): error is ImportRulesResponseError => {
-  return (error as ImportRulesResponseError).rule_id !== undefined;
-};
-
 /**
  * Modal component for importing Rules from a json file
  */
@@ -77,7 +64,7 @@ export const ImportDataModalComponent = ({
   const [selectedFiles, setSelectedFiles] = useState<FileList | null>(null);
   const [isImporting, setIsImporting] = useState(false);
   const [overwrite, setOverwrite] = useState(false);
-  const [, dispatchToaster] = useStateToaster();
+  const { addError, addSuccess } = useAppToasts();
 
   const cleanupAndCloseModal = useCallback(() => {
     setIsImporting(false);
@@ -97,31 +84,39 @@ export const ImportDataModalComponent = ({
           signal: abortCtrl.signal,
         });
 
-        // TODO: Improve error toast details for better debugging failed imports
-        // e.g. When success == true && success_count === 0 that means no rules were overwritten, etc
         if (importResponse.success) {
-          displaySuccessToast(successMessage(importResponse.success_count), dispatchToaster);
+          addSuccess(successMessage(importResponse.success_count));
         }
         if (importResponse.errors.length > 0) {
-          const formattedErrors = importResponse.errors.map((e) =>
-            failedDetailed(
-              isImportRulesResponseError(e) ? e.rule_id : e.id,
-              e.error.status_code,
-              e.error.message
-            )
+          const formattedErrors = importResponse.errors.map((e) => failedDetailed(e.error.message));
+          const error: Error & { raw_network_error?: object } = new Error(
+            formattedErrors.join('. ')
           );
-          displayErrorToast(errorMessage, formattedErrors, dispatchToaster);
+          error.stack = undefined;
+          error.name = 'Network errors';
+          error.raw_network_error = importResponse;
+          addError(error, { title: errorMessage(importResponse.errors.length) });
         }
 
         importComplete();
         cleanupAndCloseModal();
       } catch (error) {
         cleanupAndCloseModal();
-        errorToToaster({ title: errorMessage, error, dispatchToaster });
+        addError(error, { title: errorMessage(1) });
       }
     }
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [selectedFiles, overwrite]);
+  }, [
+    selectedFiles,
+    overwrite,
+    addError,
+    addSuccess,
+    cleanupAndCloseModal,
+    errorMessage,
+    failedDetailed,
+    importComplete,
+    importData,
+    successMessage,
+  ]);
 
   const handleCloseModal = useCallback(() => {
     setSelectedFiles(null);

diff --git a/.../plugins/security_solution/public/detections/pages/detection_engine/rules/translations.ts b/.../plugins/security_solution/public/detections/pages/detection_engine/rules/translations.ts
@@ -579,19 +579,21 @@ export const SUCCESSFULLY_IMPORTED_RULES = (totalRules: number) =>
     }
   );
 
-export const IMPORT_FAILED = i18n.translate(
-  'xpack.securitySolution.detectionEngine.components.importRuleModal.importFailedTitle',
-  {
-    defaultMessage: 'Failed to import rules',
-  }
-);
+export const IMPORT_FAILED = (totalRules: number) =>
+  i18n.translate(
+    'xpack.securitySolution.detectionEngine.components.importRuleModal.importFailedTitle',
+    {
+      values: { totalRules },
+      defaultMessage: 'Failed to import {totalRules} {totalRules, plural, =1 {rule} other {rules}}',
+    }
+  );
 
-export const IMPORT_FAILED_DETAILED = (ruleId: string, statusCode: number, message: string) =>
+export const IMPORT_FAILED_DETAILED = (message: string) =>
   i18n.translate(
     'xpack.securitySolution.detectionEngine.components.importRuleModal.importFailedDetailedTitle',
     {
-      values: { ruleId, statusCode, message },
-      defaultMessage: 'Rule ID: {ruleId}\n Status Code: {statusCode}\n Message: {message}',
+      values: { message },
+      defaultMessage: '{message}',
     }
   );
 

diff --git a/x-pack/plugins/security_solution/public/timelines/components/open_timeline/translations.ts b/x-pack/plugins/security_solution/public/timelines/components/open_timeline/translations.ts
@@ -373,12 +373,15 @@ export const SUCCESSFULLY_IMPORTED_TIMELINES = (totalCount: number) =>
     }
   );
 
-export const IMPORT_FAILED = i18n.translate(
-  'xpack.securitySolution.timelines.components.importTimelineModal.importFailedTitle',
-  {
-    defaultMessage: 'Failed to import',
-  }
-);
+export const IMPORT_FAILED = (totalTimelines: number) =>
+  i18n.translate(
+    'xpack.securitySolution.timelines.components.importTimelineModal.importFailedTitle',
+    {
+      values: { totalTimelines },
+      defaultMessage:
+        'Failed to import {totalTimelines} {totalTimelines, plural, =1 {rule} other {rules}}',
+    }
+  );
 
 export const IMPORT_TIMELINE = i18n.translate(
   'xpack.securitySolution.timelines.components.importTimelineModal.importTitle',
@@ -387,11 +390,11 @@ export const IMPORT_TIMELINE = i18n.translate(
   }
 );
 
-export const IMPORT_FAILED_DETAILED = (id: string, statusCode: number, message: string) =>
+export const IMPORT_FAILED_DETAILED = (message: string) =>
   i18n.translate(
     'xpack.securitySolution.timelines.components.importTimelineModal.importFailedDetailedTitle',
     {
-      values: { id, statusCode, message },
-      defaultMessage: 'Timeline ID: {id}\n Status Code: {statusCode}\n Message: {message}',
+      values: { message },
+      defaultMessage: '{message}',
     }
   );
diff --git a/...plugins/security_solution/server/lib/detection_engine/routes/__mocks__/request_context.ts b/...plugins/security_solution/server/lib/detection_engine/routes/__mocks__/request_context.ts
@@ -11,6 +11,7 @@ import { coreMock } from 'src/core/server/mocks';
 import { ActionsApiRequestHandlerContext } from '../../../../../../actions/server';
 import { AlertingApiRequestHandlerContext } from '../../../../../../alerting/server';
 import { rulesClientMock } from '../../../../../../alerting/server/mocks';
+import { actionsClientMock } from '../../../../../../actions/server/mocks';
 import { licensingMock } from '../../../../../../licensing/server/mocks';
 import { listMock } from '../../../../../../lists/server/mocks';
 import { ruleRegistryMocks } from '../../../../../../rule_registry/server/mocks';
@@ -44,6 +45,7 @@ const createMockClients = () => {
       exceptionListClient: listMock.getExceptionListClient(core.savedObjects.client),
     },
     rulesClient: rulesClientMock.create(),
+    actionsClient: actionsClientMock.create(),
     ruleDataService: ruleRegistryMocks.createRuleDataService(),
 
     config: createMockConfig(),
@@ -65,7 +67,9 @@ const createRequestContextMock = (
   return {
     core: clients.core,
     securitySolution: createSecuritySolutionRequestContextMock(clients),
-    actions: {} as unknown as jest.Mocked<ActionsApiRequestHandlerContext>,
+    actions: {
+      getActionsClient: jest.fn(() => clients.actionsClient),
+    } as unknown as jest.Mocked<ActionsApiRequestHandlerContext>,
     alerting: {
       getRulesClient: jest.fn(() => clients.rulesClient),
     } as unknown as jest.Mocked<AlertingApiRequestHandlerContext>,

diff --git a/...ins/security_solution/server/lib/detection_engine/routes/rules/import_rules_route.test.ts b/...ins/security_solution/server/lib/detection_engine/routes/rules/import_rules_route.test.ts
@@ -53,6 +53,7 @@ describe.each([
     clients.rulesClient.update.mockResolvedValue(
       getAlertMock(isRuleRegistryEnabled, getQueryRuleParams())
     );
+    clients.actionsClient.getAll.mockResolvedValue([]);
     context.core.elasticsearch.client.asCurrentUser.search.mockResolvedValue(
       elasticsearchClientMock.createSuccessTransportRequestPromise(getBasicEmptySearchResponse())
     );
@@ -77,21 +78,6 @@ describe.each([
         status_code: 500,
       });
     });
-
-    test('returns 404 if alertClient is not available on the route', async () => {
-      context.alerting.getRulesClient = jest.fn();
-      const response = await server.inject(request, context);
-      expect(response.status).toEqual(404);
-      expect(response.body).toEqual({ message: 'Not Found', status_code: 404 });
-    });
-
-    it('returns 404 if siem client is unavailable', async () => {
-      const { securitySolution, ...contextWithoutSecuritySolution } = context;
-      // @ts-expect-error
-      const response = await server.inject(request, contextWithoutSecuritySolution);
-      expect(response.status).toEqual(404);
-      expect(response.body).toEqual({ message: 'Not Found', status_code: 404 });
-    });
   });
 
   describe('unhappy paths', () => {

diff --git a/.../plugins/security_solution/server/lib/detection_engine/routes/rules/import_rules_route.ts b/.../plugins/security_solution/server/lib/detection_engine/routes/rules/import_rules_route.ts
@@ -41,7 +41,7 @@ import {
 
 import { patchRules } from '../../rules/patch_rules';
 import { legacyMigrate } from '../../rules/utils';
-import { getTupleDuplicateErrorsAndUniqueRules } from './utils';
+import { getTupleDuplicateErrorsAndUniqueRules, getInvalidConnectors } from './utils';
 import { createRulesStreamFromNdJson } from '../../rules/create_rules_stream_from_ndjson';
 import { buildRouteValidation } from '../../../../utils/build_validation/route_validation';
 import { HapiReadableStream } from '../../rules/types';
@@ -78,14 +78,11 @@ export const importRulesRoute = (
       const siemResponse = buildSiemResponse(response);
 
       try {
-        const rulesClient = context.alerting?.getRulesClient();
+        const rulesClient = context.alerting.getRulesClient();
+        const actionsClient = context.actions.getActionsClient();
         const esClient = context.core.elasticsearch.client;
         const savedObjectsClient = context.core.savedObjects.client;
-        const siemClient = context.securitySolution?.getAppClient();
-
-        if (!siemClient || !rulesClient) {
-          return siemResponse.error({ statusCode: 404 });
-        }
+        const siemClient = context.securitySolution.getAppClient();
 
         const mlAuthz = buildMlAuthz({
           license: context.licensing.license,
@@ -103,6 +100,7 @@ export const importRulesRoute = (
             body: `Invalid file extension ${fileExtension}`,
           });
         }
+
         const signalsIndex = siemClient.getSignalsIndex();
         const indexExists = await getIndexExists(esClient.asCurrentUser, signalsIndex);
         if (!isRuleRegistryEnabled && !indexExists) {
@@ -118,14 +116,24 @@ export const importRulesRoute = (
           request.body.file as HapiReadableStream,
           ...readStream,
         ]);
-        const [duplicateIdErrors, uniqueParsedObjects] = getTupleDuplicateErrorsAndUniqueRules(
-          parsedObjects,
-          request.query.overwrite
+
+        const [duplicateIdErrors, parsedObjectsWithoutDuplicateErrors] =
+          getTupleDuplicateErrorsAndUniqueRules(parsedObjects, request.query.overwrite);
+
+        const [nonExistentActionErrors, uniqueParsedObjects] = await getInvalidConnectors(
+          parsedObjectsWithoutDuplicateErrors,
+          actionsClient
         );
 
         const chunkParseObjects = chunk(CHUNK_PARSED_OBJECT_SIZE, uniqueParsedObjects);
         let importRuleResponse: ImportRuleResponse[] = [];
 
+        // If we had 100% errors and no successful rule could be imported we still have to output an error.
+        // otherwise we would output we are success importing 0 rules.
+        if (chunkParseObjects.length === 0) {
+          importRuleResponse = [...nonExistentActionErrors, ...duplicateIdErrors];
+        }
+
         while (chunkParseObjects.length) {
           const batchParseObjects = chunkParseObjects.shift() ?? [];
           const newImportRuleResponse = await Promise.all(
@@ -362,6 +370,7 @@ export const importRulesRoute = (
             }, [])
           );
           importRuleResponse = [
+            ...nonExistentActionErrors,
             ...duplicateIdErrors,
             ...importRuleResponse,
             ...newImportRuleResponse,

diff --git a/...ins/security_solution/server/lib/detection_engine/routes/rules/update_rules_bulk_route.ts b/...ins/security_solution/server/lib/detection_engine/routes/rules/update_rules_bulk_route.ts
@@ -88,7 +88,6 @@ export const updateRulesBulkRoute = (
               spaceId: context.securitySolution.getSpaceId(),
               rulesClient,
               ruleStatusClient,
-              savedObjectsClient,
               defaultOutputIndex: siemClient.getSignalsIndex(),
               ruleUpdate: payloadRule,
               isRuleRegistryEnabled,

diff --git a/.../plugins/security_solution/server/lib/detection_engine/routes/rules/update_rules_route.ts b/.../plugins/security_solution/server/lib/detection_engine/routes/rules/update_rules_route.ts
@@ -79,7 +79,6 @@ export const updateRulesRoute = (
           isRuleRegistryEnabled,
           rulesClient,
           ruleStatusClient,
-          savedObjectsClient,
           ruleUpdate: request.body,
           spaceId: context.securitySolution.getSpaceId(),
         });