Fix TODOs, add rule converter tests, consistent throw/return error be…

…havior

x-pack/plugins/security_solution/common/detection_engine/schemas/request/add_prepackaged_rules_schema.ts

@@ -17,7 +17,6 @@ import { baseCreateParams, createTypeSpecific } from './rule_schemas';
  *  - rule_id is required here
  *  - version is a required field that must exist
  */
-// TODO: test `version` overwriting `version` from baseParams
 export const addPrepackagedRulesSchema = t.intersection([
   baseCreateParams,
   createTypeSpecific,

x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/patch_rules_route.ts

@@ -29,7 +29,7 @@ export const patchRulesRoute = (router: SecuritySolutionPluginRouter, ml: SetupP
       validate: {
         // Use non-exact validation because everything is optional in patch - since everything is optional,
         // io-ts can't find the right schema from the type specific union and the exact check breaks.
-        // We do type specific exact validation after fetching the existing rule so we know the rule type.
+        // We do type specific validation after fetching the existing rule so we know the rule type.
         body: buildRouteValidationNonExact<typeof patchRulesSchema>(patchRulesSchema),
       },
       options: {

x-pack/plugins/security_solution/server/lib/detection_engine/rules/patch_rules.ts

@@ -5,7 +5,6 @@
  * 2.0.
  */
 
-import { BadRequestError } from '@kbn/securitysolution-es-utils';
 import { PartialRule } from '@kbn/alerting-plugin/server';
 import { RuleParams } from '../schemas/rule_schemas';
 import { PatchRulesOptions } from './types';
@@ -22,10 +21,6 @@ export const patchRules = async ({
   }
 
   const patchedRule = convertPatchAPIToInternalSchema(params, rule);
-  // TODO: consistently throw or return the error - probably throw all the way from convert up to route handler
-  if (patchedRule instanceof BadRequestError) {
-    throw patchedRule;
-  }
 
   const update = await rulesClient.update({
     id: rule.id,

x-pack/plugins/security_solution/server/lib/detection_engine/schemas/rule_converters.test.ts

@@ -0,0 +1,228 @@
+/*
+ * 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.
+ */
+
+import { convertPatchAPIToInternalSchema, patchTypeSpecificSnakeToCamel } from './rule_converters';
+import {
+  getEqlRuleParams,
+  getMlRuleParams,
+  getQueryRuleParams,
+  getSavedQueryRuleParams,
+  getThreatRuleParams,
+  getThresholdRuleParams,
+} from './rule_schemas.mock';
+import { getRuleMock } from '../routes/__mocks__/request_responses';
+
+describe('rule_converters', () => {
+  describe('patchTypeSpecificSnakeToCamel', () => {
+    test('should accept EQL params when existing rule type is EQL', () => {
+      const patchParams = {
+        timestamp_field: 'event.created',
+        event_category_override: 'event.not_category',
+        tiebreaker_field: 'event.created',
+      };
+      const rule = getEqlRuleParams();
+      const patchedParams = patchTypeSpecificSnakeToCamel(patchParams, rule);
+      expect(patchedParams).toEqual(
+        expect.objectContaining({
+          timestampField: 'event.created',
+          eventCategoryOverride: 'event.not_category',
+          tiebreakerField: 'event.created',
+        })
+      );
+    });
+
+    test('should reject invalid EQL params when existing rule type is EQL', () => {
+      const patchParams = {
+        timestamp_field: 1,
+        event_category_override: 1,
+        tiebreaker_field: 1,
+      };
+      const rule = getEqlRuleParams();
+      expect(() => patchTypeSpecificSnakeToCamel(patchParams, rule)).toThrowError(
+        'Invalid value "1" supplied to "timestamp_field",Invalid value "1" supplied to "event_category_override",Invalid value "1" supplied to "tiebreaker_field"'
+      );
+    });
+
+    test('should accept threat match params when existing rule type is threat match', () => {
+      const patchParams = {
+        threat_indicator_path: 'my.indicator',
+        threat_query: 'test-query',
+      };
+      const rule = getThreatRuleParams();
+      const patchedParams = patchTypeSpecificSnakeToCamel(patchParams, rule);
+      expect(patchedParams).toEqual(
+        expect.objectContaining({
+          threatIndicatorPath: 'my.indicator',
+          threatQuery: 'test-query',
+        })
+      );
+    });
+
+    test('should reject invalid threat match params when existing rule type is threat match', () => {
+      const patchParams = {
+        threat_indicator_path: 1,
+        threat_query: 1,
+      };
+      const rule = getThreatRuleParams();
+      expect(() => patchTypeSpecificSnakeToCamel(patchParams, rule)).toThrowError(
+        'Invalid value "1" supplied to "threat_query",Invalid value "1" supplied to "threat_indicator_path"'
+      );
+    });
+
+    test('should accept query params when existing rule type is query', () => {
+      const patchParams = {
+        index: ['new-test-index'],
+        language: 'lucene',
+      };
+      const rule = getQueryRuleParams();
+      const patchedParams = patchTypeSpecificSnakeToCamel(patchParams, rule);
+      expect(patchedParams).toEqual(
+        expect.objectContaining({
+          index: ['new-test-index'],
+          language: 'lucene',
+        })
+      );
+    });
+
+    test('should reject invalid query params when existing rule type is query', () => {
+      const patchParams = {
+        index: [1],
+        language: 'non-language',
+      };
+      const rule = getQueryRuleParams();
+      expect(() => patchTypeSpecificSnakeToCamel(patchParams, rule)).toThrowError(
+        'Invalid value "1" supplied to "index",Invalid value "non-language" supplied to "language"'
+      );
+    });
+
+    test('should accept saved query params when existing rule type is saved query', () => {
+      const patchParams = {
+        index: ['new-test-index'],
+        language: 'lucene',
+      };
+      const rule = getSavedQueryRuleParams();
+      const patchedParams = patchTypeSpecificSnakeToCamel(patchParams, rule);
+      expect(patchedParams).toEqual(
+        expect.objectContaining({
+          index: ['new-test-index'],
+          language: 'lucene',
+        })
+      );
+    });
+
+    test('should reject invalid saved query params when existing rule type is saved query', () => {
+      const patchParams = {
+        index: [1],
+        language: 'non-language',
+      };
+      const rule = getSavedQueryRuleParams();
+      expect(() => patchTypeSpecificSnakeToCamel(patchParams, rule)).toThrowError(
+        'Invalid value "1" supplied to "index",Invalid value "non-language" supplied to "language"'
+      );
+    });
+
+    test('should accept threshold params when existing rule type is threshold', () => {
+      const patchParams = {
+        threshold: {
+          field: ['host.name'],
+          value: 107,
+        },
+      };
+      const rule = getThresholdRuleParams();
+      const patchedParams = patchTypeSpecificSnakeToCamel(patchParams, rule);
+      expect(patchedParams).toEqual(
+        expect.objectContaining({
+          threshold: {
+            field: ['host.name'],
+            value: 107,
+          },
+        })
+      );
+    });
+
+    test('should reject invalid threshold params when existing rule type is threshold', () => {
+      const patchParams = {
+        threshold: {
+          field: ['host.name'],
+          value: 'invalid',
+        },
+      };
+      const rule = getThresholdRuleParams();
+      expect(() => patchTypeSpecificSnakeToCamel(patchParams, rule)).toThrowError(
+        'Invalid value "invalid" supplied to "threshold,value"'
+      );
+    });
+
+    test('should accept machine learning params when existing rule type is machine learning', () => {
+      const patchParams = {
+        anomaly_threshold: 5,
+      };
+      const rule = getMlRuleParams();
+      const patchedParams = patchTypeSpecificSnakeToCamel(patchParams, rule);
+      expect(patchedParams).toEqual(
+        expect.objectContaining({
+          anomalyThreshold: 5,
+        })
+      );
+    });
+
+    test('should reject invalid machine learning params when existing rule type is machine learning', () => {
+      const patchParams = {
+        anomaly_threshold: 'invalid',
+      };
+      const rule = getMlRuleParams();
+      expect(() => patchTypeSpecificSnakeToCamel(patchParams, rule)).toThrowError(
+        'Invalid value "invalid" supplied to "anomaly_threshold"'
+      );
+    });
+  });
+
+  describe('convertPatchAPIToInternalSchema', () => {
+    test('should not update version for immutable rules', () => {
+      const patchParams = {
+        index: ['new-test-index'],
+        language: 'lucene',
+      };
+      const rule = getRuleMock({ ...getQueryRuleParams(), immutable: true });
+      const patchedParams = convertPatchAPIToInternalSchema(patchParams, rule);
+      expect(patchedParams).toEqual(
+        expect.objectContaining({
+          params: expect.objectContaining({ version: 1 }),
+        })
+      );
+    });
+
+    test('should update version for mutable rules', () => {
+      const patchParams = {
+        index: ['new-test-index'],
+        language: 'lucene',
+      };
+      const rule = getRuleMock({ ...getQueryRuleParams() });
+      const patchedParams = convertPatchAPIToInternalSchema(patchParams, rule);
+      expect(patchedParams).toEqual(
+        expect.objectContaining({
+          params: expect.objectContaining({ version: 2 }),
+        })
+      );
+    });
+
+    test('should not update version due to enabled, id, or rule_id, ', () => {
+      const patchParams = {
+        enabled: false,
+        id: 'some-id',
+        rule_id: 'some-rule-id',
+      };
+      const rule = getRuleMock(getQueryRuleParams());
+      const patchedParams = convertPatchAPIToInternalSchema(patchParams, rule);
+      expect(patchedParams).toEqual(
+        expect.objectContaining({
+          params: expect.objectContaining({ version: 1 }),
+        })
+      );
+    });
+  });
+});

x-pack/plugins/security_solution/server/lib/detection_engine/schemas/rule_converters.ts

@@ -262,19 +262,18 @@ const patchMachineLearningParams = (
   };
 };
 
-export const parseValidationError = (error: string | null): BadRequestError => {
+const parseValidationError = (error: string | null): BadRequestError => {
   if (error != null) {
     return new BadRequestError(error);
   } else {
     return new BadRequestError('unknown validation error');
   }
 };
 
-// TODO: unit test this function to make sure schemas pass validation as expected
 export const patchTypeSpecificSnakeToCamel = (
   params: PatchRulesSchema,
   existingRule: RuleParams
-): TypeSpecificRuleParams | BadRequestError => {
+): TypeSpecificRuleParams => {
   // Here we do the validation of patch params by rule type to ensure that the fields that are
   // passed in to patch are of the correct type, e.g. `query` is a string. Since the combined patch schema
   // is a union of types where everything is optional, it's hard to do the validation before we know the rule type -
@@ -284,42 +283,42 @@ export const patchTypeSpecificSnakeToCamel = (
     case 'eql': {
       const [validated, error] = validateNonExact(params, eqlPatchParams);
       if (validated == null) {
-        return parseValidationError(error);
+        throw parseValidationError(error);
       }
       return patchEqlParams(validated, existingRule);
     }
     case 'threat_match': {
       const [validated, error] = validateNonExact(params, threatMatchPatchParams);
       if (validated == null) {
-        return parseValidationError(error);
+        throw parseValidationError(error);
       }
       return patchThreatMatchParams(validated, existingRule);
     }
     case 'query': {
       const [validated, error] = validateNonExact(params, queryPatchParams);
       if (validated == null) {
-        return parseValidationError(error);
+        throw parseValidationError(error);
       }
       return patchQueryParams(validated, existingRule);
     }
     case 'saved_query': {
       const [validated, error] = validateNonExact(params, savedQueryPatchParams);
       if (validated == null) {
-        return parseValidationError(error);
+        throw parseValidationError(error);
       }
       return patchSavedQueryParams(validated, existingRule);
     }
     case 'threshold': {
       const [validated, error] = validateNonExact(params, thresholdPatchParams);
       if (validated == null) {
-        return parseValidationError(error);
+        throw parseValidationError(error);
       }
       return patchThresholdParams(validated, existingRule);
     }
     case 'machine_learning': {
       const [validated, error] = validateNonExact(params, machineLearningPatchParams);
       if (validated == null) {
-        return parseValidationError(error);
+        throw parseValidationError(error);
       }
       return patchMachineLearningParams(validated, existingRule);
     }
@@ -339,16 +338,12 @@ const shouldUpdateVersion = (params: PatchRulesSchema): boolean => {
   return false;
 };
 
-// TODO: tests to ensure version gets updated as expected
 // eslint-disable-next-line complexity
 export const convertPatchAPIToInternalSchema = (
   params: PatchRulesSchema,
   existingRule: SanitizedRule<RuleParams>
-): InternalRuleUpdate | BadRequestError => {
+): InternalRuleUpdate => {
   const typeSpecificParams = patchTypeSpecificSnakeToCamel(params, existingRule.params);
-  if (typeSpecificParams instanceof BadRequestError) {
-    return typeSpecificParams;
-  }
   const existingParams = existingRule.params;
   return {
     name: params.name ?? existingRule.name,

x-pack/plugins/security_solution/server/lib/detection_engine/schemas/rule_schemas.mock.ts

@@ -15,6 +15,7 @@ import {
   MachineLearningRuleParams,
   QueryRuleParams,
   RuleParams,
+  SavedQueryRuleParams,
   ThreatRuleParams,
   ThresholdRuleParams,
 } from './rule_schemas';
@@ -125,6 +126,27 @@ export const getQueryRuleParams = (): QueryRuleParams => {
   };
 };
 
+export const getSavedQueryRuleParams = (): SavedQueryRuleParams => {
+  return {
+    ...getBaseRuleParams(),
+    type: 'saved_query',
+    language: 'kuery',
+    query: 'user.name: root or user.name: admin',
+    index: ['auditbeat-*', 'filebeat-*', 'packetbeat-*', 'winlogbeat-*'],
+    dataViewId: undefined,
+    filters: [
+      {
+        query: {
+          match_phrase: {
+            'host.name': 'some-host',
+          },
+        },
+      },
+    ],
+    savedId: 'some-id',
+  };
+};
+
 export const getThreatRuleParams = (): ThreatRuleParams => {
   return {
     ...getBaseRuleParams(),