[Security Solution] Account for missing base rule versions in is_customized calculation by xcrzx · Pull Request #213250 · elastic/kibana

xcrzx · 2025-03-05T14:40:45Z

Partially addresses: #210358

Summary

Editing of prebuilt rules with missing base versions

When the base version of a currently installed prebuilt rule is missing among the security-rule asset saved objects, and the user edits this rule:

We should mark the rule as customized, only if the new rule settings are different from the current rule settings.
- For example, adding a new tag should mark the rule as customized. Then, if the user removes this tag, the rule should remain to be marked as customized. This matches the current behavior.
- However, if the user saves the rule without making any changes to it, it should keep its is_customized field as is. This is different from the current behavior.

Importing of prebuilt rules with missing base versions

When the base version of a prebuilt rule that is being imported is missing among the security-rule asset saved objects, and the user imports this rule:

If this rule is not installed, it should be created with is_customized field set to false.
If this rule is already installed, it should be updated.
- Its is_customized field should be set to true if the rule from the import payload is not equal to the installed rule.
- Its is_customized field should be be kept unchanged (false or true) if the rule from the import payload is equal to the installed rule.

elasticmachine · 2025-03-06T14:44:07Z

Pinging @elastic/security-detections-response (Team:Detections and Resp)

elasticmachine · 2025-03-06T14:44:08Z

Pinging @elastic/security-solution (Team: SecuritySolution)

elasticmachine · 2025-03-06T14:44:10Z

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

maximpn

@xcrzx Thanks for concise implementation 🙏

I looked through the changes and left non critical comments. Local testing confirmed the changes work as expected.

maximpn · 2025-03-06T17:13:10Z

...le_management/logic/detection_rules_client/mergers/rule_source/calculate_rule_source.test.ts

+      // No base version
+      prebuiltRuleAssetClient.fetchAssetsByVersion.mockResolvedValueOnce([]);


nit: It feels like this line could potentially be forgotten when adding new tests. Maybe move it to beforeEach()?

maximpn · 2025-03-06T17:14:32Z

...ne/rule_management/logic/detection_rules_client/mergers/rule_source/calculate_rule_source.ts

  ruleCustomizationStatus,
 }: CalculateRuleSourceProps): Promise<RuleSource> {
-  if (rule.immutable) {
+  if (nextRule.immutable) {


nit: Inverse logic and early exit will make this function a bit simpler

if (!nextRule.immutable) { return { type: 'internal', }; } ...

maximpn · 2025-03-06T17:19:45Z

...server/lib/detection_engine/rule_management/logic/import/calculate_rule_source_for_import.ts

  prebuiltRuleAssetsByRuleId,
  isKnownPrebuiltRule,


nit: It could be simplified by having only prebuiltRuleAssets: PrebuiltRuleAsset | undefined matching the rule version instead of isKnownPrebuiltRule and prebuiltRuleAssetsByRuleId .

maximpn · 2025-03-06T17:22:14Z

...server/lib/detection_engine/rule_management/logic/import/calculate_rule_source_for_import.ts

 */
 export const calculateRuleSourceForImport = ({
-  rule,
+  importedRule,


nit: It doesn't look like a has been imported when this function is invoked. A name like ruleToImport or beingImportedRule feels like resonating better.

maximpn · 2025-03-06T17:31:25Z

...r/lib/detection_engine/rule_management/logic/import/calculate_rule_source_for_import.test.ts

+    });
+  });
+
+  it('calculates as non modified external type if an asset is found without a matching version and current rule present without changes', () => {


There is a missing scenario covering that is_customized: true is preserved when imported rule matches with the current.

maximpn · 2025-03-06T17:35:08Z

..._management/prebuilt_rules/prebuilt_rule_customization/customization_enabled/import_rules.ts

          rule_id: rule.rule_id,
          version: 9999,
-          rule_source: { type: 'external', is_customized: true },
+          rule_source: { type: 'external', is_customized: false },


nit: It doesn't look like something besides version should be in that assertion.

maximpn · 2025-03-06T17:37:13Z

...b/detection_engine/rule_management/logic/import/rule_source_importer/rule_source_importer.ts

  private latestPackagesInstalled: boolean = false;
  private matchingAssetsByRuleId: Record<string, PrebuiltRuleAsset> = {};
-  private knownRules: RuleSpecifier[] = [];
+  private currentRulesById: Record<string, RuleResponse> = {};


Suggested change

private currentRulesById: Record<string, RuleResponse> = {};

private currentRulesById: Record<RuleSignatureId, RuleResponse> = {};

maximpn · 2025-03-06T17:37:41Z

...b/detection_engine/rule_management/logic/import/rule_source_importer/rule_source_importer.ts

    }, {});
  }

