From 57d85096294b5ccb00322711e37ee75a711b24f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mike=20C=C3=B4t=C3=A9?= Date: Thu, 27 Aug 2020 18:01:33 -0400 Subject: [PATCH] Fix alerts unable to create / update when the name has trailing whitepace(s) (#76079) (#76170) * Trim alert name in API key name * Add API integration tests --- .../alerts/server/alerts_client.test.ts | 128 +++++++++++++++++- x-pack/plugins/alerts/server/alerts_client.ts | 4 +- .../tests/alerting/create.ts | 39 ++++++ .../tests/alerting/update.ts | 51 +++++++ 4 files changed, 219 insertions(+), 3 deletions(-) diff --git a/x-pack/plugins/alerts/server/alerts_client.test.ts b/x-pack/plugins/alerts/server/alerts_client.test.ts index d994269366ae6c..f4aef62657abc0 100644 --- a/x-pack/plugins/alerts/server/alerts_client.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client.test.ts @@ -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({ @@ -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', @@ -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', @@ -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({ diff --git a/x-pack/plugins/alerts/server/alerts_client.ts b/x-pack/plugins/alerts/server/alerts_client.ts index 80e021fc5cb6e1..74aef644d58caa 100644 --- a/x-pack/plugins/alerts/server/alerts_client.ts +++ b/x-pack/plugins/alerts/server/alerts_client.ts @@ -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, @@ -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 }); } } diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/create.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/create.ts index 8d7b9dec58cf1c..983f87405a1a64 100644 --- a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/create.ts +++ b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/create.ts @@ -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`) diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/update.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/update.ts index ab3a92d0b3f706..48269cc1c4498f 100644 --- a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/update.ts +++ b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/update.ts @@ -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`)