From 09b352d66234a27ce8fce49200cbcd3689d05975 Mon Sep 17 00:00:00 2001 From: Peter Schretlen Date: Mon, 10 Feb 2020 17:08:56 -0500 Subject: [PATCH] Webhook action - make user and password secrets optional (#56823) --- x-pack/plugins/actions/README.md | 4 +- .../builtin_action_types/webhook.test.ts | 106 ++++++++++++++++-- .../server/builtin_action_types/webhook.ts | 36 ++++-- .../builtin_action_types/webhook.tsx | 4 +- .../plugins/actions/webhook_simulation.ts | 2 +- .../actions/builtin_action_types/webhook.ts | 4 +- .../actions/builtin_action_types/webhook.ts | 75 +++++++++++++ .../spaces_only/tests/actions/index.ts | 1 + 8 files changed, 205 insertions(+), 27 deletions(-) create mode 100644 x-pack/test/alerting_api_integration/spaces_only/tests/actions/builtin_action_types/webhook.ts diff --git a/x-pack/plugins/actions/README.md b/x-pack/plugins/actions/README.md index 31de183b14ff2..154d8da1bbdb9 100644 --- a/x-pack/plugins/actions/README.md +++ b/x-pack/plugins/actions/README.md @@ -404,8 +404,8 @@ The webhook action uses [axios](https://github.com/axios/axios) to send a POST o |Property|Description|Type| |---|---|---| -|user|Username for HTTP Basic authentication|string| -|password|Password for HTTP Basic authentication|string| +|user|Username for HTTP Basic authentication|string _(optional)_| +|password|Password for HTTP Basic authentication|string _(optional)_| ### `params` diff --git a/x-pack/plugins/actions/server/builtin_action_types/webhook.test.ts b/x-pack/plugins/actions/server/builtin_action_types/webhook.test.ts index ae1d8c3fddc8b..09ab6af47e443 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/webhook.test.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/webhook.test.ts @@ -4,15 +4,28 @@ * you may not use this file except in compliance with the Elastic License. */ +jest.mock('axios', () => ({ + request: jest.fn(), +})); + import { getActionType } from './webhook'; +import { ActionType, Services } from '../types'; import { validateConfig, validateSecrets, validateParams } from '../lib'; +import { savedObjectsClientMock } from '../../../../../src/core/server/mocks'; import { configUtilsMock } from '../actions_config.mock'; -import { ActionType } from '../types'; import { createActionTypeRegistry } from './index.test'; import { Logger } from '../../../../../src/core/server'; +import axios from 'axios'; + +const axiosRequestMock = axios.request as jest.Mock; const ACTION_TYPE_ID = '.webhook'; +const services: Services = { + callCluster: async (path: string, opts: any) => {}, + savedObjectsClient: savedObjectsClientMock.create(), +}; + let actionType: ActionType; let mockedLogger: jest.Mocked; @@ -38,20 +51,18 @@ describe('secrets validation', () => { expect(validateSecrets(actionType, secrets)).toEqual(secrets); }); - test('fails when secret password is omitted', () => { + test('fails when secret user is provided, but password is omitted', () => { expect(() => { validateSecrets(actionType, { user: 'bob' }); }).toThrowErrorMatchingInlineSnapshot( - `"error validating action type secrets: [password]: expected value of type [string] but got [undefined]"` + `"error validating action type secrets: both user and password must be specified"` ); }); - test('fails when secret user is omitted', () => { + test('succeeds when basic authentication credentials are omitted', () => { expect(() => { - validateSecrets(actionType, {}); - }).toThrowErrorMatchingInlineSnapshot( - `"error validating action type secrets: [user]: expected value of type [string] but got [undefined]"` - ); + validateSecrets(actionType, {}).toEqual({}); + }); }); }); @@ -190,3 +201,82 @@ describe('params validation', () => { }); }); }); + +describe('execute()', () => { + beforeAll(() => { + axiosRequestMock.mockReset(); + actionType = getActionType({ + logger: mockedLogger, + configurationUtilities: configUtilsMock, + }); + }); + + beforeEach(() => { + axiosRequestMock.mockReset(); + axiosRequestMock.mockResolvedValue({ + status: 200, + statusText: '', + data: '', + headers: [], + config: {}, + }); + }); + + test('execute with username/password sends request with basic auth', async () => { + await actionType.executor({ + actionId: 'some-id', + services, + config: { + url: 'https://abc.def/my-webhook', + method: 'post', + headers: { + aheader: 'a value', + }, + }, + secrets: { user: 'abc', password: '123' }, + params: { body: 'some data' }, + }); + + expect(axiosRequestMock.mock.calls[0][0]).toMatchInlineSnapshot(` + Object { + "auth": Object { + "password": "123", + "username": "abc", + }, + "data": "some data", + "headers": Object { + "aheader": "a value", + }, + "method": "post", + "url": "https://abc.def/my-webhook", + } + `); + }); + + test('execute without username/password sends request without basic auth', async () => { + await actionType.executor({ + actionId: 'some-id', + services, + config: { + url: 'https://abc.def/my-webhook', + method: 'post', + headers: { + aheader: 'a value', + }, + }, + secrets: {}, + params: { body: 'some data' }, + }); + + expect(axiosRequestMock.mock.calls[0][0]).toMatchInlineSnapshot(` + Object { + "data": "some data", + "headers": Object { + "aheader": "a value", + }, + "method": "post", + "url": "https://abc.def/my-webhook", + } + `); + }); +}); diff --git a/x-pack/plugins/actions/server/builtin_action_types/webhook.ts b/x-pack/plugins/actions/server/builtin_action_types/webhook.ts index f7efb3b1e746c..e275deace0dcc 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/webhook.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/webhook.ts @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ import { i18n } from '@kbn/i18n'; -import { curry } from 'lodash'; +import { curry, isString } from 'lodash'; import axios, { AxiosError, AxiosResponse } from 'axios'; import { schema, TypeOf } from '@kbn/config-schema'; import { pipe } from 'fp-ts/lib/pipeable'; @@ -34,10 +34,20 @@ const ConfigSchema = schema.object(configSchemaProps); type ActionTypeConfigType = TypeOf; // secrets definition -type ActionTypeSecretsType = TypeOf; -const SecretsSchema = schema.object({ - user: schema.string(), - password: schema.string(), +export type ActionTypeSecretsType = TypeOf; +const secretSchemaProps = { + user: schema.nullable(schema.string()), + password: schema.nullable(schema.string()), +}; +const SecretsSchema = schema.object(secretSchemaProps, { + validate: secrets => { + // user and password must be set together (or not at all) + if (!secrets.password && !secrets.user) return; + if (secrets.password && secrets.user) return; + return i18n.translate('xpack.actions.builtin.webhook.invalidUsernamePassword', { + defaultMessage: 'both user and password must be specified', + }); + }, }); // params definition @@ -61,7 +71,7 @@ export function getActionType({ }), validate: { config: schema.object(configSchemaProps, { - validate: curry(valdiateActionTypeConfig)(configurationUtilities), + validate: curry(validateActionTypeConfig)(configurationUtilities), }), secrets: SecretsSchema, params: ParamsSchema, @@ -70,7 +80,7 @@ export function getActionType({ }; } -function valdiateActionTypeConfig( +function validateActionTypeConfig( configurationUtilities: ActionsConfigurationUtilities, configObject: ActionTypeConfigType ) { @@ -93,17 +103,19 @@ export async function executor( ): Promise { const actionId = execOptions.actionId; const { method, url, headers = {} } = execOptions.config as ActionTypeConfigType; - const { user: username, password } = execOptions.secrets as ActionTypeSecretsType; const { body: data } = execOptions.params as ActionParamsType; + const secrets: ActionTypeSecretsType = execOptions.secrets as ActionTypeSecretsType; + const basicAuth = + isString(secrets.user) && isString(secrets.password) + ? { auth: { username: secrets.user, password: secrets.password } } + : {}; + const result: Result = await promiseResult( axios.request({ method, url, - auth: { - username, - password, - }, + ...basicAuth, headers, data, }) diff --git a/x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/webhook.tsx b/x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/webhook.tsx index 3000191218932..fecf846ed6c9a 100644 --- a/x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/webhook.tsx +++ b/x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/webhook.tsx @@ -73,7 +73,7 @@ export function getActionType(): ActionTypeModel { ) ); } - if (!action.secrets.user) { + if (!action.secrets.user && action.secrets.password) { errors.user.push( i18n.translate( 'xpack.triggersActionsUI.sections.addAction.webhookAction.error.requiredHostText', @@ -83,7 +83,7 @@ export function getActionType(): ActionTypeModel { ) ); } - if (!action.secrets.password) { + if (!action.secrets.password && action.secrets.user) { errors.password.push( i18n.translate( 'xpack.triggersActionsUI.sections.addAction.webhookAction.error.requiredPasswordText', diff --git a/x-pack/test/alerting_api_integration/common/fixtures/plugins/actions/webhook_simulation.ts b/x-pack/test/alerting_api_integration/common/fixtures/plugins/actions/webhook_simulation.ts index 9a878ff0bf798..1b267f6c4976f 100644 --- a/x-pack/test/alerting_api_integration/common/fixtures/plugins/actions/webhook_simulation.ts +++ b/x-pack/test/alerting_api_integration/common/fixtures/plugins/actions/webhook_simulation.ts @@ -68,7 +68,7 @@ function webhookHandler(request: WebhookRequest, h: any) { return validateRequestUsesMethod(request, h, 'post'); case 'success_put_method': return validateRequestUsesMethod(request, h, 'put'); - case 'faliure': + case 'failure': return htmlResponse(h, 500, `Error`); } diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/webhook.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/webhook.ts index 841c96acdc3b1..da83dbf8c47e2 100644 --- a/x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/webhook.ts +++ b/x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/webhook.ts @@ -212,8 +212,8 @@ export default function webhookTest({ getService }: FtrProviderContext) { .expect(200); expect(result.status).to.eql('error'); - expect(result.message).to.match(/error calling webhook, invalid response/); - expect(result.serviceMessage).to.eql('[400] Bad Request'); + expect(result.message).to.match(/error calling webhook, retry later/); + expect(result.serviceMessage).to.eql('[500] Internal Server Error'); }); }); } diff --git a/x-pack/test/alerting_api_integration/spaces_only/tests/actions/builtin_action_types/webhook.ts b/x-pack/test/alerting_api_integration/spaces_only/tests/actions/builtin_action_types/webhook.ts new file mode 100644 index 0000000000000..5122a74d53b72 --- /dev/null +++ b/x-pack/test/alerting_api_integration/spaces_only/tests/actions/builtin_action_types/webhook.ts @@ -0,0 +1,75 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import expect from '@kbn/expect'; +import { URL, format as formatUrl } from 'url'; +import { FtrProviderContext } from '../../../../common/ftr_provider_context'; +import { + getExternalServiceSimulatorPath, + ExternalServiceSimulator, +} from '../../../../common/fixtures/plugins/actions'; + +// eslint-disable-next-line import/no-default-export +export default function webhookTest({ getService }: FtrProviderContext) { + const supertest = getService('supertest'); + const esArchiver = getService('esArchiver'); + const kibanaServer = getService('kibanaServer'); + + async function createWebhookAction( + urlWithCreds: string, + config: Record> = {} + ): Promise { + const url = formatUrl(new URL(urlWithCreds), { auth: false }); + const composedConfig = { + headers: { + 'Content-Type': 'text/plain', + }, + ...config, + url, + }; + + const { body: createdAction } = await supertest + .post('/api/action') + .set('kbn-xsrf', 'test') + .send({ + name: 'A generic Webhook action', + actionTypeId: '.webhook', + secrets: {}, + config: composedConfig, + }) + .expect(200); + + return createdAction.id; + } + + describe('webhook action', () => { + let webhookSimulatorURL: string = ''; + + // need to wait for kibanaServer to settle ... + before(() => { + webhookSimulatorURL = kibanaServer.resolveUrl( + getExternalServiceSimulatorPath(ExternalServiceSimulator.WEBHOOK) + ); + }); + + after(() => esArchiver.unload('empty_kibana')); + + it('webhook can be executed without username and password', async () => { + const webhookActionId = await createWebhookAction(webhookSimulatorURL); + const { body: result } = await supertest + .post(`/api/action/${webhookActionId}/_execute`) + .set('kbn-xsrf', 'test') + .send({ + params: { + body: 'success', + }, + }) + .expect(200); + + expect(result.status).to.eql('ok'); + }); + }); +} diff --git a/x-pack/test/alerting_api_integration/spaces_only/tests/actions/index.ts b/x-pack/test/alerting_api_integration/spaces_only/tests/actions/index.ts index accee08a00c61..fb2be8c86f4e8 100644 --- a/x-pack/test/alerting_api_integration/spaces_only/tests/actions/index.ts +++ b/x-pack/test/alerting_api_integration/spaces_only/tests/actions/index.ts @@ -17,6 +17,7 @@ export default function actionsTests({ loadTestFile }: FtrProviderContext) { loadTestFile(require.resolve('./update')); loadTestFile(require.resolve('./execute')); loadTestFile(require.resolve('./builtin_action_types/es_index')); + loadTestFile(require.resolve('./builtin_action_types/webhook')); loadTestFile(require.resolve('./type_not_enabled')); }); }