[Security Solution][Exceptions] - Fixes exceptions builder UI where i…

…nvalid values can cause overwrites of other values (#90634) ### Summary This PR is a follow-up to #89066 - which fixed the same issue occurring with indicator match lists UI. The lack of stable ids for exception item entries resulted in some funky business by where invalid values could overwrite other values when deleting entries in the builder.
elastic · Feb 25, 2021 · 99b3ef1 · 99b3ef1
1 parent ecdef59
commit 99b3ef1
Show file tree

Hide file tree

Showing 34 changed files with 1,650 additions and 542 deletions.
diff --git a/x-pack/plugins/lists/common/constants.mock.ts b/x-pack/plugins/lists/common/constants.mock.ts
@@ -8,7 +8,7 @@
 import moment from 'moment';
 
 import { OsTypeArray } from './schemas/common';
-import { EntriesArray } from './schemas/types';
+import { EntriesArray, Entry, EntryMatch, EntryNested } from './schemas/types';
 import { EndpointEntriesArray } from './schemas/types/endpoint';
 export const DATE_NOW = '2020-04-20T15:25:31.830Z';
 export const OLD_DATE_RELATIVE_TO_DATE_NOW = '2020-04-19T15:25:31.830Z';
@@ -72,6 +72,34 @@ export const ENDPOINT_ENTRIES: EndpointEntriesArray = [
   },
   { field: 'some.not.nested.field', operator: 'included', type: 'match', value: 'some value' },
 ];
+// ENTRIES_WITH_IDS should only be used to mock out functionality of a collection of transforms
+// that are UI specific and useful for UI concerns that are inserted between the
+// API and the actual user interface. In some ways these might be viewed as
+// technical debt or to compensate for the differences and preferences
+// of how ReactJS might prefer data vs. how we want to model data.
+export const ENTRIES_WITH_IDS: EntriesArray = [
+  {
+    entries: [
+      {
+        field: 'nested.field',
+        id: '123',
+        operator: 'included',
+        type: 'match',
+        value: 'some value',
+      } as EntryMatch & { id: string },
+    ],
+    field: 'some.parentField',
+    id: '123',
+    type: 'nested',
+  } as EntryNested & { id: string },
+  {
+    field: 'some.not.nested.field',
+    id: '123',
+    operator: 'included',
+    type: 'match',
+    value: 'some value',
+  } as Entry & { id: string },
+];
 export const ITEM_TYPE = 'simple';
 export const OS_TYPES: OsTypeArray = ['windows'];
 export const TAGS = [];