+  private async fetchInstalledRulesByIds(ruleIds: string[]): Promise<Record<string, RuleResponse>> {


nit: Add Map to the name for clarity

Suggested change

private async fetchInstalledRulesByIds(ruleIds: string[]): Promise<Record<string, RuleResponse>> {

private async fetchInstalledRulesMapByIds(ruleIds: string[]): Promise<Record<RuleSignatureId, RuleResponse>> {

kibanamachine · 2025-03-06T18:22:35Z

Starting backport for target branches: 8.18, 8.x, 9.0

https://github.com/elastic/kibana/actions/runs/13705510140

elasticmachine · 2025-03-06T18:23:18Z

💚 Build Succeeded

Buildkite Build
Commit: 24cdb9a

Metrics [docs]

✅ unchanged

History

💔 Build #281444 failed f4f7ff11587a2831612e6ee5b33119ae87c81a57
💔 Build #281129 failed 3d1975509672107343ccf1a0bb00e48326db4fe8

cc @xcrzx

…omized calculation (elastic#213250) **Partially addresses: elastic#210358 ## Summary ### Editing of prebuilt rules with missing base versions **When the base version** of a currently installed prebuilt rule **is missing** among the `security-rule` asset saved objects, and the user edits this rule: - We should mark the rule as customized, only if the new rule settings are different from the current rule settings. - For example, adding a new tag should mark the rule as customized. Then, if the user removes this tag, the rule should remain to be marked as customized. This matches the current behavior. - However, if the user saves the rule without making any changes to it, it should keep its `is_customized` field as is. This is different from the current behavior. ### Importing of prebuilt rules with missing base versions **When the base version** of a prebuilt rule that is being imported **is missing** among the `security-rule` asset saved objects, and the user imports this rule: - If this rule is not installed, it should be created with `is_customized` field set to `false`. - If this rule is already installed, it should be updated. - Its `is_customized` field should be set to `true` if the rule from the import payload is not equal to the installed rule. - Its `is_customized` field should be be kept unchanged (`false` or `true`) if the rule from the import payload is equal to the installed rule. (cherry picked from commit 87e7cd9)

kibanamachine · 2025-03-06T18:28:28Z

💔 Some backports could not be created

Status	Branch	Result
✅	8.18
✅	8.x
❌	9.0	Backport failed because of merge conflicts You might need to backport the following PRs to 9.0: - [Security Solution] Allow prebuilt rules import and export (#212509)

Note: Successful backport PRs will be merged automatically after passing CI.

Manual backport

To create the backport manually run:

node scripts/backport --pr 213250

Questions ?

Please refer to the Backport tool documentation

…omized calculation (elastic#213250) **Partially addresses: elastic#210358 ## Summary ### Editing of prebuilt rules with missing base versions **When the base version** of a currently installed prebuilt rule **is missing** among the `security-rule` asset saved objects, and the user edits this rule: - We should mark the rule as customized, only if the new rule settings are different from the current rule settings. - For example, adding a new tag should mark the rule as customized. Then, if the user removes this tag, the rule should remain to be marked as customized. This matches the current behavior. - However, if the user saves the rule without making any changes to it, it should keep its `is_customized` field as is. This is different from the current behavior. ### Importing of prebuilt rules with missing base versions **When the base version** of a prebuilt rule that is being imported **is missing** among the `security-rule` asset saved objects, and the user imports this rule: - If this rule is not installed, it should be created with `is_customized` field set to `false`. - If this rule is already installed, it should be updated. - Its `is_customized` field should be set to `true` if the rule from the import payload is not equal to the installed rule. - Its `is_customized` field should be be kept unchanged (`false` or `true`) if the rule from the import payload is equal to the installed rule. (cherry picked from commit 87e7cd9)

banderror · 2025-03-06T19:32:35Z

💚 All backports created successfully

Status	Branch	Result
✅	9.0

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…s_customized calculation (#213250) (#213462) # Backport This will backport the following commits from `main` to `8.x`: - [[Security Solution] Account for missing base rule versions in is_customized calculation (#213250)](#213250)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)  Co-authored-by: Dmitrii Shevchenko <dmitrii.shevchenko@elastic.co>

…is_customized calculation (#213250) (#213460) # Backport This will backport the following commits from `main` to `8.18`: - [[Security Solution] Account for missing base rule versions in is_customized calculation (#213250)](#213250)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)  Co-authored-by: Dmitrii Shevchenko <dmitrii.shevchenko@elastic.co>

…s_customized calculation (#213250) (#213466) # Backport This will backport the following commits from `main` to `9.0`: - [[Security Solution] Account for missing base rule versions in is_customized calculation (#213250)](#213250)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)  Co-authored-by: Dmitrii Shevchenko <dmitrii.shevchenko@elastic.co>

…omized calculation (elastic#213250) **Partially addresses: elastic#210358 ## Summary ### Editing of prebuilt rules with missing base versions **When the base version** of a currently installed prebuilt rule **is missing** among the `security-rule` asset saved objects, and the user edits this rule: - We should mark the rule as customized, only if the new rule settings are different from the current rule settings. - For example, adding a new tag should mark the rule as customized. Then, if the user removes this tag, the rule should remain to be marked as customized. This matches the current behavior. - However, if the user saves the rule without making any changes to it, it should keep its `is_customized` field as is. This is different from the current behavior. ### Importing of prebuilt rules with missing base versions **When the base version** of a prebuilt rule that is being imported **is missing** among the `security-rule` asset saved objects, and the user imports this rule: - If this rule is not installed, it should be created with `is_customized` field set to `false`. - If this rule is already installed, it should be updated. - Its `is_customized` field should be set to `true` if the rule from the import payload is not equal to the installed rule. - Its `is_customized` field should be be kept unchanged (`false` or `true`) if the rule from the import payload is equal to the installed rule.

xcrzx self-assigned this Mar 5, 2025

xcrzx force-pushed the import-rule-source branch from 3d19755 to f4f7ff1 Compare March 6, 2025 14:03

xcrzx marked this pull request as ready for review March 6, 2025 14:44

xcrzx requested a review from a team as a code owner March 6, 2025 14:44

xcrzx requested a review from maximpn March 6, 2025 14:44

Account for missing base rule versions in is_customized calculation

24cdb9a

xcrzx force-pushed the import-rule-source branch from f4f7ff1 to 24cdb9a Compare March 6, 2025 16:05

maximpn approved these changes Mar 6, 2025

View reviewed changes

maximpn enabled auto-merge (squash) March 6, 2025 18:01

maximpn merged commit 87e7cd9 into elastic:main Mar 6, 2025
9 checks passed

kibanamachine mentioned this pull request Mar 6, 2025

[8.18] [Security Solution] Account for missing base rule versions in is_customized calculation (#213250) #213460

Merged

kibanamachine mentioned this pull request Mar 6, 2025

[8.x] [Security Solution] Account for missing base rule versions in is_customized calculation (#213250) #213462

Merged

banderror mentioned this pull request Mar 6, 2025

[9.0] [Security Solution] Account for missing base rule versions in is_customized calculation (#213250) #213466

Merged

banderror mentioned this pull request Mar 7, 2025

[Security Solution] Relax the rules of handling missing base versions of prebuilt rules #210358

Closed

dplumlee mentioned this pull request Mar 11, 2025

[Security Solution] Enable prebuilt rules customization feature flag #212761

Merged

		// No base version
		prebuiltRuleAssetClient.fetchAssetsByVersion.mockResolvedValueOnce([]);

	private currentRulesById: Record<string, RuleResponse> = {};
	private currentRulesById: Record<RuleSignatureId, RuleResponse> = {};

	private async fetchInstalledRulesByIds(ruleIds: string[]): Promise<Record<string, RuleResponse>> {
	private async fetchInstalledRulesMapByIds(ruleIds: string[]): Promise<Record<RuleSignatureId, RuleResponse>> {

Conversation

xcrzx commented Mar 5, 2025 • edited by kibanamachine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Editing of prebuilt rules with missing base versions

Importing of prebuilt rules with missing base versions

Uh oh!

elasticmachine commented Mar 6, 2025

Uh oh!

elasticmachine commented Mar 6, 2025

Uh oh!

elasticmachine commented Mar 6, 2025

Uh oh!

maximpn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kibanamachine commented Mar 6, 2025

Uh oh!

elasticmachine commented Mar 6, 2025

💚 Build Succeeded

Metrics [docs]

History

Uh oh!

kibanamachine commented Mar 6, 2025

💔 Some backports could not be created

Manual backport

Questions ?

Uh oh!

banderror commented Mar 6, 2025

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xcrzx commented Mar 5, 2025 •

edited by kibanamachine

Loading