Skip to content

Commit

Permalink
[Security Solution] Skip isCustomized calculation when the feature fl…
Browse files Browse the repository at this point in the history
…ag is off (elastic#201825)

**Resolves: elastic#201632

## Summary

When the rule customization feature flag is disabled, we should always
return `isCustomized: false`, regardless of any changes introduced to a
rule. This ensures that we do not accidentally mark prebuilt rules as
customized in 8.16 with the feature flag off. For more details, refer to
the related issue: elastic#201632

### Main Changes

- The primary change in this PR is encapsulated in the
`calculateIsCustomized` function
- Other changes involve passing the feature flag to this function
- Added integration tests to cover all API CRUD operations that can be
performed with rules

(cherry picked from commit 22911c1)

# Conflicts:
#	x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/detection_rules_client/detection_rules_client.import_rules.test.ts
#	x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/detection_rules_client/methods/import_rule.ts
#	x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/import/calculate_rule_source_for_import.test.ts
#	x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/import/calculate_rule_source_for_import.ts
#	x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/import/calculate_rule_source_from_asset.test.ts
#	x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/import/calculate_rule_source_from_asset.ts
#	x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/import/rule_source_importer/rule_source_importer.test.ts
#	x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/import/rule_source_importer/rule_source_importer.ts
#	x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/prebuilt_rule_customization/customization_enabled/import_rules.ts
#	x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/prebuilt_rule_customization/customization_enabled/index.ts
  • Loading branch information
xcrzx committed Dec 3, 2024
1 parent 1bd2090 commit cefd75a
Show file tree
Hide file tree
Showing 40 changed files with 1,412 additions and 34 deletions.
3 changes: 2 additions & 1 deletion .buildkite/ftr_security_serverless_configs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ disabled:
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/large_prebuilt_rules_package/trial_license_complete_tier/configs/serverless.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/management/trial_license_complete_tier/configs/serverless.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/update_prebuilt_rules_package/trial_license_complete_tier/configs/serverless.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/prebuilt_rule_customization/trial_license_complete_tier/configs/serverless.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/prebuilt_rule_customization/customization_enabled/configs/serverless.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/prebuilt_rule_customization/customization_disabled/configs/serverless.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_bulk_actions/trial_license_complete_tier/configs/serverless.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_delete/trial_license_complete_tier/configs/serverless.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_delete/basic_license_essentials_tier/configs/serverless.config.ts
Expand Down
3 changes: 2 additions & 1 deletion .buildkite/ftr_security_stateful_configs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ enabled:
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/large_prebuilt_rules_package/trial_license_complete_tier/configs/ess.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/management/trial_license_complete_tier/configs/ess.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/update_prebuilt_rules_package/trial_license_complete_tier/configs/ess.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/prebuilt_rule_customization/trial_license_complete_tier/configs/ess.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/prebuilt_rule_customization/customization_enabled/configs/ess.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/prebuilt_rule_customization/customization_disabled/configs/ess.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_bulk_actions/trial_license_complete_tier/configs/ess.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_delete/trial_license_complete_tier/configs/ess.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_delete/basic_license_essentials_tier/configs/ess.config.ts
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,14 +92,20 @@ export const bulkEditRules = async ({
params: modifiedParams,
};
const ruleResponse = convertAlertingRuleToRuleResponse(updatedRule);
let isCustomized = false;
if (ruleResponse.immutable === true) {
isCustomized = calculateIsCustomized({
baseRule: baseVersionsMap.get(ruleResponse.rule_id),
nextRule: ruleResponse,
isRuleCustomizationEnabled: experimentalFeatures.prebuiltRulesCustomizationEnabled,
});
}

const ruleSource =
ruleResponse.immutable === true
? {
type: 'external' as const,
isCustomized: calculateIsCustomized(
baseVersionsMap.get(ruleResponse.rule_id),
ruleResponse
),
isCustomized,
}
: {
type: 'internal' as const,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ describe('DetectionRulesClient.createCustomRule', () => {
rulesClient,
mlAuthz,
savedObjectsClient,
isRuleCustomizationEnabled: true,
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ describe('DetectionRulesClient.createPrebuiltRule', () => {
rulesClient,
mlAuthz,
savedObjectsClient,
isRuleCustomizationEnabled: true,
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ describe('DetectionRulesClient.deleteRule', () => {
rulesClient,
mlAuthz,
savedObjectsClient,
isRuleCustomizationEnabled: true,
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ describe('DetectionRulesClient.importRule', () => {
rulesClient,
mlAuthz,
savedObjectsClient,
isRuleCustomizationEnabled: true,
});
});

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { rulesClientMock } from '@kbn/alerting-plugin/server/mocks';
import { actionsClientMock } from '@kbn/actions-plugin/server/actions_client/actions_client.mock';
import { savedObjectsClientMock } from '@kbn/core/server/mocks';

import { buildMlAuthz } from '../../../../machine_learning/__mocks__/authz';
import { getImportRulesSchemaMock } from '../../../../../../common/api/detection_engine/rule_management/mocks';
import { getRulesSchemaMock } from '../../../../../../common/api/detection_engine/model/rule_schema/mocks';
import { ruleSourceImporterMock } from '../import/rule_source_importer/rule_source_importer.mock';
import { createDetectionRulesClient } from './detection_rules_client';
import { importRule } from './methods/import_rule';
import { createRuleImportErrorObject } from '../import/errors';
import { checkRuleExceptionReferences } from '../import/check_rule_exception_references';

jest.mock('./methods/import_rule');
jest.mock('../import/check_rule_exception_references');

describe('detectionRulesClient.importRules', () => {
let subject: ReturnType<typeof createDetectionRulesClient>;
let ruleToImport: ReturnType<typeof getImportRulesSchemaMock>;
let mockRuleSourceImporter: ReturnType<typeof ruleSourceImporterMock.create>;

beforeEach(() => {
subject = createDetectionRulesClient({
actionsClient: actionsClientMock.create(),
rulesClient: rulesClientMock.create(),
mlAuthz: buildMlAuthz(),
savedObjectsClient: savedObjectsClientMock.create(),
isRuleCustomizationEnabled: true,
});

(checkRuleExceptionReferences as jest.Mock).mockReturnValue([[], []]);
(importRule as jest.Mock).mockResolvedValue(getRulesSchemaMock());

ruleToImport = getImportRulesSchemaMock();
mockRuleSourceImporter = ruleSourceImporterMock.create();
mockRuleSourceImporter.calculateRuleSource.mockReturnValue({
ruleSource: { type: 'internal' },
immutable: false,
});
});

it('returns imported rules as RuleResponses if import was successful', async () => {
const result = await subject.importRules({
allowMissingConnectorSecrets: false,
overwriteRules: false,
ruleSourceImporter: mockRuleSourceImporter,
rules: [ruleToImport, ruleToImport],
});

expect(result).toEqual([getRulesSchemaMock(), getRulesSchemaMock()]);
});

it('returns an import error if rule import throws an import error', async () => {
const importError = createRuleImportErrorObject({
ruleId: 'rule-id',
message: 'an error occurred',
});
(importRule as jest.Mock).mockReset().mockRejectedValueOnce(importError);

const result = await subject.importRules({
allowMissingConnectorSecrets: false,
overwriteRules: false,
ruleSourceImporter: mockRuleSourceImporter,
rules: [ruleToImport],
});

expect(result).toEqual([importError]);
});

it('returns a generic error if rule import throws unexpectedly', async () => {
const genericError = new Error('an unexpected error occurred');
(importRule as jest.Mock).mockReset().mockRejectedValueOnce(genericError);

const result = await subject.importRules({
allowMissingConnectorSecrets: false,
overwriteRules: false,
ruleSourceImporter: mockRuleSourceImporter,
rules: [ruleToImport],
});

expect(result).toEqual([
expect.objectContaining({
error: expect.objectContaining({
message: 'an unexpected error occurred',
ruleId: ruleToImport.rule_id,
type: 'unknown',
}),
}),
]);
});

describe('when rule has no exception list references', () => {
beforeEach(() => {
(checkRuleExceptionReferences as jest.Mock).mockReset().mockReturnValueOnce([
[
createRuleImportErrorObject({
ruleId: 'rule-id',
message: 'list not found',
}),
],
[],
]);
});

it('returns both exception list reference errors and the imported rule if import succeeds', async () => {
const result = await subject.importRules({
allowMissingConnectorSecrets: false,
overwriteRules: false,
ruleSourceImporter: mockRuleSourceImporter,
rules: [ruleToImport],
});

expect(result).toEqual([
expect.objectContaining({
error: expect.objectContaining({
message: 'list not found',
ruleId: 'rule-id',
type: 'unknown',
}),
}),
getRulesSchemaMock(),
]);
});

it('returns both exception list reference errors and the imported rule if import throws an error', async () => {
const importError = createRuleImportErrorObject({
ruleId: 'rule-id',
message: 'an error occurred',
});
(importRule as jest.Mock).mockReset().mockRejectedValueOnce(importError);

const result = await subject.importRules({
allowMissingConnectorSecrets: false,
overwriteRules: false,
ruleSourceImporter: mockRuleSourceImporter,
rules: [ruleToImport],
});

expect(result).toEqual([
expect.objectContaining({
error: expect.objectContaining({
message: 'list not found',
ruleId: 'rule-id',
type: 'unknown',
}),
}),
expect.objectContaining({
error: expect.objectContaining({
message: 'an error occurred',
ruleId: 'rule-id',
type: 'unknown',
}),
}),
]);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ describe('DetectionRulesClient.patchRule', () => {
rulesClient,
mlAuthz,
savedObjectsClient,
isRuleCustomizationEnabled: true,
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,15 @@ interface DetectionRulesClientParams {
rulesClient: RulesClient;
savedObjectsClient: SavedObjectsClientContract;
mlAuthz: MlAuthz;
isRuleCustomizationEnabled: boolean;
}

export const createDetectionRulesClient = ({
actionsClient,
rulesClient,
mlAuthz,
savedObjectsClient,
isRuleCustomizationEnabled,
}: DetectionRulesClientParams): IDetectionRulesClient => {
const prebuiltRuleAssetClient = createPrebuiltRuleAssetsClient(savedObjectsClient);

Expand Down Expand Up @@ -86,6 +88,7 @@ export const createDetectionRulesClient = ({
prebuiltRuleAssetClient,
mlAuthz,
ruleUpdate,
isRuleCustomizationEnabled,
});
});
},
Expand All @@ -98,6 +101,7 @@ export const createDetectionRulesClient = ({
prebuiltRuleAssetClient,
mlAuthz,
rulePatch,
isRuleCustomizationEnabled,
});
});
},
Expand All @@ -116,6 +120,7 @@ export const createDetectionRulesClient = ({
ruleAsset,
mlAuthz,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled,
});
});
},
Expand All @@ -128,6 +133,7 @@ export const createDetectionRulesClient = ({
importRulePayload: args,
mlAuthz,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled,
});
});
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ describe('DetectionRulesClient.updateRule', () => {
rulesClient,
mlAuthz,
savedObjectsClient,
isRuleCustomizationEnabled: true,
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ describe('DetectionRulesClient.upgradePrebuiltRule', () => {
rulesClient,
mlAuthz,
savedObjectsClient,
isRuleCustomizationEnabled: true,
});
});

Expand Down
Loading

0 comments on commit cefd75a

Please sign in to comment.