diff --git a/x-pack/plugins/lists/common/shared_imports.ts b/x-pack/plugins/lists/common/shared_imports.ts
@@ -12,6 +12,8 @@ export {
   DefaultStringArray,
   DefaultVersionNumber,
   DefaultVersionNumberDecoded,
+  addIdToItem,
+  removeIdFromItem,
   exactCheck,
   getPaths,
   foldLeftRight,

diff --git a/x-pack/plugins/lists/public/exceptions/hooks/persist_exception_item.test.ts b/x-pack/plugins/lists/public/exceptions/hooks/persist_exception_item.test.ts
@@ -7,6 +7,7 @@
 
 import { act, renderHook } from '@testing-library/react-hooks';
 
+import { ENTRIES_WITH_IDS } from '../../../common/constants.mock';
 import { coreMock } from '../../../../../../src/core/public/mocks';
 import * as api from '../api';
 import { getCreateExceptionListItemSchemaMock } from '../../../common/schemas/request/create_exception_list_item_schema.mock';
@@ -69,21 +70,54 @@ describe('usePersistExceptionItem', () => {
   });
 
   test('it invokes "updateExceptionListItem" when payload has "id"', async () => {
-    const addExceptionItem = jest.spyOn(api, 'addExceptionListItem');
-    const updateExceptionItem = jest.spyOn(api, 'updateExceptionListItem');
+    const addExceptionListItem = jest.spyOn(api, 'addExceptionListItem');
+    const updateExceptionListItem = jest.spyOn(api, 'updateExceptionListItem');
     await act(async () => {
       const { result, waitForNextUpdate } = renderHook<
         PersistHookProps,
         ReturnPersistExceptionItem
       >(() => usePersistExceptionItem({ http: mockKibanaHttpService, onError }));
 
       await waitForNextUpdate();
-      result.current[1](getUpdateExceptionListItemSchemaMock());
+      // NOTE: Take note here passing in an exception item where it's
+      // entries have been enriched with ids to ensure that they get stripped
+      // before the call goes through
+      result.current[1]({ ...getUpdateExceptionListItemSchemaMock(), entries: ENTRIES_WITH_IDS });
       await waitForNextUpdate();
 
       expect(result.current).toEqual([{ isLoading: false, isSaved: true }, result.current[1]]);
-      expect(addExceptionItem).not.toHaveBeenCalled();
-      expect(updateExceptionItem).toHaveBeenCalled();
+      expect(addExceptionListItem).not.toHaveBeenCalled();
+      expect(updateExceptionListItem).toHaveBeenCalledWith({
+        http: mockKibanaHttpService,
+        listItem: getUpdateExceptionListItemSchemaMock(),
+        signal: new AbortController().signal,
+      });
+    });
+  });
+
+  test('it invokes "addExceptionListItem" when payload does not have "id"', async () => {
+    const updateExceptionListItem = jest.spyOn(api, 'updateExceptionListItem');
+    const addExceptionListItem = jest.spyOn(api, 'addExceptionListItem');
+    await act(async () => {
+      const { result, waitForNextUpdate } = renderHook<
+        PersistHookProps,
+        ReturnPersistExceptionItem
+      >(() => usePersistExceptionItem({ http: mockKibanaHttpService, onError }));
+
+      await waitForNextUpdate();
+      // NOTE: Take note here passing in an exception item where it's
+      // entries have been enriched with ids to ensure that they get stripped
+      // before the call goes through
+      result.current[1]({ ...getCreateExceptionListItemSchemaMock(), entries: ENTRIES_WITH_IDS });
+      await waitForNextUpdate();
+
+      expect(result.current).toEqual([{ isLoading: false, isSaved: true }, result.current[1]]);
+      expect(updateExceptionListItem).not.toHaveBeenCalled();
+      expect(addExceptionListItem).toHaveBeenCalledWith({
+        http: mockKibanaHttpService,
+        listItem: getCreateExceptionListItemSchemaMock(),
+        signal: new AbortController().signal,
+      });
     });
   });
 

diff --git a/x-pack/plugins/lists/public/exceptions/hooks/persist_exception_item.ts b/x-pack/plugins/lists/public/exceptions/hooks/persist_exception_item.ts
@@ -7,9 +7,13 @@
 
 import { Dispatch, useEffect, useState } from 'react';
 
-import { UpdateExceptionListItemSchema } from '../../../common/schemas';
+import {
+  CreateExceptionListItemSchema,
+  UpdateExceptionListItemSchema,
+} from '../../../common/schemas';
 import { addExceptionListItem, updateExceptionListItem } from '../api';
-import { AddExceptionListItem, PersistHookProps } from '../types';
+import { transformNewItemOutput, transformOutput } from '../transforms';
+import { PersistHookProps } from '../types';
 
 interface PersistReturnExceptionItem {
   isLoading: boolean;
@@ -18,7 +22,7 @@ interface PersistReturnExceptionItem {
 
 export type ReturnPersistExceptionItem = [
   PersistReturnExceptionItem,
-  Dispatch<AddExceptionListItem | null>
+  Dispatch<CreateExceptionListItemSchema | UpdateExceptionListItemSchema | null>
 ];
 
 /**
@@ -32,7 +36,9 @@ export const usePersistExceptionItem = ({
   http,
   onError,
 }: PersistHookProps): ReturnPersistExceptionItem => {
-  const [exceptionListItem, setExceptionItem] = useState<AddExceptionListItem | null>(null);
+  const [exceptionListItem, setExceptionItem] = useState<
+    CreateExceptionListItemSchema | UpdateExceptionListItemSchema | null
+  >(null);
   const [isSaved, setIsSaved] = useState(false);
   const [isLoading, setIsLoading] = useState(false);
   const isUpdateExceptionItem = (item: unknown): item is UpdateExceptionListItemSchema =>
@@ -47,16 +53,25 @@ export const usePersistExceptionItem = ({
       if (exceptionListItem != null) {
         try {
           setIsLoading(true);
+
           if (isUpdateExceptionItem(exceptionListItem)) {
+            // Please see `x-pack/plugins/lists/public/exceptions/transforms.ts` doc notes
+            // for context around the temporary `id`
+            const transformedList = transformOutput(exceptionListItem);
+
             await updateExceptionListItem({
               http,
-              listItem: exceptionListItem,
+              listItem: transformedList,
               signal: abortCtrl.signal,
             });
           } else {
+            // Please see `x-pack/plugins/lists/public/exceptions/transforms.ts` doc notes
+            // for context around the temporary `id`
+            const transformedList = transformNewItemOutput(exceptionListItem);
+
             await addExceptionListItem({
               http,
-              listItem: exceptionListItem,
+              listItem: transformedList,
               signal: abortCtrl.signal,
             });
           }