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.

Sign up for GitHub

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

Jump to bottom

Fix alerts unable to create / update when the name has trailing whitepace(s) #76079

Merged
merged 2 commits into from
Aug 27, 2020
Merged

Fix alerts unable to create / update when the name has trailing whitepace(s) #76079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1bc964c
Trim alert name in API key name
mikecote Aug 27, 2020
6cb7d98
Add API integration tests
mikecote Aug 27, 2020
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
128 changes: 127 additions & 1 deletion x-pack/plugins/alerts/server/alerts_client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -652,6 +652,70 @@ describe('create()', () => {
expect(taskManager.schedule).toHaveBeenCalledTimes(0);
});

test('should trim alert name when creating API key', async () => {
const data = getMockData({ name: ' my alert name ' });
unsecuredSavedObjectsClient.bulkGet.mockResolvedValueOnce({
saved_objects: [
{
id: '1',
type: 'action',
attributes: {
actions: [],
actionTypeId: 'test',
},
references: [],
},
],
});
unsecuredSavedObjectsClient.create.mockResolvedValueOnce({
id: '1',
type: 'alert',
attributes: {
enabled: false,
name: ' my alert name ',
alertTypeId: '123',
schedule: { interval: 10000 },
params: {
bar: true,
},
createdAt: new Date().toISOString(),
actions: [
{
group: 'default',
actionRef: 'action_0',
actionTypeId: 'test',
params: {
foo: true,
},
},
],
},
references: [
{
name: 'action_0',
type: 'action',
id: '1',
},
],
});
taskManager.schedule.mockResolvedValueOnce({
id: 'task-123',
taskType: 'alerting:123',
scheduledAt: new Date(),
attempts: 1,
status: TaskStatus.Idle,
runAt: new Date(),
startedAt: null,
retryAt: null,
state: {},
params: {},
ownerId: null,
});

await alertsClient.create({ data });
expect(alertsClientParams.createAPIKey).toHaveBeenCalledWith('Alerting: 123/my alert name');
});

