Skip to content

Commit

Permalink
Allow to bump rule version for prebuilt rules (#139287) (#139498)
Browse files Browse the repository at this point in the history 
(cherry picked from commit 7429f78)

Co-authored-by: Dmitrii Shevchenko <[email protected]>
Co-authored-by: Pedro Jaramillo <[email protected]>
  • Loading branch information
@kibanamachine @xcrzx @peluja1012
3 people authored Aug 25, 2022
1 parent 9e319d1 commit 415cecc
Show file tree
Hide file tree
Showing 10 changed files with 150 additions and 108 deletions.
4 changes: 2 additions & 2 deletions ...gins/security_solution/server/lib/detection_engine/routes/rules/patch_rules_bulk_route.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions ...k/plugins/security_solution/server/lib/detection_engine/routes/rules/patch_rules_route.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions ...ns/security_solution/server/lib/detection_engine/routes/rules/utils/import_rules_utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,8 @@ export const importRules = async ({
});
await patchRules({
rulesClient,
rule: migratedRule,
params: {
existingRule: migratedRule,
nextParams: {
...parsedRule,
exceptions_list: [...exceptions],
},
Expand Down
44 changes: 22 additions & 22 deletions x-pack/plugins/security_solution/server/lib/detection_engine/rules/patch_rules.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand All @@ -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({
Expand All @@ -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: [
{
Expand All @@ -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({
Expand All @@ -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',
Expand All @@ -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({
Expand Down
28 changes: 14 additions & 14 deletions x-pack/plugins/security_solution/server/lib/detection_engine/rules/patch_rules.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,39 +13,39 @@ import { convertPatchAPIToInternalSchema } from '../schemas/rule_converters';

export const patchRules = async ({
rulesClient,
rule,
params,
existingRule,
nextParams,
}: PatchRulesOptions): Promise<PartialRule<RuleParams> | 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;
}
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/security_solution/server/lib/detection_engine/rules/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
10 changes: 5 additions & 5 deletions ...lugins/security_solution/server/lib/detection_engine/rules/update_prepacked_rules.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,15 +62,15 @@ describe('updatePrepackagedRules', () => {

expect(patchRules).toHaveBeenCalledWith(
expect.objectContaining({
params: expect.objectContaining({
nextParams: expect.objectContaining({
actions: undefined,
}),
})
);

expect(patchRules).toHaveBeenCalledWith(
expect.objectContaining({
params: expect.objectContaining({
nextParams: expect.objectContaining({
enabled: undefined,
}),
})
Expand Down Expand Up @@ -99,23 +99,23 @@ describe('updatePrepackagedRules', () => {

expect(patchRules).toHaveBeenCalledWith(
expect.objectContaining({
params: expect.objectContaining({
nextParams: expect.objectContaining({
threat_indicator_path: 'test.path',
}),
})
);

expect(patchRules).toHaveBeenCalledWith(
expect.objectContaining({
params: expect.objectContaining({
nextParams: expect.objectContaining({
threat_index: ['test-index'],
}),
})
);

expect(patchRules).toHaveBeenCalledWith(
expect.objectContaining({
params: expect.objectContaining({
nextParams: expect.objectContaining({
threat_query: 'threat:*',
}),
})
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/security_solution/server/lib/detection_engine/rules/update_prepacked_rules.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
54 changes: 42 additions & 12 deletions x-pack/plugins/security_solution/server/lib/detection_engine/schemas/rule_converters.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,42 +206,72 @@ 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 }),
})
);
});

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 }),
})
);
});

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 }),
Expand Down
Loading

0 comments on commit 415cecc

Please sign in to comment.