From 8ca90fbfc3f5f201e12053d9675c41b0906b8f9e Mon Sep 17 00:00:00 2001 From: Ievgen Sorokopud Date: Mon, 31 Jul 2023 11:51:38 +0200 Subject: [PATCH] [Security Solution] Legacy actions are deleted when user tries to save a rule and the run action interval is slower than rule run interval (#160798) ## Summary Original ticket: https://github.com/elastic/kibana/issues/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 --- .../migrate_legacy_actions.test.ts | 63 +++++-------------- .../migrate_legacy_actions.ts | 43 ++++++++----- .../retrieve_migrated_legacy_actions.test.ts | 60 ++++++++++++++++-- .../retrieve_migrated_legacy_actions.ts | 59 ++++++++++------- 4 files changed, 132 insertions(+), 93 deletions(-) diff --git a/x-pack/plugins/alerting/server/rules_client/lib/siem_legacy_actions/migrate_legacy_actions.test.ts b/x-pack/plugins/alerting/server/rules_client/lib/siem_legacy_actions/migrate_legacy_actions.test.ts index 50848be43a73f..3e6b010380751 100644 --- a/x-pack/plugins/alerting/server/rules_client/lib/siem_legacy_actions/migrate_legacy_actions.test.ts +++ b/x-pack/plugins/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/x-pack/plugins/alerting/server/rules_client/lib/siem_legacy_actions/migrate_legacy_actions.ts b/x-pack/plugins/alerting/server/rules_client/lib/siem_legacy_actions/migrate_legacy_actions.ts index c092e3fed982b..75bcb39e522b9 100644 --- a/x-pack/plugins/alerting/server/rules_client/lib/siem_legacy_actions/migrate_legacy_actions.ts +++ b/x-pack/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/x-pack/plugins/alerting/server/rules_client/lib/siem_legacy_actions/retrieve_migrated_legacy_actions.test.ts b/x-pack/plugins/alerting/server/rules_client/lib/siem_legacy_actions/retrieve_migrated_legacy_actions.test.ts index b49497d086efe..043754dff226e 100644 --- a/x-pack/plugins/alerting/server/rules_client/lib/siem_legacy_actions/retrieve_migrated_legacy_actions.test.ts +++ b/x-pack/plugins/alerting/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/x-pack/plugins/alerting/server/rules_client/lib/siem_legacy_actions/retrieve_migrated_legacy_actions.ts b/x-pack/plugins/alerting/server/rules_client/lib/siem_legacy_actions/retrieve_migrated_legacy_actions.ts index 83cd315aaa5a5..9678dff6898c8 100644 --- a/x-pack/plugins/alerting/server/rules_client/lib/siem_legacy_actions/retrieve_migrated_legacy_actions.ts +++ b/x-pack/plugins/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 ) => 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: [] };