diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/patch_rules_bulk_route.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/patch_rules_bulk_route.ts index aeff52f61cfbf..f80a5ffb94e5c 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/patch_rules_bulk_route.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/patch_rules_bulk_route.ts @@ -85,9 +85,9 @@ export const patchRulesBulkRoute = ( }); const rule = await patchRules({ - rule: migratedRule, + existingRule: migratedRule, rulesClient, - params: payloadRule, + nextParams: payloadRule, }); if (rule != null && rule.enabled != null && rule.name != null) { const ruleExecutionSummary = await ruleExecutionLog.getExecutionSummary(rule.id); diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/patch_rules_route.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/patch_rules_route.ts index f6f2ff03efa25..cdd46c2c189d6 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/patch_rules_route.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/patch_rules_route.ts @@ -77,8 +77,8 @@ export const patchRulesRoute = (router: SecuritySolutionPluginRouter, ml: SetupP const rule = await patchRules({ rulesClient, - rule: migratedRule, - params, + existingRule: migratedRule, + nextParams: params, }); if (rule != null && rule.enabled != null && rule.name != null) { const ruleExecutionSummary = await ruleExecutionLog.getExecutionSummary(rule.id); diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/utils/import_rules_utils.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/utils/import_rules_utils.ts index 49d85307cac06..a623f3887276a 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/utils/import_rules_utils.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/utils/import_rules_utils.ts @@ -129,8 +129,8 @@ export const importRules = async ({ }); await patchRules({ rulesClient, - rule: migratedRule, - params: { + existingRule: migratedRule, + nextParams: { ...parsedRule, exceptions_list: [...exceptions], }, diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/rules/patch_rules.test.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/rules/patch_rules.test.ts index d79f36f029a98..e99f7f85df156 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/rules/patch_rules.test.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/rules/patch_rules.test.ts @@ -18,48 +18,48 @@ import { describe('patchRules', () => { it('should call rulesClient.disable if the rule was enabled and enabled is false', async () => { const rulesClient = rulesClientMock.create(); - const params = { + const nextParams = { ...getCreateRulesSchemaMock(), enabled: false, }; - const rule = { + const existingRule = { ...getRuleMock(getQueryRuleParams()), enabled: true, }; rulesClient.update.mockResolvedValue(getRuleMock(getQueryRuleParams())); - await patchRules({ rulesClient, params, rule }); + await patchRules({ rulesClient, nextParams, existingRule }); expect(rulesClient.disable).toHaveBeenCalledWith( expect.objectContaining({ - id: rule.id, + id: existingRule.id, }) ); }); it('should call rulesClient.enable if the rule was disabled and enabled is true', async () => { const rulesClient = rulesClientMock.create(); - const params = { + const nextParams = { ...getCreateRulesSchemaMock(), enabled: true, }; - const rule = { + const existingRule = { ...getRuleMock(getQueryRuleParams()), enabled: false, }; rulesClient.update.mockResolvedValue(getRuleMock(getQueryRuleParams())); - await patchRules({ rulesClient, params, rule }); + await patchRules({ rulesClient, nextParams, existingRule }); expect(rulesClient.enable).toHaveBeenCalledWith( expect.objectContaining({ - id: rule.id, + id: existingRule.id, }) ); }); it('calls the rulesClient with legacy ML params', async () => { const rulesClient = rulesClientMock.create(); - const params = getCreateMachineLearningRulesSchemaMock(); - const rule = getRuleMock(getMlRuleParams()); + const nextParams = getCreateMachineLearningRulesSchemaMock(); + const existingRule = getRuleMock(getMlRuleParams()); rulesClient.update.mockResolvedValue(getRuleMock(getMlRuleParams())); - await patchRules({ rulesClient, params, rule }); + await patchRules({ rulesClient, nextParams, existingRule }); expect(rulesClient.update).toHaveBeenCalledWith( expect.objectContaining({ data: expect.objectContaining({ @@ -74,13 +74,13 @@ describe('patchRules', () => { it('calls the rulesClient with new ML params', async () => { const rulesClient = rulesClientMock.create(); - const params = { + const nextParams = { ...getCreateMachineLearningRulesSchemaMock(), machine_learning_job_id: ['new_job_1', 'new_job_2'], }; - const rule = getRuleMock(getMlRuleParams()); + const existingRule = getRuleMock(getMlRuleParams()); rulesClient.update.mockResolvedValue(getRuleMock(getMlRuleParams())); - await patchRules({ rulesClient, params, rule }); + await patchRules({ rulesClient, nextParams, existingRule }); expect(rulesClient.update).toHaveBeenCalledWith( expect.objectContaining({ data: expect.objectContaining({ @@ -96,7 +96,7 @@ describe('patchRules', () => { describe('regression tests', () => { it("updates the rule's actions if provided", async () => { const rulesClient = rulesClientMock.create(); - const params = { + const nextParams = { ...getCreateRulesSchemaMock(), actions: [ { @@ -109,9 +109,9 @@ describe('patchRules', () => { }, ], }; - const rule = getRuleMock(getQueryRuleParams()); + const existingRule = getRuleMock(getQueryRuleParams()); rulesClient.update.mockResolvedValue(getRuleMock(getQueryRuleParams())); - await patchRules({ rulesClient, params, rule }); + await patchRules({ rulesClient, nextParams, existingRule }); expect(rulesClient.update).toHaveBeenCalledWith( expect.objectContaining({ data: expect.objectContaining({ @@ -132,10 +132,10 @@ describe('patchRules', () => { it('does not update actions if none are specified', async () => { const rulesClient = rulesClientMock.create(); - const params = getCreateRulesSchemaMock(); - delete params.actions; - const rule = getRuleMock(getQueryRuleParams()); - rule.actions = [ + const nextParams = getCreateRulesSchemaMock(); + delete nextParams.actions; + const existingRule = getRuleMock(getQueryRuleParams()); + existingRule.actions = [ { actionTypeId: '.slack', id: '2933e581-d81c-4fe3-88fe-c57c6b8a5bfd', @@ -146,7 +146,7 @@ describe('patchRules', () => { }, ]; rulesClient.update.mockResolvedValue(getRuleMock(getQueryRuleParams())); - await patchRules({ rulesClient, params, rule }); + await patchRules({ rulesClient, nextParams, existingRule }); expect(rulesClient.update).toHaveBeenCalledWith( expect.objectContaining({ data: expect.objectContaining({ diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/rules/patch_rules.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/rules/patch_rules.ts index d6fc0fba71bf6..0a8342ae265a0 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/rules/patch_rules.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/rules/patch_rules.ts @@ -13,39 +13,39 @@ import { convertPatchAPIToInternalSchema } from '../schemas/rule_converters'; export const patchRules = async ({ rulesClient, - rule, - params, + existingRule, + nextParams, }: PatchRulesOptions): Promise | null> => { - if (rule == null) { + if (existingRule == null) { return null; } - const patchedRule = convertPatchAPIToInternalSchema(params, rule); + const patchedRule = convertPatchAPIToInternalSchema(nextParams, existingRule); const update = await rulesClient.update({ - id: rule.id, + id: existingRule.id, data: patchedRule, }); - if (params.throttle !== undefined) { + if (nextParams.throttle !== undefined) { await maybeMute({ rulesClient, - muteAll: rule.muteAll, - throttle: params.throttle, + muteAll: existingRule.muteAll, + throttle: nextParams.throttle, id: update.id, }); } - if (rule.enabled && params.enabled === false) { - await rulesClient.disable({ id: rule.id }); - } else if (!rule.enabled && params.enabled === true) { - await rulesClient.enable({ id: rule.id }); + if (existingRule.enabled && nextParams.enabled === false) { + await rulesClient.disable({ id: existingRule.id }); + } else if (!existingRule.enabled && nextParams.enabled === true) { + await rulesClient.enable({ id: existingRule.id }); } else { // enabled is null or undefined and we do not touch the rule } - if (params.enabled != null) { - return { ...update, enabled: params.enabled }; + if (nextParams.enabled != null) { + return { ...update, enabled: nextParams.enabled }; } else { return update; } diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/rules/types.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/rules/types.ts index 6e3b1a3dea2cb..e02daa7c88c40 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/rules/types.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/rules/types.ts @@ -79,8 +79,8 @@ export interface UpdateRulesOptions { export interface PatchRulesOptions { rulesClient: RulesClient; - params: PatchRulesSchema; - rule: RuleAlertType | null | undefined; + nextParams: PatchRulesSchema; + existingRule: RuleAlertType | null | undefined; } export interface ReadRuleOptions { diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/rules/update_prepacked_rules.test.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/rules/update_prepacked_rules.test.ts index d71274c7f1540..88d543cc77007 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/rules/update_prepacked_rules.test.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/rules/update_prepacked_rules.test.ts @@ -62,7 +62,7 @@ describe('updatePrepackagedRules', () => { expect(patchRules).toHaveBeenCalledWith( expect.objectContaining({ - params: expect.objectContaining({ + nextParams: expect.objectContaining({ actions: undefined, }), }) @@ -70,7 +70,7 @@ describe('updatePrepackagedRules', () => { expect(patchRules).toHaveBeenCalledWith( expect.objectContaining({ - params: expect.objectContaining({ + nextParams: expect.objectContaining({ enabled: undefined, }), }) @@ -99,7 +99,7 @@ describe('updatePrepackagedRules', () => { expect(patchRules).toHaveBeenCalledWith( expect.objectContaining({ - params: expect.objectContaining({ + nextParams: expect.objectContaining({ threat_indicator_path: 'test.path', }), }) @@ -107,7 +107,7 @@ describe('updatePrepackagedRules', () => { expect(patchRules).toHaveBeenCalledWith( expect.objectContaining({ - params: expect.objectContaining({ + nextParams: expect.objectContaining({ threat_index: ['test-index'], }), }) @@ -115,7 +115,7 @@ describe('updatePrepackagedRules', () => { expect(patchRules).toHaveBeenCalledWith( expect.objectContaining({ - params: expect.objectContaining({ + nextParams: expect.objectContaining({ threat_query: 'threat:*', }), }) diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/rules/update_prepacked_rules.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/rules/update_prepacked_rules.ts index 1487aa79e4874..aaee033dd8e96 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/rules/update_prepacked_rules.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/rules/update_prepacked_rules.ts @@ -99,8 +99,8 @@ export const createPromises = ( } else { return patchRules({ rulesClient, - rule: migratedRule, - params: { + existingRule: migratedRule, + nextParams: { ...rule, // Force enabled to use the enabled state from the existing rule by passing in undefined to patchRules enabled: undefined, diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/schemas/rule_converters.test.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/schemas/rule_converters.test.ts index 0a728f9c9e5c8..50cfce7ac905a 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/schemas/rule_converters.test.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/schemas/rule_converters.test.ts @@ -206,13 +206,43 @@ describe('rule_converters', () => { }); describe('convertPatchAPIToInternalSchema', () => { - test('should not update version for immutable rules', () => { - const patchParams = { + test('should set version to one specified in next params for custom rules', () => { + const nextParams = { index: ['new-test-index'], language: 'lucene', + version: 3, }; - const rule = getRuleMock({ ...getQueryRuleParams(), immutable: true }); - const patchedParams = convertPatchAPIToInternalSchema(patchParams, rule); + const existingRule = getRuleMock({ ...getQueryRuleParams(), version: 1 }); + const patchedParams = convertPatchAPIToInternalSchema(nextParams, existingRule); + expect(patchedParams).toEqual( + expect.objectContaining({ + params: expect.objectContaining({ version: 3 }), + }) + ); + }); + + test('should set version to one specified in next params for immutable rules', () => { + const nextParams = { + index: ['new-test-index'], + language: 'lucene', + version: 3, + }; + const existingRule = getRuleMock({ ...getQueryRuleParams(), version: 1, immutable: true }); + const patchedParams = convertPatchAPIToInternalSchema(nextParams, existingRule); + expect(patchedParams).toEqual( + expect.objectContaining({ + params: expect.objectContaining({ version: 3 }), + }) + ); + }); + + test('should not increment version for immutable rules if it is not specified in next params', () => { + const nextParams = { + index: ['new-test-index'], + language: 'lucene', + }; + const existingRule = getRuleMock({ ...getQueryRuleParams(), version: 1, immutable: true }); + const patchedParams = convertPatchAPIToInternalSchema(nextParams, existingRule); expect(patchedParams).toEqual( expect.objectContaining({ params: expect.objectContaining({ version: 1 }), @@ -220,13 +250,13 @@ describe('rule_converters', () => { ); }); - test('should update version for mutable rules', () => { - const patchParams = { + test('should increment version for custom rules if it is not specified in next params', () => { + const nextParams = { index: ['new-test-index'], language: 'lucene', }; - const rule = getRuleMock({ ...getQueryRuleParams() }); - const patchedParams = convertPatchAPIToInternalSchema(patchParams, rule); + const existingRule = getRuleMock({ ...getQueryRuleParams(), version: 1 }); + const patchedParams = convertPatchAPIToInternalSchema(nextParams, existingRule); expect(patchedParams).toEqual( expect.objectContaining({ params: expect.objectContaining({ version: 2 }), @@ -234,14 +264,14 @@ describe('rule_converters', () => { ); }); - test('should not update version due to enabled, id, or rule_id, ', () => { - const patchParams = { + test('should not increment version due to enabled, id, or rule_id, ', () => { + const nextParams = { enabled: false, id: 'some-id', rule_id: 'some-rule-id', }; - const rule = getRuleMock(getQueryRuleParams()); - const patchedParams = convertPatchAPIToInternalSchema(patchParams, rule); + const existingRule = getRuleMock({ ...getQueryRuleParams(), version: 1 }); + const patchedParams = convertPatchAPIToInternalSchema(nextParams, existingRule); expect(patchedParams).toEqual( expect.objectContaining({ params: expect.objectContaining({ version: 1 }), diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/schemas/rule_converters.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/schemas/rule_converters.ts index 74d55c262d17b..59811ac03900d 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/schemas/rule_converters.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/schemas/rule_converters.ts @@ -375,75 +375,87 @@ export const patchTypeSpecificSnakeToCamel = ( }; const versionExcludedKeys = ['enabled', 'id', 'rule_id']; -const shouldUpdateVersion = (params: PatchRulesSchema): boolean => { - for (const key in params) { +const incrementVersion = (nextParams: PatchRulesSchema, existingRule: RuleParams) => { + // The the version from nextParams if it's provided + if (nextParams.version) { + return nextParams.version; + } + + // If the rule is immutable, keep the current version + if (existingRule.immutable) { + return existingRule.version; + } + + // For custom rules, check modified params to deicide whether version increment is needed + for (const key in nextParams) { if (!versionExcludedKeys.includes(key)) { - return true; + return existingRule.version + 1; } } - return false; + return existingRule.version; }; // eslint-disable-next-line complexity export const convertPatchAPIToInternalSchema = ( - params: PatchRulesSchema & { + nextParams: PatchRulesSchema & { related_integrations?: RelatedIntegrationArray; required_fields?: RequiredFieldArray; setup?: SetupGuide; }, existingRule: SanitizedRule ): InternalRuleUpdate => { - const typeSpecificParams = patchTypeSpecificSnakeToCamel(params, existingRule.params); + const typeSpecificParams = patchTypeSpecificSnakeToCamel(nextParams, existingRule.params); const existingParams = existingRule.params; return { - name: params.name ?? existingRule.name, - tags: params.tags ?? existingRule.tags, + name: nextParams.name ?? existingRule.name, + tags: nextParams.tags ?? existingRule.tags, params: { - author: params.author ?? existingParams.author, - buildingBlockType: params.building_block_type ?? existingParams.buildingBlockType, - description: params.description ?? existingParams.description, + author: nextParams.author ?? existingParams.author, + buildingBlockType: nextParams.building_block_type ?? existingParams.buildingBlockType, + description: nextParams.description ?? existingParams.description, ruleId: existingParams.ruleId, - falsePositives: params.false_positives ?? existingParams.falsePositives, - from: params.from ?? existingParams.from, + falsePositives: nextParams.false_positives ?? existingParams.falsePositives, + from: nextParams.from ?? existingParams.from, immutable: existingParams.immutable, - license: params.license ?? existingParams.license, - outputIndex: params.output_index ?? existingParams.outputIndex, - timelineId: params.timeline_id ?? existingParams.timelineId, - timelineTitle: params.timeline_title ?? existingParams.timelineTitle, - meta: params.meta ?? existingParams.meta, - maxSignals: params.max_signals ?? existingParams.maxSignals, - relatedIntegrations: params.related_integrations ?? existingParams.relatedIntegrations, - requiredFields: params.required_fields ?? existingParams.requiredFields, - riskScore: params.risk_score ?? existingParams.riskScore, - riskScoreMapping: params.risk_score_mapping ?? existingParams.riskScoreMapping, - ruleNameOverride: params.rule_name_override ?? existingParams.ruleNameOverride, - setup: params.setup ?? existingParams.setup, - severity: params.severity ?? existingParams.severity, - severityMapping: params.severity_mapping ?? existingParams.severityMapping, - threat: params.threat ?? existingParams.threat, - timestampOverride: params.timestamp_override ?? existingParams.timestampOverride, + license: nextParams.license ?? existingParams.license, + outputIndex: nextParams.output_index ?? existingParams.outputIndex, + timelineId: nextParams.timeline_id ?? existingParams.timelineId, + timelineTitle: nextParams.timeline_title ?? existingParams.timelineTitle, + meta: nextParams.meta ?? existingParams.meta, + maxSignals: nextParams.max_signals ?? existingParams.maxSignals, + relatedIntegrations: nextParams.related_integrations ?? existingParams.relatedIntegrations, + requiredFields: nextParams.required_fields ?? existingParams.requiredFields, + riskScore: nextParams.risk_score ?? existingParams.riskScore, + riskScoreMapping: nextParams.risk_score_mapping ?? existingParams.riskScoreMapping, + ruleNameOverride: nextParams.rule_name_override ?? existingParams.ruleNameOverride, + setup: nextParams.setup ?? existingParams.setup, + severity: nextParams.severity ?? existingParams.severity, + severityMapping: nextParams.severity_mapping ?? existingParams.severityMapping, + threat: nextParams.threat ?? existingParams.threat, + timestampOverride: nextParams.timestamp_override ?? existingParams.timestampOverride, timestampOverrideFallbackDisabled: - params.timestamp_override_fallback_disabled ?? + nextParams.timestamp_override_fallback_disabled ?? existingParams.timestampOverrideFallbackDisabled, - to: params.to ?? existingParams.to, - references: params.references ?? existingParams.references, - namespace: params.namespace ?? existingParams.namespace, - note: params.note ?? existingParams.note, + to: nextParams.to ?? existingParams.to, + references: nextParams.references ?? existingParams.references, + namespace: nextParams.namespace ?? existingParams.namespace, + note: nextParams.note ?? existingParams.note, // Always use the version from the request if specified. If it isn't specified, leave immutable rules alone and // increment the version of mutable rules by 1. - version: - params.version ?? existingParams.immutable - ? existingParams.version - : shouldUpdateVersion(params) - ? existingParams.version + 1 - : existingParams.version, - exceptionsList: params.exceptions_list ?? existingParams.exceptionsList, + version: incrementVersion(nextParams, existingParams), + exceptionsList: nextParams.exceptions_list ?? existingParams.exceptionsList, ...typeSpecificParams, }, - schedule: { interval: params.interval ?? existingRule.schedule.interval }, - actions: params.actions ? params.actions.map(transformRuleToAlertAction) : existingRule.actions, - throttle: params.throttle ? transformToAlertThrottle(params.throttle) : existingRule.throttle, - notifyWhen: params.throttle ? transformToNotifyWhen(params.throttle) : existingRule.notifyWhen, + schedule: { interval: nextParams.interval ?? existingRule.schedule.interval }, + actions: nextParams.actions + ? nextParams.actions.map(transformRuleToAlertAction) + : existingRule.actions, + throttle: nextParams.throttle + ? transformToAlertThrottle(nextParams.throttle) + : existingRule.throttle, + notifyWhen: nextParams.throttle + ? transformToNotifyWhen(nextParams.throttle) + : existingRule.notifyWhen, }; };