[Security Solution] Legacy actions are deleted when user tries to sav…

…e a rule and the run action interval is slower than rule run interval (elastic#160798) ## Summary Original ticket: elastic#157462 With these changes we fix the legacy actions data loss (on migration) issue. One of the first steps of the migration we retrieve legacy actions and immediately delete them. Then we do validation which might throw an exception and all legacy actions will be lost in this case. As a solution we will do legacy actions validation before deleting them and throwing exception in case those are broken. This means that in case legacy action is broken user will need to export the rule, fix it manually and import it again. Or just re-create it from scratch. https://github.com/elastic/kibana/assets/2700761/a23f5d43-3758-4ab7-8e63-bd93016e338d
kibanamachine · Jul 31, 2023 · 8ca90fb · 8ca90fb
1 parent 9036b15
commit 8ca90fb
Show file tree

Hide file tree

Showing 4 changed files with 132 additions and 93 deletions.
diff --git a/...ugins/alerting/server/rules_client/lib/siem_legacy_actions/migrate_legacy_actions.test.ts b/...ugins/alerting/server/rules_client/lib/siem_legacy_actions/migrate_legacy_actions.test.ts
@@ -124,18 +124,17 @@ describe('migrateLegacyActions', () => {
     jest.clearAllMocks();
   });
 
-  it('should return empty migratedActions when error is thrown within method', async () => {
+  it('should throw an exception when error is thrown within method', async () => {
     (retrieveMigratedLegacyActions as jest.Mock).mockRejectedValueOnce(new Error('test failure'));
-    const migratedActions = await migrateLegacyActions(context, {
-      ruleId,
-      attributes,
-    });
+    await expect(
+      migrateLegacyActions(context, {
+        ruleId,
+        attributes,
+      })
+    ).rejects.toThrowError(
+      `Failed to migrate legacy actions for SIEM rule ${ruleId}: test failure`
+    );
 
-    expect(migratedActions).toEqual({
-      resultedActions: [],
-      hasLegacyActions: false,
-      resultedReferences: [],
-    });
     expect(context.logger.error).toHaveBeenCalledWith(
       `migrateLegacyActions(): Failed to migrate legacy actions for SIEM rule ${ruleId}: test failure`
     );
@@ -168,7 +167,11 @@ describe('migrateLegacyActions', () => {
     });
     await migrateLegacyActions(context, { ruleId, attributes });
 
-    expect(retrieveMigratedLegacyActions).toHaveBeenCalledWith(context, { ruleId });
+    expect(retrieveMigratedLegacyActions).toHaveBeenCalledWith(
+      context,
+      { ruleId },
+      expect.any(Function)
+    );
   });
 
   it('should not call validateActions and injectReferencesIntoActions if skipActionsValidation=true', async () => {
@@ -178,44 +181,6 @@ describe('migrateLegacyActions', () => {
     expect(injectReferencesIntoActions).not.toHaveBeenCalled();
   });
 
-  it('should call validateActions and injectReferencesIntoActions if attributes provided', async () => {
-    (retrieveMigratedLegacyActions as jest.Mock).mockResolvedValueOnce({
-      legacyActions: legacyActionsMock,
-      legacyActionsReferences: legacyReferencesMock,
-    });
-
-    (injectReferencesIntoActions as jest.Mock).mockReturnValue('actions-with-references');
-    await migrateLegacyActions(context, { ruleId, attributes });
-
-    expect(validateActions).toHaveBeenCalledWith(context, ruleType, {
-      ...attributes,
-      actions: 'actions-with-references',
-    });
-
-    expect(injectReferencesIntoActions).toHaveBeenCalledWith(
-      'rule_id_1',
-      [
-        {
-          actionRef: 'action_0',
-          actionTypeId: '.email',
-          group: 'default',
-          params: {
-            message: 'Rule {{context.rule.name}} generated {{state.signals_count}} alerts',
-            subject: 'Test Actions',
-            to: ['test@test.com'],
-          },
-          uuid: '11403909-ca9b-49ba-9d7a-7e5320e68d05',
-          frequency: {
-            notifyWhen: 'onThrottleInterval',
-            summary: true,
-            throttle: '1d',
-          },
-        },
-      ],
-      [{ id: 'cc85da20-d480-11ed-8e69-1df522116c28', name: 'action_0', type: 'action' }]
-    );
-  });
-
   it('should set frequency props from rule level to existing actions', async () => {
     const result = await migrateLegacyActions(context, {
       ruleId,

diff --git a/...ck/plugins/alerting/server/rules_client/lib/siem_legacy_actions/migrate_legacy_actions.ts b/...ck/plugins/alerting/server/rules_client/lib/siem_legacy_actions/migrate_legacy_actions.ts
@@ -5,6 +5,9 @@
  * 2.0.
  */
 
+import Boom from '@hapi/boom';
+import { i18n } from '@kbn/i18n';
+
 import { AlertConsumers } from '@kbn/rule-data-utils';
 
 import type { SavedObjectReference } from '@kbn/core/server';
@@ -49,15 +52,14 @@ export const migrateLegacyActions: MigrateLegacyActions = async (
       };
     }
 
-    const { legacyActions, legacyActionsReferences } = await retrieveMigratedLegacyActions(
-      context,
-      {
-        ruleId,
+    const validateLegacyActions = async (
+      legacyActions: RawRuleAction[],
+      legacyActionsReferences: SavedObjectReference[]
+    ) => {
+      // sometimes we don't need to validate legacy actions. For example, when delete rules or update rule from payload
+      if (skipActionsValidation === true) {
+        return;
       }
-    );
-
-    // sometimes we don't need to validate legacy actions. For example, when delete rules or update rule from payload
-    if (skipActionsValidation !== true) {
       const ruleType = context.ruleTypeRegistry.get(attributes.alertTypeId);
       await validateActions(context, ruleType, {
         ...attributes,
@@ -66,7 +68,15 @@ export const migrateLegacyActions: MigrateLegacyActions = async (
         notifyWhen: undefined,
         actions: injectReferencesIntoActions(ruleId, legacyActions, legacyActionsReferences),
       });
-    }
+    };
+
+    const { legacyActions, legacyActionsReferences } = await retrieveMigratedLegacyActions(
+      context,
+      {
+        ruleId,
+      },
+      validateLegacyActions
+    );
 
     // fix references for a case when actions present in a rule
     if (actions.length) {
@@ -103,11 +113,14 @@ export const migrateLegacyActions: MigrateLegacyActions = async (
     context.logger.error(
       `migrateLegacyActions(): Failed to migrate legacy actions for SIEM rule ${ruleId}: ${e.message}`
     );
-
-    return {
-      resultedActions: [],
-      hasLegacyActions: false,
-      resultedReferences: [],
-    };
+    throw Boom.badRequest(
+      i18n.translate('xpack.alerting.rulesClient.validateLegacyActions.errorSummary', {
+        defaultMessage: 'Failed to migrate legacy actions for SIEM rule {ruleId}: {errorMessage}',
+        values: {
+          ruleId,
+          errorMessage: e.message,
+        },
+      })
+    );
   }
 };
diff --git a/...ting/server/rules_client/lib/siem_legacy_actions/retrieve_migrated_legacy_actions.test.ts b/...ting/server/rules_client/lib/siem_legacy_actions/retrieve_migrated_legacy_actions.test.ts
@@ -69,7 +69,8 @@ describe('Legacy rule action migration logic', () => {
           unsecuredSavedObjectsClient: savedObjectsClient,
           logger,
         } as unknown as RulesClientContext,
-        { ruleId }
+        { ruleId },
+        () => Promise.resolve()
       );
 
       expect(deleteRuleMock).not.toHaveBeenCalled();
@@ -92,7 +93,8 @@ describe('Legacy rule action migration logic', () => {
           unsecuredSavedObjectsClient: savedObjectsClient,
           logger,
         } as unknown as RulesClientContext,
-        { ruleId }
+        { ruleId },
+        () => Promise.resolve()
       );
 
       expect(deleteRuleMock).not.toHaveBeenCalled();
@@ -116,7 +118,8 @@ describe('Legacy rule action migration logic', () => {
           unsecuredSavedObjectsClient: savedObjectsClient,
           logger,
         } as unknown as RulesClientContext,
-        { ruleId }
+        { ruleId },
+        () => Promise.resolve()
       );
 
       expect(deleteRuleMock).not.toHaveBeenCalled();
@@ -146,7 +149,8 @@ describe('Legacy rule action migration logic', () => {
           unsecuredSavedObjectsClient: savedObjectsClient,
           logger,
         } as unknown as RulesClientContext,
-        { ruleId }
+        { ruleId },
+        () => Promise.resolve()
       );
 
       expect(deleteRuleMock).toHaveBeenCalledWith(expect.any(Object), { id: '456' });
@@ -192,7 +196,8 @@ describe('Legacy rule action migration logic', () => {
           unsecuredSavedObjectsClient: savedObjectsClient,
           logger,
         } as unknown as RulesClientContext,
-        { ruleId }
+        { ruleId },
+        () => Promise.resolve()
       );
 
       expect(deleteRuleMock).toHaveBeenCalledWith(expect.any(Object), { id: '456' });
@@ -237,7 +242,8 @@ describe('Legacy rule action migration logic', () => {
           unsecuredSavedObjectsClient: savedObjectsClient,
           logger,
         } as unknown as RulesClientContext,
-        { ruleId }
+        { ruleId },
+        () => Promise.resolve()
       );
 
       expect(deleteRuleMock).toHaveBeenCalledWith(expect.any(Object), { id: '456' });
@@ -263,5 +269,47 @@ describe('Legacy rule action migration logic', () => {
         legacyActionsReferences: [{ id: '456', name: 'action_0', type: 'action' }],
       });
     });
+
+    test('it calls validateLegacyActions on migration a rule with legacy actions', async () => {
+      // siem.notifications is not created for a rule with no actions
+      findMock.mockResolvedValueOnce({
+        page: 1,
+        perPage: 1,
+        total: 1,
+        data: [legacyGetDailyNotificationResult(connectorId, ruleId)],
+      });
+      // siem-detection-engine-rule-actions SO is still created
+      savedObjectsClient.find.mockResolvedValueOnce(
+        legacyGetSiemNotificationRuleActionsSOResultWithSingleHit(['daily'], ruleId, connectorId)
+      );
+
+      const validateLegacyActions = jest.fn();
+      await retrieveMigratedLegacyActions(
+        {
+          unsecuredSavedObjectsClient: savedObjectsClient,
+          logger,
+        } as unknown as RulesClientContext,
+        { ruleId },
+        validateLegacyActions
+      );
+
+      expect(validateLegacyActions).toHaveBeenCalledWith(
+        [
+          {
+            actionRef: 'action_0',
+            actionTypeId: '.email',
+            frequency: { notifyWhen: 'onThrottleInterval', summary: true, throttle: '1d' },
+            group: 'default',
+            params: {
+              message: 'Rule {{context.rule.name}} generated {{state.signals_count}} alerts',
+              subject: 'Test Actions',
+              to: ['test@test.com'],
+            },
+            uuid: expect.any(String),
+          },
+        ],
+        [{ id: '456', name: 'action_0', type: 'action' }]
+      );
+    });
   });
 });
diff --git a/.../alerting/server/rules_client/lib/siem_legacy_actions/retrieve_migrated_legacy_actions.ts b/.../alerting/server/rules_client/lib/siem_legacy_actions/retrieve_migrated_legacy_actions.ts
@@ -15,7 +15,11 @@ import { transformFromLegacyActions } from './transform_legacy_actions';
 
 type RetrieveMigratedLegacyActions = (
   context: RulesClientContext,
-  { ruleId }: { ruleId: string }
+  { ruleId }: { ruleId: string },
+  validateLegacyActions: (
+    legacyActions: RawRuleAction[],
+    legacyActionsReferences: SavedObjectReference[]
+  ) => Promise<void>
 ) => Promise<{ legacyActions: RawRuleAction[]; legacyActionsReferences: SavedObjectReference[] }>;
 
 /**
@@ -27,7 +31,8 @@ type RetrieveMigratedLegacyActions = (
  */
 export const retrieveMigratedLegacyActions: RetrieveMigratedLegacyActions = async (
   context,
-  { ruleId }
+  { ruleId },
+  validateLegacyActions
 ) => {
   const { unsecuredSavedObjectsClient } = context;
   try {
@@ -71,17 +76,19 @@ export const retrieveMigratedLegacyActions: RetrieveMigratedLegacyActions = asyn
       return { legacyActions: [], legacyActionsReferences: [] };
     }
 
-    await Promise.all([
-      // If the legacy notification rule type ("siem.notification") exist,
-      // migration and cleanup are needed
-      siemNotificationsExist && deleteRule(context, { id: siemNotification.data[0].id }),
-      // Delete the legacy sidecar SO if it exists
-      legacyRuleNotificationSOsExist &&
-        unsecuredSavedObjectsClient.delete(
-          legacyRuleActionsSavedObjectType,
-          legacyRuleActionsSO.saved_objects[0].id
-        ),
-    ]);
+    const deleteLegacyActions = async () => {
+      await Promise.all([
+        // If the legacy notification rule type ("siem.notification") exist,
+        // migration and cleanup are needed
+        siemNotificationsExist && deleteRule(context, { id: siemNotification.data[0].id }),
+        // Delete the legacy sidecar SO if it exists
+        legacyRuleNotificationSOsExist &&
+          unsecuredSavedObjectsClient.delete(
+            legacyRuleActionsSavedObjectType,
+            legacyRuleActionsSO.saved_objects[0].id
+          ),
+      ]);
+    };
 
     // If legacy notification sidecar ("siem-detection-engine-rule-actions")
     // exist, migration is needed
@@ -95,22 +102,28 @@ export const retrieveMigratedLegacyActions: RetrieveMigratedLegacyActions = asyn
         legacyRuleActionsSO.saved_objects[0].attributes.ruleThrottle === 'no_actions' ||
         legacyRuleActionsSO.saved_objects[0].attributes.ruleThrottle === 'rule'
       ) {
+        await deleteLegacyActions();
         return { legacyActions: [], legacyActionsReferences: [] };
       }
 
-      return {
-        legacyActions: transformFromLegacyActions(
-          legacyRuleActionsSO.saved_objects[0].attributes,
-          legacyRuleActionsSO.saved_objects[0].references
-        ),
-        legacyActionsReferences:
-          // only action references need to be saved
-          legacyRuleActionsSO.saved_objects[0].references.filter(({ type }) => type === 'action') ??
-          [],
-      };
+      const legacyActions = transformFromLegacyActions(
+        legacyRuleActionsSO.saved_objects[0].attributes,
+        legacyRuleActionsSO.saved_objects[0].references
+      );
+      // only action references need to be saved
+      const legacyActionsReferences =
+        legacyRuleActionsSO.saved_objects[0].references.filter(({ type }) => type === 'action') ??
+        [];
+      await validateLegacyActions(legacyActions, legacyActionsReferences);
+
+      // Delete legacy actions only after the validation
+      await deleteLegacyActions();
+      return { legacyActions, legacyActionsReferences };
     }
+    await deleteLegacyActions();
   } catch (e) {
     context.logger.debug(`Migration has failed for rule ${ruleId}: ${e.message}`);
+    throw e;
   }
 
   return { legacyActions: [], legacyActionsReferences: [] };