From 63b2ddf68705faee8b526a450b8e0e3831dcf43a Mon Sep 17 00:00:00 2001 From: Yara Tercero Date: Tue, 23 Feb 2021 14:55:42 -0800 Subject: [PATCH] [Security Solution][Exceptions] - Update exceptions modal to use existing lists plugin useApi hook (#92348) Doing a quick refactor to help with #90634 . While working on #90634 found that I would introduce a circular dependency if I didn't refactor the hook used by the exception builder component to make use of the existing useApi hook in the lists plugin. #90634 adds temporary ids to item entries to mitigate some React key requirements and the logic to remove/add these id's isn't at the boundary but in the hooks (so as not to pollute the data for everyone wanting to use the api. An upside is that it removed some of the looping and seemed to speed things up a bit. I briefly considered adding the bulk SO endpoints for exception items, but they are still experimental. --- .../public/exceptions/hooks/use_api.test.ts | 63 +++++++++++++++- .../lists/public/exceptions/hooks/use_api.ts | 39 +++++++++- .../exceptions/use_add_exception.tsx | 74 ++++++++----------- 3 files changed, 129 insertions(+), 47 deletions(-) diff --git a/x-pack/plugins/lists/public/exceptions/hooks/use_api.test.ts b/x-pack/plugins/lists/public/exceptions/hooks/use_api.test.ts index 0fec2ab23994b..e61e74ca33236 100644 --- a/x-pack/plugins/lists/public/exceptions/hooks/use_api.test.ts +++ b/x-pack/plugins/lists/public/exceptions/hooks/use_api.test.ts @@ -7,13 +7,20 @@ import { act, renderHook } from '@testing-library/react-hooks'; +import { getUpdateExceptionListItemSchemaMock } from '../../../common/schemas/request/update_exception_list_item_schema.mock'; import { coreMock } from '../../../../../../src/core/public/mocks'; import * as api from '../api'; import { getExceptionListSchemaMock } from '../../../common/schemas/response/exception_list_schema.mock'; import { getFoundExceptionListItemSchemaMock } from '../../../common/schemas/response/found_exception_list_item_schema.mock'; import { getExceptionListItemSchemaMock } from '../../../common/schemas/response/exception_list_item_schema.mock'; +import { getCreateExceptionListItemSchemaMock } from '../../../common/schemas/request/create_exception_list_item_schema.mock'; import { HttpStart } from '../../../../../../src/core/public'; -import { ApiCallByIdProps, ApiCallByListIdProps } from '../types'; +import { + AddExceptionListItemProps, + ApiCallByIdProps, + ApiCallByListIdProps, + UpdateExceptionListItemProps, +} from '../types'; import { ExceptionsApi, useApi } from './use_api'; @@ -366,4 +373,58 @@ describe('useApi', () => { expect(onErrorMock).toHaveBeenCalledWith(mockError); }); }); + + test('it invokes "addExceptionListItem" when "addExceptionListItem" used', async () => { + const payload = getExceptionListItemSchemaMock(); + const itemToCreate = getCreateExceptionListItemSchemaMock(); + const spyOnFetchExceptionListItemById = jest + .spyOn(api, 'addExceptionListItem') + .mockResolvedValue(payload); + + await act(async () => { + const { result, waitForNextUpdate } = renderHook(() => + useApi(mockKibanaHttpService) + ); + await waitForNextUpdate(); + + await result.current.addExceptionListItem({ + listItem: itemToCreate, + }); + + const expected: AddExceptionListItemProps = { + http: mockKibanaHttpService, + listItem: itemToCreate, + signal: new AbortController().signal, + }; + + expect(spyOnFetchExceptionListItemById).toHaveBeenCalledWith(expected); + }); + }); + + test('it invokes "updateExceptionListItem" when "getExceptionItem" used', async () => { + const payload = getExceptionListItemSchemaMock(); + const itemToUpdate = getUpdateExceptionListItemSchemaMock(); + const spyOnUpdateExceptionListItem = jest + .spyOn(api, 'updateExceptionListItem') + .mockResolvedValue(payload); + + await act(async () => { + const { result, waitForNextUpdate } = renderHook(() => + useApi(mockKibanaHttpService) + ); + await waitForNextUpdate(); + + await result.current.updateExceptionListItem({ + listItem: itemToUpdate, + }); + + const expected: UpdateExceptionListItemProps = { + http: mockKibanaHttpService, + listItem: itemToUpdate, + signal: new AbortController().signal, + }; + + expect(spyOnUpdateExceptionListItem).toHaveBeenCalledWith(expected); + }); + }); }); diff --git a/x-pack/plugins/lists/public/exceptions/hooks/use_api.ts b/x-pack/plugins/lists/public/exceptions/hooks/use_api.ts index 91050c5fff795..b0c831ef3b857 100644 --- a/x-pack/plugins/lists/public/exceptions/hooks/use_api.ts +++ b/x-pack/plugins/lists/public/exceptions/hooks/use_api.ts @@ -9,11 +9,22 @@ import { useMemo } from 'react'; import * as Api from '../api'; import { HttpStart } from '../../../../../../src/core/public'; -import { ExceptionListItemSchema, ExceptionListSchema } from '../../../common/schemas'; +import { + CreateExceptionListItemSchema, + ExceptionListItemSchema, + ExceptionListSchema, + UpdateExceptionListItemSchema, +} from '../../../common/schemas'; import { ApiCallFindListsItemsMemoProps, ApiCallMemoProps, ApiListExportProps } from '../types'; import { getIdsAndNamespaces } from '../utils'; export interface ExceptionsApi { + addExceptionListItem: (arg: { + listItem: CreateExceptionListItemSchema; + }) => Promise; + updateExceptionListItem: (arg: { + listItem: UpdateExceptionListItemSchema; + }) => Promise; deleteExceptionItem: (arg: ApiCallMemoProps) => Promise; deleteExceptionList: (arg: ApiCallMemoProps) => Promise; getExceptionItem: ( @@ -29,6 +40,19 @@ export interface ExceptionsApi { export const useApi = (http: HttpStart): ExceptionsApi => { return useMemo( (): ExceptionsApi => ({ + async addExceptionListItem({ + listItem, + }: { + listItem: CreateExceptionListItemSchema; + }): Promise { + const abortCtrl = new AbortController(); + + return Api.addExceptionListItem({ + http, + listItem, + signal: abortCtrl.signal, + }); + }, async deleteExceptionItem({ id, namespaceType, @@ -184,6 +208,19 @@ export const useApi = (http: HttpStart): ExceptionsApi => { onError(error); } }, + async updateExceptionListItem({ + listItem, + }: { + listItem: UpdateExceptionListItemSchema; + }): Promise { + const abortCtrl = new AbortController(); + + return Api.updateExceptionListItem({ + http, + listItem, + signal: abortCtrl.signal, + }); + }, }), [http] ); 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 index a8e6c72e3e165..614f5301c82e2 100644 --- 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 @@ -10,11 +10,9 @@ import { UpdateDocumentByQueryResponse } from 'elasticsearch'; import { HttpStart } from '../../../../../../../src/core/public'; import { - addExceptionListItem, - updateExceptionListItem, ExceptionListItemSchema, CreateExceptionListItemSchema, - UpdateExceptionListItemSchema, + useApi, } from '../../../lists_plugin_deps'; import { updateAlertStatus } from '../../../detections/containers/detection_engine/alerts/api'; import { getUpdateAlertsQuery } from '../../../detections/components/alerts_table/actions'; @@ -25,6 +23,7 @@ import { import { getQueryFilter } from '../../../../common/detection_engine/get_query_filter'; import { Index } from '../../../../common/detection_engine/schemas/common/schemas'; import { formatExceptionItemForUpdate, prepareExceptionItemsForBulkClose } from './helpers'; +import { useKibana } from '../../lib/kibana'; /** * Adds exception items to the list. Also optionally closes alerts. @@ -66,11 +65,13 @@ export const useAddOrUpdateException = ({ onError, onSuccess, }: UseAddOrUpdateExceptionProps): ReturnUseAddOrUpdateException => { + const { services } = useKibana(); const [isLoading, setIsLoading] = useState(false); const addOrUpdateExceptionRef = useRef(null); + const { addExceptionListItem, updateExceptionListItem } = useApi(services.http); const addOrUpdateException = useCallback( async (ruleId, exceptionItemsToAddOrUpdate, alertIdToClose, bulkCloseIndex) => { - if (addOrUpdateExceptionRef.current !== null) { + if (addOrUpdateExceptionRef.current != null) { addOrUpdateExceptionRef.current( ruleId, exceptionItemsToAddOrUpdate, @@ -86,49 +87,33 @@ export const useAddOrUpdateException = ({ let isSubscribed = true; const abortCtrl = new AbortController(); - const addOrUpdateItems = async ( - exceptionItemsToAddOrUpdate: Array - ): Promise => { - const toAdd: CreateExceptionListItemSchema[] = []; - const toUpdate: UpdateExceptionListItemSchema[] = []; - exceptionItemsToAddOrUpdate.forEach( - (item: ExceptionListItemSchema | CreateExceptionListItemSchema) => { - if ('id' in item && item.id !== undefined) { - toUpdate.push(formatExceptionItemForUpdate(item)); - } else { - toAdd.push(item); - } - } - ); - - const promises: Array> = []; - toAdd.forEach((item: CreateExceptionListItemSchema) => { - promises.push( - addExceptionListItem({ - http, - listItem: item, - signal: abortCtrl.signal, - }) - ); - }); - toUpdate.forEach((item: UpdateExceptionListItemSchema) => { - promises.push( - updateExceptionListItem({ - http, - listItem: item, - signal: abortCtrl.signal, - }) - ); - }); - await Promise.all(promises); - }; - - const addOrUpdateExceptionItems: AddOrUpdateExceptionItemsFunc = async ( + const onUpdateExceptionItemsAndAlertStatus: AddOrUpdateExceptionItemsFunc = async ( ruleId, exceptionItemsToAddOrUpdate, alertIdToClose, bulkCloseIndex ) => { + const addOrUpdateItems = async ( + exceptionListItems: Array + ): Promise => { + await Promise.all( + exceptionListItems.map( + (item: ExceptionListItemSchema | CreateExceptionListItemSchema) => { + if ('id' in item && item.id != null) { + const formattedExceptionItem = formatExceptionItemForUpdate(item); + return updateExceptionListItem({ + listItem: formattedExceptionItem, + }); + } else { + return addExceptionListItem({ + listItem: item, + }); + } + } + ) + ); + }; + try { setIsLoading(true); let alertIdResponse: UpdateDocumentByQueryResponse | undefined; @@ -170,7 +155,6 @@ export const useAddOrUpdateException = ({ const updated = (alertIdResponse?.updated ?? 0) + (bulkResponse?.updated ?? 0); const conflicts = alertIdResponse?.version_conflicts ?? 0 + (bulkResponse?.version_conflicts ?? 0); - if (isSubscribed) { setIsLoading(false); onSuccess(updated, conflicts); @@ -187,12 +171,12 @@ export const useAddOrUpdateException = ({ } }; - addOrUpdateExceptionRef.current = addOrUpdateExceptionItems; + addOrUpdateExceptionRef.current = onUpdateExceptionItemsAndAlertStatus; return (): void => { isSubscribed = false; abortCtrl.abort(); }; - }, [http, onSuccess, onError]); + }, [http, onSuccess, onError, updateExceptionListItem, addExceptionListItem]); return [{ isLoading }, addOrUpdateException]; };