[SIEM][Detection Engine] Fix rule notification critical bugs

## Summary

Fixes critical bugs found during testing of the rule notification.

* Fixes a bug where when you turn on rules quickly such as ML rules you would see these message below. This message can also be seen when you first create a rule with an action notification. This is a race condition with how we update rules multiple times when we really should only update it once and do it before enabling a rule

```
server    log   [12:18:35.986] [error][alerting][alerting][plugins][plugins] Executing Alert "63b828b5-24b9-4d55-83ee-8a8201fe2d76" has resulted in Error: [security_exception] missing authentication credentials for REST request [/_security/user/_has_privileges], with { header={ WWW-Authenticate={ 0="Bearer realm=\"security\"" & 1="ApiKey" & 2="Basic realm=\"security\" charset=\"UTF-8\"" } } 
``` 

* Fixes a bug where we were using `ruleParams.interval` when we should have been using `ruleAlertSavedObject.attributes.schedule.interval`. When changing rule notifications to run daily, weekly, etc.. you would see this exception being thrown:

```
server    log   [21:23:08.028] [error][alerting][alerting][plugins][plugins] Executing Alert "fedcccc0-7c69-4e2f-83f8-d8ee88ab5484" has resulted in Error: "from" or "to" was not provided to signals count query
```

* Fixes misc typing issues found
* Fixes it to where we no longer make multiple DB calls but rather pass down objects we already have.
* Changes the work flow to where we only update, create, or patch the alerting object once which fixes the race condition and improves the backend performance.
* Removes left over unused code
* Applied https://en.wikipedia.org/wiki/Single-entry_single-exit to functions where it made sense and easier to read.


### Checklist

- [x] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios
This commit is contained in:
Frank Hassanabad 2020-04-08 17:36:20 -06:00 committed by GitHub
parent 0c35762f27
commit c643148f36
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
28 changed files with 75 additions and 142 deletions

View file

