Addressing PR comments.

elastic · Jul 24, 2024 · 524e08d · 524e08d
1 parent 5419937
commit 524e08d
Show file tree

Hide file tree

Showing 14 changed files with 51 additions and 41 deletions.
diff --git a/...tion/rule/methods/disable/disable.test.ts → ...rule/methods/disable/disable_rule.test.ts b/...tion/rule/methods/disable/disable.test.ts → ...rule/methods/disable/disable_rule.test.ts
@@ -107,7 +107,7 @@ beforeEach(() => {
 
 setGlobalDate();
 
-describe('disable()', () => {
+describe('disableRule()', () => {
   let rulesClient: RulesClient;
   const existingRule = {
     id: '1',
@@ -158,7 +158,7 @@ describe('disable()', () => {
 
   describe('authorization', () => {
     test('ensures user is authorised to disable this type of rule under the consumer', async () => {
-      await rulesClient.disable({ id: '1' });
+      await rulesClient.disableRule({ id: '1' });
 
       expect(authorization.ensureAuthorized).toHaveBeenCalledWith({
         entity: 'rule',
@@ -173,7 +173,7 @@ describe('disable()', () => {
         new Error(`Unauthorized to disable a "myType" alert for "myApp"`)
       );
 
-      await expect(rulesClient.disable({ id: '1' })).rejects.toMatchInlineSnapshot(
+      await expect(rulesClient.disableRule({ id: '1' })).rejects.toMatchInlineSnapshot(
         `[Error: Unauthorized to disable a "myType" alert for "myApp"]`
       );
 
@@ -188,7 +188,7 @@ describe('disable()', () => {
 
   describe('auditLogger', () => {
     test('logs audit event when disabling a rule', async () => {
-      await rulesClient.disable({ id: '1' });
+      await rulesClient.disableRule({ id: '1' });
       expect(auditLogger.log).toHaveBeenCalledWith(
         expect.objectContaining({
           event: expect.objectContaining({
@@ -203,7 +203,7 @@ describe('disable()', () => {
     test('logs audit event when not authorised to disable a rule', async () => {
       authorization.ensureAuthorized.mockRejectedValue(new Error('Unauthorized'));
 
-      await expect(rulesClient.disable({ id: '1' })).rejects.toThrow();
+      await expect(rulesClient.disableRule({ id: '1' })).rejects.toThrow();
       expect(auditLogger.log).toHaveBeenCalledWith(
         expect.objectContaining({
           event: expect.objectContaining({
@@ -226,7 +226,7 @@ describe('disable()', () => {
   });
 
   test('disables a rule', async () => {
-    await rulesClient.disable({ id: '1' });
+    await rulesClient.disableRule({ id: '1' });
     expect(unsecuredSavedObjectsClient.get).not.toHaveBeenCalled();
     expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith(
       RULE_SAVED_OBJECT_TYPE,
@@ -304,7 +304,7 @@ describe('disable()', () => {
       },
       ownerId: null,
     });
-    await rulesClient.disable({ id: '1', untrack: true });
+    await rulesClient.disableRule({ id: '1', untrack: true });
     expect(unsecuredSavedObjectsClient.get).not.toHaveBeenCalled();
     expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith(
       RULE_SAVED_OBJECT_TYPE,
@@ -393,7 +393,7 @@ describe('disable()', () => {
 
   test('disables the rule even if unable to retrieve task manager doc to generate untrack event log events', async () => {
     taskManager.get.mockRejectedValueOnce(new Error('Fail'));
-    await rulesClient.disable({ id: '1', untrack: true });
+    await rulesClient.disableRule({ id: '1', untrack: true });
     expect(unsecuredSavedObjectsClient.get).not.toHaveBeenCalled();
     expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith(
       RULE_SAVED_OBJECT_TYPE,
@@ -441,12 +441,12 @@ describe('disable()', () => {
 
     expect(eventLogger.logEvent).toHaveBeenCalledTimes(0);
     expect(rulesClientParams.logger.warn).toHaveBeenCalledWith(
-      `rulesClient.disable('1') - Could not write untrack events - Fail`
+      `rulesClient.disableRule('1') - Could not write untrack events - Fail`
     );
   });
 
   test('should not untrack rule alert if untrack is false', async () => {
-    await rulesClient.disable({ id: '1', untrack: false });
+    await rulesClient.disableRule({ id: '1', untrack: false });
     expect(unsecuredSavedObjectsClient.get).not.toHaveBeenCalled();
     expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith(
       RULE_SAVED_OBJECT_TYPE,
@@ -496,7 +496,7 @@ describe('disable()', () => {
 
   test('falls back when getDecryptedAsInternalUser throws an error', async () => {
     encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValueOnce(new Error('Fail'));
-    await rulesClient.disable({ id: '1' });
+    await rulesClient.disableRule({ id: '1' });
     expect(unsecuredSavedObjectsClient.get).toHaveBeenCalledWith(RULE_SAVED_OBJECT_TYPE, '1');
     expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith(
       RULE_SAVED_OBJECT_TYPE,
@@ -548,7 +548,7 @@ describe('disable()', () => {
       },
     });
 
-    await rulesClient.disable({ id: '1' });
+    await rulesClient.disableRule({ id: '1' });
     expect(unsecuredSavedObjectsClient.update).not.toHaveBeenCalled();
     expect(taskManager.bulkDisable).not.toHaveBeenCalled();
     expect(taskManager.removeIfExists).not.toHaveBeenCalledWith();
@@ -557,7 +557,7 @@ describe('disable()', () => {
   test('swallows error when failing to load decrypted saved object', async () => {
     encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValueOnce(new Error('Fail'));
 
-    await rulesClient.disable({ id: '1' });
+    await rulesClient.disableRule({ id: '1' });
     expect(unsecuredSavedObjectsClient.update).toHaveBeenCalled();
     expect(taskManager.bulkDisable).toHaveBeenCalled();
     expect(taskManager.removeIfExists).not.toHaveBeenCalledWith();
@@ -569,7 +569,7 @@ describe('disable()', () => {
   test('throws when unsecuredSavedObjectsClient update fails', async () => {
     unsecuredSavedObjectsClient.update.mockRejectedValueOnce(new Error('Failed to update'));
 
-    await expect(rulesClient.disable({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot(
+    await expect(rulesClient.disableRule({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot(
       `"Failed to update"`
     );
     expect(taskManager.bulkDisable).not.toHaveBeenCalled();
@@ -579,22 +579,32 @@ describe('disable()', () => {
   test('throws when failing to disable task', async () => {
     taskManager.bulkDisable.mockRejectedValueOnce(new Error('Failed to disable task'));
 
-    await expect(rulesClient.disable({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot(
+    await expect(rulesClient.disableRule({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot(
       `"Failed to disable task"`
     );
     expect(taskManager.removeIfExists).not.toHaveBeenCalledWith();
   });
 
-  test('throws if API params do not match the schema', async () => {
+  test('throws if id param does not match the schema', async () => {
     await expect(
       // @ts-ignore: this is what we are testing
-      rulesClient.disable({ id: 1 })
+      rulesClient.disableRule({ id: 1 })
     ).rejects.toThrowErrorMatchingInlineSnapshot(
       `"Error validating disable rule parameters - [id]: expected value of type [string] but got [number]"`
     );
     expect(taskManager.removeIfExists).not.toHaveBeenCalledWith();
   });
 
+  test('throws if untrack param does not match the schema', async () => {
+    await expect(
+      // @ts-ignore: this is what we are testing
+      rulesClient.disableRule({ id: '1', untrack: 'foo' })
+    ).rejects.toThrowErrorMatchingInlineSnapshot(
+      `"Error validating disable rule parameters - [untrack]: expected value of type [boolean] but got [string]"`
+    );
+    expect(taskManager.removeIfExists).not.toHaveBeenCalledWith();
+  });
+
   test('removes task document if scheduled task id does not match rule id', async () => {
     encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue({
       ...existingRule,
@@ -603,7 +613,7 @@ describe('disable()', () => {
         scheduledTaskId: 'task-123',
       },
     });
-    await rulesClient.disable({ id: '1' });
+    await rulesClient.disableRule({ id: '1' });
     expect(unsecuredSavedObjectsClient.get).not.toHaveBeenCalled();
     expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith(
       RULE_SAVED_OBJECT_TYPE,
@@ -654,7 +664,7 @@ describe('disable()', () => {
       },
     });
     taskManager.removeIfExists.mockRejectedValueOnce(new Error('Failed to remove task'));
-    await expect(rulesClient.disable({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot(
+    await expect(rulesClient.disableRule({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot(
       `"Failed to remove task"`
     );
     expect(unsecuredSavedObjectsClient.get).not.toHaveBeenCalled();
@@ -707,7 +717,7 @@ describe('disable()', () => {
       encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue(existingDecryptedSiemRule);
       (migrateLegacyActions as jest.Mock).mockResolvedValue(migrateLegacyActionsMock);
 
-      await rulesClient.disable({ id: '1' });
+      await rulesClient.disableRule({ id: '1' });
 
       expect(migrateLegacyActions).toHaveBeenCalledWith(expect.any(Object), {
         attributes: expect.objectContaining({ consumer: AlertConsumers.SIEM }),

diff --git a/...plication/rule/methods/disable/disable.ts → ...tion/rule/methods/disable/disable_rule.ts b/...plication/rule/methods/disable/disable.ts → ...tion/rule/methods/disable/disable_rule.ts
@@ -18,13 +18,13 @@ import { RULE_SAVED_OBJECT_TYPE } from '../../../../saved_objects';
 import { DisableRuleParams } from './types';
 import { disableRuleParamsSchema } from './schemas';
 
-export async function disable(
+export async function disableRule(
   context: RulesClientContext,
   { id, untrack = false }: DisableRuleParams
 ): Promise<void> {
   return await retryIfConflicts(
     context.logger,
-    `rulesClient.disable('${id}')`,
+    `rulesClient.disableRule('${id}')`,
     async () => await disableWithOCC(context, { id, untrack })
   );
 }

diff --git a/x-pack/plugins/alerting/server/application/rule/methods/disable/index.ts b/x-pack/plugins/alerting/server/application/rule/methods/disable/index.ts
@@ -5,5 +5,5 @@
  * 2.0.
  */
 
-export { disable } from './disable';
+export { disableRule } from './disable_rule';
 export type { DisableRuleParams } from './types';
diff --git a/x-pack/plugins/alerting/server/routes/legacy/disable.test.ts b/x-pack/plugins/alerting/server/routes/legacy/disable.test.ts
@@ -38,7 +38,7 @@ describe('disableAlertRoute', () => {
 
     expect(config.path).toMatchInlineSnapshot(`"/api/alerts/alert/{id}/_disable"`);
 
-    rulesClient.disable.mockResolvedValueOnce();
+    rulesClient.disableRule.mockResolvedValueOnce();
 
     const [context, req, res] = mockHandlerArguments(
       { rulesClient },
@@ -52,8 +52,8 @@ describe('disableAlertRoute', () => {
 
     expect(await handler(context, req, res)).toEqual(undefined);
 
-    expect(rulesClient.disable).toHaveBeenCalledTimes(1);
-    expect(rulesClient.disable.mock.calls[0]).toMatchInlineSnapshot(`
+    expect(rulesClient.disableRule).toHaveBeenCalledTimes(1);
+    expect(rulesClient.disableRule.mock.calls[0]).toMatchInlineSnapshot(`
       Array [
         Object {
           "id": "1",
@@ -72,7 +72,7 @@ describe('disableAlertRoute', () => {
 
     const [, handler] = router.post.mock.calls[0];
 
-    rulesClient.disable.mockRejectedValue(new RuleTypeDisabledError('Fail', 'license_invalid'));
+    rulesClient.disableRule.mockRejectedValue(new RuleTypeDisabledError('Fail', 'license_invalid'));
 
     const [context, req, res] = mockHandlerArguments({ rulesClient }, { params: {}, body: {} }, [
       'ok',

diff --git a/x-pack/plugins/alerting/server/routes/legacy/disable.ts b/x-pack/plugins/alerting/server/routes/legacy/disable.ts
@@ -39,7 +39,7 @@ export const disableAlertRoute = (
       const rulesClient = (await context.alerting).getRulesClient();
       const { id } = req.params;
       try {
-        await rulesClient.disable({ id });
+        await rulesClient.disableRule({ id });
         return res.noContent();
       } catch (e) {
         if (e instanceof RuleTypeDisabledError) {

diff --git a/x-pack/plugins/alerting/server/routes/rule/apis/disable/disable_rule_route.test.ts b/x-pack/plugins/alerting/server/routes/rule/apis/disable/disable_rule_route.test.ts
@@ -33,7 +33,7 @@ describe('disableRuleRoute', () => {
 
     expect(config.path).toMatchInlineSnapshot(`"/api/alerting/rule/{id}/_disable"`);
 
-    rulesClient.disable.mockResolvedValueOnce();
+    rulesClient.disableRule.mockResolvedValueOnce();
 
     const [context, req, res] = mockHandlerArguments(
       { rulesClient },
@@ -47,8 +47,8 @@ describe('disableRuleRoute', () => {
 
     expect(await handler(context, req, res)).toEqual(undefined);
 
-    expect(rulesClient.disable).toHaveBeenCalledTimes(1);
-    expect(rulesClient.disable.mock.calls[0]).toMatchInlineSnapshot(`
+    expect(rulesClient.disableRule).toHaveBeenCalledTimes(1);
+    expect(rulesClient.disableRule.mock.calls[0]).toMatchInlineSnapshot(`
       Array [
         Object {
           "id": "1",
@@ -68,7 +68,7 @@ describe('disableRuleRoute', () => {
 
     const [, handler] = router.post.mock.calls[0];
 
-    rulesClient.disable.mockRejectedValue(new RuleTypeDisabledError('Fail', 'license_invalid'));
+    rulesClient.disableRule.mockRejectedValue(new RuleTypeDisabledError('Fail', 'license_invalid'));
 
     const [context, req, res] = mockHandlerArguments({ rulesClient }, { params: {}, body: {} }, [
       'ok',

diff --git a/x-pack/plugins/alerting/server/routes/rule/apis/disable/disable_rule_route.ts b/x-pack/plugins/alerting/server/routes/rule/apis/disable/disable_rule_route.ts
@@ -42,7 +42,7 @@ export const disableRuleRoute = (
         const disableParams = { id, untrack };
 
         try {
-          await rulesClient.disable(disableParams);
+          await rulesClient.disableRule(disableParams);
           return res.noContent();
         } catch (e) {
           if (e instanceof RuleTypeDisabledError) {

diff --git a/x-pack/plugins/alerting/server/rules_client.mock.ts b/x-pack/plugins/alerting/server/rules_client.mock.ts
@@ -22,7 +22,7 @@ const createRulesClientMock = () => {
     delete: jest.fn(),
     update: jest.fn(),
     enable: jest.fn(),
-    disable: jest.fn(),
+    disableRule: jest.fn(),
     updateApiKey: jest.fn(),
     muteAll: jest.fn(),
     unmuteAll: jest.fn(),

diff --git a/x-pack/plugins/alerting/server/rules_client/lib/untrack_rule_alerts.ts b/x-pack/plugins/alerting/server/rules_client/lib/untrack_rule_alerts.ts
@@ -89,7 +89,7 @@ export const untrackRuleAlerts = async (
     } catch (error) {
       // this should not block the rest of the disable process
       context.logger.warn(
-        `rulesClient.disable('${id}') - Could not write untrack events - ${error.message}`
+        `rulesClient.disableRule('${id}') - Could not write untrack events - ${error.message}`
       );
     }
   });

diff --git a/x-pack/plugins/alerting/server/rules_client/rules_client.ts b/x-pack/plugins/alerting/server/rules_client/rules_client.ts
@@ -55,7 +55,7 @@ import {
 import { bulkEnableRules, BulkEnableRulesParams } from '../application/rule/methods/bulk_enable';
 import { updateApiKey } from './methods/update_api_key';
 import { enable } from './methods/enable';
-import { disable } from '../application/rule/methods/disable/disable';
+import { disableRule } from '../application/rule/methods/disable/disable_rule';
 import { clearExpiredSnoozes } from './methods/clear_expired_snoozes';
 import { muteInstance } from '../application/rule/methods/mute_alert/mute_instance';
 import { muteAll } from './methods/mute_all';
@@ -168,7 +168,7 @@ export class RulesClient {
   public updateApiKey = (options: { id: string }) => updateApiKey(this.context, options);
 
   public enable = (options: { id: string }) => enable(this.context, options);
-  public disable = (params: DisableRuleParams) => disable(this.context, params);
+  public disableRule = (params: DisableRuleParams) => disableRule(this.context, params);
 
   public snooze = (options: SnoozeRuleOptions) => snoozeRule(this.context, options);
   public unsnooze = (options: UnsnoozeParams) => unsnoozeRule(this.context, options);

diff --git a/x-pack/plugins/alerting/server/rules_client_conflict_retries.test.ts b/x-pack/plugins/alerting/server/rules_client_conflict_retries.test.ts
@@ -173,7 +173,7 @@ async function enable(success: boolean) {
 
 async function disable(success: boolean) {
   try {
-    await rulesClient.disable({ id: MockRuleId });
+    await rulesClient.disableRule({ id: MockRuleId });
   } catch (err) {
     return expectConflict(success, err);
   }

diff --git a/...ne/rule_management/logic/detection_rules_client/detection_rules_client.patch_rule.test.ts b/...ne/rule_management/logic/detection_rules_client/detection_rules_client.patch_rule.test.ts
@@ -107,7 +107,7 @@ describe('DetectionRulesClient.patchRule', () => {
     const rule = await detectionRulesClient.patchRule({ rulePatch });
 
     expect(rule.enabled).toBe(false);
-    expect(rulesClient.disable).toHaveBeenCalledWith(
+    expect(rulesClient.disableRule).toHaveBeenCalledWith(
       expect.objectContaining({
         id: existingRule.id,
       })

diff --git a/...e/rule_management/logic/detection_rules_client/detection_rules_client.update_rule.test.ts b/...e/rule_management/logic/detection_rules_client/detection_rules_client.update_rule.test.ts
@@ -111,7 +111,7 @@ describe('DetectionRulesClient.updateRule', () => {
 
     await detectionRulesClient.updateRule({ ruleUpdate });
 
-    expect(rulesClient.disable).toHaveBeenCalledWith(
+    expect(rulesClient.disableRule).toHaveBeenCalledWith(
       expect.objectContaining({
         id: existingRule.id,
       })

diff --git a/...olution/server/lib/detection_engine/rule_management/logic/detection_rules_client/utils.ts b/...olution/server/lib/detection_engine/rule_management/logic/detection_rules_client/utils.ts
@@ -22,7 +22,7 @@ export const toggleRuleEnabledOnUpdate = async (
   updatedRule: RuleResponse
 ): Promise<{ enabled: boolean }> => {
   if (existingRule.enabled && !updatedRule.enabled) {
-    await rulesClient.disable({ id: existingRule.id });
+    await rulesClient.disableRule({ id: existingRule.id });
     return { enabled: false };
   }