From 2273d9e13b4a5ddba3d0c90f52b276507abad527 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mike=20C=C3=B4t=C3=A9?= Date: Tue, 2 Feb 2021 07:21:41 -0500 Subject: [PATCH] Add support for custom alert ids (#89814) (#89911) * Add support for custom alert ids * UUID v4 also supported * Change ESO custom id error message * Update api integration test * Use errors.createBadRequestError --- .../server/alerts_client/alerts_client.ts | 3 +- .../server/alerts_client/tests/create.test.ts | 67 +++++++++++++++++ .../alerts/server/routes/create.test.ts | 73 ++++++++++++++++++- x-pack/plugins/alerts/server/routes/create.ts | 9 ++- ...ypted_saved_objects_client_wrapper.test.ts | 8 +- .../encrypted_saved_objects_client_wrapper.ts | 44 +++++------ .../spaces_only/tests/alerting/create.ts | 69 ++++++++++++++++++ 7 files changed, 244 insertions(+), 29 deletions(-) diff --git a/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts b/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts index 457079229de94..569b54f21f906 100644 --- a/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts +++ b/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts @@ -149,6 +149,7 @@ export interface CreateOptions { | 'executionStatus' > & { actions: NormalizedAlertAction[] }; options?: { + id?: string; migrationVersion?: Record; }; } @@ -226,7 +227,7 @@ export class AlertsClient { data, options, }: CreateOptions): Promise> { - const id = SavedObjectsUtils.generateId(); + const id = options?.id || SavedObjectsUtils.generateId(); try { await this.authorization.ensureAuthorized( diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/create.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/create.test.ts index 0424a1295c9b9..2e3dac76f72e5 100644 --- a/x-pack/plugins/alerts/server/alerts_client/tests/create.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client/tests/create.test.ts @@ -462,6 +462,73 @@ describe('create()', () => { expect(actionsClient.isActionTypeEnabled).toHaveBeenCalledWith('test', { notifyUsage: true }); }); + test('creates an alert with a custom id', async () => { + const data = getMockData(); + const createdAttributes = { + ...data, + alertTypeId: '123', + schedule: { interval: '10s' }, + params: { + bar: true, + }, + createdAt: '2019-02-12T21:01:22.479Z', + createdBy: 'elastic', + updatedBy: 'elastic', + updatedAt: '2019-02-12T21:01:22.479Z', + muteAll: false, + mutedInstanceIds: [], + actions: [ + { + group: 'default', + actionRef: 'action_0', + actionTypeId: 'test', + params: { + foo: true, + }, + }, + ], + }; + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ + id: '123', + type: 'alert', + attributes: createdAttributes, + 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, + }); + const result = await alertsClient.create({ data, options: { id: '123' } }); + expect(result.id).toEqual('123'); + expect(unsecuredSavedObjectsClient.create.mock.calls[0][2]).toMatchInlineSnapshot(` + Object { + "id": "123", + "references": Array [ + Object { + "id": "1", + "name": "action_0", + "type": "action", + }, + ], + } + `); + }); + test('creates an alert with multiple actions', async () => { const data = getMockData({ actions: [ diff --git a/x-pack/plugins/alerts/server/routes/create.test.ts b/x-pack/plugins/alerts/server/routes/create.test.ts index fc531821f25b6..d0e21ac99a264 100644 --- a/x-pack/plugins/alerts/server/routes/create.test.ts +++ b/x-pack/plugins/alerts/server/routes/create.test.ts @@ -82,7 +82,7 @@ describe('createAlertRoute', () => { const [config, handler] = router.post.mock.calls[0]; - expect(config.path).toMatchInlineSnapshot(`"/api/alerts/alert"`); + expect(config.path).toMatchInlineSnapshot(`"/api/alerts/alert/{id?}"`); alertsClient.create.mockResolvedValueOnce(createResult); @@ -125,6 +125,9 @@ describe('createAlertRoute', () => { ], "throttle": "30s", }, + "options": Object { + "id": undefined, + }, }, ] `); @@ -134,6 +137,74 @@ describe('createAlertRoute', () => { }); }); + it('allows providing a custom id', async () => { + const expectedResult = { + ...createResult, + id: 'custom-id', + }; + const licenseState = licenseStateMock.create(); + const router = httpServiceMock.createRouter(); + + createAlertRoute(router, licenseState); + + const [config, handler] = router.post.mock.calls[0]; + + expect(config.path).toMatchInlineSnapshot(`"/api/alerts/alert/{id?}"`); + + alertsClient.create.mockResolvedValueOnce(expectedResult); + + const [context, req, res] = mockHandlerArguments( + { alertsClient }, + { + params: { id: 'custom-id' }, + body: mockedAlert, + }, + ['ok'] + ); + + expect(await handler(context, req, res)).toEqual({ body: expectedResult }); + + expect(alertsClient.create).toHaveBeenCalledTimes(1); + expect(alertsClient.create.mock.calls[0]).toMatchInlineSnapshot(` + Array [ + Object { + "data": Object { + "actions": Array [ + Object { + "group": "default", + "id": "2", + "params": Object { + "foo": true, + }, + }, + ], + "alertTypeId": "1", + "consumer": "bar", + "name": "abc", + "notifyWhen": "onActionGroupChange", + "params": Object { + "bar": true, + }, + "schedule": Object { + "interval": "10s", + }, + "tags": Array [ + "foo", + ], + "throttle": "30s", + }, + "options": Object { + "id": "custom-id", + }, + }, + ] + `); + + expect(res.ok).toHaveBeenCalledWith({ + body: expectedResult, + }); + }); + it('ensures the license allows creating alerts', async () => { const licenseState = licenseStateMock.create(); const router = httpServiceMock.createRouter(); diff --git a/x-pack/plugins/alerts/server/routes/create.ts b/x-pack/plugins/alerts/server/routes/create.ts index 2b6735d9063df..46151893baef5 100644 --- a/x-pack/plugins/alerts/server/routes/create.ts +++ b/x-pack/plugins/alerts/server/routes/create.ts @@ -45,8 +45,13 @@ export const bodySchema = schema.object({ export const createAlertRoute = (router: AlertingRouter, licenseState: ILicenseState) => { router.post( { - path: `${BASE_ALERT_API_PATH}/alert`, + path: `${BASE_ALERT_API_PATH}/alert/{id?}`, validate: { + params: schema.maybe( + schema.object({ + id: schema.maybe(schema.string()), + }) + ), body: bodySchema, }, }, @@ -59,10 +64,12 @@ export const createAlertRoute = (router: AlertingRouter, licenseState: ILicenseS } const alertsClient = context.alerting.getAlertsClient(); const alert = req.body; + const params = req.params; const notifyWhen = alert?.notifyWhen ? (alert.notifyWhen as AlertNotifyWhenType) : null; try { const alertRes: Alert = await alertsClient.create({ data: { ...alert, notifyWhen }, + options: { id: params?.id }, }); return res.ok({ body: alertRes, diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts index 90700f8fa7521..7ec700607f3e0 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts @@ -83,11 +83,11 @@ describe('#create', () => { expect(mockBaseClient.create).toHaveBeenCalledWith('unknown-type', attributes, options); }); - it('fails if type is registered and ID is specified', async () => { + it('fails if type is registered and non-UUID ID is specified', async () => { const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }; await expect(wrapper.create('known-type', attributes, { id: 'some-id' })).rejects.toThrowError( - 'Predefined IDs are not allowed for saved objects with encrypted attributes, unless the ID has been generated using `SavedObjectsUtils.generateId`.' + 'Predefined IDs are not allowed for saved objects with encrypted attributes unless the ID is a UUID.' ); expect(mockBaseClient.create).not.toHaveBeenCalled(); @@ -310,7 +310,7 @@ describe('#bulkCreate', () => { ); }); - it('fails if ID is specified for registered type', async () => { + it('fails if non-UUID ID is specified for registered type', async () => { const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }; const bulkCreateParams = [ @@ -319,7 +319,7 @@ describe('#bulkCreate', () => { ]; await expect(wrapper.bulkCreate(bulkCreateParams)).rejects.toThrowError( - 'Predefined IDs are not allowed for saved objects with encrypted attributes, unless the ID has been generated using `SavedObjectsUtils.generateId`.' + 'Predefined IDs are not allowed for saved objects with encrypted attributes unless the ID is a UUID.' ); expect(mockBaseClient.bulkCreate).not.toHaveBeenCalled(); diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts index c3008a8e86505..21475f6a4f5d2 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts @@ -59,7 +59,7 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon return await this.options.baseClient.create(type, attributes, options); } - const id = getValidId(options.id, options.version, options.overwrite); + const id = this.getValidId(options.id, options.version, options.overwrite); const namespace = getDescriptorNamespace( this.options.baseTypeRegistry, type, @@ -93,7 +93,7 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon return object; } - const id = getValidId(object.id, object.version, options?.overwrite); + const id = this.getValidId(object.id, object.version, options?.overwrite); const namespace = getDescriptorNamespace( this.options.baseTypeRegistry, object.type, @@ -307,27 +307,27 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon return response; } -} -// Saved objects with encrypted attributes should have IDs that are hard to guess especially -// since IDs are part of the AAD used during encryption, that's why we control them within this -// wrapper and don't allow consumers to specify their own IDs directly unless overwriting the original document. -function getValidId( - id: string | undefined, - version: string | undefined, - overwrite: boolean | undefined -) { - if (id) { - // only allow a specified ID if we're overwriting an existing ESO with a Version - // this helps us ensure that the document really was previously created using ESO - // and not being used to get around the specified ID limitation - const canSpecifyID = (overwrite && version) || SavedObjectsUtils.isRandomId(id); - if (!canSpecifyID) { - throw new Error( - 'Predefined IDs are not allowed for saved objects with encrypted attributes, unless the ID has been generated using `SavedObjectsUtils.generateId`.' - ); + // Saved objects with encrypted attributes should have IDs that are hard to guess especially + // since IDs are part of the AAD used during encryption, that's why we control them within this + // wrapper and don't allow consumers to specify their own IDs directly unless overwriting the original document. + private getValidId( + id: string | undefined, + version: string | undefined, + overwrite: boolean | undefined + ) { + if (id) { + // only allow a specified ID if we're overwriting an existing ESO with a Version + // this helps us ensure that the document really was previously created using ESO + // and not being used to get around the specified ID limitation + const canSpecifyID = (overwrite && version) || SavedObjectsUtils.isRandomId(id); + if (!canSpecifyID) { + throw this.errors.createBadRequestError( + 'Predefined IDs are not allowed for saved objects with encrypted attributes unless the ID is a UUID.' + ); + } + return id; } - return id; + return SavedObjectsUtils.generateId(); } - return SavedObjectsUtils.generateId(); } diff --git a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/create.ts b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/create.ts index 8bf0a2a0f034f..7e25707c10c78 100644 --- a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/create.ts +++ b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/create.ts @@ -111,6 +111,75 @@ export default function createAlertTests({ getService }: FtrProviderContext) { }); }); + it('should allow providing custom saved object ids (uuid v1)', async () => { + const customId = '09570bb0-6299-11eb-8fde-9fe5ce6ea450'; + const response = await supertest + .post(`${getUrlPrefix(Spaces.space1.id)}/api/alerts/alert/${customId}`) + .set('kbn-xsrf', 'foo') + .send(getTestAlertData()); + + expect(response.status).to.eql(200); + objectRemover.add(Spaces.space1.id, response.body.id, 'alert', 'alerts'); + expect(response.body.id).to.eql(customId); + // Ensure AAD isn't broken + await checkAAD({ + supertest, + spaceId: Spaces.space1.id, + type: 'alert', + id: customId, + }); + }); + + it('should allow providing custom saved object ids (uuid v4)', async () => { + const customId = 'b3bc6d83-3192-4ffd-9702-ad4fb88617ba'; + const response = await supertest + .post(`${getUrlPrefix(Spaces.space1.id)}/api/alerts/alert/${customId}`) + .set('kbn-xsrf', 'foo') + .send(getTestAlertData()); + + expect(response.status).to.eql(200); + objectRemover.add(Spaces.space1.id, response.body.id, 'alert', 'alerts'); + expect(response.body.id).to.eql(customId); + // Ensure AAD isn't broken + await checkAAD({ + supertest, + spaceId: Spaces.space1.id, + type: 'alert', + id: customId, + }); + }); + + it('should not allow providing simple custom ids (non uuid)', async () => { + const customId = '1'; + const response = await supertest + .post(`${getUrlPrefix(Spaces.space1.id)}/api/alerts/alert/${customId}`) + .set('kbn-xsrf', 'foo') + .send(getTestAlertData()); + + expect(response.status).to.eql(400); + expect(response.body).to.eql({ + statusCode: 400, + error: 'Bad Request', + message: + 'Predefined IDs are not allowed for saved objects with encrypted attributes unless the ID is a UUID.: Bad Request', + }); + }); + + it('should return 409 when document with id already exists', async () => { + const customId = '5031f8f0-629a-11eb-b500-d1931a8e5df7'; + const createdAlertResponse = await supertest + .post(`${getUrlPrefix(Spaces.space1.id)}/api/alerts/alert/${customId}`) + .set('kbn-xsrf', 'foo') + .send(getTestAlertData()) + .expect(200); + objectRemover.add(Spaces.space1.id, createdAlertResponse.body.id, 'alert', 'alerts'); + await supertest + .post(`${getUrlPrefix(Spaces.space1.id)}/api/alerts/alert/${customId}`) + .set('kbn-xsrf', 'foo') + .send(getTestAlertData()) + .expect(409); + }); + it('should handle create alert request appropriately when consumer is unknown', async () => { const response = await supertest .post(`${getUrlPrefix(Spaces.space1.id)}/api/alerts/alert`)