Skip to content

Commit

Permalink
[Security Solution] [Timeline] Delete saved searches on timeline dele…
Browse files Browse the repository at this point in the history
…te, prevent creating double saved searches (#174562)

## Summary

This pr fixes 2 issues currently present in the timeline es|ql via
discover integration: first, when timelines are deleted, the saved
object ids associated with those timelines that point to saved searches
are currently not deleted. If a user deletes a timeline and then tries
to create another with the same name, the esql is not able to be
persisted because no new saved search saved object can be created, as
the name of the old one collides with the new one. This pr fixes that by
deleting the saved search saved object on delete, although the issue
with timeline allowing multiple timelines with the same name while saved
search does not will remain an issue, need to use the onDuplicateTitle
callback that is currently a no-op to handle this here
https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/public/common/components/discover_in_timeline/use_discover_in_timeline_actions.tsx#L213
. Second, the use of react-query useMutation that creates the saved
search saved object will no longer create two, as it currently does, as
we check the status of the request in the useEffect that fires the
request. This might be a race condition bug with the saved search saved
objects client too, as normally objects are not supposed to be created
with the same name,
https://github.com/elastic/kibana/blob/main/src/plugins/saved_search/public/services/saved_searches/save_saved_searches.ts#L68,
but I'm not sure.

### Checklist

- [x]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [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
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
  • Loading branch information
kqualters-elastic authored Jan 12, 2024
1 parent ad5c8ce commit 7b69611
Show file tree
Hide file tree
Showing 14 changed files with 149 additions and 45 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@

import * as rt from 'io-ts';

export const deleteTimelinesSchema = rt.type({
const searchId = rt.partial({ searchIds: rt.array(rt.string) });

const baseDeleteTimelinesSchema = rt.type({
savedObjectIds: rt.array(rt.string),
});

export const deleteTimelinesSchema = rt.intersection([baseDeleteTimelinesSchema, searchId]);
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ paths:
type: array
items:
type: string
searchId:
type: array
items:
type: string
responses:
200:
description: Indicates the timeline was successfully deleted.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,15 @@ export const useDiscoverInTimelineActions = (
const timeline = useShallowEqualSelector(
(state) => getTimeline(state, TimelineId.active) ?? timelineDefaults
);
const { savedSearchId } = timeline;
const { savedSearchId, version } = timeline;

// We're using a ref here to prevent a cyclic hook-dependency chain of updateSavedSearch
const timelineRef = useRef(timeline);
timelineRef.current = timeline;

const queryClient = useQueryClient();

const { mutateAsync: saveSavedSearch } = useMutation({
const { mutateAsync: saveSavedSearch, status } = useMutation({
mutationFn: ({
savedSearch,
savedSearchOptions,
Expand All @@ -75,6 +75,7 @@ export const useDiscoverInTimelineActions = (
}
queryClient.invalidateQueries({ queryKey: ['savedSearchById', savedSearchId] });
},
mutationKey: [version],
});

const getDefaultDiscoverAppState: () => Promise<DiscoverAppState> = useCallback(async () => {
Expand Down Expand Up @@ -217,7 +218,7 @@ export const useDiscoverInTimelineActions = (
const responseIsEmpty = !response || !response?.id;
if (responseIsEmpty) {
throw new Error('Response is empty');
} else if (!savedSearchId && !responseIsEmpty) {
} else if (!savedSearchId && !responseIsEmpty && status !== 'loading') {
dispatch(
timelineActions.updateSavedSearchId({
id: TimelineId.active,
Expand All @@ -236,7 +237,7 @@ export const useDiscoverInTimelineActions = (
}
}
},
[persistSavedSearch, savedSearchId, dispatch, discoverDataService]
[persistSavedSearch, savedSearchId, dispatch, discoverDataService, status]
);

const initializeLocalSavedSearch = useCallback(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,14 @@ interface Props {
onComplete?: () => void;
isModalOpen: boolean;
savedObjectIds: string[];
savedSearchIds?: string[];
title: string | null;
}
/**
* Renders a button that when clicked, displays the `Delete Timeline` modal
*/
export const DeleteTimelineModalOverlay = React.memo<Props>(
({ deleteTimelines, isModalOpen, savedObjectIds, title, onComplete }) => {
({ deleteTimelines, isModalOpen, savedObjectIds, title, onComplete, savedSearchIds }) => {
const { addSuccess } = useAppToasts();
const { tabName: timelineType } = useParams<{ tabName: TimelineType }>();

Expand All @@ -43,20 +44,28 @@ export const DeleteTimelineModalOverlay = React.memo<Props>(
}
}, [onComplete]);
const onDelete = useCallback(() => {
if (savedObjectIds.length > 0) {
if (savedObjectIds.length > 0 && savedSearchIds != null && savedSearchIds.length > 0) {
deleteTimelines(savedObjectIds, savedSearchIds);
addSuccess({
title:
timelineType === TimelineType.template
? i18n.SUCCESSFULLY_DELETED_TIMELINE_TEMPLATES(savedObjectIds.length)
: i18n.SUCCESSFULLY_DELETED_TIMELINES(savedObjectIds.length),
});
} else if (savedObjectIds.length > 0) {
deleteTimelines(savedObjectIds);

addSuccess({
title:
timelineType === TimelineType.template
? i18n.SUCCESSFULLY_DELETED_TIMELINE_TEMPLATES(savedObjectIds.length)
: i18n.SUCCESSFULLY_DELETED_TIMELINES(savedObjectIds.length),
});
}

if (onComplete != null) {
onComplete();
}
}, [deleteTimelines, savedObjectIds, onComplete, addSuccess, timelineType]);
}, [deleteTimelines, savedObjectIds, onComplete, addSuccess, timelineType, savedSearchIds]);
return (
<>
{isModalOpen && <RemovePopover data-test-subj="remove-popover" />}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,7 @@ import * as i18n from './translations';
import type { DeleteTimelines, OpenTimelineResult } from './types';
import { EditTimelineActions } from './export_timeline';
import { useEditTimelineActions } from './edit_timeline_actions';

const getExportedIds = (selectedTimelines: OpenTimelineResult[]) => {
const array = Array.isArray(selectedTimelines) ? selectedTimelines : [selectedTimelines];
return array.reduce(
(acc, item) => (item.savedObjectId != null ? [...acc, item.savedObjectId] : [...acc]),
[] as string[]
);
};
import { getSelectedTimelineIdsAndSearchIds, getRequestIds } from '.';

export const useEditTimelineBatchActions = ({
deleteTimelines,
Expand Down Expand Up @@ -56,7 +49,13 @@ export const useEditTimelineBatchActions = ({
[disableExportTimelineDownloader, onCloseDeleteTimelineModal, tableRef]
);

const selectedIds = useMemo(() => getExportedIds(selectedItems ?? []), [selectedItems]);
const { timelineIds, searchIds } = useMemo(() => {
if (selectedItems != null) {
return getRequestIds(getSelectedTimelineIdsAndSearchIds(selectedItems));
} else {
return { timelineIds: [], searchIds: undefined };
}
}, [selectedItems]);

const handleEnableExportTimelineDownloader = useCallback(
() => enableExportTimelineDownloader(),
Expand Down Expand Up @@ -102,7 +101,8 @@ export const useEditTimelineBatchActions = ({
<>
<EditTimelineActions
deleteTimelines={deleteTimelines}
ids={selectedIds}
ids={timelineIds}
savedSearchIds={searchIds}
isEnableDownloader={isEnableDownloader}
isDeleteTimelineModalOpen={isDeleteTimelineModalOpen}
onComplete={onCompleteBatchActions.bind(null, closePopover)}
Expand All @@ -121,7 +121,8 @@ export const useEditTimelineBatchActions = ({
[
selectedItems,
deleteTimelines,
selectedIds,
timelineIds,
searchIds,
isEnableDownloader,
isDeleteTimelineModalOpen,
onCompleteBatchActions,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,15 @@ export interface ExportTimeline {
export const EditTimelineActionsComponent: React.FC<{
deleteTimelines: DeleteTimelines | undefined;
ids: string[];
savedSearchIds?: string[];
isEnableDownloader: boolean;
isDeleteTimelineModalOpen: boolean;
onComplete: () => void;
title: string;
}> = ({
deleteTimelines,
ids,
savedSearchIds,
isEnableDownloader,
isDeleteTimelineModalOpen,
onComplete,
Expand All @@ -46,6 +48,7 @@ export const EditTimelineActionsComponent: React.FC<{
isModalOpen={isDeleteTimelineModalOpen}
onComplete={onComplete}
savedObjectIds={ids}
savedSearchIds={savedSearchIds}
title={title}
/>
)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,14 +74,51 @@ export type OpenTimelineOwnProps = OwnProps &
>;

/** Returns a collection of selected timeline ids */
export const getSelectedTimelineIds = (selectedItems: OpenTimelineResult[]): string[] =>
selectedItems.reduce<string[]>(
(validSelections, timelineResult) =>
timelineResult.savedObjectId != null
? [...validSelections, timelineResult.savedObjectId]
: validSelections,
[]
export const getSelectedTimelineIdsAndSearchIds = (
selectedItems: OpenTimelineResult[]
): Array<{ timelineId: string; searchId?: string | null }> => {
return selectedItems.reduce<Array<{ timelineId: string; searchId?: string | null }>>(
(validSelections, timelineResult) => {
if (timelineResult.savedObjectId != null && timelineResult.savedSearchId != null) {
return [
...validSelections,
{ timelineId: timelineResult.savedObjectId, searchId: timelineResult.savedSearchId },
];
} else if (timelineResult.savedObjectId != null) {
return [...validSelections, { timelineId: timelineResult.savedObjectId }];
} else {
return validSelections;
}
},
[] as Array<{ timelineId: string; searchId?: string | null }>
);
};

interface DeleteTimelinesValues {
timelineIds: string[];
searchIds: string[];
}

export const getRequestIds = (
timelineIdsWithSearch: Array<{ timelineId: string; searchId?: string | null }>
) => {
return timelineIdsWithSearch.reduce<DeleteTimelinesValues>(
(acc, { timelineId, searchId }) => {
let requestValues = acc;
if (searchId != null) {
requestValues = { ...requestValues, searchIds: [...requestValues.searchIds, searchId] };
}
if (timelineId != null) {
requestValues = {
...requestValues,
timelineIds: [...requestValues.timelineIds, timelineId],
};
}
return requestValues;
},
{ timelineIds: [], searchIds: [] }
);
};

/** Manages the state (e.g table selection) of the (pure) `OpenTimeline` component */
// eslint-disable-next-line react/display-name
Expand Down Expand Up @@ -208,7 +245,7 @@ export const StatefulOpenTimelineComponent = React.memo<OpenTimelineOwnProps>(
// };

const deleteTimelines: DeleteTimelines = useCallback(
async (timelineIds: string[]) => {
async (timelineIds: string[], searchIds?: string[]) => {
startTransaction({
name: timelineIds.length > 1 ? TIMELINE_ACTIONS.BULK_DELETE : TIMELINE_ACTIONS.DELETE,
});
Expand All @@ -225,24 +262,26 @@ export const StatefulOpenTimelineComponent = React.memo<OpenTimelineOwnProps>(
);
}

await deleteTimelinesByIds(timelineIds);
await deleteTimelinesByIds(timelineIds, searchIds);
refetch();
},
[startTransaction, timelineSavedObjectId, refetch, dispatch, dataViewId, selectedPatterns]
);

const onDeleteOneTimeline: OnDeleteOneTimeline = useCallback(
async (timelineIds: string[]) => {
async (timelineIds: string[], searchIds?: string[]) => {
// The type for `deleteTimelines` is incorrect, it returns a Promise
await deleteTimelines(timelineIds);
await deleteTimelines(timelineIds, searchIds);
},
[deleteTimelines]
);

/** Invoked when the user clicks the action to delete the selected timelines */
const onDeleteSelected: OnDeleteSelected = useCallback(async () => {
// The type for `deleteTimelines` is incorrect, it returns a Promise
await deleteTimelines(getSelectedTimelineIds(selectedItems));
const timelineIdsWithSearch = getSelectedTimelineIdsAndSearchIds(selectedItems);
const { timelineIds, searchIds } = getRequestIds(timelineIdsWithSearch);
await deleteTimelines(timelineIds, searchIds);

// NOTE: we clear the selection state below, but if the server fails to
// delete a timeline, it will remain selected in the table:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,12 @@ export const OpenTimeline = React.memo<OpenTimelineProps>(
[actionItem]
);

const actionItemSavedSearchId = useMemo(() => {
return actionItem != null && actionItem.savedSearchId != null
? [actionItem.savedSearchId]
: undefined;
}, [actionItem]);

const onRefreshBtnClick = useCallback(() => {
if (refetch != null) {
refetch();
Expand Down Expand Up @@ -197,6 +203,7 @@ export const OpenTimeline = React.memo<OpenTimelineProps>(
<EditTimelineActions
deleteTimelines={deleteTimelines}
ids={actionItemId}
savedSearchIds={actionItemSavedSearchId}
isDeleteTimelineModalOpen={isDeleteTimelineModalOpen}
isEnableDownloader={isEnableDownloader}
onComplete={onCompleteEditTimelineAction}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
*/

import type React from 'react';
import type { AllTimelinesVariables } from '../../containers/all';
import type { TimelineModel } from '../../store/model';
import type {
RowRendererId,
Expand Down Expand Up @@ -59,6 +58,7 @@ export interface OpenTimelineResult {
pinnedEventIds?: Readonly<Record<string, boolean>> | null;
queryType?: { hasEql: boolean; hasQuery: boolean };
savedObjectId?: string | null;
savedSearchId?: string | null;
status?: TimelineStatus | null;
title?: string | null;
templateTimelineId?: string | null;
Expand All @@ -77,7 +77,7 @@ export interface EuiSearchBarQuery {
}

/** Performs IO to delete the specified timelines */
export type DeleteTimelines = (timelineIds: string[], variables?: AllTimelinesVariables) => void;
export type DeleteTimelines = (timelineIds: string[], searchIds?: string[]) => void;

/** Invoked when the user clicks the action create rule from timeline */
export type OnCreateRuleFromTimeline = (savedObjectId: string) => void;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ export const getAllTimeline = memoizeOne(
)
: null,
savedObjectId: timeline.savedObjectId,
savedSearchId: timeline.savedSearchId,
status: timeline.status,
title: timeline.title,
updated: timeline.updated,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -480,13 +480,20 @@ export const persistFavorite = async ({
return decodeResponseFavoriteTimeline(response);
};

export const deleteTimelinesByIds = async (savedObjectIds: string[]) => {
export const deleteTimelinesByIds = async (savedObjectIds: string[], searchIds?: string[]) => {
let requestBody;

try {
requestBody = JSON.stringify({
savedObjectIds,
});
if (searchIds) {
requestBody = JSON.stringify({
savedObjectIds,
searchIds,
});
} else {
requestBody = JSON.stringify({
savedObjectIds,
});
}
} catch (err) {
return Promise.reject(new Error(`Failed to stringify query: ${JSON.stringify(err)}`));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@ export const deleteTimelinesRoute = (

try {
const frameworkRequest = await buildFrameworkRequest(context, security, request);
const { savedObjectIds } = request.body;
const { savedObjectIds, searchIds } = request.body;

await deleteTimeline(frameworkRequest, savedObjectIds);
await deleteTimeline(frameworkRequest, savedObjectIds, searchIds);
return response.ok({ body: { data: { deleteTimeline: true } } });
} catch (err) {
const error = transformError(err);
Expand Down
Loading

0 comments on commit 7b69611

Please sign in to comment.