[Security Solution] [Platform] Fix critical bug when migrating action within update route (#116512)

* WIP - need to figure out how to delete old siem-detection action SO's after each test

* WIP - adds some fixes for the update rules utility that differ from patch rules utility

* fix type checks

* cleanup

* remove commented out code

* rename const to use capital snake case

* naming integration tests, adds expect for disabled rules that get migrated, adds expect for pre-migrated rules

x-pack/plugins/security_solution/common/constants.ts

@@ -225,6 +225,11 @@ export const INTERNAL_RULE_ID_KEY = `${INTERNAL_IDENTIFIER}_rule_id` as const;
 export const INTERNAL_RULE_ALERT_ID_KEY = `${INTERNAL_IDENTIFIER}_rule_alert_id` as const;
 export const INTERNAL_IMMUTABLE_KEY = `${INTERNAL_IDENTIFIER}_immutable` as const;
 
+/**
+ * Internal actions route
+ */
+export const UPDATE_OR_CREATE_LEGACY_ACTIONS = '/internal/api/detection/legacy/notifications';
+
 /**
  * Detection engine routes
  */

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

@@ -78,7 +78,7 @@ export const updateRulesBulkRoute = (
               id: payloadRule.id,
             });
 
-            await legacyMigrate({
+            const migratedRule = await legacyMigrate({
               rulesClient,
               savedObjectsClient,
               rule: existingRule,
@@ -89,6 +89,8 @@ export const updateRulesBulkRoute = (
               rulesClient,
               ruleStatusClient,
               defaultOutputIndex: siemClient.getSignalsIndex(),
+              existingRule,
+              migratedRule,
               ruleUpdate: payloadRule,
               isRuleRegistryEnabled,
             });

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

@@ -69,7 +69,7 @@ export const updateRulesRoute = (
           id: request.body.id,
         });
 
-        await legacyMigrate({
+        const migratedRule = await legacyMigrate({
           rulesClient,
           savedObjectsClient,
           rule: existingRule,
@@ -79,6 +79,8 @@ export const updateRulesRoute = (
           isRuleRegistryEnabled,
           rulesClient,
           ruleStatusClient,
+          existingRule,
+          migratedRule,
           ruleUpdate: request.body,
           spaceId: context.securitySolution.getSpaceId(),
         });

x-pack/plugins/security_solution/server/lib/detection_engine/rules/types.ts

@@ -274,6 +274,8 @@ export interface UpdateRulesOptions {
   ruleStatusClient: IRuleExecutionLogClient;
   rulesClient: RulesClient;
   defaultOutputIndex: string;
+  existingRule: SanitizedAlert<RuleParams> | null | undefined;
+  migratedRule: SanitizedAlert<RuleParams> | null | undefined;
   ruleUpdate: UpdateRulesSchema;
 }
 

x-pack/plugins/security_solution/server/lib/detection_engine/rules/update_rules.mock.ts

@@ -12,6 +12,8 @@ import {
   getUpdateRulesSchemaMock,
 } from '../../../../common/detection_engine/schemas/request/rule_schemas.mock';
 import { ruleExecutionLogClientMock } from '../rule_execution_log/__mocks__/rule_execution_log_client';
+import { getAlertMock } from '../routes/__mocks__/request_responses';
+import { getQueryRuleParams } from '../schemas/rule_schemas.mock';
 
 export const getUpdateRulesOptionsMock = (isRuleRegistryEnabled: boolean) => ({
   spaceId: 'default',
@@ -19,6 +21,8 @@ export const getUpdateRulesOptionsMock = (isRuleRegistryEnabled: boolean) => ({
   ruleStatusClient: ruleExecutionLogClientMock.create(),
   savedObjectsClient: savedObjectsClientMock.create(),
   defaultOutputIndex: '.siem-signals-default',
+  existingRule: getAlertMock(isRuleRegistryEnabled, getQueryRuleParams()),
+  migratedRule: getAlertMock(isRuleRegistryEnabled, getQueryRuleParams()),
   ruleUpdate: getUpdateRulesSchemaMock(),
   isRuleRegistryEnabled,
 });
@@ -29,6 +33,8 @@ export const getUpdateMlRulesOptionsMock = (isRuleRegistryEnabled: boolean) => (
   ruleStatusClient: ruleExecutionLogClientMock.create(),
   savedObjectsClient: savedObjectsClientMock.create(),
   defaultOutputIndex: '.siem-signals-default',
+  existingRule: getAlertMock(isRuleRegistryEnabled, getQueryRuleParams()),
+  migratedRule: getAlertMock(isRuleRegistryEnabled, getQueryRuleParams()),
   ruleUpdate: getUpdateMachineLearningSchemaMock(),
   isRuleRegistryEnabled,
 });

x-pack/plugins/security_solution/server/lib/detection_engine/rules/update_rules.ts

@@ -10,13 +10,19 @@ import { validate } from '@kbn/securitysolution-io-ts-utils';
 import { DEFAULT_MAX_SIGNALS } from '../../../../common/constants';
 import { transformRuleToAlertAction } from '../../../../common/detection_engine/transform_actions';
 import { PartialAlert } from '../../../../../alerting/server';
-import { readRules } from './read_rules';
+
 import { UpdateRulesOptions } from './types';
 import { addTags } from './add_tags';
 import { typeSpecificSnakeToCamel } from '../schemas/rule_converters';
 import { internalRuleUpdate, RuleParams } from '../schemas/rule_schemas';
 import { enableRule } from './enable_rule';
-import { maybeMute, transformToAlertThrottle, transformToNotifyWhen } from './utils';
+import {
+  maybeMute,
+  transformToAlertThrottle,
+  transformToNotifyWhen,
+  updateActions,
+  updateThrottleNotifyWhen,
+} from './utils';
 
 class UpdateError extends Error {
   public readonly statusCode: number;
@@ -27,19 +33,14 @@ class UpdateError extends Error {
 }
 
 export const updateRules = async ({
-  isRuleRegistryEnabled,
   spaceId,
   rulesClient,
   ruleStatusClient,
   defaultOutputIndex,
+  existingRule,
+  migratedRule,
   ruleUpdate,
 }: UpdateRulesOptions): Promise<PartialAlert<RuleParams> | null> => {
-  const existingRule = await readRules({
-    isRuleRegistryEnabled,
-    rulesClient,
-    ruleId: ruleUpdate.rule_id,
-    id: ruleUpdate.id,
-  });
   if (existingRule == null) {
     return null;
   }
@@ -85,9 +86,24 @@ export const updateRules = async ({
       ...typeSpecificParams,
     },
     schedule: { interval: ruleUpdate.interval ?? '5m' },
-    actions: ruleUpdate.actions != null ? ruleUpdate.actions.map(transformRuleToAlertAction) : [],
-    throttle: transformToAlertThrottle(ruleUpdate.throttle),
-    notifyWhen: transformToNotifyWhen(ruleUpdate.throttle),
+    actions: updateActions(
+      transformRuleToAlertAction,
+      migratedRule?.actions,
+      existingRule.actions,
+      ruleUpdate?.actions
+    ),
+    throttle: updateThrottleNotifyWhen(
+      transformToAlertThrottle,
+      migratedRule?.throttle,
+      existingRule.throttle,
+      ruleUpdate?.throttle
+    ),
+    notifyWhen: updateThrottleNotifyWhen(
+      transformToNotifyWhen,
+      migratedRule?.notifyWhen,
+      existingRule.notifyWhen,
+      ruleUpdate?.throttle
+    ),
   };
 
   const [validated, errors] = validate(newInternalRule, internalRuleUpdate);

x-pack/plugins/security_solution/server/lib/detection_engine/rules/utils.ts

@@ -5,7 +5,7 @@
  * 2.0.
  */
 
-import { pickBy, isEmpty } from 'lodash/fp';
+import { pickBy, isEmpty, isEqual } from 'lodash/fp';
 import type {
   FromOrUndefined,
   MachineLearningJobIdOrUndefined,
@@ -64,10 +64,14 @@ import { RulesClient } from '../../../../../alerting/server';
 // eslint-disable-next-line no-restricted-imports
 import { LegacyRuleActions } from '../rule_actions/legacy_types';
 import { FullResponseSchema } from '../../../../common/detection_engine/schemas/request';
-import { transformAlertToRuleAction } from '../../../../common/detection_engine/transform_actions';
+import {
+  transformAlertToRuleAction,
+  transformRuleToAlertAction,
+} from '../../../../common/detection_engine/transform_actions';
 // eslint-disable-next-line no-restricted-imports
 import { legacyRuleActionsSavedObjectType } from '../rule_actions/legacy_saved_object_mappings';
 import { LegacyMigrateParams } from './types';
+import { RuleAlertAction } from '../../../../common/detection_engine/types';
 
 export const calculateInterval = (
   interval: string | undefined,
@@ -331,6 +335,10 @@ export const legacyMigrate = async ({
     }),
     savedObjectsClient.find({
       type: legacyRuleActionsSavedObjectType,
+      hasReference: {
+        type: 'alert',
+        id: rule.id,
+      },
     }),
   ]);
 
@@ -344,8 +352,10 @@ export const legacyMigrate = async ({
           )
         : null,
     ]);
+
+    const { id, ...restOfRule } = rule;
     const migratedRule = {
-      ...rule,
+      ...restOfRule,
       actions: siemNotification.data[0].actions,
       throttle: siemNotification.data[0].schedule.interval,
       notifyWhen: transformToNotifyWhen(siemNotification.data[0].throttle),
@@ -354,7 +364,39 @@ export const legacyMigrate = async ({
       id: rule.id,
       data: migratedRule,
     });
-    return migratedRule;
+    return { id: rule.id, ...migratedRule };
   }
   return rule;
 };
+
+export const updateThrottleNotifyWhen = (
+  transform: typeof transformToAlertThrottle | typeof transformToNotifyWhen,
+  migratedRuleThrottle: string | null | undefined,
+  existingRuleThrottle: string | null | undefined,
+  ruleUpdateThrottle: string | null | undefined
+) => {
+  if (existingRuleThrottle == null && ruleUpdateThrottle == null && migratedRuleThrottle != null) {
+    return migratedRuleThrottle;
+  }
+  return transform(ruleUpdateThrottle);
+};
+
+export const updateActions = (
+  transform: typeof transformRuleToAlertAction,
+  migratedRuleActions: AlertAction[] | null | undefined,
+  existingRuleActions: AlertAction[] | null | undefined,
+  ruleUpdateActions: RuleAlertAction[] | null | undefined
+) => {
+  // if the existing rule actions and the rule update actions are equivalent (aka no change)
+  // but the migrated actions and the ruleUpdateActions (or existing rule actions, associatively)
+  // are not equivalent, we know that the rules' actions were migrated and we need to apply
+  // that change to the update request so it is not overwritten by the rule update payload
+  if (
+    existingRuleActions?.length === 0 &&
+    ruleUpdateActions == null &&
+    !isEqual(existingRuleActions, migratedRuleActions)
+  ) {
+    return migratedRuleActions;
+  }
+  return ruleUpdateActions != null ? ruleUpdateActions.map(transform) : [];
+};

x-pack/test/detection_engine_api_integration/security_and_spaces/tests/patch_rules.ts

@@ -21,6 +21,7 @@ import {
   getSimpleMlRuleOutput,
   createRule,
   getSimpleMlRule,
+  createLegacyRuleAction,
 } from '../../utils';
 
 // eslint-disable-next-line import/no-default-export
@@ -187,6 +188,46 @@ export default ({ getService }: FtrProviderContext) => {
         expect(bodyToCompare).to.eql(outputRule);
       });
 
+      it('should return the rule with migrated actions after the enable patch', async () => {
+        const [connector, rule] = await Promise.all([
+          supertest
+            .post(`/api/actions/connector`)
+            .set('kbn-xsrf', 'foo')
+            .send({
+              name: 'My action',
+              connector_type_id: '.slack',
+              secrets: {
+                webhookUrl: 'http://localhost:1234',
+              },
+            }),
+          createRule(supertest, getSimpleRule('rule-1')),
+        ]);
+        await createLegacyRuleAction(supertest, rule.id, connector.body.id);
+
+        // patch disable the rule
+        const patchResponse = await supertest
+          .patch(DETECTION_ENGINE_RULES_URL)
+          .set('kbn-xsrf', 'true')
+          .send({ id: rule.id, enabled: false })
+          .expect(200);
+
+        const outputRule = getSimpleRuleOutput();
+        outputRule.actions = [
+          {
+            action_type_id: '.slack',
+            group: 'default',
+            id: connector.body.id,
+            params: {
+              message:
+                'Hourly\nRule {{context.rule.name}} generated {{state.signals_count}} alerts',
+            },
+          },
+        ];
+        outputRule.throttle = '1m';
+        const bodyToCompare = removeServerGeneratedProperties(patchResponse.body);
+        expect(bodyToCompare).to.eql(outputRule);
+      });
+
       it('should give a 404 if it is given a fake id', async () => {
         const { body } = await supertest
           .patch(DETECTION_ENGINE_RULES_URL)

x-pack/test/detection_engine_api_integration/security_and_spaces/tests/patch_rules_bulk.ts

@@ -19,6 +19,7 @@ import {
   getSimpleRuleOutputWithoutRuleId,
   removeServerGeneratedPropertiesIncludingRuleId,
   createRule,
+  createLegacyRuleAction,
 } from '../../utils';
 
 // eslint-disable-next-line import/no-default-export
@@ -126,6 +127,55 @@ export default ({ getService }: FtrProviderContext) => {
         expect(bodyToCompare2).to.eql(outputRule2);
       });
 
+      it('should bulk disable two rules and migrate their actions', async () => {
+        const [connector, rule1, rule2] = await Promise.all([
+          supertest
+            .post(`/api/actions/connector`)
+            .set('kbn-xsrf', 'foo')
+            .send({
+              name: 'My action',
+              connector_type_id: '.slack',
+              secrets: {
+                webhookUrl: 'http://localhost:1234',
+              },
+            }),
+          createRule(supertest, getSimpleRule('rule-1')),
+          createRule(supertest, getSimpleRule('rule-2')),
+        ]);
+        await Promise.all([
+          createLegacyRuleAction(supertest, rule1.id, connector.body.id),
+          createLegacyRuleAction(supertest, rule2.id, connector.body.id),
+        ]);
+        // patch a simple rule's name
+        const { body } = await supertest
+          .patch(`${DETECTION_ENGINE_RULES_URL}/_bulk_update`)
+          .set('kbn-xsrf', 'true')
+          .send([
+            { id: rule1.id, enabled: false },
+            { id: rule2.id, enabled: false },
+          ])
+          .expect(200);
+
+        // @ts-expect-error
+        body.forEach((response) => {
+          const outputRule = getSimpleRuleOutput(response.rule_id, false);
+          outputRule.actions = [
+            {
+              action_type_id: '.slack',
+              group: 'default',
+              id: connector.body.id,
+              params: {
+                message:
+                  'Hourly\nRule {{context.rule.name}} generated {{state.signals_count}} alerts',
+              },
+            },
+          ];
+          outputRule.throttle = '1m';
+          const bodyToCompare = removeServerGeneratedProperties(response);
+          expect(bodyToCompare).to.eql(outputRule);
+        });
+      });
+
       it('should patch a single rule property of name using the auto-generated id', async () => {
         const createdBody = await createRule(supertest, getSimpleRule('rule-1'));
 

x-pack/test/detection_engine_api_integration/security_and_spaces/tests/update_rules.ts

@@ -23,6 +23,7 @@ import {
   getSimpleMlRuleUpdate,
   createRule,
   getSimpleRule,
+  createLegacyRuleAction,
 } from '../../utils';
 
 // eslint-disable-next-line import/no-default-export
@@ -130,6 +131,55 @@ export default ({ getService }: FtrProviderContext) => {
         expect(bodyToCompare).to.eql(outputRule);
       });
 
+      it('should update a single rule property of name using an auto-generated rule_id and migrate the actions', async () => {
+        const rule = getSimpleRule('rule-1');
+        delete rule.rule_id;
+        const [connector, createRuleBody] = await Promise.all([
+          supertest
+            .post(`/api/actions/connector`)
+            .set('kbn-xsrf', 'foo')
+            .send({
+              name: 'My action',
+              connector_type_id: '.slack',
+              secrets: {
+                webhookUrl: 'http://localhost:1234',
+              },
+            }),
+          createRule(supertest, rule),
+        ]);
+        await createLegacyRuleAction(supertest, createRuleBody.id, connector.body.id);
+
+        // update a simple rule's name
+        const updatedRule = getSimpleRuleUpdate('rule-1');
+        updatedRule.rule_id = createRuleBody.rule_id;
+        updatedRule.name = 'some other name';
+        delete updatedRule.id;
+
+        const { body } = await supertest
+          .put(DETECTION_ENGINE_RULES_URL)
+          .set('kbn-xsrf', 'true')
+          .send(updatedRule)
+          .expect(200);
+
+        const outputRule = getSimpleRuleOutputWithoutRuleId();
+        outputRule.name = 'some other name';
+        outputRule.version = 2;
+        outputRule.actions = [
+          {
+            action_type_id: '.slack',
+            group: 'default',
+            id: connector.body.id,
+            params: {
+              message:
+                'Hourly\nRule {{context.rule.name}} generated {{state.signals_count}} alerts',
+            },
+          },
+        ];
+        outputRule.throttle = '1m';
+        const bodyToCompare = removeServerGeneratedPropertiesIncludingRuleId(body);
+        expect(bodyToCompare).to.eql(outputRule);
+      });
+
       it('should update a single rule property of name using the auto-generated id', async () => {
         const createdBody = await createRule(supertest, getSimpleRule('rule-1'));
 

x-pack/test/detection_engine_api_integration/security_and_spaces/tests/update_rules_bulk.ts

@@ -20,6 +20,7 @@ import {
   getSimpleRuleUpdate,
   createRule,
   getSimpleRule,
+  createLegacyRuleAction,
 } from '../../utils';
 
 // eslint-disable-next-line import/no-default-export
@@ -94,6 +95,64 @@ export default ({ getService }: FtrProviderContext) => {
         expect(bodyToCompare2).to.eql(outputRule2);
       });
 
+      it('should update two rule properties of name using the two rules rule_id and migrate actions', async () => {
+        const [connector, rule1, rule2] = await Promise.all([
+          supertest
+            .post(`/api/actions/connector`)
+            .set('kbn-xsrf', 'foo')
+            .send({
+              name: 'My action',
+              connector_type_id: '.slack',
+              secrets: {
+                webhookUrl: 'http://localhost:1234',
+              },
+            }),
+          createRule(supertest, getSimpleRule('rule-1')),
+          createRule(supertest, getSimpleRule('rule-2')),
+        ]);
+        await Promise.all([
+          createLegacyRuleAction(supertest, rule1.id, connector.body.id),
+          createLegacyRuleAction(supertest, rule2.id, connector.body.id),
+        ]);
+
+        expect(rule1.actions).to.eql([]);
+        expect(rule2.actions).to.eql([]);
+
+        const updatedRule1 = getSimpleRuleUpdate('rule-1');
+        updatedRule1.name = 'some other name';
+
+        const updatedRule2 = getSimpleRuleUpdate('rule-2');
+        updatedRule2.name = 'some other name';
+
+        // update both rule names
+        const { body } = await supertest
+          .put(`${DETECTION_ENGINE_RULES_URL}/_bulk_update`)
+          .set('kbn-xsrf', 'true')
+          .send([updatedRule1, updatedRule2])
+          .expect(200);
+
+        // @ts-expect-error
+        body.forEach((response) => {
+          const outputRule = getSimpleRuleOutput(response.rule_id);
+          outputRule.name = 'some other name';
+          outputRule.version = 2;
+          outputRule.actions = [
+            {
+              action_type_id: '.slack',
+              group: 'default',
+              id: connector.body.id,
+              params: {
+                message:
+                  'Hourly\nRule {{context.rule.name}} generated {{state.signals_count}} alerts',
+              },
+            },
+          ];
+          outputRule.throttle = '1m';
+          const bodyToCompare = removeServerGeneratedProperties(response);
+          expect(bodyToCompare).to.eql(outputRule);
+        });
+      });
+
       it('should update a single rule property of name using an id', async () => {
         const createRuleBody = await createRule(supertest, getSimpleRule('rule-1'));
 

x-pack/test/detection_engine_api_integration/utils.ts

@@ -48,6 +48,7 @@ import {
   DETECTION_ENGINE_SIGNALS_MIGRATION_URL,
   INTERNAL_IMMUTABLE_KEY,
   INTERNAL_RULE_ID_KEY,
+  UPDATE_OR_CREATE_LEGACY_ACTIONS,
 } from '../../plugins/security_solution/common/constants';
 import { RACAlert } from '../../plugins/security_solution/server/lib/detection_engine/rule_types/types';
 
@@ -516,6 +517,29 @@ export const createSignalsIndex = async (
   }, 'createSignalsIndex');
 };
 
+export const createLegacyRuleAction = async (
+  supertest: SuperTest.SuperTest<SuperTest.Test>,
+  alertId: string,
+  connectorId: string
+): Promise<unknown> =>
+  supertest
+    .post(`${UPDATE_OR_CREATE_LEGACY_ACTIONS}`)
+    .set('kbn-xsrf', 'true')
+    .query({ alert_id: alertId })
+    .send({
+      name: 'Legacy notification with one action',
+      interval: '1m',
+      actions: [
+        {
+          id: connectorId,
+          group: 'default',
+          params: {
+            message: 'Hourly\nRule {{context.rule.name}} generated {{state.signals_count}} alerts',
+          },
+          actionTypeId: '.slack',
+        },
+      ],
+    });
 /**
  * Deletes the signals index for use inside of afterEach blocks of tests
  * @param supertest The supertest client library