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

[7.x] [Actions] Adding hasAuth to Webhook Configuration to avoid confusing UX (#81390) #81751

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all 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
13 changes: 10 additions & 3 deletions x-pack/plugins/actions/server/builtin_action_types/webhook.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,9 @@ describe('config validation', () => {
};

test('config validation passes when only required fields are provided', () => {
const config: Record<string, string> = {
const config: Record<string, string | boolean> = {
url: 'http://mylisteningserver:9200/endpoint',
hasAuth: true,
};
expect(validateConfig(actionType, config)).toEqual({
...defaultValues,
Expand All @@ -101,9 +102,10 @@ describe('config validation', () => {

test('config validation passes when valid methods are provided', () => {
['post', 'put'].forEach((method) => {
const config: Record<string, string> = {
const config: Record<string, string | boolean> = {
url: 'http://mylisteningserver:9200/endpoint',
method,
hasAuth: true,
};
expect(validateConfig(actionType, config)).toEqual({
...defaultValues,
Expand All @@ -127,8 +129,9 @@ describe('config validation', () => {
});

test('config validation passes when a url is specified', () => {
const config: Record<string, string> = {
const config: Record<string, string | boolean> = {
url: 'http://mylisteningserver:9200/endpoint',
hasAuth: true,
};
expect(validateConfig(actionType, config)).toEqual({
...defaultValues,
Expand All @@ -155,6 +158,7 @@ describe('config validation', () => {
headers: {
'Content-Type': 'application/json',
},
hasAuth: true,
};
expect(validateConfig(actionType, config)).toEqual({
...defaultValues,
Expand Down Expand Up @@ -184,6 +188,7 @@ describe('config validation', () => {
headers: {
'Content-Type': 'application/json',
},
hasAuth: true,
};

expect(validateConfig(actionType, config)).toEqual({
Expand Down Expand Up @@ -263,6 +268,7 @@ describe('execute()', () => {
headers: {
aheader: 'a value',
},
hasAuth: true,
};
await actionType.executor({
actionId: 'some-id',
Expand Down Expand Up @@ -320,6 +326,7 @@ describe('execute()', () => {
headers: {
aheader: 'a value',
},
hasAuth: false,
};
const secrets: ActionTypeSecretsType = { user: null, password: null };
await actionType.executor({
Expand Down
5 changes: 3 additions & 2 deletions x-pack/plugins/actions/server/builtin_action_types/webhook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ const configSchemaProps = {
defaultValue: WebhookMethods.POST,
}),
headers: nullableType(HeadersSchema),
hasAuth: schema.boolean({ defaultValue: true }),
};
const ConfigSchema = schema.object(configSchemaProps);
export type ActionTypeConfigType = TypeOf<typeof ConfigSchema>;
Expand Down Expand Up @@ -128,12 +129,12 @@ export async function executor(
execOptions: WebhookActionTypeExecutorOptions
): Promise<ActionTypeExecutorResult<unknown>> {
const actionId = execOptions.actionId;
const { method, url, headers = {} } = execOptions.config;
const { method, url, headers = {}, hasAuth } = execOptions.config;
const { body: data } = execOptions.params;

const secrets: ActionTypeSecretsType = execOptions.secrets;
const basicAuth =
isString(secrets.user) && isString(secrets.password)
hasAuth && isString(secrets.user) && isString(secrets.password)
? { auth: { username: secrets.user, password: secrets.password } }
: {};

Expand Down
57 changes: 57 additions & 0 deletions x-pack/plugins/actions/server/saved_objects/migrations.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,63 @@ describe('7.10.0', () => {
});
});

describe('7.11.0', () => {
beforeEach(() => {
jest.resetAllMocks();
encryptedSavedObjectsSetup.createMigration.mockImplementation(
(shouldMigrateWhenPredicate, migration) => migration
);
});

test('add hasAuth = true for .webhook actions with user and password', () => {
const migration711 = getMigrations(encryptedSavedObjectsSetup)['7.11.0'];
const action = getMockDataForWebhook({}, true);
expect(migration711(action, context)).toMatchObject({
...action,
attributes: {
...action.attributes,
config: {
hasAuth: true,
},
},
});
});

test('add hasAuth = false for .webhook actions without user and password', () => {
const migration711 = getMigrations(encryptedSavedObjectsSetup)['7.11.0'];
const action = getMockDataForWebhook({}, false);
expect(migration711(action, context)).toMatchObject({
...action,
attributes: {
...action.attributes,
config: {
hasAuth: false,
},
},
});
});
});

function getMockDataForWebhook(
overwrites: Record<string, unknown> = {},
hasUserAndPassword: boolean
): SavedObjectUnsanitizedDoc<RawAction> {
const secrets = hasUserAndPassword
? { user: 'test', password: '123' }
: { user: '', password: '' };
return {
attributes: {
name: 'abc',
actionTypeId: '.webhook',
config: {},
secrets,
...overwrites,
},
id: uuid.v4(),
type: 'action',
};
}

function getMockDataForEmail(
overwrites: Record<string, unknown> = {}
): SavedObjectUnsanitizedDoc<RawAction> {
Expand Down
10 changes: 10 additions & 0 deletions x-pack/plugins/actions/server/saved_objects/migrations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,18 @@ export function getMigrations(
pipeMigrations(renameCasesConfigurationObject, addHasAuthConfigurationObject)
);

const migrationWebhookConnectorHasAuth = encryptedSavedObjects.createMigration<
RawAction,
RawAction
>(
(doc): doc is SavedObjectUnsanitizedDoc<RawAction> =>
doc.attributes.actionTypeId === '.webhook',
pipeMigrations(addHasAuthConfigurationObject)
);

return {
'7.10.0': executeMigrationWithErrorHandling(migrationActions, '7.10.0'),
'7.11.0': executeMigrationWithErrorHandling(migrationWebhookConnectorHasAuth, '7.11.0'),
};
}

Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/translations/translations/ja-JP.json
Original file line number Diff line number Diff line change
Expand Up @@ -20258,7 +20258,6 @@
"xpack.triggersActionsUI.sections.actionsConnectorsList.unableToLoadActionTypesMessage": "アクションタイプを読み込めません",
"xpack.triggersActionsUI.sections.addAction.webhookAction.error.requiredHeaderKeyText": "キーが必要です。",
"xpack.triggersActionsUI.sections.addAction.webhookAction.error.requiredHeaderValueText": "値が必要です。",
"xpack.triggersActionsUI.sections.addAction.webhookAction.error.requiredHostText": "ユーザー名が必要です。",
"xpack.triggersActionsUI.sections.addAction.webhookAction.error.requiredMethodText": "メソッドが必要です",
"xpack.triggersActionsUI.sections.addAction.webhookAction.error.requiredPasswordText": "パスワードが必要です。",
"xpack.triggersActionsUI.sections.addAlert.error.greaterThenThreshold0Text": "しきい値 1 はしきい値 0 よりも大きい値にしてください。",
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/translations/translations/zh-CN.json
Original file line number Diff line number Diff line change
Expand Up @@ -20278,7 +20278,6 @@
"xpack.triggersActionsUI.sections.actionsConnectorsList.unableToLoadActionTypesMessage": "无法加载操作类型",
"xpack.triggersActionsUI.sections.addAction.webhookAction.error.requiredHeaderKeyText": "“键”必填。",
"xpack.triggersActionsUI.sections.addAction.webhookAction.error.requiredHeaderValueText": "“值”必填。",
"xpack.triggersActionsUI.sections.addAction.webhookAction.error.requiredHostText": "“用户名”必填。",
"xpack.triggersActionsUI.sections.addAction.webhookAction.error.requiredMethodText": "“方法”必填",
"xpack.triggersActionsUI.sections.addAction.webhookAction.error.requiredPasswordText": "“密码”必填。",
"xpack.triggersActionsUI.sections.addAlert.error.greaterThenThreshold0Text": "阈值 1 应 > 阈值 0。",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ export interface WebhookConfig {
method: string;
url: string;
headers: Record<string, string>;
hasAuth: boolean;
}

export interface WebhookSecrets {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ describe('actionTypeRegistry.get() works', () => {
});

describe('webhook connector validation', () => {
test('connector validation succeeds when connector config is valid', () => {
test('connector validation succeeds when hasAuth is true and connector config is valid', () => {
const actionConnector = {
secrets: {
user: 'user',
Expand All @@ -42,6 +42,35 @@ describe('webhook connector validation', () => {
method: 'PUT',
url: 'http://test.com',
headers: { 'content-type': 'text' },
hasAuth: true,
},
} as WebhookActionConnector;

expect(actionTypeModel.validateConnector(actionConnector)).toEqual({
errors: {
url: [],
method: [],
user: [],
password: [],
},
});
});

test('connector validation succeeds when hasAuth is false and connector config is valid', () => {
const actionConnector = {
secrets: {
user: '',
password: '',
},
id: 'test',
actionTypeId: '.webhook',
name: 'webhook',
isPreconfigured: false,
config: {
method: 'PUT',
url: 'http://test.com',
headers: { 'content-type': 'text' },
hasAuth: false,
},
} as WebhookActionConnector;

Expand All @@ -65,6 +94,7 @@ describe('webhook connector validation', () => {
name: 'webhook',
config: {
method: 'PUT',
hasAuth: true,
},
} as WebhookActionConnector;

Expand All @@ -73,7 +103,7 @@ describe('webhook connector validation', () => {
url: ['URL is required.'],
method: [],
user: [],
password: ['Password is required.'],
password: ['Password is required when username is used.'],
},
});
});
Expand All @@ -90,6 +120,7 @@ describe('webhook connector validation', () => {
config: {
method: 'PUT',
url: 'invalid.url',
hasAuth: true,
},
} as WebhookActionConnector;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,26 +74,46 @@ export function getActionType(): ActionTypeModel<
)
);
}
if (!action.secrets.user && action.secrets.password) {
if (action.config.hasAuth && !action.secrets.user && !action.secrets.password) {
errors.user.push(
i18n.translate(
'xpack.triggersActionsUI.sections.addAction.webhookAction.error.requiredHostText',
'xpack.triggersActionsUI.sections.addAction.webhookAction.error.requiredAuthUserNameText',
{
defaultMessage: 'Username is required.',
}
)
);
}
if (!action.secrets.password && action.secrets.user) {
if (action.config.hasAuth && !action.secrets.user && !action.secrets.password) {
errors.password.push(
i18n.translate(
'xpack.triggersActionsUI.sections.addAction.webhookAction.error.requiredPasswordText',
'xpack.triggersActionsUI.sections.addAction.webhookAction.error.requiredAuthPasswordText',
{
defaultMessage: 'Password is required.',
}
)
);
}
if (action.secrets.user && !action.secrets.password) {
errors.password.push(
i18n.translate(
'xpack.triggersActionsUI.sections.addAction.webhookAction.error.requiredPasswordText',
{
defaultMessage: 'Password is required when username is used.',
}
)
);
}
if (!action.secrets.user && action.secrets.password) {
errors.user.push(
i18n.translate(
'xpack.triggersActionsUI.sections.addAction.webhookAction.error.requiredUserText',
{
defaultMessage: 'Username is required when password is used.',
}
)
);
}
return validationResult;
},
validateParams: (actionParams: WebhookActionParams): ValidationResult => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ describe('WebhookActionConnectorFields renders', () => {
method: 'PUT',
url: 'http:\\test',
headers: { 'content-type': 'text' },
hasAuth: true,
},
} as WebhookActionConnector;
const wrapper = mountWithIntl(
Expand All @@ -50,7 +51,9 @@ describe('WebhookActionConnectorFields renders', () => {
secrets: {},
actionTypeId: '.webhook',
isPreconfigured: false,
config: {},
config: {
hasAuth: true,
},
} as WebhookActionConnector;
const wrapper = mountWithIntl(
<WebhookActionConnectorFields
Expand Down Expand Up @@ -80,6 +83,7 @@ describe('WebhookActionConnectorFields renders', () => {
method: 'PUT',
url: 'http:\\test',
headers: { 'content-type': 'text' },
hasAuth: true,
},
} as WebhookActionConnector;
const wrapper = mountWithIntl(
Expand Down
Loading