[SIEM][Detection Engine] critical blocker, fixs broken rules versioning

## Summary

* Fixes broken rules versioning
* Fixes unit tests

Implementation is more safe in that it ensures that if you have a factory/immutable rule it will do an early bail out unless the immutable rule is asking for a version change from the file system.

If it's not an immutable it will still _not_ bump the version number if it is a change to the enabled/disabled only.

Testing:
---

* Test that if you enable either a factory or non-factory rule it does not bump the version number. 
* Test that if you go to an immutable rule on the file system and bump the version number on the file system and reinitialize it either through the UI or backend scripts then it will bump the version number.
* Test that if you update a non-factory rule then it does auto-increment the version number.

Things to look out for
---
Did I forget to whitelist anything in the code that would cause it to _not_ bump the version number for some reason?

## Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~- [ ] This was checked for cross-browser compatibility, [including a check against IE11](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility)~~

~~- [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)~~

~~- [ ] [Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials~~

- [x] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios

~~- [ ] This was checked for [keyboard-only and screenreader accessibility](https://developer.mozilla.org/en-US/docs/Learn/Tools_and_testing/Cross_browser_testing/Accessibility#Accessibility_testing_checklist)~~

### For maintainers

~~- [ ] This was checked for breaking API changes and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)~~

- [x] This includes a feature addition or change that requires a release note and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)

x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/update_rules.test.ts

@@ -5,6 +5,7 @@
  */
 
 import { calculateInterval, calculateName, calculateVersion } from './update_rules';
