Skip to content

Commit

Permalink
[Security Solution] Fix prebuilt rule duplication logic to copy relat…
Browse files Browse the repository at this point in the history 
…ed integrations and required fields from the original rule (elastic#191065)

**Fixes: elastic#190628
**Related to:** elastic#173595,
elastic#173594

## Summary

As stated in the bug ticket, when duplicating a prebuilt rule, the
"Related Integrations" and "Required Fields" values should be inherited
from the original rule, as it was specified in the Acceptance Criteria
for elastic#173595 and
elastic#173594.

This PR:

- Removes the logic that resets these fields to empty arrays for
duplicated prebuilt rules - we needed this logic in the past because
these fields were not editable in the UI, but we don't need it anymore.
- Updates the corresponding unit tests.

## Screenshots

These screenshots were taken after introducing the fixes.

**Original prebuilt rule:**

<img width="1463" alt="Screenshot_2024-08-23_at_13_25_07"
src="https://github.com/user-attachments/assets/ad8673f5-aba3-40c8-ae91-bbd7d334b119">

**Duplicated prebuilt rule:**

<img width="1469" alt="Screenshot_2024-08-23_at_13_25_43"
src="https://github.com/user-attachments/assets/03761a2b-6f53-4bab-bf4c-a71c6860802b">

### Checklist

- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [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

(cherry picked from commit b144c05)

# Conflicts:
#	x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/actions/duplicate_rule.test.ts
#	x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/actions/duplicate_rule.ts
  • Loading branch information
@banderror
banderror committed Aug 27, 2024
1 parent 0654db2 commit 3a0dd99
Show file tree
Hide file tree
Showing 2 changed files with 100 additions and 160 deletions.
234 changes: 88 additions & 146 deletions ...solution/server/lib/detection_engine/rule_management/logic/actions/duplicate_rule.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import { v4 as uuidv4 } from 'uuid';
import type { SanitizedRule } from '@kbn/alerting-plugin/common';

import type { RuleParams } from '../../../rule_schema';
import { duplicateRule } from './duplicate_rule';

Expand Down Expand Up @@ -35,13 +36,25 @@ describe('duplicateRule', () => {
meta: undefined,
maxSignals: 100,
responseActions: [],
relatedIntegrations: [],
requiredFields: [],
relatedIntegrations: [
{
package: 'aws',
version: '~1.2.3',
integration: 'route53',
},
],
requiredFields: [
{
name: 'event.action',
type: 'keyword',
ecs: true,
},
],
riskScore: 42,
riskScoreMapping: [],
severity: 'low',
severityMapping: [],
setup: 'Some setup guide.',
setup: `## Config\n\nThe 'Audit Detailed File Share' audit policy must be configured...`,
threat: [],
to: 'now',
references: [],
Expand Down Expand Up @@ -93,151 +106,64 @@ describe('duplicateRule', () => {
jest.clearAllMocks();
});

it('returns an object with fields copied from a given rule', async () => {
const rule = createTestRule();
const result = await duplicateRule({
rule,
});

expect(result).toEqual({
name: expect.anything(), // covered in a separate test
params: {
...rule.params,
ruleSource: {
type: 'internal',
},
ruleId: expect.anything(), // covered in a separate test
},
tags: rule.tags,
alertTypeId: rule.alertTypeId,
consumer: rule.consumer,
schedule: rule.schedule,
actions: rule.actions,
enabled: false, // covered in a separate test
});
});

it('appends [Duplicate] to the name', async () => {
const rule = createTestRule();
rule.name = 'PowerShell Keylogging Script';
const result = await duplicateRule({
rule,
});

expect(result).toEqual(
expect.objectContaining({
name: 'PowerShell Keylogging Script [Duplicate]',
})
);
});

it('generates a new ruleId', async () => {
const rule = createTestRule();
const result = await duplicateRule({
rule,
});

expect(result).toEqual(
expect.objectContaining({
params: expect.objectContaining({
ruleId: 'new ruleId',
}),
})
);
});

it('makes sure the duplicated rule is disabled', async () => {
const rule = createTestRule();
rule.enabled = true;
const result = await duplicateRule({
rule,
});

expect(result).toEqual(
expect.objectContaining({
enabled: false,
})
);
});

describe('when duplicating a prebuilt (immutable) rule', () => {
const createPrebuiltRule = () => {
describe('when duplicating any kind of rule', () => {
it('appends [Duplicate] to the name', async () => {
const rule = createTestRule();
rule.params.immutable = true;
return rule;
};

it('transforms it to a custom (mutable) rule', async () => {
const rule = createPrebuiltRule();
rule.name = 'PowerShell Keylogging Script';
const result = await duplicateRule({
rule,
});

expect(result).toEqual(
expect.objectContaining({
params: expect.objectContaining({
immutable: false,
}),
name: 'PowerShell Keylogging Script [Duplicate]',
})
);
});

it('resets related integrations to an empty array', async () => {
const rule = createPrebuiltRule();
rule.params.relatedIntegrations = [
{
package: 'aws',
version: '~1.2.3',
integration: 'route53',
},
];

it('generates a new ruleId', async () => {
const rule = createTestRule();
const result = await duplicateRule({
rule,
});

expect(result).toEqual(
expect.objectContaining({
params: expect.objectContaining({
relatedIntegrations: [],
ruleId: 'new ruleId',
}),
})
);
});

it('resets required fields to an empty array', async () => {
const rule = createPrebuiltRule();
rule.params.requiredFields = [
{
name: 'event.action',
type: 'keyword',
ecs: true,
},
];

it('makes sure the duplicated rule is disabled', async () => {
const rule = createTestRule();
rule.enabled = true;
const result = await duplicateRule({
rule,
});

expect(result).toEqual(
expect.objectContaining({
params: expect.objectContaining({
requiredFields: [],
}),
enabled: false,
})
);
});
});

describe('when duplicating a custom (mutable) rule', () => {
const createCustomRule = () => {
describe('when duplicating a prebuilt rule', () => {
const createPrebuiltRule = () => {
const rule = createTestRule();
rule.params.immutable = false;
rule.params.immutable = true;
rule.params.ruleSource = {
type: 'external',
isCustomized: false,
};
return rule;
};

it('keeps it custom', async () => {
const rule = createCustomRule();
it('transforms it to a custom rule', async () => {
const rule = createPrebuiltRule();
const result = await duplicateRule({
rule,
});
Expand All @@ -246,71 +172,87 @@ describe('duplicateRule', () => {
expect.objectContaining({
params: expect.objectContaining({
immutable: false,
ruleSource: {
type: 'internal',
},
}),
})
);
});

it('copies related integrations as is', async () => {
const rule = createCustomRule();
rule.params.relatedIntegrations = [
{
package: 'aws',
version: '~1.2.3',
integration: 'route53',
},
];

it('copies fields from the original rule', async () => {
const rule = createPrebuiltRule();
const result = await duplicateRule({
rule,
});

expect(result).toEqual(
expect.objectContaining({
params: expect.objectContaining({
relatedIntegrations: rule.params.relatedIntegrations,
}),
})
);
expect(result).toEqual({
name: expect.anything(), // covered in a separate test
params: {
...rule.params,
ruleId: expect.anything(), // covered in a separate test
immutable: expect.anything(), // covered in a separate test
ruleSource: expect.anything(), // covered in a separate test
},
tags: rule.tags,
alertTypeId: rule.alertTypeId,
consumer: rule.consumer,
schedule: rule.schedule,
actions: rule.actions,
systemActions: rule.actions,
enabled: false, // covered in a separate test
});
});
});

it('copies required fields as is', async () => {
const rule = createCustomRule();
rule.params.requiredFields = [
{
name: 'event.action',
type: 'keyword',
ecs: true,
},
];
describe('when duplicating a custom rule', () => {
const createCustomRule = () => {
const rule = createTestRule();
rule.params.immutable = false;
rule.params.ruleSource = {
type: 'internal',
};
return rule;
};

it('keeps it custom', async () => {
const rule = createCustomRule();
const result = await duplicateRule({
rule,
});

expect(result).toEqual(
expect.objectContaining({
params: expect.objectContaining({
requiredFields: rule.params.requiredFields,
immutable: false,
ruleSource: {
type: 'internal',
},
}),
})
);
});

it('copies setup guide as is', async () => {
it('copies fields from the original rule', async () => {
const rule = createCustomRule();
rule.params.setup = `## Config\n\nThe 'Audit Detailed File Share' audit policy must be configured...`;
const result = await duplicateRule({
rule,
});

expect(result).toEqual(
expect.objectContaining({
params: expect.objectContaining({
setup: rule.params.setup,
}),
})
);
expect(result).toEqual({
name: expect.anything(), // covered in a separate test
params: {
...rule.params,
ruleId: expect.anything(), // covered in a separate test
},
tags: rule.tags,
alertTypeId: rule.alertTypeId,
consumer: rule.consumer,
schedule: rule.schedule,
actions: rule.actions,
systemActions: rule.actions,
enabled: false, // covered in a separate test
});
});
});
});
26 changes: 12 additions & 14 deletions ...rity_solution/server/lib/detection_engine/rule_management/logic/actions/duplicate_rule.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import type { SanitizedRule } from '@kbn/alerting-plugin/common';
import { SERVER_APP_ID } from '../../../../../../common/constants';
import type { InternalRuleCreate, RuleParams } from '../../../rule_schema';
import { transformToActionFrequency } from '../../normalization/rule_actions';
import { convertImmutableToRuleSource } from '../../normalization/rule_converters';

const DUPLICATE_TITLE = i18n.translate(
'xpack.securitySolution.detectionEngine.rules.cloneRule.duplicateTitle',
Expand All @@ -27,17 +26,18 @@ interface DuplicateRuleParams {

export const duplicateRule = async ({ rule }: DuplicateRuleParams): Promise<InternalRuleCreate> => {
// Generate a new static ruleId
const ruleId = uuidv4();

// If it's a prebuilt rule, reset Related Integrations, Required Fields and Setup Guide.
// We do this because for now we don't allow the users to edit these fields for custom rules.
const isPrebuilt = rule.params.immutable;
const relatedIntegrations = isPrebuilt ? [] : rule.params.relatedIntegrations;
const requiredFields = isPrebuilt ? [] : rule.params.requiredFields;
const actions = transformToActionFrequency(rule.actions, rule.throttle);
const ruleId: InternalRuleCreate['params']['ruleId'] = uuidv4();

// Duplicated rules are always considered custom rules
const immutable = false;
const immutable: InternalRuleCreate['params']['immutable'] = false;
const ruleSource: InternalRuleCreate['params']['ruleSource'] = {
type: 'internal',
};

const actions: InternalRuleCreate['actions'] = transformToActionFrequency(
rule.actions,
rule.throttle
);

return {
name: `${rule.name} [${DUPLICATE_TITLE}]`,
Expand All @@ -46,11 +46,9 @@ export const duplicateRule = async ({ rule }: DuplicateRuleParams): Promise<Inte
consumer: SERVER_APP_ID,
params: {
...rule.params,
immutable,
ruleSource: convertImmutableToRuleSource(immutable),
ruleId,
relatedIntegrations,
requiredFields,
immutable,
ruleSource,
exceptionsList: [],
},
schedule: rule.schedule,
Expand Down

0 comments on commit 3a0dd99

Please sign in to comment.