[Security Solution] Removes Detection Rules version bump logic (#151392)

## Summary Resolves #137168 by removing the logic to bump the `version` of security rules. Also adds an SO migration to set the initial `revision` as the current `version`. Note: this was originally a branched off #147398, so the large commit list is resulting from there as Github doesn't seem to re-write after after a rebase w/ `main` and a force push. ### Checklist ~[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials~ * Found no reference with regards to the auto-incrementing version logic in [current docs](https://www.elastic.co/guide/en/security/current/detection-engine-overview.html). - [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
elastic · Mar 24, 2023 · f1dbb30 · f1dbb30
1 parent bb0f75a
commit f1dbb30
Show file tree

Hide file tree

Showing 16 changed files with 27 additions and 202 deletions.
diff --git a/x-pack/plugins/alerting/server/saved_objects/migrations/8.8/index.ts b/x-pack/plugins/alerting/server/saved_objects/migrations/8.8/index.ts
@@ -8,15 +8,15 @@
 import { SavedObjectUnsanitizedDoc } from '@kbn/core-saved-objects-server';
 import { EncryptedSavedObjectsPluginSetup } from '@kbn/encrypted-saved-objects-plugin/server';
 import { v4 as uuidv4 } from 'uuid';
-import { createEsoMigration, pipeMigrations } from '../utils';
+import { createEsoMigration, isDetectionEngineAADRuleType, pipeMigrations } from '../utils';
 import { RawRule } from '../../../types';
 
 function addRevision(doc: SavedObjectUnsanitizedDoc<RawRule>): SavedObjectUnsanitizedDoc<RawRule> {
   return {
     ...doc,
     attributes: {
       ...doc.attributes,
-      revision: 0,
+      revision: isDetectionEngineAADRuleType(doc) ? (doc.attributes.params.version as number) : 0,
     },
   };
 }

diff --git a/x-pack/plugins/alerting/server/saved_objects/migrations/index.test.ts b/x-pack/plugins/alerting/server/saved_objects/migrations/index.test.ts
@@ -2660,6 +2660,14 @@ describe('successful migrations', () => {
       const migratedAlert880 = migration880(rule, migrationContext);
       expect(migratedAlert880.attributes.revision).toEqual(0);
     });
+
+    test('migrates security rule version to revision', () => {
+      const migration880 = getMigrations(encryptedSavedObjectsSetup, {}, isPreconfigured)['8.8.0'];
+
+      const rule = getMockData({ alertTypeId: ruleTypeMappings.eql, params: { version: 2 } });
+      const migratedAlert880 = migration880(rule, migrationContext);
+      expect(migratedAlert880.attributes.revision).toEqual(2);
+    });
   });
 
   describe('Metrics Inventory Threshold rule', () => {

diff --git a/.../security_solution/server/lib/detection_engine/rule_management/logic/crud/update_rules.ts b/.../security_solution/server/lib/detection_engine/rule_management/logic/crud/update_rules.ts
@@ -66,12 +66,7 @@ export const updateRules = async ({
       references: ruleUpdate.references ?? [],
       namespace: ruleUpdate.namespace,
       note: ruleUpdate.note,
-      // Always use the version from the request if specified. If it isn't specified, leave immutable rules alone and
-      // increment the version of mutable rules by 1.
-      version:
-        ruleUpdate.version ?? existingRule.params.immutable
-          ? existingRule.params.version
-          : existingRule.params.version + 1,
+      version: ruleUpdate.version ?? existingRule.params.version,
       exceptionsList: ruleUpdate.exceptions_list ?? [],
       ...typeSpecificParams,
     },

diff --git a/...olution/server/lib/detection_engine/rule_management/normalization/rule_converters.test.ts b/...olution/server/lib/detection_engine/rule_management/normalization/rule_converters.test.ts
@@ -5,7 +5,7 @@
  * 2.0.
  */
 
-import { convertPatchAPIToInternalSchema, patchTypeSpecificSnakeToCamel } from './rule_converters';
+import { patchTypeSpecificSnakeToCamel } from './rule_converters';
 import {
   getEqlRuleParams,
   getMlRuleParams,
@@ -15,7 +15,6 @@ import {
   getThreatRuleParams,
   getThresholdRuleParams,
 } from '../../rule_schema/mocks';
-import { getRuleMock } from '../../routes/__mocks__/request_responses';
 
 describe('rule_converters', () => {
   describe('patchTypeSpecificSnakeToCamel', () => {
@@ -204,79 +203,4 @@ describe('rule_converters', () => {
       );
     });
   });
-
-  describe('convertPatchAPIToInternalSchema', () => {
-    test('should set version to one specified in next params for custom rules', () => {
-      const nextParams = {
-        index: ['new-test-index'],
-        language: 'lucene',
-        version: 3,
-      };
-      const existingRule = getRuleMock({ ...getQueryRuleParams(), version: 1 });
-      const patchedParams = convertPatchAPIToInternalSchema(nextParams, existingRule);
-      expect(patchedParams).toEqual(
-        expect.objectContaining({
-          params: expect.objectContaining({ version: 3 }),
-        })
-      );
-    });
-
-    test('should set version to one specified in next params for immutable rules', () => {
-      const nextParams = {
-        index: ['new-test-index'],
-        language: 'lucene',
-        version: 3,
-      };
-      const existingRule = getRuleMock({ ...getQueryRuleParams(), version: 1, immutable: true });
-      const patchedParams = convertPatchAPIToInternalSchema(nextParams, existingRule);
-      expect(patchedParams).toEqual(
-        expect.objectContaining({
-          params: expect.objectContaining({ version: 3 }),
-        })
-      );
-    });
-
-    test('should not increment version for immutable rules if it is not specified in next params', () => {
-      const nextParams = {
-        index: ['new-test-index'],
-        language: 'lucene',
-      };
-      const existingRule = getRuleMock({ ...getQueryRuleParams(), version: 1, immutable: true });
-      const patchedParams = convertPatchAPIToInternalSchema(nextParams, existingRule);
-      expect(patchedParams).toEqual(
-        expect.objectContaining({
-          params: expect.objectContaining({ version: 1 }),
-        })
-      );
-    });
-
-    test('should increment version for custom rules if it is not specified in next params', () => {
-      const nextParams = {
-        index: ['new-test-index'],
-        language: 'lucene',
-      };
-      const existingRule = getRuleMock({ ...getQueryRuleParams(), version: 1 });
-      const patchedParams = convertPatchAPIToInternalSchema(nextParams, existingRule);
-      expect(patchedParams).toEqual(
-        expect.objectContaining({
-          params: expect.objectContaining({ version: 2 }),
-        })
-      );
-    });
-
-    test('should not increment version due to enabled, id, or rule_id, ', () => {
-      const nextParams = {
-        enabled: false,
-        id: 'some-id',
-        rule_id: 'some-rule-id',
-      };
-      const existingRule = getRuleMock({ ...getQueryRuleParams(), version: 1 });
-      const patchedParams = convertPatchAPIToInternalSchema(nextParams, existingRule);
-      expect(patchedParams).toEqual(
-        expect.objectContaining({
-          params: expect.objectContaining({ version: 1 }),
-        })
-      );
-    });
-  });
 });
diff --git a/...ity_solution/server/lib/detection_engine/rule_management/normalization/rule_converters.ts b/...ity_solution/server/lib/detection_engine/rule_management/normalization/rule_converters.ts
@@ -390,27 +390,6 @@ export const patchTypeSpecificSnakeToCamel = (
   }
 };
 
-const versionExcludedKeys = ['enabled', 'id', 'rule_id'];
-const incrementVersion = (nextParams: PatchRuleRequestBody, existingRule: RuleParams) => {
-  // The the version from nextParams if it's provided
-  if (nextParams.version) {
-    return nextParams.version;
-  }
-
-  // If the rule is immutable, keep the current version
-  if (existingRule.immutable) {
-    return existingRule.version;
-  }
-
-  // For custom rules, check modified params to deicide whether version increment is needed
-  for (const key in nextParams) {
-    if (!versionExcludedKeys.includes(key)) {
-      return existingRule.version + 1;
-    }
-  }
-  return existingRule.version;
-};
-
 // eslint-disable-next-line complexity
 export const convertPatchAPIToInternalSchema = (
   nextParams: PatchRuleRequestBody & {
@@ -456,9 +435,7 @@ export const convertPatchAPIToInternalSchema = (
       references: nextParams.references ?? existingParams.references,
       namespace: nextParams.namespace ?? existingParams.namespace,
       note: nextParams.note ?? existingParams.note,
-      // Always use the version from the request if specified. If it isn't specified, leave immutable rules alone and
-      // increment the version of mutable rules by 1.
-      version: incrementVersion(nextParams, existingParams),
+      version: nextParams.version ?? existingParams.version,
       exceptionsList: nextParams.exceptions_list ?? existingParams.exceptionsList,
       ...typeSpecificParams,
     },

diff --git a/x-pack/test/detection_engine_api_integration/basic/tests/import_rules.ts b/x-pack/test/detection_engine_api_integration/basic/tests/import_rules.ts
@@ -360,7 +360,6 @@ export default ({ getService }: FtrProviderContext): void => {
           output_index: '',
         };
         ruleOutput.name = 'some other name';
-        ruleOutput.version = 2;
         ruleOutput.revision = 0;
         expect(bodyToCompare).to.eql(ruleOutput);
       });

diff --git a/x-pack/test/detection_engine_api_integration/basic/tests/patch_rules.ts b/x-pack/test/detection_engine_api_integration/basic/tests/patch_rules.ts
@@ -49,7 +49,6 @@ export default ({ getService }: FtrProviderContext) => {
 
         const outputRule = getSimpleRuleOutput();
         outputRule.name = 'some other name';
-        outputRule.version = 2;
         outputRule.revision = 1;
         const bodyToCompare = removeServerGeneratedProperties(body);
         expect(bodyToCompare).to.eql(outputRule);
@@ -86,7 +85,6 @@ export default ({ getService }: FtrProviderContext) => {
 
         const outputRule = getSimpleRuleOutputWithoutRuleId();
         outputRule.name = 'some other name';
-        outputRule.version = 2;
         outputRule.revision = 1;
         const bodyToCompare = removeServerGeneratedPropertiesIncludingRuleId(body);
         expect(bodyToCompare).to.eql(outputRule);
@@ -104,13 +102,12 @@ export default ({ getService }: FtrProviderContext) => {
 
         const outputRule = getSimpleRuleOutput();
         outputRule.name = 'some other name';
-        outputRule.version = 2;
         outputRule.revision = 1;
         const bodyToCompare = removeServerGeneratedProperties(body);
         expect(bodyToCompare).to.eql(outputRule);
       });
 
-      it('should not change the version of a rule when it patches only enabled', async () => {
+      it('should not change the revision of a rule when it patches only enabled', async () => {
         await createRule(supertest, log, getSimpleRule('rule-1'));
 
         // patch a simple rule's enabled to false
@@ -127,7 +124,7 @@ export default ({ getService }: FtrProviderContext) => {
         expect(bodyToCompare).to.eql(outputRule);
       });
 
-      it('should change the version of a rule when it patches enabled and another property', async () => {
+      it('should change the revision of a rule when it patches enabled and another property', async () => {
         await createRule(supertest, log, getSimpleRule('rule-1'));
 
         // patch a simple rule's enabled to false and another property
@@ -140,7 +137,6 @@ export default ({ getService }: FtrProviderContext) => {
         const outputRule = getSimpleRuleOutput();
         outputRule.enabled = false;
         outputRule.severity = 'low';
-        outputRule.version = 2;
         outputRule.revision = 1;
 
         const bodyToCompare = removeServerGeneratedProperties(body);
@@ -168,7 +164,6 @@ export default ({ getService }: FtrProviderContext) => {
         outputRule.name = 'some other name';
         outputRule.timeline_title = 'some title';
         outputRule.timeline_id = 'some id';
-        outputRule.version = 3;
         outputRule.revision = 2;
 
         const bodyToCompare = removeServerGeneratedProperties(body);

diff --git a/x-pack/test/detection_engine_api_integration/basic/tests/patch_rules_bulk.ts b/x-pack/test/detection_engine_api_integration/basic/tests/patch_rules_bulk.ts
@@ -49,7 +49,6 @@ export default ({ getService }: FtrProviderContext) => {
 
         const outputRule = getSimpleRuleOutput();
         outputRule.name = 'some other name';
-        outputRule.version = 2;
         outputRule.revision = 1;
         const bodyToCompare = removeServerGeneratedProperties(body[0]);
         expect(bodyToCompare).to.eql(outputRule);
@@ -71,12 +70,10 @@ export default ({ getService }: FtrProviderContext) => {
 
         const outputRule1 = getSimpleRuleOutput();
         outputRule1.name = 'some other name';
-        outputRule1.version = 2;
         outputRule1.revision = 1;
 
         const outputRule2 = getSimpleRuleOutput('rule-2');
         outputRule2.name = 'some other name';
-        outputRule2.version = 2;
         outputRule2.revision = 1;
 
         const bodyToCompare1 = removeServerGeneratedProperties(body[0]);
@@ -97,7 +94,6 @@ export default ({ getService }: FtrProviderContext) => {
 
         const outputRule = getSimpleRuleOutput();
         outputRule.name = 'some other name';
-        outputRule.version = 2;
         outputRule.revision = 1;
         const bodyToCompare = removeServerGeneratedProperties(body[0]);
         expect(bodyToCompare).to.eql(outputRule);
@@ -119,12 +115,10 @@ export default ({ getService }: FtrProviderContext) => {
 
         const outputRule1 = getSimpleRuleOutputWithoutRuleId('rule-1');
         outputRule1.name = 'some other name';
-        outputRule1.version = 2;
         outputRule1.revision = 1;
 
         const outputRule2 = getSimpleRuleOutputWithoutRuleId('rule-2');
         outputRule2.name = 'some other name';
-        outputRule2.version = 2;
         outputRule2.revision = 1;
 
         const bodyToCompare1 = removeServerGeneratedPropertiesIncludingRuleId(body[0]);
@@ -145,13 +139,12 @@ export default ({ getService }: FtrProviderContext) => {
 
         const outputRule = getSimpleRuleOutput();
         outputRule.name = 'some other name';
-        outputRule.version = 2;
         outputRule.revision = 1;
         const bodyToCompare = removeServerGeneratedProperties(body[0]);
         expect(bodyToCompare).to.eql(outputRule);
       });
 
-      it('should not change the version of a rule when it patches only enabled', async () => {
+      it('should not change the revision of a rule when it patches only enabled', async () => {
         await createRule(supertest, log, getSimpleRule('rule-1'));
 
         // patch a simple rule's enabled to false
@@ -168,7 +161,7 @@ export default ({ getService }: FtrProviderContext) => {
         expect(bodyToCompare).to.eql(outputRule);
       });
 
-      it('should change the version of a rule when it patches enabled and another property', async () => {
+      it('should change the revision of a rule when it patches enabled and another property', async () => {
         await createRule(supertest, log, getSimpleRule('rule-1'));
 
         // patch a simple rule's enabled to false and another property
@@ -181,7 +174,6 @@ export default ({ getService }: FtrProviderContext) => {
         const outputRule = getSimpleRuleOutput();
         outputRule.enabled = false;
         outputRule.severity = 'low';
-        outputRule.version = 2;
         outputRule.revision = 1;
 
         const bodyToCompare = removeServerGeneratedProperties(body[0]);
@@ -209,7 +201,6 @@ export default ({ getService }: FtrProviderContext) => {
         outputRule.name = 'some other name';
         outputRule.timeline_title = 'some title';
         outputRule.timeline_id = 'some id';
-        outputRule.version = 3;
         outputRule.revision = 2;
 
         const bodyToCompare = removeServerGeneratedProperties(body[0]);
@@ -264,7 +255,6 @@ export default ({ getService }: FtrProviderContext) => {
 
         const outputRule = getSimpleRuleOutput();
         outputRule.name = 'some other name';
-        outputRule.version = 2;
         outputRule.revision = 1;
 
         const bodyToCompare = removeServerGeneratedProperties(body[0]);
@@ -295,7 +285,6 @@ export default ({ getService }: FtrProviderContext) => {
 
         const outputRule = getSimpleRuleOutput();
         outputRule.name = 'some other name';
-        outputRule.version = 2;
         outputRule.revision = 1;
 
         const bodyToCompare = removeServerGeneratedProperties(body[0]);

diff --git a/x-pack/test/detection_engine_api_integration/basic/tests/update_rules.ts b/x-pack/test/detection_engine_api_integration/basic/tests/update_rules.ts
@@ -56,7 +56,6 @@ export default ({ getService }: FtrProviderContext) => {
 
         const outputRule = getSimpleRuleOutput();
         outputRule.name = 'some other name';
-        outputRule.version = 2;
         outputRule.revision = 1;
         const bodyToCompare = removeServerGeneratedProperties(body);
         expect(bodyToCompare).to.eql(outputRule);
@@ -102,7 +101,6 @@ export default ({ getService }: FtrProviderContext) => {
 
         const outputRule = getSimpleRuleOutputWithoutRuleId();
         outputRule.name = 'some other name';
-        outputRule.version = 2;
         outputRule.revision = 1;
         const bodyToCompare = removeServerGeneratedPropertiesIncludingRuleId(body);
         expect(bodyToCompare).to.eql(outputRule);
@@ -125,13 +123,12 @@ export default ({ getService }: FtrProviderContext) => {
 
         const outputRule = getSimpleRuleOutput();
         outputRule.name = 'some other name';
-        outputRule.version = 2;
         outputRule.revision = 1;
         const bodyToCompare = removeServerGeneratedProperties(body);
         expect(bodyToCompare).to.eql(outputRule);
       });
 
-      it('should change the version of a rule when it updates enabled and another property', async () => {
+      it('should change the revision of a rule when it updates enabled and another property', async () => {
         await createRule(supertest, log, getSimpleRule('rule-1'));
 
         // update a simple rule's enabled to false and another property
@@ -148,7 +145,6 @@ export default ({ getService }: FtrProviderContext) => {
         const outputRule = getSimpleRuleOutput();
         outputRule.enabled = false;
         outputRule.severity = 'low';
-        outputRule.version = 2;
         outputRule.revision = 1;
 
         const bodyToCompare = removeServerGeneratedProperties(body);
@@ -181,7 +177,6 @@ export default ({ getService }: FtrProviderContext) => {
 
         const outputRule = getSimpleRuleOutput();
         outputRule.name = 'some other name';
-        outputRule.version = 3;
         outputRule.revision = 2;
 
         const bodyToCompare = removeServerGeneratedProperties(body);