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 (#201825)

**Resolves: #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: #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 4, 2024
1 parent 1bd2090 commit 5a5ddd9
Show file tree
Hide file tree
Showing 33 changed files with 586 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
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
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ describe('applyRulePatch', () => {
rulePatch,
existingRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled: true,
});
expect(patchedRule).toEqual(
expect.objectContaining({
Expand Down Expand Up @@ -65,6 +66,7 @@ describe('applyRulePatch', () => {
rulePatch,
existingRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled: true,
});
expect(patchedRule).toEqual(
expect.objectContaining({
Expand Down Expand Up @@ -94,6 +96,7 @@ describe('applyRulePatch', () => {
rulePatch,
existingRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled: true,
})
).rejects.toThrowError(
'event_category_override: Expected string, received number, tiebreaker_field: Expected string, received number, timestamp_field: Expected string, received number'
Expand All @@ -119,6 +122,7 @@ describe('applyRulePatch', () => {
rulePatch,
existingRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled: true,
})
).rejects.toThrowError('alert_suppression.group_by: Expected array, received string');
});
Expand All @@ -134,6 +138,7 @@ describe('applyRulePatch', () => {
rulePatch,
existingRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled: true,
});
expect(patchedRule).toEqual(
expect.objectContaining({
Expand All @@ -154,6 +159,7 @@ describe('applyRulePatch', () => {
rulePatch,
existingRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled: true,
})
).rejects.toThrowError(
'threat_query: Expected string, received number, threat_indicator_path: Expected string, received number'
Expand All @@ -170,6 +176,7 @@ describe('applyRulePatch', () => {
rulePatch,
existingRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled: true,
});
expect(patchedRule).toEqual(
expect.objectContaining({
Expand All @@ -190,6 +197,7 @@ describe('applyRulePatch', () => {
rulePatch,
existingRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled: true,
})
).rejects.toThrowError(
"index.0: Expected string, received number, language: Invalid enum value. Expected 'kuery' | 'lucene', received 'non-language'"
Expand All @@ -206,6 +214,7 @@ describe('applyRulePatch', () => {
rulePatch,
existingRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled: true,
});
expect(patchedRule).toEqual(
expect.objectContaining({
Expand All @@ -226,6 +235,7 @@ describe('applyRulePatch', () => {
rulePatch,
existingRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled: true,
})
).rejects.toThrowError(
"index.0: Expected string, received number, language: Invalid enum value. Expected 'kuery' | 'lucene', received 'non-language'"
Expand All @@ -244,6 +254,7 @@ describe('applyRulePatch', () => {
rulePatch,
existingRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled: true,
});
expect(patchedRule).toEqual(
expect.objectContaining({
Expand All @@ -268,6 +279,7 @@ describe('applyRulePatch', () => {
rulePatch,
existingRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled: true,
})
).rejects.toThrowError('threshold.value: Expected number, received string');
});
Expand All @@ -285,6 +297,7 @@ describe('applyRulePatch', () => {
rulePatch,
existingRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled: true,
});
expect(patchedRule).toEqual(
expect.objectContaining({
Expand All @@ -308,6 +321,7 @@ describe('applyRulePatch', () => {
rulePatch,
existingRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled: true,
});
expect(patchedRule).toEqual(
expect.objectContaining({
Expand All @@ -330,6 +344,7 @@ describe('applyRulePatch', () => {
rulePatch,
existingRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled: true,
});
expect(patchedRule).toEqual(
expect.objectContaining({
Expand All @@ -354,6 +369,7 @@ describe('applyRulePatch', () => {
rulePatch,
existingRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled: true,
});
expect(patchedRule).toEqual(
expect.objectContaining({
Expand All @@ -376,6 +392,7 @@ describe('applyRulePatch', () => {
rulePatch,
existingRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled: true,
});
expect(patchedRule).toEqual(
expect.objectContaining({
Expand All @@ -394,6 +411,7 @@ describe('applyRulePatch', () => {
rulePatch,
existingRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled: true,
})
).rejects.toThrowError('anomaly_threshold: Expected number, received string');
});
Expand All @@ -410,6 +428,7 @@ describe('applyRulePatch', () => {
rulePatch,
existingRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled: true,
});

expect(patchedRule).toEqual(
Expand All @@ -432,6 +451,7 @@ describe('applyRulePatch', () => {
rulePatch,
existingRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled: true,
});
expect(patchedRule).toEqual(
expect.objectContaining({
Expand All @@ -450,6 +470,7 @@ describe('applyRulePatch', () => {
rulePatch,
existingRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled: true,
})
).rejects.toThrowError('new_terms_fields: Expected array, received string');
});
Expand All @@ -472,6 +493,7 @@ describe('applyRulePatch', () => {
rulePatch,
existingRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled: true,
});
expect(patchedRule).toEqual(
expect.objectContaining({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,15 @@ interface ApplyRulePatchProps {
prebuiltRuleAssetClient: IPrebuiltRuleAssetsClient;
existingRule: RuleResponse;
rulePatch: PatchRuleRequestBody;
isRuleCustomizationEnabled: boolean;
}

// eslint-disable-next-line complexity
export const applyRulePatch = async ({
rulePatch,
existingRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled,
}: ApplyRulePatchProps): Promise<RuleResponse> => {
const typeSpecificParams = patchTypeSpecificParams(rulePatch, existingRule);

Expand Down Expand Up @@ -122,6 +124,7 @@ export const applyRulePatch = async ({
nextRule.rule_source = await calculateRuleSource({
rule: nextRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled,
});

return nextRule;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,14 @@ interface ApplyRuleUpdateProps {
prebuiltRuleAssetClient: IPrebuiltRuleAssetsClient;
existingRule: RuleResponse;
ruleUpdate: RuleUpdateProps;
isRuleCustomizationEnabled: boolean;
}

export const applyRuleUpdate = async ({
prebuiltRuleAssetClient,
existingRule,
ruleUpdate,
isRuleCustomizationEnabled,
}: ApplyRuleUpdateProps): Promise<RuleResponse> => {
const nextRule: RuleResponse = {
...applyRuleDefaults(ruleUpdate),
Expand All @@ -46,6 +48,7 @@ export const applyRuleUpdate = async ({
nextRule.rule_source = await calculateRuleSource({
rule: nextRule,
prebuiltRuleAssetClient,
isRuleCustomizationEnabled,
});

return nextRule;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,22 @@ import { calculateRuleFieldsDiff } from '../../../../../prebuilt_rules/logic/dif
import { convertRuleToDiffable } from '../../../../../../../../common/detection_engine/prebuilt_rules/diff/convert_rule_to_diffable';
import { convertPrebuiltRuleAssetToRuleResponse } from '../../converters/convert_prebuilt_rule_asset_to_rule_response';

export function calculateIsCustomized(
baseRule: PrebuiltRuleAsset | undefined,
nextRule: RuleResponse
) {
interface CalculateIsCustomizedArgs {
baseRule: PrebuiltRuleAsset | undefined;
nextRule: RuleResponse;
isRuleCustomizationEnabled: boolean;
}

export function calculateIsCustomized({
baseRule,
nextRule,
isRuleCustomizationEnabled,
}: CalculateIsCustomizedArgs) {
if (!isRuleCustomizationEnabled) {
// We don't want to accidentally mark rules as customized when customization is disabled.
return false;
}

if (baseRule == null) {
// If the base version is missing, we consider the rule to be customized
return true;
Expand Down
Loading

0 comments on commit 5a5ddd9

Please sign in to comment.