[Security Solution] DetectionRulesClient: move public methods out a…

…nd add APM spans (#184820) **Partially addresses: #184364 ## Summary This PR is second step in refactoring our newly added `detectionRulesClient`. Changes in this PR: - every public method was extracted into its own file for readability - `_createRule`, `_updateRule`, `_patchRule` and `_upgradePrebuiltRuleWithTypeChange` private methods were removed, their code inlined into the public methods - `toggleRuleEnabledOnUpdate`, `validateMlAuth` and `ClientError` were moved to `utils.ts` - methods are now wrapped in `withSecuritySpan` to report perf stats to APM - renamed `*.rules_management_client.test.ts` -> `*.detection_rules_client.test.ts` - now using the whole `detectionRulesClient` in tests, not just separate methods - simplified parameters of `createDetectionRulesClient`. Now 2 parameters are needed instead of 5, **DetectionRulesClient method showing up in APM** <img width="918" alt="Schermafbeelding 2024-06-05 om 14 00 36" src="https://github.com/elastic/kibana/assets/15949146/c8b469f7-9d0b-4534-a1c9-f35327ec2c4c"> **Extracted methods** Upon reviewing the private methods in `detection_rules_client.ts`, it became apparent that extracting these methods into separate files may not be the most effective approach to improve readability. The primary reason is that these private methods do not provide clear abstractions, making them difficult to name appropriately. Take `_updateRule` as an example. This method combines an existing rule with a rule update to create an InternalRuleUpdate object, which is then passed to `rulesClient.update`. If we were to extract this into a separate file, we would need to import it for use in the public `updateRule` method. This would result in an `updateRule` method that calls `_updateRule`, creating confusion about what the inner `_updateRule` does. Also, extracting only private methods does not significantly improve readability, as these methods do not contain a large amount of code. So I ended up inlining the code from most of these private methods directly into the public methods.
elastic · Jun 6, 2024 · 4ddec38 · 4ddec38
1 parent 78b31bb
commit 4ddec38
Show file tree

Hide file tree

Showing 19 changed files with 587 additions and 592 deletions.
diff --git a/...etection_engine/rule_management/logic/rule_management/__mocks__/detection_rules_client.ts b/...etection_engine/rule_management/logic/rule_management/__mocks__/detection_rules_client.ts
@@ -5,19 +5,7 @@
  * 2.0.
  */
 
-import type {
-  IDetectionRulesClient,
-  CreateRuleOptions,
-  _UpdateRuleProps,
-  _PatchRuleProps,
-} from '../detection_rules_client';
-import type { RulesClient } from '@kbn/alerting-plugin/server';
-import type {
-  RuleCreateProps,
-  RuleObjectId,
-} from '../../../../../../../common/api/detection_engine';
-import type { PrebuiltRuleAsset } from '../../../../prebuilt_rules';
-import type { RuleAlertType } from '../../../../rule_schema';
+import type { IDetectionRulesClient } from '../detection_rules_client';
 
 export type DetectionRulesClientMock = jest.Mocked<IDetectionRulesClient>;
 
@@ -39,36 +27,3 @@ export const detectionRulesClientMock: {
 } = {
   create: createDetectionRulesClientMock,
 };
-
-/* Mocks for internal methods */
-export const _createRule: jest.Mock<
-  (
-    rulesClient: RulesClient,
-    params: RuleCreateProps,
-    options: CreateRuleOptions
-  ) => Promise<RuleAlertType>
-> = jest.fn();
-
-export const _updateRule: jest.Mock<
-  (rulesClient: RulesClient, updateRulePayload: _UpdateRuleProps) => Promise<RuleAlertType>
-> = jest.fn();
-
-export const patchRuleMock: jest.Mock<
-  (rulesClient: RulesClient, patchRulePayload: _PatchRuleProps) => Promise<RuleAlertType>
-> = jest.fn();
-
-export const _upgradePrebuiltRuleWithTypeChange: jest.Mock<
-  (
-    rulesClient: RulesClient,
-    ruleAsset: PrebuiltRuleAsset,
-    existingRule: RuleAlertType
-  ) => Promise<RuleAlertType>
-> = jest.fn();
-
-export const _toggleRuleEnabledOnUpdate: jest.Mock<
-  (rulesClient: RulesClient, existingRule: RuleAlertType, enabled: boolean) => Promise<void>
-> = jest.fn();
-
-export const _deleteRule: jest.Mock<
-  (rulesClient: RulesClient, deleteRulePayload: { ruleId: RuleObjectId }) => Promise<void>
-> = jest.fn();
diff --git a/...ustom_rule.rule_management_client.test.ts → ...ustom_rule.detection_rules_client.test.ts b/...ustom_rule.rule_management_client.test.ts → ...ustom_rule.detection_rules_client.test.ts
@@ -7,8 +7,6 @@
 
 import { rulesClientMock } from '@kbn/alerting-plugin/server/mocks';
 
-import { createCustomRule } from './detection_rules_client';
-
 import {
   getCreateRulesSchemaMock,
   getCreateMachineLearningRulesSchemaMock,
@@ -17,23 +15,28 @@ import {
 import { DEFAULT_INDICATOR_SOURCE_PATH } from '../../../../../../common/constants';
 import { buildMlAuthz } from '../../../../machine_learning/authz';
 import { throwAuthzError } from '../../../../machine_learning/validation';
+import { createDetectionRulesClient } from './detection_rules_client';
+import type { IDetectionRulesClient } from './detection_rules_client';
 
 jest.mock('../../../../machine_learning/authz');
 jest.mock('../../../../machine_learning/validation');
 
 describe('DetectionRulesClient.createCustomRule', () => {
   let rulesClient: ReturnType<typeof rulesClientMock.create>;
+  let detectionRulesClient: IDetectionRulesClient;
+
   const mlAuthz = (buildMlAuthz as jest.Mock)();
 
   beforeEach(() => {
     jest.resetAllMocks();
     rulesClient = rulesClientMock.create();
+    detectionRulesClient = createDetectionRulesClient(rulesClient, mlAuthz);
   });
 
   it('should create a rule with the correct parameters and options', async () => {
     const params = getCreateRulesSchemaMock();
 
-    await createCustomRule(rulesClient, { params }, mlAuthz);
+    await detectionRulesClient.createCustomRule({ params });
 
     expect(rulesClient.create).toHaveBeenCalledWith(
       expect.objectContaining({
@@ -44,8 +47,6 @@ describe('DetectionRulesClient.createCustomRule', () => {
             immutable: false,
           }),
         }),
-        options: {},
-        allowMissingConnectorSecrets: undefined,
       })
     );
   });
@@ -56,18 +57,16 @@ describe('DetectionRulesClient.createCustomRule', () => {
     });
 
     await expect(
-      createCustomRule(rulesClient, { params: getCreateMachineLearningRulesSchemaMock() }, mlAuthz)
+      detectionRulesClient.createCustomRule({ params: getCreateMachineLearningRulesSchemaMock() })
     ).rejects.toThrow('mocked MLAuth error');
 
     expect(rulesClient.create).not.toHaveBeenCalled();
   });
 
   it('calls the rulesClient with legacy ML params', async () => {
-    await createCustomRule(
-      rulesClient,
-      { params: getCreateMachineLearningRulesSchemaMock() },
-      mlAuthz
-    );
+    await detectionRulesClient.createCustomRule({
+      params: getCreateMachineLearningRulesSchemaMock(),
+    });
 
     expect(rulesClient.create).toHaveBeenCalledWith(
       expect.objectContaining({
@@ -78,23 +77,17 @@ describe('DetectionRulesClient.createCustomRule', () => {
             immutable: false,
           }),
         }),
-        options: {},
-        allowMissingConnectorSecrets: undefined,
       })
     );
   });
 
   it('calls the rulesClient with ML params', async () => {
-    await createCustomRule(
-      rulesClient,
-      {
-        params: {
-          ...getCreateMachineLearningRulesSchemaMock(),
-          machine_learning_job_id: ['new_job_1', 'new_job_2'],
-        },
+    await detectionRulesClient.createCustomRule({
+      params: {
+        ...getCreateMachineLearningRulesSchemaMock(),
+        machine_learning_job_id: ['new_job_1', 'new_job_2'],
       },
-      mlAuthz
-    );
+    });
 
     expect(rulesClient.create).toHaveBeenCalledWith(
       expect.objectContaining({
@@ -105,8 +98,6 @@ describe('DetectionRulesClient.createCustomRule', () => {
             immutable: false,
           }),
         }),
-        options: {},
-        allowMissingConnectorSecrets: undefined,
       })
     );
   });
