Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution][Detections] Handle conflicts on alert status update #75492

Merged
merged 24 commits into from
Sep 4, 2020
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -117,7 +118,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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in any way blocking this PR, but just thinking, I think this component would benefit from using useReducer. The number of useStates are growing and (just personally) working on adding error states in here recently, felt like it was getting crowded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me! I don't think I will address it here, but if I get time, will take a stab at that soon.

const { loading: isSignalIndexLoading, signalIndexName } = useSignalIndex();
const [
{ isLoading: isSignalIndexPatternLoading, indexPatterns: signalIndexPatterns },
Expand All @@ -135,10 +136,20 @@ export const AddExceptionModal = memo(function AddExceptionModal({
},
[addError, onCancel]
);
const onSuccess = useCallback(() => {
addSuccess(i18n.ADD_EXCEPTION_SUCCESS);
onConfirm(shouldCloseAlert, shouldBulkCloseAlert);
}, [addSuccess, onConfirm, shouldBulkCloseAlert, shouldCloseAlert]);

const onSuccess = useCallback(
(updated: number, conflicts: number) => {
madirey marked this conversation as resolved.
Show resolved Hide resolved
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(
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,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;
}

/**
Expand Down Expand Up @@ -130,6 +130,8 @@ export const useAddOrUpdateException = ({
});
}

let conflicts = 0;
let updated = 0;
if (bulkCloseIndex != null) {
const filter = getQueryFilter(
'',
Expand All @@ -139,20 +141,23 @@ export const useAddOrUpdateException = ({
prepareExceptionItemsForBulkClose(exceptionItemsToAddOrUpdate),
false
);
await updateAlertStatus({

const response = await updateAlertStatus({
query: {
query: filter,
},
status: 'closed',
signal: abortCtrl.signal,
});
conflicts = response.version_conflicts;
updated = response.updated;
}

await addOrUpdateItems(exceptionItemsToAddOrUpdate);

if (isSubscribed) {
setIsLoading(false);
onSuccess();
onSuccess(updated, conflicts);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super nit: could this also be shortened a bit to just be:

Suggested change
onSuccess(updated, conflicts);
onSuccess(response.updated ?? 0, response.version_conflicts ?? 0);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this, but response was out of scope at this point. I did just uncover a related bug here (actually, the unit tests uncovered it)... let me know what you think about the updates. :)

}
} catch (error) {
if (isSubscribed) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}));
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Expand All @@ -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) =>
Expand All @@ -44,5 +45,5 @@ export const useAppToasts = (): UseAppToasts => {
[addAppError, addError]
);

return { api: toasts, addError: _addError, addSuccess };
return { api: toasts, addError: _addError, addSuccess, addWarning };
};
14 changes: 14 additions & 0 deletions x-pack/plugins/security_solution/public/common/translations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.detectionEngine.alerts.updateAlertStatusFailed', {
madirey marked this conversation as resolved.
Show resolved Hide resolved
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.detectionEngine.alerts.updateAlertStatusFailedDetailed', {
madirey marked this conversation as resolved.
Show resolved Hide resolved
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.`,
});
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ import { isEmpty } from 'lodash/fp';
import React, { useCallback, useEffect, useMemo, useState } from 'react';
import { connect, ConnectedProps } from 'react-redux';
import { Dispatch } from 'redux';

import { Status } from '../../../../common/detection_engine/schemas/common/schemas';
import { Filter, esQuery } from '../../../../../../../src/plugins/data/public';
import { TimelineIdLiteral } from '../../../../common/types/timeline';
import { useAppToasts } from '../../../common/hooks/use_app_toasts';
import { useFetchIndexPatterns } from '../../containers/detection_engine/rules/fetch_index_patterns';
import { StatefulEventsViewer } from '../../../common/components/events_viewer';
import { HeaderSection } from '../../../common/components/header_section';
Expand All @@ -32,6 +32,7 @@ import {
} from './default_config';
import { FILTER_OPEN, AlertsTableFilterGroup } from './alerts_filter_group';
import { AlertsUtilityBar } from './alerts_utility_bar';
import * as i18nCommon from '../../../common/translations';
import * as i18n from './translations';
import {
SetEventsDeletedProps,
Expand Down Expand Up @@ -90,6 +91,7 @@ export const AlertsTableComponent: React.FC<AlertsTableComponentProps> = ({
);
const kibana = useKibana();
const [, dispatchToaster] = useStateToaster();
const { addWarning } = useAppToasts();
const { initializeTimeline, setSelectAll, setIndexToAdd } = useManageTimeline();

const getGlobalQuery = useCallback(
Expand Down Expand Up @@ -130,21 +132,29 @@ export const AlertsTableComponent: React.FC<AlertsTableComponentProps> = ({
);

const onAlertStatusUpdateSuccess = useCallback(
(count: number, status: Status) => {
let title: string;
switch (status) {
case 'closed':
title = i18n.CLOSED_ALERT_SUCCESS_TOAST(count);
break;
case 'open':
title = i18n.OPENED_ALERT_SUCCESS_TOAST(count);
break;
case 'in-progress':
title = i18n.IN_PROGRESS_ALERT_SUCCESS_TOAST(count);
(updated: number, conflicts: number, status: Status) => {
if (conflicts > 0) {
// Partial failure
addWarning({
title: i18nCommon.UPDATE_ALERT_STATUS_FAILED(conflicts),
text: i18nCommon.UPDATE_ALERT_STATUS_FAILED_DETAILED(updated, conflicts),
});
} else {
let title: string;
switch (status) {
case 'closed':
title = i18n.CLOSED_ALERT_SUCCESS_TOAST(updated);
break;
case 'open':
title = i18n.OPENED_ALERT_SUCCESS_TOAST(updated);
break;
case 'in-progress':
title = i18n.IN_PROGRESS_ALERT_SUCCESS_TOAST(updated);
}
displaySuccessToast(title, dispatchToaster);
}
displaySuccessToast(title, dispatchToaster);
},
[dispatchToaster]
[addWarning, dispatchToaster]
);

const onAlertStatusUpdateFailure = useCallback(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
} from '@elastic/eui';
import styled from 'styled-components';

import { useAppToasts } from '../../../../common/hooks/use_app_toasts';
import { TimelineId } from '../../../../../common/types/timeline';
import { DEFAULT_INDEX_PATTERN } from '../../../../../common/constants';
import { Status } from '../../../../../common/detection_engine/schemas/common/schemas';
Expand All @@ -33,6 +34,7 @@ import {
AddExceptionModalBaseProps,
} from '../../../../common/components/exceptions/add_exception_modal';
import { getMappedNonEcsValue } from '../../../../common/components/exceptions/helpers';
import * as i18nCommon from '../../../../common/translations';
import * as i18n from '../translations';
import {
useStateToaster,
Expand Down Expand Up @@ -73,6 +75,8 @@ const AlertContextMenuComponent: React.FC<AlertContextMenuProps> = ({
);
const eventId = ecsRowData._id;

const { addWarning } = useAppToasts();

const onButtonClick = useCallback(() => {
setPopover(!isPopoverOpen);
}, [isPopoverOpen]);
Expand Down Expand Up @@ -125,22 +129,30 @@ const AlertContextMenuComponent: React.FC<AlertContextMenuProps> = ({
);

const onAlertStatusUpdateSuccess = useCallback(
(count: number, newStatus: Status) => {
let title: string;
switch (newStatus) {
case 'closed':
title = i18n.CLOSED_ALERT_SUCCESS_TOAST(count);
break;
case 'open':
title = i18n.OPENED_ALERT_SUCCESS_TOAST(count);
break;
case 'in-progress':
title = i18n.IN_PROGRESS_ALERT_SUCCESS_TOAST(count);
(updated: number, conflicts: number, newStatus: Status) => {
if (conflicts > 0) {
// Partial failure
addWarning({
title: i18nCommon.UPDATE_ALERT_STATUS_FAILED(conflicts),
text: i18nCommon.UPDATE_ALERT_STATUS_FAILED_DETAILED(updated, conflicts),
});
} else {
let title: string;
switch (newStatus) {
case 'closed':
title = i18n.CLOSED_ALERT_SUCCESS_TOAST(updated);
break;
case 'open':
title = i18n.OPENED_ALERT_SUCCESS_TOAST(updated);
break;
case 'in-progress':
title = i18n.IN_PROGRESS_ALERT_SUCCESS_TOAST(updated);
}
displaySuccessToast(title, dispatchToaster);
}
displaySuccessToast(title, dispatchToaster);
setAlertStatus(newStatus);
},
[dispatchToaster]
[dispatchToaster, addWarning]
);

const onAlertStatusUpdateFailure = useCallback(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export interface UpdateAlertStatusActionProps {
selectedStatus: Status;
setEventsLoading: ({ eventIds, isLoading }: SetEventsLoadingProps) => void;
setEventsDeleted: ({ eventIds, isDeleted }: SetEventsDeletedProps) => void;
onAlertStatusUpdateSuccess: (count: number, status: Status) => void;
onAlertStatusUpdateSuccess: (updated: number, conflicts: number, status: Status) => void;
onAlertStatusUpdateFailure: (status: Status, error: Error) => void;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ describe('Detections Alerts API', () => {
});
expect(fetchMock).toHaveBeenCalledWith('/api/detection_engine/signals/status', {
body:
'{"status":"closed","bool":{"filter":{"terms":{"_id":["b4ee5c32e3a321057edcc953ca17228c6fdfe5ba43fdbbdaffa8cefa11605cc5"]}}}}',
'{"conflicts":"proceed","status":"closed","bool":{"filter":{"terms":{"_id":["b4ee5c32e3a321057edcc953ca17228c6fdfe5ba43fdbbdaffa8cefa11605cc5"]}}}}',
method: 'POST',
signal: abortCtrl.signal,
});
Expand All @@ -81,7 +81,7 @@ describe('Detections Alerts API', () => {
});
expect(fetchMock).toHaveBeenCalledWith('/api/detection_engine/signals/status', {
body:
'{"status":"open","bool":{"filter":{"terms":{"_id":["b4ee5c32e3a321057edcc953ca17228c6fdfe5ba43fdbbdaffa8cefa11605cc5"]}}}}',
'{"conflicts":"proceed","status":"open","bool":{"filter":{"terms":{"_id":["b4ee5c32e3a321057edcc953ca17228c6fdfe5ba43fdbbdaffa8cefa11605cc5"]}}}}',
method: 'POST',
signal: abortCtrl.signal,
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export const updateAlertStatus = async ({
}: UpdateAlertStatusProps): Promise<UpdateDocumentByQueryResponse> =>
KibanaServices.get().http.fetch(DETECTION_ENGINE_SIGNALS_STATUS_URL, {
method: 'POST',
body: JSON.stringify({ status, ...query }),
body: JSON.stringify({ conflicts: 'proceed', status, ...query }),
signal,
});

Expand Down
Loading