From cb593154dc97049bf011e57ac79f9214ff714f80 Mon Sep 17 00:00:00 2001 From: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Date: Tue, 27 Apr 2021 09:51:46 -0400 Subject: [PATCH] [Alerting] Return `400 Bad Request` errors when creating/enabling/updating rules using API key authentication (#98088) (#98465) * Catching API key creation errors and throwing bad request errors instead * Catching API key creation errors and throwing bad request errors instead * Adding warning to docs * Updating error messages * Updating tests * Updating warning wording in docs Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: ymao1 --- docs/api/alerting/create_rule.asciidoc | 2 + docs/api/alerting/enable_rule.asciidoc | 2 + docs/api/alerting/update_rule.asciidoc | 2 + .../server/alerts_client/alerts_client.ts | 56 ++++++++++++++----- .../server/alerts_client/tests/create.test.ts | 12 ++++ .../server/alerts_client/tests/enable.test.ts | 11 ++++ .../server/alerts_client/tests/update.test.ts | 47 ++++++++++++++++ .../tests/update_api_key.test.ts | 21 ++++++- 8 files changed, 136 insertions(+), 17 deletions(-) diff --git a/docs/api/alerting/create_rule.asciidoc b/docs/api/alerting/create_rule.asciidoc index 01b6dfc40fcf6..59b17c5c3b5e1 100644 --- a/docs/api/alerting/create_rule.asciidoc +++ b/docs/api/alerting/create_rule.asciidoc @@ -6,6 +6,8 @@ Create {kib} rules. +WARNING: This API supports <> only. + [[create-rule-api-request]] ==== Request diff --git a/docs/api/alerting/enable_rule.asciidoc b/docs/api/alerting/enable_rule.asciidoc index 60f18b3510904..112d4bbf61faa 100644 --- a/docs/api/alerting/enable_rule.asciidoc +++ b/docs/api/alerting/enable_rule.asciidoc @@ -6,6 +6,8 @@ Enable a rule. +WARNING: This API supports <> only. + [[enable-rule-api-request]] ==== Request diff --git a/docs/api/alerting/update_rule.asciidoc b/docs/api/alerting/update_rule.asciidoc index 76c88a009be01..ec82e60a8e879 100644 --- a/docs/api/alerting/update_rule.asciidoc +++ b/docs/api/alerting/update_rule.asciidoc @@ -6,6 +6,8 @@ Update the attributes for an existing rule. +WARNING: This API supports <> only. + [[update-rule-api-request]] ==== Request diff --git a/x-pack/plugins/alerting/server/alerts_client/alerts_client.ts b/x-pack/plugins/alerting/server/alerts_client/alerts_client.ts index 210bdf954ada4..1db990edef2a9 100644 --- a/x-pack/plugins/alerting/server/alerts_client/alerts_client.ts +++ b/x-pack/plugins/alerting/server/alerts_client/alerts_client.ts @@ -260,9 +260,14 @@ export class AlertsClient { ); const username = await this.getUserName(); - const createdAPIKey = data.enabled - ? await this.createAPIKey(this.generateAPIKeyName(alertType.id, data.name)) - : null; + let createdAPIKey = null; + try { + createdAPIKey = data.enabled + ? await this.createAPIKey(this.generateAPIKeyName(alertType.id, data.name)) + : null; + } catch (error) { + throw Boom.badRequest(`Error creating rule: could not create API key - ${error.message}`); + } this.validateActions(alertType, data.actions); @@ -727,9 +732,16 @@ export class AlertsClient { const { actions, references } = await this.denormalizeActions(data.actions); const username = await this.getUserName(); - const createdAPIKey = attributes.enabled - ? await this.createAPIKey(this.generateAPIKeyName(alertType.id, data.name)) - : null; + + let createdAPIKey = null; + try { + createdAPIKey = attributes.enabled + ? await this.createAPIKey(this.generateAPIKeyName(alertType.id, data.name)) + : null; + } catch (error) { + throw Boom.badRequest(`Error updating rule: could not create API key - ${error.message}`); + } + const apiKeyAttributes = this.apiKeyAsAlertAttributes(createdAPIKey, username); const notifyWhen = getAlertNotifyWhenType(data.notifyWhen, data.throttle); @@ -837,12 +849,21 @@ export class AlertsClient { } const username = await this.getUserName(); + + let createdAPIKey = null; + try { + createdAPIKey = await this.createAPIKey( + this.generateAPIKeyName(attributes.alertTypeId, attributes.name) + ); + } catch (error) { + throw Boom.badRequest( + `Error updating API key for rule: could not create API key - ${error.message}` + ); + } + const updateAttributes = this.updateMeta({ ...attributes, - ...this.apiKeyAsAlertAttributes( - await this.createAPIKey(this.generateAPIKeyName(attributes.alertTypeId, attributes.name)), - username - ), + ...this.apiKeyAsAlertAttributes(createdAPIKey, username), updatedAt: new Date().toISOString(), updatedBy: username, }); @@ -944,13 +965,20 @@ export class AlertsClient { if (attributes.enabled === false) { const username = await this.getUserName(); + + let createdAPIKey = null; + try { + createdAPIKey = await this.createAPIKey( + this.generateAPIKeyName(attributes.alertTypeId, attributes.name) + ); + } catch (error) { + throw Boom.badRequest(`Error enabling rule: could not create API key - ${error.message}`); + } + const updateAttributes = this.updateMeta({ ...attributes, enabled: true, - ...this.apiKeyAsAlertAttributes( - await this.createAPIKey(this.generateAPIKeyName(attributes.alertTypeId, attributes.name)), - username - ), + ...this.apiKeyAsAlertAttributes(createdAPIKey, username), updatedBy: username, updatedAt: new Date().toISOString(), }); diff --git a/x-pack/plugins/alerting/server/alerts_client/tests/create.test.ts b/x-pack/plugins/alerting/server/alerts_client/tests/create.test.ts index 158c9478e6be1..6f493ced47371 100644 --- a/x-pack/plugins/alerting/server/alerts_client/tests/create.test.ts +++ b/x-pack/plugins/alerting/server/alerts_client/tests/create.test.ts @@ -1701,6 +1701,18 @@ describe('create()', () => { ); }); + test('throws an error if API key creation throws', async () => { + const data = getMockData(); + alertsClientParams.createAPIKey.mockImplementation(() => { + throw new Error('no'); + }); + expect( + async () => await alertsClient.create({ data }) + ).rejects.toThrowErrorMatchingInlineSnapshot( + `"Error creating rule: could not create API key - no"` + ); + }); + test('throws error when ensureActionTypeEnabled throws', async () => { const data = getMockData(); alertTypeRegistry.ensureAlertTypeEnabled.mockImplementation(() => { diff --git a/x-pack/plugins/alerting/server/alerts_client/tests/enable.test.ts b/x-pack/plugins/alerting/server/alerts_client/tests/enable.test.ts index db24d192c7755..7b0d6d7b1f10b 100644 --- a/x-pack/plugins/alerting/server/alerts_client/tests/enable.test.ts +++ b/x-pack/plugins/alerting/server/alerts_client/tests/enable.test.ts @@ -359,6 +359,17 @@ describe('enable()', () => { ); }); + test('throws an error if API key creation throws', async () => { + alertsClientParams.createAPIKey.mockImplementation(() => { + throw new Error('no'); + }); + expect( + async () => await alertsClient.enable({ id: '1' }) + ).rejects.toThrowErrorMatchingInlineSnapshot( + `"Error enabling rule: could not create API key - no"` + ); + }); + test('falls back when failing to getDecryptedAsInternalUser', async () => { encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValue(new Error('Fail')); diff --git a/x-pack/plugins/alerting/server/alerts_client/tests/update.test.ts b/x-pack/plugins/alerting/server/alerts_client/tests/update.test.ts index 24cef4677a9a2..cdbfbbac9f9a1 100644 --- a/x-pack/plugins/alerting/server/alerts_client/tests/update.test.ts +++ b/x-pack/plugins/alerting/server/alerts_client/tests/update.test.ts @@ -692,6 +692,53 @@ describe('update()', () => { `); }); + it('throws an error if API key creation throws', async () => { + alertsClientParams.createAPIKey.mockImplementation(() => { + throw new Error('no'); + }); + expect( + async () => + await alertsClient.update({ + id: '1', + data: { + schedule: { interval: '10s' }, + name: 'abc', + tags: ['foo'], + params: { + bar: true, + }, + throttle: null, + notifyWhen: 'onActiveAlert', + actions: [ + { + group: 'default', + id: '1', + params: { + foo: true, + }, + }, + { + group: 'default', + id: '1', + params: { + foo: true, + }, + }, + { + group: 'default', + id: '2', + params: { + foo: true, + }, + }, + ], + }, + }) + ).rejects.toThrowErrorMatchingInlineSnapshot( + `"Error updating rule: could not create API key - no"` + ); + }); + it('should validate params', async () => { alertTypeRegistry.get.mockReturnValueOnce({ id: '123', diff --git a/x-pack/plugins/alerting/server/alerts_client/tests/update_api_key.test.ts b/x-pack/plugins/alerting/server/alerts_client/tests/update_api_key.test.ts index e0be54054e593..18bae8d34a8da 100644 --- a/x-pack/plugins/alerting/server/alerts_client/tests/update_api_key.test.ts +++ b/x-pack/plugins/alerting/server/alerts_client/tests/update_api_key.test.ts @@ -99,13 +99,13 @@ describe('updateApiKey()', () => { references: [], }); encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue(existingEncryptedAlert); + }); + + test('updates the API key for the alert', async () => { alertsClientParams.createAPIKey.mockResolvedValueOnce({ apiKeysEnabled: true, result: { id: '234', name: '123', api_key: 'abc' }, }); - }); - - test('updates the API key for the alert', async () => { await alertsClient.updateApiKey({ id: '1' }); expect(unsecuredSavedObjectsClient.get).not.toHaveBeenCalled(); expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', { @@ -145,7 +145,22 @@ describe('updateApiKey()', () => { ); }); + test('throws an error if API key creation throws', async () => { + alertsClientParams.createAPIKey.mockImplementation(() => { + throw new Error('no'); + }); + expect( + async () => await alertsClient.updateApiKey({ id: '1' }) + ).rejects.toThrowErrorMatchingInlineSnapshot( + `"Error updating API key for rule: could not create API key - no"` + ); + }); + test('falls back to SOC when getDecryptedAsInternalUser throws an error', async () => { + alertsClientParams.createAPIKey.mockResolvedValueOnce({ + apiKeysEnabled: true, + result: { id: '234', name: '123', api_key: 'abc' }, + }); encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValueOnce(new Error('Fail')); unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1',