Skip to content

Commit

Permalink
[Alerting] explicitly pass config utils to action validators
Browse files Browse the repository at this point in the history
resolves elastic#64659

Previously, for action validators that needed to perform whitelisting
checks, they would need to curry in the config utilties providing the
whitelisting functions, into their validators.  This is unwieldy -
the config utilities should be passed explicitly in.
  • Loading branch information
pmuellr committed May 9, 2020
1 parent 4d32610 commit 0d5f453
Show file tree
Hide file tree
Showing 14 changed files with 179 additions and 122 deletions.
1 change: 1 addition & 0 deletions x-pack/plugins/actions/server/action_type_registry.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ const createActionTypeRegistryMock = () => {
ensureActionTypeEnabled: jest.fn(),
isActionTypeEnabled: jest.fn(),
isActionExecutable: jest.fn(),
getConfigUtils: jest.fn(),
};
return mocked;
};
Expand Down
4 changes: 4 additions & 0 deletions x-pack/plugins/actions/server/action_type_registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ export class ActionTypeRegistry {
this.preconfiguredActions = constructorParams.preconfiguredActions;
}

public getConfigUtils() {
return this.actionsConfigUtils;
}

/**
* Returns if the action type registry has the given action type registered
*/
Expand Down
10 changes: 6 additions & 4 deletions x-pack/plugins/actions/server/actions_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,9 @@ export class ActionsClient {
public async create({ action }: CreateOptions): Promise<ActionResult> {
const { actionTypeId, name, config, secrets } = action;
const actionType = this.actionTypeRegistry.get(actionTypeId);
const validatedActionTypeConfig = validateConfig(actionType, config);
const validatedActionTypeSecrets = validateSecrets(actionType, secrets);
const configUtils = this.actionTypeRegistry.getConfigUtils();
const validatedActionTypeConfig = validateConfig(actionType, config, configUtils);
const validatedActionTypeSecrets = validateSecrets(actionType, secrets, configUtils);

this.actionTypeRegistry.ensureActionTypeEnabled(actionTypeId);

Expand Down Expand Up @@ -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 configUtils = this.actionTypeRegistry.getConfigUtils();
const validatedActionTypeConfig = validateConfig(actionType, config, configUtils);
const validatedActionTypeSecrets = validateSecrets(actionType, secrets, configUtils);

this.actionTypeRegistry.ensureActionTypeEnabled(actionTypeId);

Expand Down
48 changes: 26 additions & 22 deletions x-pack/plugins/actions/server/builtin_action_types/email.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ jest.mock('./lib/send_email', () => ({
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';
Expand All @@ -31,6 +30,7 @@ const services = actionsMock.createServices();

let actionType: ActionType;
let mockedLogger: jest.Mocked<Logger>;
const mockedConfigUtils = actionsMock.createConfigUtils();

beforeEach(() => {
jest.resetAllMocks();
Expand All @@ -51,7 +51,7 @@ describe('config validation', () => {
service: 'gmail',
from: '[email protected]',
};
expect(validateConfig(actionType, config)).toEqual({
expect(validateConfig(actionType, config, mockedConfigUtils)).toEqual({
...config,
host: null,
port: null,
Expand All @@ -61,7 +61,7 @@ describe('config validation', () => {
delete config.service;
config.host = 'elastic.co';
config.port = 8080;
expect(validateConfig(actionType, config)).toEqual({
expect(validateConfig(actionType, config, mockedConfigUtils)).toEqual({
...config,
service: null,
secure: null,
Expand All @@ -75,38 +75,42 @@ describe('config validation', () => {

// empty object
expect(() => {
validateConfig(actionType, {});
validateConfig(actionType, {}, mockedConfigUtils);
}).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, mockedConfigUtils);
}).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' }, mockedConfigUtils);
}).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 }, mockedConfigUtils);
}).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',
},
mockedConfigUtils
);
}).toThrowErrorMatchingInlineSnapshot(
`"error validating action type config: [service] value 'bad-nodemailer-service' is not valid"`
);
Expand All @@ -120,7 +124,7 @@ describe('config validation', () => {
actionType = getActionType({
logger: mockedLogger,
configurationUtilities: {
...actionsConfigMock.create(),
...actionsMock.createConfigUtils(),
isWhitelistedHostname: hostname => hostname === NODEMAILER_AOL_SERVICE_HOST,
},
});
Expand All @@ -147,23 +151,23 @@ describe('config validation', () => {
port: 42,
};

const validatedConfig1 = validateConfig(actionType, whitelistedConfig1);
const validatedConfig1 = validateConfig(actionType, whitelistedConfig1, mockedConfigUtils);
expect(validatedConfig1.service).toEqual(whitelistedConfig1.service);
expect(validatedConfig1.from).toEqual(whitelistedConfig1.from);

const validatedConfig2 = validateConfig(actionType, whitelistedConfig2);
const validatedConfig2 = validateConfig(actionType, whitelistedConfig2, mockedConfigUtils);
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, mockedConfigUtils);
}).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, mockedConfigUtils);
}).toThrowErrorMatchingInlineSnapshot(
`"error validating action type config: [host] value 'smtp.gmail.com' is not in the whitelistedHosts configuration"`
);
Expand All @@ -176,17 +180,17 @@ describe('secrets validation', () => {
user: 'bob',
password: 'supersecret',
};
expect(validateSecrets(actionType, secrets)).toEqual(secrets);
expect(validateSecrets(actionType, secrets, mockedConfigUtils)).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, {}, mockedConfigUtils)).toEqual(secrets);
expect(validateSecrets(actionType, { user: null }, mockedConfigUtils)).toEqual(secrets);
expect(validateSecrets(actionType, { password: null }, mockedConfigUtils)).toEqual(secrets);
});
});