@ -65,8 +65,6 @@ export const INTERNAL_IDENTIFIER = '__internal';
export const INTERNAL_RULE_ID_KEY = `${INTERNAL_IDENTIFIER}_rule_id`;
export const INTERNAL_RULE_ALERT_ID_KEY = `${INTERNAL_IDENTIFIER}_rule_alert_id`;
export const INTERNAL_IMMUTABLE_KEY = `${INTERNAL_IDENTIFIER}_immutable`;
export const INTERNAL_NOTIFICATION_ID_KEY = `${INTERNAL_IDENTIFIER}_notification_id`;
export const INTERNAL_NOTIFICATION_RULE_ID_KEY = `${INTERNAL_IDENTIFIER}_notification_rule_id`;
/**
* Detection engine routes

View file

@ -6,5 +6,5 @@
import { INTERNAL_RULE_ALERT_ID_KEY } from '../../../../common/constants';
export const addTags = (tags: string[] = [], ruleAlertId: string): string[] =>
export const addTags = (tags: string[], ruleAlertId: string): string[] =>
Array.from(new Set([...tags, `${INTERNAL_RULE_ALERT_ID_KEY}:${ruleAlertId}`]));

View file

@ -24,7 +24,6 @@ describe('createNotifications', () => {
enabled: true,
interval: '',
name: '',
tags: [],
});
expect(alertsClient.create).toHaveBeenCalledWith(
@ -52,7 +51,6 @@ describe('createNotifications', () => {
enabled: true,
interval: '',
name: '',
tags: [],
});
expect(alertsClient.create).toHaveBeenCalledWith(

View file

@ -17,12 +17,11 @@ export const createNotifications = async ({
ruleAlertId,
interval,
name,
tags,
}: CreateNotificationParams): Promise<Alert> =>
alertsClient.create({
data: {
name,
tags: addTags(tags, ruleAlertId),
tags: addTags([], ruleAlertId),
alertTypeId: NOTIFICATIONS_ID,
consumer: APP_ID,
params: {
@ -30,7 +29,7 @@ export const createNotifications = async ({
},
schedule: { interval },
enabled,
actions: actions?.map(transformRuleToAlertAction),
actions: actions.map(transformRuleToAlertAction),
throttle: null,
},
});

View file

@ -45,7 +45,9 @@ export const rulesNotificationAlertType = ({
const ruleParams = { ...ruleAlertParams, name: ruleName, id: ruleAlertSavedObject.id };
const fromInMs = parseScheduleDates(
previousStartedAt ? previousStartedAt.toISOString() : `now-${ruleParams.interval}`
previousStartedAt
? previousStartedAt.toISOString()
: `now-${ruleAlertSavedObject.attributes.schedule.interval}`
)?.format('x');
const toInMs = parseScheduleDates(startedAt.toISOString())?.format('x');

View file

@ -45,10 +45,11 @@ export interface Clients {
alertsClient: AlertsClient;
}
export type UpdateNotificationParams = Omit<NotificationAlertParams, 'interval' | 'actions'> & {
export type UpdateNotificationParams = Omit<
NotificationAlertParams,
'interval' | 'actions' | 'tags'
> & {
actions: RuleAlertAction[];
id?: string;
tags?: string[];
interval: string | null | undefined;
ruleAlertId: string;
} & Clients;
@ -64,8 +65,6 @@ export interface NotificationAlertParams {
ruleAlertId: string;
interval: string;
name: string;
tags?: string[];
throttle?: null;
}
export type CreateNotificationParams = NotificationAlertParams & Clients;

View file

@ -9,6 +9,7 @@ import { updateNotifications } from './update_notifications';
import { readNotifications } from './read_notifications';
import { createNotifications } from './create_notifications';
import { getNotificationResult } from '../routes/__mocks__/request_responses';
import { UpdateNotificationParams } from './types';
jest.mock('./read_notifications');
jest.mock('./create_notifications');
@ -30,7 +31,6 @@ describe('updateNotifications', () => {
enabled: true,
interval: '10m',
name: '',
tags: [],
});
expect(alertsClient.update).toHaveBeenCalledWith(
@ -48,14 +48,13 @@ describe('updateNotifications', () => {
it('should create a new notification if did not exist', async () => {
(readNotifications as jest.Mock).mockResolvedValue(null);
const params = {
const params: UpdateNotificationParams = {
alertsClient,
actions: [],
ruleAlertId: 'new-rule-id',
enabled: true,
interval: '10m',
name: '',
tags: [],
};
await updateNotifications(params);
@ -73,7 +72,6 @@ describe('updateNotifications', () => {
enabled: true,
interval: null,
name: '',
tags: [],
});
expect(alertsClient.delete).toHaveBeenCalledWith(
@ -98,7 +96,6 @@ describe('updateNotifications', () => {
enabled: true,
interval: '10m',
name: '',
tags: [],
});
expect(alertsClient.update).toHaveBeenCalledWith(
@ -125,10 +122,8 @@ describe('updateNotifications', () => {
alertsClient,
actions: [],
enabled: true,
id: notification.id,
ruleAlertId,
name: notification.name,
tags: notification.tags,
interval: null,
});

View file

@ -15,50 +15,41 @@ export const updateNotifications = async ({
alertsClient,
actions,
enabled,
id,
ruleAlertId,
name,
tags,
interval,
}: UpdateNotificationParams): Promise<PartialAlert | null> => {
const notification = await readNotifications({ alertsClient, id, ruleAlertId });
const notification = await readNotifications({ alertsClient, id: undefined, ruleAlertId });
if (interval && notification) {
const result = await alertsClient.update({
return alertsClient.update({
id: notification.id,
data: {
tags: addTags(tags, ruleAlertId),
tags: addTags([], ruleAlertId),
name,
schedule: {
interval,
},
actions: actions?.map(transformRuleToAlertAction),
actions: actions.map(transformRuleToAlertAction),
params: {
ruleAlertId,
},
throttle: null,
},
});
return result;
}
if (interval && !notification) {
const result = await createNotifications({
} else if (interval && !notification) {
return createNotifications({
alertsClient,
enabled,
tags,
name,
interval,
actions,
ruleAlertId,
});
return result;
}
if (!interval && notification) {
} else if (!interval && notification) {
await alertsClient.delete({ id: notification.id });
return null;
} else {
return null;
}
return null;
};

View file

@ -144,6 +144,7 @@ export const createRulesBulkRoute = (router: IRouter) => {
note,
version,
lists,
actions: throttle === 'rule' ? actions : [], // Only enable actions if throttle is set to rule, otherwise we are a notification and should not enable it,
});
const ruleActions = await updateRulesNotifications({

View file

@ -132,6 +132,7 @@ export const createRulesRoute = (router: IRouter): void => {
note,
version: 1,
lists,
actions: throttle === 'rule' ? actions : [], // Only enable actions if throttle is rule, otherwise we are a notification and should not enable it,
});
const ruleActions = await updateRulesNotifications({

View file

@ -196,6 +196,7 @@ export const importRulesRoute = (router: IRouter, config: LegacyServices['config
note,
version,
lists,
actions: [], // Actions are not imported nor exported at this time
});
resolve({ rule_id: ruleId, status_code: 200 });
} else if (rule != null && request.query.overwrite) {

View file

@ -122,6 +122,7 @@ export const updateRulesBulkRoute = (router: IRouter) => {
note,
version,
lists,
actions,
});
if (rule != null) {
const ruleActions = await updateRulesNotifications({

View file

@ -118,6 +118,7 @@ export const updateRulesRoute = (router: IRouter) => {
note,
version,
lists,
actions: throttle === 'rule' ? actions : [], // Only enable actions if throttle is rule, otherwise we are a notification and should not enable it
});
if (rule != null) {

View file

@ -9,6 +9,7 @@ import { AlertServices } from '../../../../../../../plugins/alerting/server';
import { ruleActionsSavedObjectType } from './saved_object_mappings';
import { IRuleActionsAttributesSavedObjectAttributes } from './types';
import { getThrottleOptions, getRuleActionsFromSavedObject } from './utils';
import { RulesActionsSavedObject } from './get_rule_actions_saved_object';
interface CreateRuleActionsSavedObject {
ruleAlertId: string;
@ -22,12 +23,7 @@ export const createRuleActionsSavedObject = async ({
savedObjectsClient,
actions = [],
throttle,
}: CreateRuleActionsSavedObject): Promise<{
id: string;
actions: RuleAlertAction[];
alertThrottle: string | null;
ruleThrottle: string;
}> => {
}: CreateRuleActionsSavedObject): Promise<RulesActionsSavedObject> => {
const ruleActionsSavedObject = await savedObjectsClient.create<
IRuleActionsAttributesSavedObjectAttributes
>(ruleActionsSavedObjectType, {

View file

@ -18,8 +18,9 @@ export const deleteRuleActionsSavedObject = async ({
savedObjectsClient,
}: DeleteRuleActionsSavedObject): Promise<{} | null> => {
const ruleActions = await getRuleActionsSavedObject({ ruleAlertId, savedObjectsClient });
if (!ruleActions) return null;
return savedObjectsClient.delete(ruleActionsSavedObjectType, ruleActions.id);
if (ruleActions != null) {
return savedObjectsClient.delete(ruleActionsSavedObjectType, ruleActions.id);
} else {
return null;
}
};

View file

@ -15,15 +15,17 @@ interface GetRuleActionsSavedObject {
savedObjectsClient: AlertServices['savedObjectsClient'];
}
export const getRuleActionsSavedObject = async ({
ruleAlertId,
savedObjectsClient,
}: GetRuleActionsSavedObject): Promise<{
export interface RulesActionsSavedObject {
id: string;
actions: RuleAlertAction[];
alertThrottle: string | null;
ruleThrottle: string;
} | null> => {
}
export const getRuleActionsSavedObject = async ({
ruleAlertId,
savedObjectsClient,
}: GetRuleActionsSavedObject): Promise<RulesActionsSavedObject | null> => {
const { saved_objects } = await savedObjectsClient.find<
IRuleActionsAttributesSavedObjectAttributes
>({
@ -35,7 +37,7 @@ export const getRuleActionsSavedObject = async ({
if (!saved_objects[0]) {
return null;
} else {
return getRuleActionsFromSavedObject(saved_objects[0]);
}
return getRuleActionsFromSavedObject(saved_objects[0]);
};

View file

@ -24,16 +24,17 @@ export const updateOrCreateRuleActionsSavedObject = async ({
actions,
throttle,
}: UpdateOrCreateRuleActionsSavedObject): Promise<RuleActions> => {
const currentRuleActions = await getRuleActionsSavedObject({ ruleAlertId, savedObjectsClient });
const ruleActions = await getRuleActionsSavedObject({ ruleAlertId, savedObjectsClient });
if (currentRuleActions) {
if (ruleActions != null) {
return updateRuleActionsSavedObject({
ruleAlertId,
savedObjectsClient,
actions,
throttle,
}) as Promise<RuleActions>;
ruleActions,
});
} else {
return createRuleActionsSavedObject({ ruleAlertId, savedObjectsClient, actions, throttle });
}
return createRuleActionsSavedObject({ ruleAlertId, savedObjectsClient, actions, throttle });
};

View file

@ -6,7 +6,7 @@
import { AlertServices } from '../../../../../../../plugins/alerting/server';
import { ruleActionsSavedObjectType } from './saved_object_mappings';
import { getRuleActionsSavedObject } from './get_rule_actions_saved_object';
import { RulesActionsSavedObject } from './get_rule_actions_saved_object';
import { RuleAlertAction } from '../../../../common/detection_engine/types';
import { getThrottleOptions } from './utils';
import { IRuleActionsAttributesSavedObjectAttributes } from './types';
@ -16,6 +16,7 @@ interface DeleteRuleActionsSavedObject {
savedObjectsClient: AlertServices['savedObjectsClient'];
actions: RuleAlertAction[] | undefined;
throttle: string | null | undefined;
ruleActions: RulesActionsSavedObject;
}
export const updateRuleActionsSavedObject = async ({
@ -23,16 +24,8 @@ export const updateRuleActionsSavedObject = async ({
savedObjectsClient,
actions,
throttle,
}: DeleteRuleActionsSavedObject): Promise<{
ruleThrottle: string;
alertThrottle: string | null;
actions: RuleAlertAction[];
id: string;
} | null> => {
const ruleActions = await getRuleActionsSavedObject({ ruleAlertId, savedObjectsClient });
if (!ruleActions) return null;
ruleActions,
}: DeleteRuleActionsSavedObject): Promise<RulesActionsSavedObject> => {
const throttleOptions = throttle
? getThrottleOptions(throttle)
: {

View file

@ -34,6 +34,7 @@ describe('createRules', () => {
interval: '',
name: '',
tags: [],
actions: [],
});
expect(alertsClient.create).toHaveBeenCalledWith(

View file

@ -4,6 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/
import { transformRuleToAlertAction } from '../../../../common/detection_engine/transform_actions';
import { Alert } from '../../../../../../../plugins/alerting/common';
import { APP_ID, SIGNALS_ID } from '../../../../common/constants';
import { CreateRuleParams } from './types';
@ -42,6 +43,7 @@ export const createRules = async ({
note,
version,
lists,
actions,
}: CreateRuleParams): Promise<Alert> => {
// TODO: Remove this and use regular lists once the feature is stable for a release
const listsParam = hasListsFeature() ? { lists } : {};
@ -81,7 +83,7 @@ export const createRules = async ({
},
schedule: { interval },
enabled,
actions: [],
actions: actions.map(transformRuleToAlertAction),
throttle: null,
},
});

View file

@ -83,6 +83,7 @@ export const installPrepackagedRules = (
note,
version,
lists,
actions: [], // At this time there is no pre-packaged actions
}),
];
}, []);

View file

@ -120,7 +120,7 @@ export const patchRules = async ({
id: rule.id,
data: {
tags: addTags(tags ?? rule.tags, rule.params.ruleId, immutable ?? rule.params.immutable),
throttle: rule.throttle,
throttle: null,
name: calculateName({ updatedName: name, originalName: rule.name }),
schedule: {
interval: calculateInterval(interval, rule.schedule.interval),

View file

@ -142,12 +142,12 @@ export interface Clients {
actionsClient: ActionsClient;
}
export type PatchRuleParams = Partial<Omit<RuleAlertParams, 'actions' | 'throttle'>> & {
export type PatchRuleParams = Partial<Omit<RuleAlertParams, 'throttle'>> & {
id: string | undefined | null;
savedObjectsClient: SavedObjectsClientContract;
} & Clients;
export type UpdateRuleParams = Omit<RuleAlertParams, 'immutable' | 'actions' | 'throttle'> & {
export type UpdateRuleParams = Omit<RuleAlertParams, 'immutable' | 'throttle'> & {
id: string | undefined | null;
savedObjectsClient: SavedObjectsClientContract;
} & Clients;
@ -157,7 +157,7 @@ export type DeleteRuleParams = Clients & {
ruleId: string | undefined | null;
};
export type CreateRuleParams = Omit<RuleAlertParams, 'ruleId' | 'actions' | 'throttle'> & {
export type CreateRuleParams = Omit<RuleAlertParams, 'ruleId' | 'throttle'> & {
ruleId: string;
} & Clients;

View file

@ -1,54 +0,0 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import {
AlertsClient,
AlertServices,
PartialAlert,
} from '../../../../../../../plugins/alerting/server';
import { getRuleActionsSavedObject } from '../rule_actions/get_rule_actions_saved_object';
import { readRules } from './read_rules';
import { transformRuleToAlertAction } from '../../../../common/detection_engine/transform_actions';
interface UpdateRuleActions {
alertsClient: AlertsClient;
savedObjectsClient: AlertServices['savedObjectsClient'];
ruleAlertId: string;
}
export const updateRuleActions = async ({
alertsClient,
savedObjectsClient,
ruleAlertId,
}: UpdateRuleActions): Promise<PartialAlert | null> => {
const rule = await readRules({ alertsClient, id: ruleAlertId });
if (rule == null) {
return null;
}
const ruleActions = await getRuleActionsSavedObject({
savedObjectsClient,
ruleAlertId,
});
if (!ruleActions) {
return null;
}
return alertsClient.update({
id: ruleAlertId,
data: {
actions: !ruleActions.alertThrottle
? ruleActions.actions.map(transformRuleToAlertAction)
: [],
throttle: null,
name: rule.name,
tags: rule.tags,
schedule: rule.schedule,
params: rule.params,
},
});
};

View file

@ -35,6 +35,7 @@ describe('updateRules', () => {
interval: '',
name: '',
tags: [],
actions: [],
});
expect(alertsClient.disable).toHaveBeenCalledWith(
@ -61,6 +62,7 @@ describe('updateRules', () => {
interval: '',
name: '',
tags: [],
actions: [],
});
expect(alertsClient.enable).toHaveBeenCalledWith(
@ -89,6 +91,7 @@ describe('updateRules', () => {
interval: '',
name: '',
tags: [],
actions: [],
});
expect(alertsClient.update).toHaveBeenCalledWith(

View file

@ -4,6 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/
import { transformRuleToAlertAction } from '../../../../common/detection_engine/transform_actions';
import { PartialAlert } from '../../../../../../../plugins/alerting/server';
import { readRules } from './read_rules';
import { IRuleSavedAttributesSavedObjectAttributes, UpdateRuleParams } from './types';
@ -46,6 +47,7 @@ export const updateRules = async ({
lists,
anomalyThreshold,
machineLearningJobId,
actions,
}: UpdateRuleParams): Promise<PartialAlert | null> => {
const rule = await readRules({ alertsClient, ruleId, id });
if (rule == null) {
@ -90,8 +92,8 @@ export const updateRules = async ({
tags: addTags(tags, rule.params.ruleId, rule.params.immutable),
name,
schedule: { interval },
actions: rule.actions,
throttle: rule.throttle,
actions: actions.map(transformRuleToAlertAction),
throttle: null,
params: {
description,
ruleId: rule.params.ruleId,

View file

@ -8,7 +8,6 @@ import { RuleAlertAction } from '../../../../common/detection_engine/types';
import { AlertsClient, AlertServices } from '../../../../../../../plugins/alerting/server';
import { updateOrCreateRuleActionsSavedObject } from '../rule_actions/update_or_create_rule_actions_saved_object';
import { updateNotifications } from '../notifications/update_notifications';
import { updateRuleActions } from './update_rule_actions';
import { RuleActions } from '../rule_actions/types';
interface UpdateRulesNotifications {
@ -37,19 +36,13 @@ export const updateRulesNotifications = async ({
throttle,
});
await updateRuleActions({
alertsClient,
savedObjectsClient,
ruleAlertId,
});
await updateNotifications({
alertsClient,
ruleAlertId,
enabled,
name,
actions: ruleActions.actions,
interval: ruleActions?.alertThrottle,
interval: ruleActions.alertThrottle,
});
return ruleActions;

View file

@ -162,5 +162,10 @@ export interface AlertAttributes {
}
export interface RuleAlertAttributes extends AlertAttributes {
params: Omit<RuleAlertParams, 'ruleId'> & { ruleId: string };
params: Omit<
RuleAlertParams,
'ruleId' | 'name' | 'enabled' | 'interval' | 'tags' | 'actions' | 'throttle'
> & {
ruleId: string;
};
}