@@ -115,13 +106,9 @@ describe('DetectionRulesClient.createCustomRule', () => {
     const params = getCreateThreatMatchRulesSchemaMock();
     delete params.threat_indicator_path;
 
-    await createCustomRule(
-      rulesClient,
-      {
-        params,
-      },
-      mlAuthz
-    );
+    await detectionRulesClient.createCustomRule({
+      params,
+    });
 
     expect(rulesClient.create).toHaveBeenCalledWith(
       expect.objectContaining({
@@ -131,14 +118,13 @@ describe('DetectionRulesClient.createCustomRule', () => {
             immutable: false,
           }),
         }),
-        options: {},
-        allowMissingConnectorSecrets: undefined,
       })
     );
   });
 
   it('does not populate a threatIndicatorPath value for other rules if empty', async () => {
-    await createCustomRule(rulesClient, { params: getCreateRulesSchemaMock() }, mlAuthz);
+    await detectionRulesClient.createCustomRule({ params: getCreateRulesSchemaMock() });
+
     expect(rulesClient.create).not.toHaveBeenCalledWith(
       expect.objectContaining({
         data: expect.objectContaining({
@@ -147,8 +133,6 @@ describe('DetectionRulesClient.createCustomRule', () => {
             immutable: false,
           }),
         }),
-        options: {},
-        allowMissingConnectorSecrets: undefined,
       })
     );
   });