+import { UpdateRuleParams } from './types';
 
 describe('update_rules', () => {
   describe('#calculateInterval', () => {
@@ -25,20 +26,26 @@ describe('update_rules', () => {
   });
 
   describe('#calculateVersion', () => {
-    test('given preVersion and nextVersion numbers being null it will return a 1', () => {
-      expect(calculateVersion(null, null)).toEqual(1);
+    test('returning the same version number if given an immutable but no updated version number', () => {
+      expect(calculateVersion(true, 1, { description: 'some description change' })).toEqual(1);
     });
 
-    test('given preVersion and nextVersion numbers being undefined it will return a 1', () => {
-      expect(calculateVersion(undefined, undefined)).toEqual(1);
+    test('returning an updated version number if given an immutable and an updated version number', () => {
+      expect(calculateVersion(true, 2, { description: 'some description change' })).toEqual(2);
     });
 
-    test('given prevVersion as null and nextVersion being defined, nextVersion will be returned', () => {
-      expect(calculateVersion(undefined, 5)).toEqual(5);
+    test('returning an updated version number if not given an immutable but but an updated description', () => {
+      expect(calculateVersion(false, 1, { description: 'some description change' })).toEqual(2);
     });
 
-    test('given prevVersion as being defined but nextVersion is not, prevVersion will be incremented by 1', () => {
-      expect(calculateVersion(5, undefined)).toEqual(6);
+    test('returning the same version number but a undefined description', () => {
+      expect(calculateVersion(false, 1, { description: undefined })).toEqual(1);
+    });
+
+    test('returning an updated version number if not given an immutable but an updated falsy value', () => {
+      expect(
+        calculateVersion(false, 1, ({ description: false } as unknown) as UpdateRuleParams)
+      ).toEqual(2);
     });
   });
 

x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/update_rules.ts

@@ -4,8 +4,7 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import { defaults } from 'lodash/fp';
-import { AlertAction, IntervalSchedule } from '../../../../../alerting/server/types';
+import { defaults, pickBy, isEmpty } from 'lodash/fp';
 import { readRules } from './read_rules';
 import { UpdateRuleParams, IRuleSavedAttributesSavedObjectAttributes } from './types';
 import { addTags } from './add_tags';
@@ -25,23 +24,37 @@ export const calculateInterval = (
 };
 
 export const calculateVersion = (
-  prevVersion: number | null | undefined,
-  nextVersion: number | null | undefined
-) => {
-  if (nextVersion == null) {
-    if (prevVersion != null) {
-      return prevVersion + 1;
+  immutable: boolean,
+  currentVersion: number,
+  updateProperties: Partial<Omit<UpdateRuleParams, 'enabled' | 'ruleId'>>
+): number => {
+  // early return if we are pre-packaged/immutable rule to be safe. We are never responsible
+  // for changing the version number of an immutable. Immutables are only responsible for changing
+  // their own version number. This would be really bad if an immutable version number is bumped by us
+  // due to a bug, hence the extra check and early bail if that is detected.
+  if (immutable === true) {
+    if (updateProperties.version != null) {
+      // we are an immutable rule but we are asking to update the version number so go ahead
+      // and update it to what is asked.
+      return updateProperties.version;
     } else {
-      // really should never hit this code but to just be
-      // safe let us always check the prev version and if
-      // its null or undefined return a 1
-      return 1;
+      // we are immutable and not asking to update the version number so return the existing version
+      return currentVersion;
     }
+  }
+
+  // white list all properties but the enabled/disabled flag. We don't want to auto-increment
+  // the version number if only the enabled/disabled flag is being set. Likewise if we get other
+  // properties we are not expecting such as updatedAt we do not to cause a version number bump
+  // on that either.
+  const removedNullValues = pickBy<UpdateRuleParams>(
+    (value: unknown) => value != null,
+    updateProperties
+  );
+  if (isEmpty(removedNullValues)) {
+    return currentVersion;
   } else {
-    // The user wants to custom update their version number which
-    // means this could be in the past. Up to the user if they want
-    // to do this
-    return nextVersion;
+    return currentVersion + 1;
   }
 };
 
@@ -101,15 +114,35 @@ export const updateRules = async ({
     return null;
   }
 
-  // TODO: Remove this as cast as soon as rule.actions TypeScript bug is fixed
-  // where it is trying to return AlertAction[] or RawAlertAction[]
-  const actions = (rule.actions as AlertAction[] | undefined) || [];
-
-  const params = rule.params || {};
+  const calculatedVersion = calculateVersion(rule.params.immutable, rule.params.version, {
+    description,
+    falsePositives,
+    query,
+    language,
+    outputIndex,
+    savedId,
+    timelineId,
+    timelineTitle,
+    meta,
+    filters,
+    from,
+    index,
+    interval,
+    maxSignals,
+    riskScore,
+    name,
+    severity,
+    tags,
+    threats,
+    to,
+    type,
+    references,
+    version,
+  });
 
   const nextParams = defaults(
     {
-      ...params,
+      ...rule.params,
     },
     {
       description,
@@ -133,7 +166,7 @@ export const updateRules = async ({
       type,
       updatedAt: new Date().toISOString(),
       references,
-      version: calculateVersion(rule.params.version, version),
+      version: calculatedVersion,
     }
   );
 
@@ -183,16 +216,9 @@ export const updateRules = async ({
       ),
       name: calculateName({ updatedName: name, originalName: rule.name }),
       schedule: {
-        interval: calculateInterval(
-          interval,
-          // TODO: we assume the schedule is an interval schedule due to a problem
-          // in the Alerting api, which should be addressed by the following
-          // issue: https://github.com/elastic/kibana/issues/49703
-          // Once this issue is closed, the type should be correctly returned by alerting
-          (rule.schedule as IntervalSchedule).interval
-        ),
+        interval: calculateInterval(interval, rule.schedule.interval),
       },
-      actions,
+      actions: rule.actions,
       params: nextParams,
     },
   });

x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/signal_rule_alert_type.ts

@@ -264,8 +264,6 @@ export const signalRulesAlertType = ({
             }
           }
         } catch (err) {
-          // TODO: Error handling and writing of errors into a signal that has error
-          // handling/conditions
           logger.error(
             `Error from signal rule name: "${name}", id: "${alertId}", rule_id: "${ruleId}", ${err.message}`
           );