Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Webhook action - make user and password secrets optional #56823

Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions x-pack/plugins/actions/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`

Expand Down
106 changes: 98 additions & 8 deletions x-pack/plugins/actions/server/builtin_action_types/webhook.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<Logger>;

Expand All @@ -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({});
});
});
});

Expand Down Expand Up @@ -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",
}
`);
});
});
36 changes: 24 additions & 12 deletions x-pack/plugins/actions/server/builtin_action_types/webhook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -34,10 +34,20 @@ const ConfigSchema = schema.object(configSchemaProps);
type ActionTypeConfigType = TypeOf<typeof ConfigSchema>;

// secrets definition
type ActionTypeSecretsType = TypeOf<typeof SecretsSchema>;
const SecretsSchema = schema.object({
user: schema.string(),
password: schema.string(),
export type ActionTypeSecretsType = TypeOf<typeof SecretsSchema>;
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', {
peterschretlen marked this conversation as resolved.
Show resolved Hide resolved
defaultMessage: 'both user and password must be specified',
});
},
});

// params definition
Expand All @@ -61,7 +71,7 @@ export function getActionType({
}),
validate: {
config: schema.object(configSchemaProps, {
validate: curry(valdiateActionTypeConfig)(configurationUtilities),
validate: curry(validateActionTypeConfig)(configurationUtilities),
}),
secrets: SecretsSchema,
params: ParamsSchema,
Expand All @@ -70,7 +80,7 @@ export function getActionType({
};
}

function valdiateActionTypeConfig(
function validateActionTypeConfig(
configurationUtilities: ActionsConfigurationUtilities,
configObject: ActionTypeConfigType
) {
Expand All @@ -93,17 +103,19 @@ export async function executor(
): Promise<ActionTypeExecutorResult> {
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<AxiosResponse, AxiosError> = await promiseResult(
axios.request({
method,
url,
auth: {
username,
password,
},
...basicAuth,
headers,
data,
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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`);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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/);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expected result here was incorrect due to a typo in the webhook simulator when the body is failure

expect(result.serviceMessage).to.eql('[500] Internal Server Error');
});
});
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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<string, string | Record<string, string>> = {}
): Promise<string> {
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 = '<could not determine kibana url>';

// 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 () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is in the spaces_only functional test config because it does not use security, so the webhook simulator can be used without auth.

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');
});
});
}
Original file line number Diff line number Diff line change
Expand Up @@ -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'));
});
}