[9.0] [Security Solution] Account for missing base rule versions in i…

…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>
elastic · Mar 6, 2025 · 7aef465 · 7aef465
1 parent bbf06ab
commit 7aef465
Show file tree

Hide file tree

Showing 15 changed files with 310 additions and 235 deletions.
diff --git a/...ection_engine/prebuilt_rules/logic/rule_objects/__mocks__/prebuilt_rule_objects_client.ts b/...ection_engine/prebuilt_rules/logic/rule_objects/__mocks__/prebuilt_rule_objects_client.ts
@@ -0,0 +1,15 @@
+/*
+ * 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.
+ */
+
+export const createPrebuiltRuleObjectsClient = () => {
+  return {
+    fetchInstalledRulesByIds: jest.fn(),
+    fetchInstalledRules: jest.fn(),
+    fetchInstalledRuleVersionsByIds: jest.fn(),
+    fetchInstalledRuleVersions: jest.fn(),
+  };
+};
diff --git a/...rity_solution/server/lib/detection_engine/rule_management/api/rules/import_rules/route.ts b/...rity_solution/server/lib/detection_engine/rule_management/api/rules/import_rules/route.ts
@@ -40,6 +40,7 @@ import {
 } from '../../../utils/utils';
 import { RULE_MANAGEMENT_IMPORT_EXPORT_SOCKET_TIMEOUT_MS } from '../../timeouts';
 import { PrebuiltRulesCustomizationDisabledReason } from '../../../../../../../common/detection_engine/prebuilt_rules/prebuilt_rule_customization_status';
+import { createPrebuiltRuleObjectsClient } from '../../../../prebuilt_rules/logic/rule_objects/prebuilt_rule_objects_client';
 
 const CHUNK_PARSED_OBJECT_SIZE = 50;
 
@@ -86,6 +87,7 @@ export const importRulesRoute = (router: SecuritySolutionPluginRouter, config: C
             'licensing',
           ]);
 
+          const rulesClient = await ctx.alerting.getRulesClient();
           const detectionRulesClient = ctx.securitySolution.getDetectionRulesClient();
           const ruleCustomizationStatus = detectionRulesClient.getRuleCustomizationStatus();
           const actionsClient = ctx.actions.getActionsClient();
@@ -159,6 +161,7 @@ export const importRulesRoute = (router: SecuritySolutionPluginRouter, config: C
             config,
             context: ctx.securitySolution,
             prebuiltRuleAssetsClient: createPrebuiltRuleAssetsClient(savedObjectsClient),
+            prebuiltRuleObjectsClient: createPrebuiltRuleObjectsClient(rulesClient),
             ruleCustomizationStatus: detectionRulesClient.getRuleCustomizationStatus(),
           });
 