diff --git a/...n/server/lib/detection_engine/rule_management/logic/rule_management/create_custom_rule.ts b/...n/server/lib/detection_engine/rule_management/logic/rule_management/create_custom_rule.ts
@@ -0,0 +1,36 @@
+/*
+ * 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 type { RulesClient } from '@kbn/alerting-plugin/server';
+import type { RuleCreateProps } from '../../../../../../common/api/detection_engine';
+import type { MlAuthz } from '../../../../machine_learning/authz';
+import type { RuleAlertType, RuleParams } from '../../../rule_schema';
+import { withSecuritySpan } from '../../../../../utils/with_security_span';
+import { convertCreateAPIToInternalSchema } from '../../normalization/rule_converters';
+
+import { validateMlAuth } from './utils';
+
+export interface CreateCustomRuleProps {
+  params: RuleCreateProps;
+}
+
+export const createCustomRule = async (
+  rulesClient: RulesClient,
+  createCustomRulePayload: CreateCustomRuleProps,
+  mlAuthz: MlAuthz
+): Promise<RuleAlertType> =>
+  withSecuritySpan('DetectionRulesClient.createCustomRule', async () => {
+    const { params } = createCustomRulePayload;
+    await validateMlAuth(mlAuthz, params.type);
+
+    const internalRule = convertCreateAPIToInternalSchema(params, { immutable: false });
+    const rule = await rulesClient.create<RuleParams>({
+      data: internalRule,
+    });
+
+    return rule;
+  });
diff --git a/...built_rule.rule_management_client.test.ts → ...built_rule.detection_rules_client.test.ts b/...built_rule.rule_management_client.test.ts → ...built_rule.detection_rules_client.test.ts
@@ -7,8 +7,6 @@
 
 import { rulesClientMock } from '@kbn/alerting-plugin/server/mocks';
 
-import { createPrebuiltRule } from './detection_rules_client';
-
 import {
   getCreateRulesSchemaMock,
   getCreateMachineLearningRulesSchemaMock,
@@ -17,23 +15,28 @@ import {
 import { DEFAULT_INDICATOR_SOURCE_PATH } from '../../../../../../common/constants';
 import { buildMlAuthz } from '../../../../machine_learning/authz';
 import { throwAuthzError } from '../../../../machine_learning/validation';
+import { createDetectionRulesClient } from './detection_rules_client';
+import type { IDetectionRulesClient } from './detection_rules_client';
 
 jest.mock('../../../../machine_learning/authz');
 jest.mock('../../../../machine_learning/validation');
 
 describe('DetectionRulesClient.createPrebuiltRule', () => {
   let rulesClient: ReturnType<typeof rulesClientMock.create>;
+  let detectionRulesClient: IDetectionRulesClient;
+
   const mlAuthz = (buildMlAuthz as jest.Mock)();
 
   beforeEach(() => {
     jest.resetAllMocks();
     rulesClient = rulesClientMock.create();
+    detectionRulesClient = createDetectionRulesClient(rulesClient, mlAuthz);
   });
 
   it('creates a rule with the correct parameters and options', async () => {
     const ruleAsset = { ...getCreateRulesSchemaMock(), version: 1, rule_id: 'rule-id' };
 
-    await createPrebuiltRule(rulesClient, { ruleAsset }, mlAuthz);
+    await detectionRulesClient.createPrebuiltRule({ ruleAsset });
 
     expect(rulesClient.create).toHaveBeenCalledWith(
       expect.objectContaining({
@@ -45,8 +48,6 @@ describe('DetectionRulesClient.createPrebuiltRule', () => {
             immutable: true,
           }),
         }),
-        options: {},
-        allowMissingConnectorSecrets: undefined,
       })
     );
   });
@@ -57,7 +58,7 @@ describe('DetectionRulesClient.createPrebuiltRule', () => {
       throw new Error('mocked MLAuth error');
     });
 
-    await expect(createPrebuiltRule(rulesClient, { ruleAsset }, mlAuthz)).rejects.toThrow(
+    await expect(detectionRulesClient.createPrebuiltRule({ ruleAsset })).rejects.toThrow(
       'mocked MLAuth error'
     );
 
@@ -70,13 +71,9 @@ describe('DetectionRulesClient.createPrebuiltRule', () => {
       version: 1,
       rule_id: 'rule-id',
     };
-    await createPrebuiltRule(
-      rulesClient,
-      {
-        ruleAsset,
-      },
-      mlAuthz
-    );
+    await detectionRulesClient.createPrebuiltRule({
+      ruleAsset,
+    });
 
     expect(rulesClient.create).toHaveBeenCalledWith(
       expect.objectContaining({
@@ -88,8 +85,6 @@ describe('DetectionRulesClient.createPrebuiltRule', () => {
             immutable: true,
           }),
         }),
-        options: {},
-        allowMissingConnectorSecrets: undefined,
       })
     );
   });
@@ -101,13 +96,9 @@ describe('DetectionRulesClient.createPrebuiltRule', () => {
       version: 1,
       rule_id: 'rule-id',
     };
-    await createPrebuiltRule(
-      rulesClient,
-      {
-        ruleAsset,
-      },
-      mlAuthz
-    );
+    await detectionRulesClient.createPrebuiltRule({
+      ruleAsset,
+    });
 
     expect(rulesClient.create).toHaveBeenCalledWith(
       expect.objectContaining({
@@ -119,8 +110,6 @@ describe('DetectionRulesClient.createPrebuiltRule', () => {
             immutable: true,
           }),
         }),
-        options: {},
-        allowMissingConnectorSecrets: undefined,
       })
     );
   });
@@ -129,13 +118,9 @@ describe('DetectionRulesClient.createPrebuiltRule', () => {
     const ruleAsset = { ...getCreateThreatMatchRulesSchemaMock(), version: 1, rule_id: 'rule-id' };
     delete ruleAsset.threat_indicator_path;
 
-    await createPrebuiltRule(
-      rulesClient,
-      {
-        ruleAsset,
-      },
-      mlAuthz
-    );
+    await detectionRulesClient.createPrebuiltRule({
+      ruleAsset,
+    });
 
     expect(rulesClient.create).toHaveBeenCalledWith(
       expect.objectContaining({
@@ -146,15 +131,13 @@ describe('DetectionRulesClient.createPrebuiltRule', () => {
             immutable: true,
           }),
         }),
-        options: {},
-        allowMissingConnectorSecrets: undefined,
       })
     );
   });
 
   it('does not populate a threatIndicatorPath value for other rules if empty', async () => {
     const ruleAsset = { ...getCreateRulesSchemaMock(), version: 1, rule_id: 'rule-id' };
-    await createPrebuiltRule(rulesClient, { ruleAsset }, mlAuthz);
+    await detectionRulesClient.createPrebuiltRule({ ruleAsset });
     expect(rulesClient.create).not.toHaveBeenCalledWith(
       expect.objectContaining({
         data: expect.objectContaining({
@@ -164,8 +147,6 @@ describe('DetectionRulesClient.createPrebuiltRule', () => {
             immutable: true,
           }),
         }),
-        options: {},
-        allowMissingConnectorSecrets: undefined,
       })
     );
   });