[RAM][Security Solution][Alerts] moves legacy actions migration to ru…

…lesClient (elastic#153101) ## Summary - this PR is the first part of work related to conditional logic actions. The rest of PRs, will be merged after this one. As they depend on a work implemented here. [List of tickets](elastic/security-team#2894 (comment)) - addresses elastic#151919 - moves code related to legacy actions migration from D&R to RulesClient, [details](elastic#151919 (comment)) - similarly to D&R part, now rulesClient read APIs, would return legacy actions within rule - similarly, every mutation API in rulesClient, would migrate legacy actions, and remove sidecar SO - each migrated legacy action will have also [`frequency` object](https://github.com/elastic/kibana/blob/8.7/x-pack/plugins/alerting/server/types.ts#L234-L238), that would allow to have notifyWhen/throttle on action level once elastic#151916 is implemented, which is targeted in 8.8, right after this PR. But before it's merged, `frequency` is getting removed in [update/bulk_edit/create APIs](https://github.com/elastic/kibana/blob/8.7/x-pack/plugins/alerting/server/rules_client/methods/update.ts#L151-L160). Hence it's not reflected in most of the tests at this point. - cleanup of legacy actions related code in D&R - adds unit tests for RulesClient - keeps functional/e2e tests in D&R Changes in behaviour, introduced in this PR: - since, migration happens within single rulesClient API call, revision in migrated rule will increment by `1` only. - legacy actions from sidecar SO, now will be merged with rules actions, if there any. Before, in the previous implementation, there was inconsistency in a way how legacy and rules actions were treated. - On read: actions existing in rule, [would take precedence over legacy ones ](https://github.com/elastic/kibana/blob/8.7/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/normalization/rule_actions.ts#L94-L114) - On migration: SO actions [only saved](https://github.com/elastic/kibana/blob/8.7/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/rule_actions/legacy_action_migration.ts#L114). If any actions present in rule, they will be lost. Here is an example video from **main** branch <details> <summary>Here is an example video from MAIN branch, where action in rule is overwritten by legacy action</summary> https://user-images.githubusercontent.com/92328789/230397535-d3fcd644-7cf9-4970-a573-18fd8c9f2235.mov </details> So, depends on sequence of events, different actions could be saved for identical use case: rule has both legacy and existing action. - if rule migrated through update API, existing in rule action will be saved - if rule migrated through enable/disable API, bulk edit, legacy action will be saved In this implementation, both existing in rule and legacy actions will be merged, to prevent loss of actions <details> <summary>Here is an example video from this PR branch, where actions are merged</summary> <video src="https://user-images.githubusercontent.com/92328789/230405569-f1da38e9-4e36-46a8-9654-f664e0a31063.mov" /> </details> - when duplicating rule, we don't migrate source rule anymore. It can lead to unwanted API key regeneration, with possible loss of privileges, earlier associated with the source rule. As part of UX, when duplicating any entity, users would not be expecting source entity to be changed TODO: - performance improvement issue for future elastic#154438 - currently, in main branch, when migration is performed through rule enabling, actions not showing anymore in UI. Relevant ticket is elastic#154567 I haven't fixed it in this PR, as in[ the next one ](elastic#153113), we will display notifyWhen/throttle on action level in UI, rather than on rule level. So, once that PR is merged, actions should be displayed in new UI ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Garrett Spong <spong@users.noreply.github.com>
saarikabhasi · Apr 19, 2023 · 57cbaca · 57cbaca
1 parent 3f894de
commit 57cbaca
Show file tree

Hide file tree

Showing 81 changed files with 3,044 additions and 1,976 deletions.
diff --git a/x-pack/plugins/alerting/server/rules_client/lib/index.ts b/x-pack/plugins/alerting/server/rules_client/lib/index.ts
@@ -15,5 +15,7 @@ export { checkAuthorizationAndGetTotal } from './check_authorization_and_get_tot
 export { scheduleTask } from './schedule_task';
 export { createNewAPIKeySet } from './create_new_api_key_set';
 export { recoverRuleAlerts } from './recover_rule_alerts';
+export { migrateLegacyActions } from './siem_legacy_actions/migrate_legacy_actions';
+export { formatLegacyActions } from './siem_legacy_actions/format_legacy_actions';
 export { addGeneratedActionValues } from './add_generated_action_values';
 export { incrementRevision } from './increment_revision';
diff --git a/...et_bulk_rule_actions_saved_object.test.ts → ...acy_actions/format_legacy_actions.test.ts b/...et_bulk_rule_actions_saved_object.test.ts → ...acy_actions/format_legacy_actions.test.ts
@@ -5,23 +5,23 @@
  * 2.0.
  */
 
-import type { SavedObjectsFindOptions, SavedObjectsFindResult } from '@kbn/core/server';
+import type { SavedObjectsFindResult, SavedObjectAttribute } from '@kbn/core/server';
 
 import { loggingSystemMock, savedObjectsClientMock } from '@kbn/core/server/mocks';
 
-// eslint-disable-next-line no-restricted-imports
-import { legacyGetBulkRuleActionsSavedObject } from './legacy_get_bulk_rule_actions_saved_object';
-// eslint-disable-next-line no-restricted-imports
-import type { LegacyRulesActionsSavedObject } from './legacy_get_rule_actions_saved_object';
-// eslint-disable-next-line no-restricted-imports
-import { legacyRuleActionsSavedObjectType } from './legacy_saved_object_mappings';
-// eslint-disable-next-line no-restricted-imports
-import type { LegacyIRuleActionsAttributesSavedObjectAttributes } from './legacy_types';
+import { Rule } from '../../../types';
 
-describe('legacy_get_bulk_rule_actions_saved_object', () => {
+import {
+  legacyGetBulkRuleActionsSavedObject,
+  LegacyActionsObj,
+  formatLegacyActions,
+} from './format_legacy_actions';
+import { legacyRuleActionsSavedObjectType } from './types';
+
+describe('legacyGetBulkRuleActionsSavedObject', () => {
   let savedObjectsClient: ReturnType<typeof savedObjectsClientMock.create>;
   let logger: ReturnType<typeof loggingSystemMock.createLogger>;
-  type FuncReturn = Record<string, LegacyRulesActionsSavedObject>;
+  type FuncReturn = Record<string, LegacyActionsObj>;
 
   beforeEach(() => {
     logger = loggingSystemMock.createLogger();
@@ -34,20 +34,17 @@ describe('legacy_get_bulk_rule_actions_saved_object', () => {
     });
   });
 
-  test('calls "savedObjectsClient.find" with the expected "hasReferences"', () => {
-    legacyGetBulkRuleActionsSavedObject({ alertIds: ['123'], savedObjectsClient, logger });
-    const [[arg1]] = savedObjectsClient.find.mock.calls;
-    expect(arg1).toEqual<SavedObjectsFindOptions>({
+  test('calls "savedObjectsClient.find" with the expected "hasReferences"', async () => {
+    await legacyGetBulkRuleActionsSavedObject({ alertIds: ['123'], savedObjectsClient, logger });
+    expect(savedObjectsClient.find).toHaveBeenCalledWith({
       hasReference: [{ id: '123', type: 'alert' }],
       perPage: 10000,
       type: legacyRuleActionsSavedObjectType,
     });
   });
 
   test('returns nothing transformed through the find if it does not return any matches against the alert id', async () => {
-    const savedObjects: Array<
-      SavedObjectsFindResult<LegacyIRuleActionsAttributesSavedObjectAttributes>
-    > = [];
+    const savedObjects: Array<SavedObjectsFindResult<SavedObjectAttribute>> = [];
     savedObjectsClient.find.mockResolvedValue({
       total: 0,
       per_page: 0,
@@ -64,9 +61,7 @@ describe('legacy_get_bulk_rule_actions_saved_object', () => {
   });
 
   test('returns 1 action transformed through the find if 1 was found for 1 single alert id', async () => {
-    const savedObjects: Array<
-      SavedObjectsFindResult<LegacyIRuleActionsAttributesSavedObjectAttributes>
-    > = [
+    const savedObjects: Array<SavedObjectsFindResult<SavedObjectAttribute>> = [
       {
         score: 0,
         id: '123',
@@ -111,12 +106,15 @@ describe('legacy_get_bulk_rule_actions_saved_object', () => {
     });
     expect(returnValue).toEqual<FuncReturn>({
       'alert-123': {
-        id: '123',
-        alertThrottle: '1d',
         ruleThrottle: '1d',
-        actions: [
+        legacyRuleActions: [
           {
-            action_type_id: 'action_type_1',
+            actionTypeId: 'action_type_1',
+            frequency: {
+              notifyWhen: 'onThrottleInterval',
+              summary: true,
+              throttle: '1d',
+            },
             group: 'group_1',
             id: 'action-123',
             params: {},
@@ -127,9 +125,7 @@ describe('legacy_get_bulk_rule_actions_saved_object', () => {
   });
 
   test('returns 1 action transformed through the find for 2 alerts with 1 action each', async () => {
-    const savedObjects: Array<
-      SavedObjectsFindResult<LegacyIRuleActionsAttributesSavedObjectAttributes>
-    > = [
+    const savedObjects: Array<SavedObjectsFindResult<SavedObjectAttribute>> = [
       {
         score: 0,
         id: '123',
@@ -203,25 +199,32 @@ describe('legacy_get_bulk_rule_actions_saved_object', () => {
     });
     expect(returnValue).toEqual<FuncReturn>({
       'alert-123': {
-        id: '123',
-        alertThrottle: '1d',
         ruleThrottle: '1d',
-        actions: [
+
+        legacyRuleActions: [
           {
-            action_type_id: 'action_type_1',
+            actionTypeId: 'action_type_1',
+            frequency: {
+              notifyWhen: 'onThrottleInterval',
+              summary: true,
+              throttle: '1d',
+            },
             group: 'group_1',
             id: 'action-123',
             params: {},
           },
         ],
       },
       'alert-456': {
-        id: '456',
-        alertThrottle: '1d',
         ruleThrottle: '1d',
-        actions: [
+        legacyRuleActions: [
           {
-            action_type_id: 'action_type_2',
+            actionTypeId: 'action_type_2',
+            frequency: {
+              notifyWhen: 'onThrottleInterval',
+              summary: true,
+              throttle: '1d',
+            },
             group: 'group_2',
             id: 'action-456',
             params: {},
@@ -232,9 +235,7 @@ describe('legacy_get_bulk_rule_actions_saved_object', () => {
   });
 
   test('returns 2 actions transformed through the find if they were found for 1 single alert id', async () => {
-    const savedObjects: Array<
-      SavedObjectsFindResult<LegacyIRuleActionsAttributesSavedObjectAttributes>
-    > = [
+    const savedObjects: Array<SavedObjectsFindResult<SavedObjectAttribute>> = [
       {
         score: 0,
         id: '123',
@@ -290,18 +291,26 @@ describe('legacy_get_bulk_rule_actions_saved_object', () => {
     });
     expect(returnValue).toEqual<FuncReturn>({
       'alert-123': {
-        id: '123',
-        alertThrottle: '1d',
         ruleThrottle: '1d',
-        actions: [
+        legacyRuleActions: [
           {
-            action_type_id: 'action_type_1',
+            actionTypeId: 'action_type_1',
+            frequency: {
+              notifyWhen: 'onThrottleInterval',
+              summary: true,
+              throttle: '1d',
+            },
             group: 'group_1',
             id: 'action-123',
             params: {},
           },
           {
-            action_type_id: 'action_type_2',
+            actionTypeId: 'action_type_2',
+            frequency: {
+              notifyWhen: 'onThrottleInterval',
+              summary: true,
+              throttle: '1d',
+            },
             group: 'group_2',
             id: 'action-456',
             params: {},
@@ -312,9 +321,7 @@ describe('legacy_get_bulk_rule_actions_saved_object', () => {
   });
 
   test('returns only 1 action if for some unusual reason the actions reference is missing an item for 1 single alert id', async () => {
-    const savedObjects: Array<
-      SavedObjectsFindResult<LegacyIRuleActionsAttributesSavedObjectAttributes>
-    > = [
+    const savedObjects: Array<SavedObjectsFindResult<SavedObjectAttribute>> = [
       {
         score: 0,
         id: '123',
@@ -366,12 +373,15 @@ describe('legacy_get_bulk_rule_actions_saved_object', () => {
     });
     expect(returnValue).toEqual<FuncReturn>({
       'alert-123': {
-        id: '123',
-        alertThrottle: '1d',
         ruleThrottle: '1d',
-        actions: [
+        legacyRuleActions: [
           {
-            action_type_id: 'action_type_1',
+            actionTypeId: 'action_type_1',
+            frequency: {
+              notifyWhen: 'onThrottleInterval',
+              summary: true,
+              throttle: '1d',
+            },
             group: 'group_1',
             id: 'action-123',
             params: {},
@@ -382,9 +392,7 @@ describe('legacy_get_bulk_rule_actions_saved_object', () => {
   });
 
   test('returns only 1 action if for some unusual reason the action is missing from the attributes', async () => {
-    const savedObjects: Array<
-      SavedObjectsFindResult<LegacyIRuleActionsAttributesSavedObjectAttributes>
-    > = [
+    const savedObjects: Array<SavedObjectsFindResult<SavedObjectAttribute>> = [
       {
         score: 0,
         id: '123',
@@ -435,12 +443,15 @@ describe('legacy_get_bulk_rule_actions_saved_object', () => {
     });
     expect(returnValue).toEqual<FuncReturn>({
       'alert-123': {
-        id: '123',
-        alertThrottle: '1d',
         ruleThrottle: '1d',
-        actions: [
+        legacyRuleActions: [
           {
-            action_type_id: 'action_type_1',
+            actionTypeId: 'action_type_1',
+            frequency: {
+              notifyWhen: 'onThrottleInterval',
+              summary: true,
+              throttle: '1d',
+            },
             group: 'group_1',
             id: 'action-123',
             params: {},
@@ -451,9 +462,7 @@ describe('legacy_get_bulk_rule_actions_saved_object', () => {
   });
 
   test('returns nothing if the alert id is missing within the references array', async () => {
-    const savedObjects: Array<
-      SavedObjectsFindResult<LegacyIRuleActionsAttributesSavedObjectAttributes>
-    > = [
+    const savedObjects: Array<SavedObjectsFindResult<SavedObjectAttribute>> = [
       {
         score: 0,
         id: '123',
@@ -495,3 +504,112 @@ describe('legacy_get_bulk_rule_actions_saved_object', () => {
     expect(returnValue).toEqual<FuncReturn>({});
   });
 });
+
+describe('formatLegacyActions', () => {
+  let savedObjectsClient: ReturnType<typeof savedObjectsClientMock.create>;
+  let logger: ReturnType<typeof loggingSystemMock.createLogger>;
+
+  beforeEach(() => {
+    logger = loggingSystemMock.createLogger();
+    savedObjectsClient = savedObjectsClientMock.create();
+  });
+
+  it('should return not modified rule when error is thrown within method', async () => {
+    savedObjectsClient.find.mockRejectedValueOnce(new Error('test failure'));
+    const mockRules = [{ id: 'mock-id0' }, { id: 'mock-id1' }] as Rule[];
+    expect(
+      await formatLegacyActions(mockRules, {
+        logger,
+        savedObjectsClient,
+      })
+    ).toEqual(mockRules);
+
+    expect(logger.error).toHaveBeenCalledWith(
+      `formatLegacyActions(): Failed to read legacy actions for SIEM rules mock-id0, mock-id1: test failure`
+    );
+  });
+
+  it('should format rule correctly', async () => {
+    const savedObjects: Array<SavedObjectsFindResult<SavedObjectAttribute>> = [
+      {
+        score: 0,
+        id: '123',
+        type: legacyRuleActionsSavedObjectType,
+        references: [
+          {
+            name: 'alert_0',
+            id: 'alert-123',
+            type: 'alert',
+          },
+          {
+            name: 'action_0',
+            id: 'action-123',
+            type: 'action',
+          },
+        ],
+        attributes: {
+          actions: [
+            {
+              group: 'group_1',
+              params: {},
+              action_type_id: 'action_type_1',
+              actionRef: 'action_0',
+            },
+          ],
+          ruleThrottle: '1d',
+          alertThrottle: '1d',
+        },
+      },
+    ];
+    savedObjectsClient.find.mockResolvedValue({
+      total: 0,
+      per_page: 0,
+      page: 1,
+      saved_objects: savedObjects,
+    });
+
+    const mockRules = [
+      {
+        id: 'alert-123',
+        actions: [
+          {
+            actionTypeId: 'action_type_2',
+            group: 'group_1',
+            id: 'action-456',
+            params: {},
+          },
+        ],
+      },
+    ] as Rule[];
+    const migratedRules = await formatLegacyActions(mockRules, {
+      logger,
+      savedObjectsClient,
+    });
+
+    expect(migratedRules).toEqual([
+      {
+        // actions have been merged
+        actions: [
+          {
+            actionTypeId: 'action_type_2',
+            group: 'group_1',
+            id: 'action-456',
+            params: {},
+          },
+          {
+            actionTypeId: 'action_type_1',
+            frequency: { notifyWhen: 'onThrottleInterval', summary: true, throttle: '1d' },
+            group: 'group_1',
+            id: 'action-123',
+            params: {},
+          },
+        ],
+        id: 'alert-123',
+        // muteAll set to false
+        muteAll: false,
+        notifyWhen: 'onThrottleInterval',
+        throttle: '1d',
+      },
+    ]);
+  });
+});