[SIEM][Detection Engine] Fix rule notification critical bugs (#63076)

## 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

x-pack/legacy/plugins/siem/common/constants.ts

@@ -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

x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/add_tags.ts

@@ -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}`]));

x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/create_notifications.test.ts

@@ -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(

x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/create_notifications.ts

@@ -17,20 +17,19 @@ 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: {
         ruleAlertId,
       },
       schedule: { interval },
       enabled,
-      actions: actions?.map(transformRuleToAlertAction),
+      actions: actions.map(transformRuleToAlertAction),
       throttle: null,
     },
   });

x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/rules_notification_alert_type.ts

@@ -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');
 

x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/types.ts

@@ -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;

x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/update_notifications.test.ts

@@ -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,
     });
 

x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/update_notifications.ts

@@ -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;
 };

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts

@@ -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({

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_route.ts

@@ -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({

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/import_rules_route.ts

@@ -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) {

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/update_rules_bulk_route.ts

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

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/update_rules_route.ts

@@ -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) {

x-pack/legacy/plugins/siem/server/lib/detection_engine/rule_actions/create_rule_actions_saved_object.ts

@@ -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, {

x-pack/legacy/plugins/siem/server/lib/detection_engine/rule_actions/delete_rule_actions_saved_object.ts

@@ -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;
+  }
 };

x-pack/legacy/plugins/siem/server/lib/detection_engine/rule_actions/get_rule_actions_saved_object.ts

@@ -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]);
 };

x-pack/legacy/plugins/siem/server/lib/detection_engine/rule_actions/update_or_create_rule_actions_saved_object.ts

@@ -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 });
 };

x-pack/legacy/plugins/siem/server/lib/detection_engine/rule_actions/update_rule_actions_saved_object.ts

@@ -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,23 +16,16 @@ interface DeleteRuleActionsSavedObject {
   savedObjectsClient: AlertServices['savedObjectsClient'];
   actions: RuleAlertAction[] | undefined;
   throttle: string | null | undefined;
+  ruleActions: RulesActionsSavedObject;
 }
 
 export const updateRuleActionsSavedObject = async ({
   ruleAlertId,
   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)
     : {

x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/create_rules.test.ts

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