diff --git a/...olution/server/lib/detection_engine/rule_management/logic/bulk_actions/bulk_edit_rules.ts b/...olution/server/lib/detection_engine/rule_management/logic/bulk_actions/bulk_edit_rules.ts
@@ -70,8 +70,8 @@ export const bulkEditRules = async ({
   const result = await rulesClient.bulkEdit<RuleParams>({
     ids: rules.map((rule) => rule.id),
     operations,
-    paramsModifier: async (rule) => {
-      const ruleParams = rule.params;
+    paramsModifier: async (currentRule) => {
+      const ruleParams = currentRule.params;
 
       await validateBulkEditRule({
         mlAuthz,
@@ -85,23 +85,23 @@ export const bulkEditRules = async ({
         paramsActions
       );
 
-      // Update rule source
-      const updatedRule = {
-        ...rule,
+      const nextRule = convertAlertingRuleToRuleResponse({
+        ...currentRule,
         params: modifiedParams,
-      };
-      const ruleResponse = convertAlertingRuleToRuleResponse(updatedRule);
+      });
+
       let isCustomized = false;
-      if (ruleResponse.immutable === true) {
+      if (nextRule.immutable === true) {
         isCustomized = calculateIsCustomized({
-          baseRule: baseVersionsMap.get(ruleResponse.rule_id),
-          nextRule: ruleResponse,
+          baseRule: baseVersionsMap.get(nextRule.rule_id),
+          currentRule: convertAlertingRuleToRuleResponse(currentRule),
+          nextRule,
           ruleCustomizationStatus,
         });
       }
 
       const ruleSource =
-        ruleResponse.immutable === true
+        nextRule.immutable === true
           ? {
               type: 'external' as const,
               isCustomized,

diff --git a/...detection_engine/rule_management/logic/detection_rules_client/mergers/apply_rule_patch.ts b/...detection_engine/rule_management/logic/detection_rules_client/mergers/apply_rule_patch.ts
@@ -123,7 +123,8 @@ export const applyRulePatch = async ({
   };
 
   nextRule.rule_source = await calculateRuleSource({
-    rule: nextRule,
+    nextRule,
+    currentRule: existingRule,
     prebuiltRuleAssetClient,
     ruleCustomizationStatus,
   });

diff --git a/...etection_engine/rule_management/logic/detection_rules_client/mergers/apply_rule_update.ts b/...etection_engine/rule_management/logic/detection_rules_client/mergers/apply_rule_update.ts
@@ -47,7 +47,8 @@ export const applyRuleUpdate = async ({
   };
 
   nextRule.rule_source = await calculateRuleSource({
-    rule: nextRule,
+    nextRule,
+    currentRule: existingRule,
     prebuiltRuleAssetClient,
     ruleCustomizationStatus,
   });

diff --git a/...le_management/logic/detection_rules_client/mergers/rule_source/calculate_is_customized.ts b/...le_management/logic/detection_rules_client/mergers/rule_source/calculate_is_customized.ts
@@ -19,34 +19,68 @@ import {
 interface CalculateIsCustomizedArgs {
   baseRule: PrebuiltRuleAsset | undefined;
   nextRule: RuleResponse;
+  // Current rule can be undefined in case of importing a prebuilt rule that is not installed
+  currentRule: RuleResponse | undefined;
   ruleCustomizationStatus: PrebuiltRulesCustomizationStatus;
 }
 
 export function calculateIsCustomized({
   baseRule,
   nextRule,
+  currentRule,
   ruleCustomizationStatus,
 }: CalculateIsCustomizedArgs) {
   if (
     ruleCustomizationStatus.customizationDisabledReason ===
     PrebuiltRulesCustomizationDisabledReason.FeatureFlag
   ) {
-    // We don't want to accidentally mark rules as customized when customization is disabled.
+    // 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
+  if (baseRule) {
+    // Base version is available, so we can determine the customization status
+    // by comparing the base version with the next version
+    return areRulesEqual(convertPrebuiltRuleAssetToRuleResponse(baseRule), nextRule) === false;
+  }
+  // Base version is not available, apply a heuristic to determine the
+  // customization status
+
+  if (currentRule == null) {
+    // Current rule is not installed and base rule is not available, so we can't
+    // determine if the rule is customized. Defaulting to false.
+    return false;
+  }
+
+  if (
+    currentRule.rule_source.type === 'external' &&
+    currentRule.rule_source.is_customized === true
+  ) {
+    // If the rule was previously customized, there's no way to determine
+    // whether the customization remained or was reverted. Keeping it as
+    // customized in this case.
     return true;
   }
 
-  const baseRuleWithDefaults = convertPrebuiltRuleAssetToRuleResponse(baseRule);
+  // If the rule has not been customized before, its customization status can be
+  // determined by comparing the current version with the next version.
+  return areRulesEqual(currentRule, nextRule) === false;
+}
 
+/**
+ * A helper function to determine if two rules are equal
+ *
+ * @param ruleA
+ * @param ruleB
+ * @returns true if all rule fields are equal, false otherwise
+ */
+function areRulesEqual(ruleA: RuleResponse, ruleB: RuleResponse) {
   const fieldsDiff = calculateRuleFieldsDiff({
     base_version: MissingVersion,
-    current_version: convertRuleToDiffable(baseRuleWithDefaults),
-    target_version: convertRuleToDiffable(nextRule),
+    current_version: convertRuleToDiffable(ruleA),
+    target_version: convertRuleToDiffable(ruleB),
   });
 
-  return Object.values(fieldsDiff).some((diff) => diff.has_update);
+  return Object.values(fieldsDiff).every((diff) => diff.has_update === false);
 }
diff --git a/...management/logic/detection_rules_client/mergers/rule_source/calculate_rule_source.test.ts b/...management/logic/detection_rules_client/mergers/rule_source/calculate_rule_source.test.ts
@@ -48,7 +48,8 @@ describe('calculateRuleSource', () => {
 
     const result = await calculateRuleSource({
       prebuiltRuleAssetClient,
-      rule,
+      nextRule: rule,
+      currentRule: undefined,
       ruleCustomizationStatus,
     });
     expect(result).toEqual({
@@ -65,7 +66,8 @@ describe('calculateRuleSource', () => {
 
     const result = await calculateRuleSource({
       prebuiltRuleAssetClient,
-      rule,
+      nextRule: rule,
+      currentRule: rule,
       ruleCustomizationStatus,
     });
     expect(result).toEqual(
@@ -86,7 +88,8 @@ describe('calculateRuleSource', () => {
 
     const result = await calculateRuleSource({
       prebuiltRuleAssetClient,
-      rule,
+      nextRule: rule,
+      currentRule: rule,
       ruleCustomizationStatus,
     });
     expect(result).toEqual(
@@ -109,7 +112,8 @@ describe('calculateRuleSource', () => {
 
     const result = await calculateRuleSource({
       prebuiltRuleAssetClient,
-      rule,
+      nextRule: rule,
+      currentRule: rule,
       ruleCustomizationStatus,
     });
     expect(result).toEqual(
@@ -130,7 +134,8 @@ describe('calculateRuleSource', () => {
 
     const result = await calculateRuleSource({
       prebuiltRuleAssetClient,
-      rule,
+      nextRule: rule,
+      currentRule: rule,
       ruleCustomizationStatus: {
         isRulesCustomizationEnabled: false,
         customizationDisabledReason: PrebuiltRulesCustomizationDisabledReason.FeatureFlag,
@@ -154,7 +159,8 @@ describe('calculateRuleSource', () => {
 
     const result = await calculateRuleSource({
       prebuiltRuleAssetClient,
-      rule,
+      nextRule: rule,
+      currentRule: rule,
       ruleCustomizationStatus: {
         isRulesCustomizationEnabled: false,
         customizationDisabledReason: PrebuiltRulesCustomizationDisabledReason.License,
@@ -167,4 +173,107 @@ describe('calculateRuleSource', () => {
       })
     );
   });
+
+  describe('missing base versions', () => {
+    it('return is_customized false when the base version and current version are missing', async () => {
+      const rule = getSampleRule();
+      rule.immutable = true;
+
+      // No base version
+      prebuiltRuleAssetClient.fetchAssetsByVersion.mockResolvedValueOnce([]);
+
+      const result = await calculateRuleSource({
+        prebuiltRuleAssetClient,
+        nextRule: rule,
+        currentRule: undefined,
+        ruleCustomizationStatus,
+      });
+      expect(result).toEqual(
+        expect.objectContaining({
+          type: 'external',
+          is_customized: false,
+        })
+      );
+    });
+
+    it('returns is_customized true when the current version is already customized', async () => {
+      const rule = getSampleRule();
+      rule.immutable = true;
+      rule.rule_source = {
+        type: 'external',
+        is_customized: true,
+      };
+
+      // No base version
+      prebuiltRuleAssetClient.fetchAssetsByVersion.mockResolvedValueOnce([]);
+
+      const result = await calculateRuleSource({
+        prebuiltRuleAssetClient,
+        nextRule: rule,
+        currentRule: rule,
+        ruleCustomizationStatus,
+      });
+      expect(result).toEqual(
+        expect.objectContaining({
+          type: 'external',
+          is_customized: true,
+        })
+      );
+    });
+
+    it('returns is_customized false when the current version is not customized and the next version has no changes', async () => {
+      const rule = getSampleRule();
+      rule.immutable = true;
+      rule.rule_source = {
+        type: 'external',
+        is_customized: false,
+      };
+
+      // No base version
+      prebuiltRuleAssetClient.fetchAssetsByVersion.mockResolvedValueOnce([]);
+
+      const result = await calculateRuleSource({
+        prebuiltRuleAssetClient,
+        nextRule: rule,
+        currentRule: rule,
+        ruleCustomizationStatus,
+      });
+      expect(result).toEqual(
+        expect.objectContaining({
+          type: 'external',
+          is_customized: false,
+        })
+      );
+    });
+
+    it('returns is_customized true when the current version is not customized and the next version has changes', async () => {
+      const rule = getSampleRule();
+      rule.immutable = true;
+      rule.rule_source = {
+        type: 'external',
+        is_customized: false,
+      };
+
+      const nextRule = {
+        ...rule,
+        name: 'Updated name',
+      };
+
+      // No base version
+      prebuiltRuleAssetClient.fetchAssetsByVersion.mockResolvedValueOnce([]);
+
+      const result = await calculateRuleSource({
+        prebuiltRuleAssetClient,
+        nextRule,
+        currentRule: rule,
+        ruleCustomizationStatus,
+      });
+      expect(result).toEqual(
+        expect.objectContaining({
+          type: 'external',
+          is_customized: true,
+        })
+      );
+    });
+  });
 });
diff --git a/...rule_management/logic/detection_rules_client/mergers/rule_source/calculate_rule_source.ts b/...rule_management/logic/detection_rules_client/mergers/rule_source/calculate_rule_source.ts
@@ -16,29 +16,32 @@ import { calculateIsCustomized } from './calculate_is_customized';
 
 interface CalculateRuleSourceProps {
   prebuiltRuleAssetClient: IPrebuiltRuleAssetsClient;
-  rule: RuleResponse;
+  nextRule: RuleResponse;
+  currentRule: RuleResponse | undefined;
   ruleCustomizationStatus: PrebuiltRulesCustomizationStatus;
 }
 
 export async function calculateRuleSource({
   prebuiltRuleAssetClient,
-  rule,
+  nextRule,
+  currentRule,
   ruleCustomizationStatus,
 }: CalculateRuleSourceProps): Promise<RuleSource> {
-  if (rule.immutable) {
+  if (nextRule.immutable) {
     // This is a prebuilt rule and, despite the name, they are not immutable. So
     // we need to recalculate `ruleSource.isCustomized` based on the rule's contents.
     const prebuiltRulesResponse = await prebuiltRuleAssetClient.fetchAssetsByVersion([
       {
-        rule_id: rule.rule_id,
-        version: rule.version,
+        rule_id: nextRule.rule_id,
+        version: nextRule.version,
       },
     ]);
     const baseRule: PrebuiltRuleAsset | undefined = prebuiltRulesResponse.at(0);
 
     const isCustomized = calculateIsCustomized({
       baseRule,
-      nextRule: rule,
+      nextRule,
+      currentRule,
       ruleCustomizationStatus,
     });