test('should validate params', async () => {
const data = getMockData();
alertTypeRegistry.get.mockReturnValue({
Expand Down Expand Up @@ -2896,9 +2960,13 @@ describe('update()', () => {
type: 'alert',
attributes: {
enabled: true,
tags: ['foo'],
alertTypeId: 'myType',
schedule: { interval: '10s' },
consumer: 'myApp',
scheduledTaskId: 'task-123',
params: {},
throttle: null,
actions: [
{
group: 'default',
Expand Down Expand Up @@ -2927,7 +2995,7 @@ describe('update()', () => {
unsecuredSavedObjectsClient.get.mockResolvedValue(existingAlert);
encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue(existingDecryptedAlert);
alertTypeRegistry.get.mockReturnValue({
id: '123',
id: 'myType',
name: 'Test',
actionGroups: [{ id: 'default', name: 'Default' }],
defaultActionGroupId: 'default',
Expand Down Expand Up @@ -3489,6 +3557,64 @@ describe('update()', () => {
);
});

it('should trim alert name in the API key name', async () => {
unsecuredSavedObjectsClient.bulkGet.mockResolvedValueOnce({
saved_objects: [
{
id: '1',
type: 'action',
attributes: {
actions: [],
actionTypeId: 'test',
},
references: [],
},
],
});
unsecuredSavedObjectsClient.update.mockResolvedValueOnce({
id: '1',
type: 'alert',
attributes: {
enabled: false,
name: ' my alert name ',
schedule: { interval: '10s' },
params: {
bar: true,
},
createdAt: new Date().toISOString(),
actions: [
{
group: 'default',
actionRef: 'action_0',
actionTypeId: 'test',
params: {
foo: true,
},
},
],
scheduledTaskId: 'task-123',
apiKey: null,
},
updated_at: new Date().toISOString(),
references: [
{
name: 'action_0',
type: 'action',
id: '1',
},
],
});
await alertsClient.update({
id: '1',
data: {
...existingAlert.attributes,
name: ' my alert name ',
},
});

expect(alertsClientParams.createAPIKey).toHaveBeenCalledWith('Alerting: myType/my alert name');
});

it('swallows error when invalidate API key throws', async () => {
alertsClientParams.invalidateAPIKey.mockRejectedValueOnce(new Error('Fail'));
unsecuredSavedObjectsClient.bulkGet.mockResolvedValueOnce({
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/alerts/server/alerts_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/

import Boom from 'boom';
import { omit, isEqual, map, uniq, pick, truncate } from 'lodash';
import { omit, isEqual, map, uniq, pick, truncate, trim } from 'lodash';
import { i18n } from '@kbn/i18n';
import {
Logger,
Expand Down Expand Up @@ -940,7 +940,7 @@ export class AlertsClient {
}

private generateAPIKeyName(alertTypeId: string, alertName: string) {
return truncate(`Alerting: ${alertTypeId}/${alertName}`, { length: 256 });
return truncate(`Alerting: ${alertTypeId}/${trim(alertName)}`, { length: 256 });
}
}

Expand Down
39 changes: 39 additions & 0 deletions x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/create.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,45 @@ export default function createAlertTests({ getService }: FtrProviderContext) {
}
});

it('should handle create alert request appropriately when alert name has leading and trailing whitespaces', async () => {
const response = await supertestWithoutAuth
.post(`${getUrlPrefix(space.id)}/api/alerts/alert`)
.set('kbn-xsrf', 'foo')
.auth(user.username, user.password)
.send(
getTestAlertData({
name: ' leading and trailing whitespace ',
})
);

switch (scenario.id) {
case 'no_kibana_privileges at space1':
case 'global_read at space1':
case 'space_1_all at space2':
expect(response.statusCode).to.eql(403);
expect(response.body).to.eql({
error: 'Forbidden',
message: getConsumerUnauthorizedErrorMessage(
'create',
'test.noop',
'alertsFixture'
),
statusCode: 403,
});
break;
case 'superuser at space1':
case 'space_1_all at space1':
case 'space_1_all_alerts_none_actions at space1':
case 'space_1_all_with_restricted_fixture at space1':
expect(response.statusCode).to.eql(200);
expect(response.body.name).to.eql(' leading and trailing whitespace ');
objectRemover.add(space.id, response.body.id, 'alert', 'alerts');
break;
default:
throw new Error(`Scenario untested: ${JSON.stringify(scenario)}`);
}
});

it('should handle create alert request appropriately when alert type is unregistered', async () => {
const response = await supertestWithoutAuth
.post(`${getUrlPrefix(space.id)}/api/alerts/alert`)
Expand Down
51 changes: 51 additions & 0 deletions x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/update.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,57 @@ export default function createUpdateTests({ getService }: FtrProviderContext) {
}
});

it('should handle update alert request appropriately when alert name has leading and trailing whitespaces', async () => {
const { body: createdAlert } = await supertest
.post(`${getUrlPrefix(space.id)}/api/alerts/alert`)
.set('kbn-xsrf', 'foo')
.send(getTestAlertData())
.expect(200);
objectRemover.add(space.id, createdAlert.id, 'alert', 'alerts');

const updatedData = {
name: ' leading and trailing whitespace ',
tags: ['bar'],
params: {
foo: true,
},
schedule: { interval: '12s' },
actions: [],
throttle: '1m',
};
const response = await supertestWithoutAuth
.put(`${getUrlPrefix(space.id)}/api/alerts/alert/${createdAlert.id}`)
.set('kbn-xsrf', 'foo')
.auth(user.username, user.password)
.send(updatedData);

switch (scenario.id) {
case 'no_kibana_privileges at space1':
case 'space_1_all at space2':
case 'global_read at space1':
expect(response.statusCode).to.eql(403);
expect(response.body).to.eql({
error: 'Forbidden',
message: getConsumerUnauthorizedErrorMessage(
'update',
'test.noop',
'alertsFixture'
),
statusCode: 403,
});
break;
case 'superuser at space1':
case 'space_1_all at space1':
case 'space_1_all_alerts_none_actions at space1':
case 'space_1_all_with_restricted_fixture at space1':
expect(response.statusCode).to.eql(200);
expect(response.body.name).to.eql(' leading and trailing whitespace ');
break;
default:
throw new Error(`Scenario untested: ${JSON.stringify(scenario)}`);
}
});

it(`shouldn't update alert from another space`, async () => {
const { body: createdAlert } = await supertest
.post(`${getUrlPrefix(space.id)}/api/alerts/alert`)
Expand Down