Expand All @@ -197,7 +201,7 @@ describe('params validation', () => {
subject: 'this is a test',
message: 'this is the message',
};
expect(validateParams(actionType, params)).toMatchInlineSnapshot(`
expect(validateParams(actionType, params, mockedConfigUtils)).toMatchInlineSnapshot(`
Object {
"bcc": Array [],
"cc": Array [],
Expand All @@ -213,7 +217,7 @@ describe('params validation', () => {
test('params validation fails when params is not valid', () => {
// empty object
expect(() => {
validateParams(actionType, {});
validateParams(actionType, {}, mockedConfigUtils);
}).toThrowErrorMatchingInlineSnapshot(
`"error validating action params: [subject]: expected value of type [string] but got [undefined]"`
);
Expand Down
30 changes: 19 additions & 11 deletions x-pack/plugins/actions/server/builtin_action_types/es_index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ const ACTION_TYPE_ID = '.index';
const services = actionsMock.createServices();

let actionType: ActionType;
const mockedConfigUtils = actionsMock.createConfigUtils();

beforeAll(() => {
const { actionTypeRegistry } = createActionTypeRegistry();
Expand All @@ -43,23 +44,23 @@ describe('config validation', () => {
refresh: false,
};

expect(validateConfig(actionType, config)).toEqual({
expect(validateConfig(actionType, config, mockedConfigUtils)).toEqual({
...config,
index: 'testing-123',
refresh: false,
executionTimeField: null,
});

config.executionTimeField = 'field-123';
expect(validateConfig(actionType, config)).toEqual({
expect(validateConfig(actionType, config, mockedConfigUtils)).toEqual({
...config,
index: 'testing-123',
refresh: false,
executionTimeField: 'field-123',
});

config.executionTimeField = null;
expect(validateConfig(actionType, config)).toEqual({
expect(validateConfig(actionType, config, mockedConfigUtils)).toEqual({
...config,
index: 'testing-123',
refresh: false,
Expand All @@ -69,14 +70,21 @@ describe('config validation', () => {
delete config.index;

expect(() => {
validateConfig(actionType, { index: 666 });
validateConfig(actionType, { index: 666 }, mockedConfigUtils);
}).toThrowErrorMatchingInlineSnapshot(
`"error validating action type config: [index]: expected value of type [string] but got [number]"`
);
delete config.executionTimeField;

expect(() => {
validateConfig(actionType, { index: 'testing-123', executionTimeField: true });
validateConfig(
actionType,
{
index: 'testing-123',
executionTimeField: true,
},
mockedConfigUtils
);
}).toThrowErrorMatchingInlineSnapshot(`
"error validating action type config: [executionTimeField]: types that failed validation:
- [executionTimeField.0]: expected value of type [string] but got [boolean]
Expand All @@ -85,7 +93,7 @@ describe('config validation', () => {

delete config.refresh;
expect(() => {
validateConfig(actionType, { index: 'testing-123', refresh: 'foo' });
validateConfig(actionType, { index: 'testing-123', refresh: 'foo' }, mockedConfigUtils);
}).toThrowErrorMatchingInlineSnapshot(
`"error validating action type config: [refresh]: expected value of type [boolean] but got [string]"`
);
Expand All @@ -97,7 +105,7 @@ describe('config validation', () => {
};

expect(() => {
validateConfig(actionType, baseConfig);
validateConfig(actionType, baseConfig, mockedConfigUtils);
}).toThrowErrorMatchingInlineSnapshot(
`"error validating action type config: [index]: expected value of type [string] but got [undefined]"`
);
Expand All @@ -109,7 +117,7 @@ describe('params validation', () => {
const params: Record<string, unknown> = {
documents: [{ rando: 'thing' }],
};
expect(validateParams(actionType, params)).toMatchInlineSnapshot(`
expect(validateParams(actionType, params, mockedConfigUtils)).toMatchInlineSnapshot(`
Object {
"documents": Array [
Object {
Expand All @@ -122,19 +130,19 @@ describe('params validation', () => {

test('params validation fails when params is not valid', () => {
expect(() => {
validateParams(actionType, { documents: [{}], jim: 'bob' });
validateParams(actionType, { documents: [{}], jim: 'bob' }, mockedConfigUtils);
}).toThrowErrorMatchingInlineSnapshot(
`"error validating action params: [jim]: definition for this key is missing"`
);

expect(() => {
validateParams(actionType, {});
validateParams(actionType, {}, mockedConfigUtils);
}).toThrowErrorMatchingInlineSnapshot(
`"error validating action params: [documents]: expected value of type [array] but got [undefined]"`
);

expect(() => {
validateParams(actionType, { documents: ['should be an object'] });
validateParams(actionType, { documents: ['should be an object'] }, mockedConfigUtils);
}).toThrowErrorMatchingInlineSnapshot(
`"error validating action params: [documents.0]: could not parse record value from json input"`
);
Expand Down
Loading

0 comments on commit 0d5f453

Please sign in to comment.