[Alerting] refactor action whitelisting functions

elastic#64659 In this PR, the action whitelisting utilities have been refactored to allow them to (eventually) be used in plugins other than the actions plugin. Prior to this, actions required deeper integration with the internal of the actions plugin. This also adds some generic typing to the actionType config, secrets, and params properties, since they are now referred to in multiple places within the actionType.
pmuellr · May 26, 2020 · 8136c91 · 8136c91
1 parent a2451c9
commit 8136c91
Show file tree

Hide file tree

Showing 18 changed files with 306 additions and 236 deletions.
diff --git a/x-pack/plugins/actions/server/action_type_registry.mock.ts b/x-pack/plugins/actions/server/action_type_registry.mock.ts
@@ -5,6 +5,7 @@
  */
 
 import { ActionTypeRegistryContract } from './types';
+import { createValidationServiceMock } from './mocks';
 
 const createActionTypeRegistryMock = () => {
   const mocked: jest.Mocked<ActionTypeRegistryContract> = {
@@ -15,6 +16,7 @@ const createActionTypeRegistryMock = () => {
     ensureActionTypeEnabled: jest.fn(),
     isActionTypeEnabled: jest.fn(),
     isActionExecutable: jest.fn(),
+    getValidationService: jest.fn(() => createValidationServiceMock()),
   };
   return mocked;
 };

diff --git a/x-pack/plugins/actions/server/action_type_registry.ts b/x-pack/plugins/actions/server/action_type_registry.ts
@@ -8,9 +8,9 @@ import Boom from 'boom';
 import { i18n } from '@kbn/i18n';
 import { RunContext, TaskManagerSetupContract } from '../../task_manager/server';
 import { ExecutorError, TaskRunnerFactory, ILicenseState } from './lib';
-import { ActionType, PreConfiguredAction } from './types';
+import { ActionType, PreConfiguredAction, ActionValidationService } from './types';
 import { ActionType as CommonActionType } from '../common';
-import { ActionsConfigurationUtilities } from './actions_config';
+import { ActionsConfigurationUtilities, getValidationService } from './actions_config';
 
 export interface ActionTypeRegistryOpts {
   taskManager: TaskManagerSetupContract;
@@ -27,13 +27,15 @@ export class ActionTypeRegistry {
   private readonly actionsConfigUtils: ActionsConfigurationUtilities;
   private readonly licenseState: ILicenseState;
   private readonly preconfiguredActions: PreConfiguredAction[];
+  private readonly validationService: ActionValidationService;
 
   constructor(constructorParams: ActionTypeRegistryOpts) {
     this.taskManager = constructorParams.taskManager;
     this.taskRunnerFactory = constructorParams.taskRunnerFactory;
     this.actionsConfigUtils = constructorParams.actionsConfigUtils;
     this.licenseState = constructorParams.licenseState;
     this.preconfiguredActions = constructorParams.preconfiguredActions;
+    this.validationService = getValidationService(this.actionsConfigUtils);
   }
 
   /**
@@ -43,6 +45,13 @@ export class ActionTypeRegistry {
     return this.actionTypes.has(id);
   }
 
+  /**
+   * Return the validation service.
+   */
+  public getValidationService(): ActionValidationService {
+    return this.validationService;
+  }
+
   /**
    * Throws error if action type is not enabled.
    */

diff --git a/x-pack/plugins/actions/server/actions_client.ts b/x-pack/plugins/actions/server/actions_client.ts
@@ -75,9 +75,10 @@ export class ActionsClient {
    */
   public async create({ action }: CreateOptions): Promise<ActionResult> {
     const { actionTypeId, name, config, secrets } = action;
+    const validationService = this.actionTypeRegistry.getValidationService();
     const actionType = this.actionTypeRegistry.get(actionTypeId);
-    const validatedActionTypeConfig = validateConfig(actionType, config);
-    const validatedActionTypeSecrets = validateSecrets(actionType, secrets);
+    const validatedActionTypeConfig = validateConfig(actionType, config, validationService);
+    const validatedActionTypeSecrets = validateSecrets(actionType, secrets, validationService);
 
     this.actionTypeRegistry.ensureActionTypeEnabled(actionTypeId);
 
@@ -119,8 +120,9 @@ export class ActionsClient {
     const { actionTypeId } = existingObject.attributes;
     const { name, config, secrets } = action;
     const actionType = this.actionTypeRegistry.get(actionTypeId);
-    const validatedActionTypeConfig = validateConfig(actionType, config);
-    const validatedActionTypeSecrets = validateSecrets(actionType, secrets);
+    const validationService = this.actionTypeRegistry.getValidationService();
+    const validatedActionTypeConfig = validateConfig(actionType, config, validationService);
+    const validatedActionTypeSecrets = validateSecrets(actionType, secrets, validationService);
 
     this.actionTypeRegistry.ensureActionTypeEnabled(actionTypeId);
 

diff --git a/x-pack/plugins/actions/server/actions_config.ts b/x-pack/plugins/actions/server/actions_config.ts
@@ -10,7 +10,7 @@ import { URL } from 'url';
 import { curry } from 'lodash';
 import { pipe } from 'fp-ts/lib/pipeable';
 
-import { ActionsConfigType } from './types';
+import { ActionsConfigType, ActionValidationService } from './types';
 import { ActionTypeDisabledError } from './lib';
 
 export enum WhitelistedHosts {
@@ -109,3 +109,13 @@ export function getActionsConfigurationUtilities(
     },
   };
 }
+
+export function getValidationService(
+  configUtils: ActionsConfigurationUtilities
+): ActionValidationService {
+  const { isWhitelistedHostname, isWhitelistedUri } = configUtils;
+  return {
+    isWhitelistedHostname,
+    isWhitelistedUri,
+  };
+}
diff --git a/x-pack/plugins/actions/server/builtin_action_types/email.test.ts b/x-pack/plugins/actions/server/builtin_action_types/email.test.ts
@@ -8,29 +8,21 @@ jest.mock('./lib/send_email', () => ({
   sendEmail: jest.fn(),
 }));
 
-import { Logger } from '../../../../../src/core/server';
-
 import { ActionType, ActionTypeExecutorOptions } from '../types';
-import { actionsConfigMock } from '../actions_config.mock';
 import { validateConfig, validateSecrets, validateParams } from '../lib';
 import { createActionTypeRegistry } from './index.test';
 import { sendEmail } from './lib/send_email';
 import { actionsMock } from '../mocks';
-import {
-  ActionParamsType,
-  ActionTypeConfigType,
-  ActionTypeSecretsType,
-  getActionType,
-} from './email';
+import { ActionParamsType, ActionTypeConfigType, ActionTypeSecretsType } from './email';
 
 const sendEmailMock = sendEmail as jest.Mock;
 
 const ACTION_TYPE_ID = '.email';
 
 const services = actionsMock.createServices();
+const validationService = actionsMock.createValidationService();
 
 let actionType: ActionType;
-let mockedLogger: jest.Mocked<Logger>;
 
 beforeEach(() => {
   jest.resetAllMocks();
@@ -51,7 +43,8 @@ describe('config validation', () => {
       service: 'gmail',
       from: '[email protected]',
     };
-    expect(validateConfig(actionType, config)).toEqual({
+    validationService.isWhitelistedHostname.mockReturnValue(true);
+    expect(validateConfig(actionType, config, validationService)).toEqual({
       ...config,
       host: null,
       port: null,
@@ -61,7 +54,7 @@ describe('config validation', () => {
     delete config.service;
     config.host = 'elastic.co';
     config.port = 8080;
-    expect(validateConfig(actionType, config)).toEqual({
+    expect(validateConfig(actionType, config, validationService)).toEqual({
       ...config,
       service: null,
       secure: null,
@@ -75,38 +68,42 @@ describe('config validation', () => {
 
     // empty object
     expect(() => {
-      validateConfig(actionType, {});
+      validateConfig(actionType, {}, validationService);
     }).toThrowErrorMatchingInlineSnapshot(
       `"error validating action type config: [from]: expected value of type [string] but got [undefined]"`
     );
 
     // no service or host/port
     expect(() => {
-      validateConfig(actionType, baseConfig);
+      validateConfig(actionType, baseConfig, validationService);
     }).toThrowErrorMatchingInlineSnapshot(
       `"error validating action type config: either [service] or [host]/[port] is required"`
     );
 
     // host but no port
     expect(() => {
-      validateConfig(actionType, { ...baseConfig, host: 'elastic.co' });
+      validateConfig(actionType, { ...baseConfig, host: 'elastic.co' }, validationService);
     }).toThrowErrorMatchingInlineSnapshot(
       `"error validating action type config: [port] is required if [service] is not provided"`
     );
 
     // port but no host
     expect(() => {
-      validateConfig(actionType, { ...baseConfig, port: 8080 });
+      validateConfig(actionType, { ...baseConfig, port: 8080 }, validationService);
     }).toThrowErrorMatchingInlineSnapshot(
       `"error validating action type config: [host] is required if [service] is not provided"`
     );
 
     // invalid service
     expect(() => {
-      validateConfig(actionType, {
-        ...baseConfig,
-        service: 'bad-nodemailer-service',
-      });
+      validateConfig(
+        actionType,
+        {
+          ...baseConfig,
+          service: 'bad-nodemailer-service',
+        },
+        validationService
+      );
     }).toThrowErrorMatchingInlineSnapshot(
       `"error validating action type config: [service] value 'bad-nodemailer-service' is not valid"`
     );
@@ -117,13 +114,9 @@ describe('config validation', () => {
   const NODEMAILER_AOL_SERVICE_HOST = 'smtp.aol.com';
 
   test('config validation handles email host whitelisting', () => {
-    actionType = getActionType({
-      logger: mockedLogger,
-      configurationUtilities: {
-        ...actionsConfigMock.create(),
-        isWhitelistedHostname: (hostname) => hostname === NODEMAILER_AOL_SERVICE_HOST,
-      },
-    });
+    validationService.isWhitelistedHostname.mockImplementation(
+      (hostname) => hostname === NODEMAILER_AOL_SERVICE_HOST
+    );
     const baseConfig = {
       from: '[email protected]',
     };
@@ -147,23 +140,23 @@ describe('config validation', () => {
       port: 42,
     };
 
-    const validatedConfig1 = validateConfig(actionType, whitelistedConfig1);
+    const validatedConfig1 = validateConfig(actionType, whitelistedConfig1, validationService);
     expect(validatedConfig1.service).toEqual(whitelistedConfig1.service);
     expect(validatedConfig1.from).toEqual(whitelistedConfig1.from);
 
-    const validatedConfig2 = validateConfig(actionType, whitelistedConfig2);
+    const validatedConfig2 = validateConfig(actionType, whitelistedConfig2, validationService);
     expect(validatedConfig2.host).toEqual(whitelistedConfig2.host);
     expect(validatedConfig2.port).toEqual(whitelistedConfig2.port);
     expect(validatedConfig2.from).toEqual(whitelistedConfig2.from);
 
     expect(() => {
-      validateConfig(actionType, notWhitelistedConfig1);
+      validateConfig(actionType, notWhitelistedConfig1, validationService);
     }).toThrowErrorMatchingInlineSnapshot(
       `"error validating action type config: [service] value 'gmail' resolves to host 'smtp.gmail.com' which is not in the whitelistedHosts configuration"`
     );
 
     expect(() => {
-      validateConfig(actionType, notWhitelistedConfig2);
+      validateConfig(actionType, notWhitelistedConfig2, validationService);
     }).toThrowErrorMatchingInlineSnapshot(
       `"error validating action type config: [host] value 'smtp.gmail.com' is not in the whitelistedHosts configuration"`
     );
@@ -176,17 +169,17 @@ describe('secrets validation', () => {
       user: 'bob',
       password: 'supersecret',
     };
-    expect(validateSecrets(actionType, secrets)).toEqual(secrets);
+    expect(validateSecrets(actionType, secrets, validationService)).toEqual(secrets);
   });
 
   test('secrets validation succeeds when secrets props are null/undefined', () => {
     const secrets: Record<string, unknown> = {
       user: null,
       password: null,
     };
-    expect(validateSecrets(actionType, {})).toEqual(secrets);
-    expect(validateSecrets(actionType, { user: null })).toEqual(secrets);
-    expect(validateSecrets(actionType, { password: null })).toEqual(secrets);
+    expect(validateSecrets(actionType, {}, validationService)).toEqual(secrets);
+    expect(validateSecrets(actionType, { user: null }, validationService)).toEqual(secrets);
+    expect(validateSecrets(actionType, { password: null }, validationService)).toEqual(secrets);
   });
 });
 
@@ -197,7 +190,7 @@ describe('params validation', () => {
       subject: 'this is a test',
       message: 'this is the message',
     };
-    expect(validateParams(actionType, params)).toMatchInlineSnapshot(`
+    expect(validateParams(actionType, params, validationService)).toMatchInlineSnapshot(`
       Object {
         "bcc": Array [],
         "cc": Array [],
@@ -213,7 +206,7 @@ describe('params validation', () => {
   test('params validation fails when params is not valid', () => {
     // empty object
     expect(() => {
-      validateParams(actionType, {});
+      validateParams(actionType, {}, validationService);
     }).toThrowErrorMatchingInlineSnapshot(
       `"error validating action params: [subject]: expected value of type [string] but got [undefined]"`
     );