From be237b3b1e0a878d880575d53508b49f7b9d29c2 Mon Sep 17 00:00:00 2001 From: Ying Mao Date: Thu, 22 Apr 2021 16:20:39 -0400 Subject: [PATCH 1/6] Catching API key creation errors and throwing bad request errors instead --- .../server/alerts_client/alerts_client.ts | 56 ++++++++++++++----- .../alerting/server/alerts_client_factory.ts | 21 ++++--- 2 files changed, 56 insertions(+), 21 deletions(-) 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..edfe1d88dd817 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 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 creating API key - ${error.message}`); + } + const apiKeyAttributes = this.apiKeyAsAlertAttributes(createdAPIKey, username); const notifyWhen = getAlertNotifyWhenType(data.notifyWhen, data.throttle); @@ -837,12 +849,19 @@ export class AlertsClient { } const username = await this.getUserName(); + + let createdAPIKey = null; + try { + createdAPIKey = attributes.enabled + ? await this.createAPIKey(this.generateAPIKeyName(attributes.alertTypeId, attributes.name)) + : null; + } catch (error) { + throw Boom.badRequest(`Error creating 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 +963,22 @@ export class AlertsClient { if (attributes.enabled === false) { const username = await this.getUserName(); + + let createdAPIKey = null; + try { + createdAPIKey = attributes.enabled + ? await this.createAPIKey( + this.generateAPIKeyName(attributes.alertTypeId, attributes.name) + ) + : null; + } catch (error) { + throw Boom.badRequest(`Error creating 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_factory.ts b/x-pack/plugins/alerting/server/alerts_client_factory.ts index 05e50346f56cf..3bb3406792d42 100644 --- a/x-pack/plugins/alerting/server/alerts_client_factory.ts +++ b/x-pack/plugins/alerting/server/alerts_client_factory.ts @@ -5,6 +5,7 @@ * 2.0. */ +import Boom from '@hapi/boom'; import { KibanaRequest, Logger, @@ -116,13 +117,19 @@ export class AlertsClientFactory { if (!securityPluginStart) { return { apiKeysEnabled: false }; } - // Create an API key using the new grant API - in this case the Kibana system user is creating the - // API key for the user, instead of having the user create it themselves, which requires api_key - // privileges - const createAPIKeyResult = await securityPluginStart.authc.apiKeys.grantAsInternalUser( - request, - { name, role_descriptors: {} } - ); + let createAPIKeyResult = null; + try { + // Create an API key using the new grant API - in this case the Kibana system user is creating the + // API key for the user, instead of having the user create it themselves, which requires api_key + // privileges + createAPIKeyResult = await securityPluginStart.authc.apiKeys.grantAsInternalUser( + request, + { name, role_descriptors: {} } + ); + } catch (error) { + throw Boom.badRequest(`Error creating API key - ${error.message}`); + } + if (!createAPIKeyResult) { return { apiKeysEnabled: false }; } From f189fd98d8447edb6caf17376d69a5e29f1b5f00 Mon Sep 17 00:00:00 2001 From: Ying Mao Date: Fri, 23 Apr 2021 10:51:27 -0400 Subject: [PATCH 2/6] Catching API key creation errors and throwing bad request errors instead --- .../server/alerts_client/alerts_client.ts | 14 ++++++------- .../alerting/server/alerts_client_factory.ts | 20 +++++++------------ 2 files changed, 13 insertions(+), 21 deletions(-) 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 edfe1d88dd817..8a4a97adce9c1 100644 --- a/x-pack/plugins/alerting/server/alerts_client/alerts_client.ts +++ b/x-pack/plugins/alerting/server/alerts_client/alerts_client.ts @@ -852,9 +852,9 @@ export class AlertsClient { let createdAPIKey = null; try { - createdAPIKey = attributes.enabled - ? await this.createAPIKey(this.generateAPIKeyName(attributes.alertTypeId, attributes.name)) - : null; + createdAPIKey = await this.createAPIKey( + this.generateAPIKeyName(attributes.alertTypeId, attributes.name) + ); } catch (error) { throw Boom.badRequest(`Error creating API key - ${error.message}`); } @@ -966,11 +966,9 @@ export class AlertsClient { let createdAPIKey = null; try { - createdAPIKey = attributes.enabled - ? await this.createAPIKey( - this.generateAPIKeyName(attributes.alertTypeId, attributes.name) - ) - : null; + createdAPIKey = await this.createAPIKey( + this.generateAPIKeyName(attributes.alertTypeId, attributes.name) + ); } catch (error) { throw Boom.badRequest(`Error creating API key - ${error.message}`); } diff --git a/x-pack/plugins/alerting/server/alerts_client_factory.ts b/x-pack/plugins/alerting/server/alerts_client_factory.ts index 3bb3406792d42..167114b8de422 100644 --- a/x-pack/plugins/alerting/server/alerts_client_factory.ts +++ b/x-pack/plugins/alerting/server/alerts_client_factory.ts @@ -5,7 +5,6 @@ * 2.0. */ -import Boom from '@hapi/boom'; import { KibanaRequest, Logger, @@ -117,18 +116,13 @@ export class AlertsClientFactory { if (!securityPluginStart) { return { apiKeysEnabled: false }; } - let createAPIKeyResult = null; - try { - // Create an API key using the new grant API - in this case the Kibana system user is creating the - // API key for the user, instead of having the user create it themselves, which requires api_key - // privileges - createAPIKeyResult = await securityPluginStart.authc.apiKeys.grantAsInternalUser( - request, - { name, role_descriptors: {} } - ); - } catch (error) { - throw Boom.badRequest(`Error creating API key - ${error.message}`); - } + // Create an API key using the new grant API - in this case the Kibana system user is creating the + // API key for the user, instead of having the user create it themselves, which requires api_key + // privileges + const createAPIKeyResult = await securityPluginStart.authc.apiKeys.grantAsInternalUser( + request, + { name, role_descriptors: {} } + ); if (!createAPIKeyResult) { return { apiKeysEnabled: false }; From d34b509e3c0ba4936679be6c3af8c241d155abe1 Mon Sep 17 00:00:00 2001 From: Ying Mao Date: Fri, 23 Apr 2021 12:42:27 -0400 Subject: [PATCH 3/6] Adding warning to docs --- docs/api/alerting/create_rule.asciidoc | 2 ++ docs/api/alerting/enable_rule.asciidoc | 2 ++ docs/api/alerting/update_rule.asciidoc | 2 ++ x-pack/plugins/alerting/server/alerts_client_factory.ts | 1 - 4 files changed, 6 insertions(+), 1 deletion(-) diff --git a/docs/api/alerting/create_rule.asciidoc b/docs/api/alerting/create_rule.asciidoc index 01b6dfc40fcf6..6075f50339598 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 only supports <> and does not support <>. + [[create-rule-api-request]] ==== Request diff --git a/docs/api/alerting/enable_rule.asciidoc b/docs/api/alerting/enable_rule.asciidoc index 60f18b3510904..888c6455f8db4 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 only supports <> and does not support <>. + [[enable-rule-api-request]] ==== Request diff --git a/docs/api/alerting/update_rule.asciidoc b/docs/api/alerting/update_rule.asciidoc index 76c88a009be01..400ee460f921c 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 only supports <> and does not support <>. + [[update-rule-api-request]] ==== Request diff --git a/x-pack/plugins/alerting/server/alerts_client_factory.ts b/x-pack/plugins/alerting/server/alerts_client_factory.ts index 167114b8de422..05e50346f56cf 100644 --- a/x-pack/plugins/alerting/server/alerts_client_factory.ts +++ b/x-pack/plugins/alerting/server/alerts_client_factory.ts @@ -123,7 +123,6 @@ export class AlertsClientFactory { request, { name, role_descriptors: {} } ); - if (!createAPIKeyResult) { return { apiKeysEnabled: false }; } From 43fd3925bbe17baea874407ab5f6cb77e1ec0844 Mon Sep 17 00:00:00 2001 From: Ying Mao Date: Fri, 23 Apr 2021 12:52:12 -0400 Subject: [PATCH 4/6] Updating error messages --- .../alerting/server/alerts_client/alerts_client.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) 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 8a4a97adce9c1..1db990edef2a9 100644 --- a/x-pack/plugins/alerting/server/alerts_client/alerts_client.ts +++ b/x-pack/plugins/alerting/server/alerts_client/alerts_client.ts @@ -266,7 +266,7 @@ export class AlertsClient { ? await this.createAPIKey(this.generateAPIKeyName(alertType.id, data.name)) : null; } catch (error) { - throw Boom.badRequest(`Error creating API key - ${error.message}`); + throw Boom.badRequest(`Error creating rule: could not create API key - ${error.message}`); } this.validateActions(alertType, data.actions); @@ -739,7 +739,7 @@ export class AlertsClient { ? await this.createAPIKey(this.generateAPIKeyName(alertType.id, data.name)) : null; } catch (error) { - throw Boom.badRequest(`Error creating API key - ${error.message}`); + throw Boom.badRequest(`Error updating rule: could not create API key - ${error.message}`); } const apiKeyAttributes = this.apiKeyAsAlertAttributes(createdAPIKey, username); @@ -856,7 +856,9 @@ export class AlertsClient { this.generateAPIKeyName(attributes.alertTypeId, attributes.name) ); } catch (error) { - throw Boom.badRequest(`Error creating API key - ${error.message}`); + throw Boom.badRequest( + `Error updating API key for rule: could not create API key - ${error.message}` + ); } const updateAttributes = this.updateMeta({ @@ -970,7 +972,7 @@ export class AlertsClient { this.generateAPIKeyName(attributes.alertTypeId, attributes.name) ); } catch (error) { - throw Boom.badRequest(`Error creating API key - ${error.message}`); + throw Boom.badRequest(`Error enabling rule: could not create API key - ${error.message}`); } const updateAttributes = this.updateMeta({ From 204bb915f10c344769d07e58b777ad376d9b219c Mon Sep 17 00:00:00 2001 From: Ying Mao Date: Fri, 23 Apr 2021 13:40:24 -0400 Subject: [PATCH 5/6] Updating tests --- .../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 +++++++-- 4 files changed, 88 insertions(+), 3 deletions(-) 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', From 7579d8ef801cdc4d7db1e38ff076ba95d8e2b66a Mon Sep 17 00:00:00 2001 From: Ying Mao Date: Mon, 26 Apr 2021 19:25:27 -0400 Subject: [PATCH 6/6] Updating warning wording in docs --- docs/api/alerting/create_rule.asciidoc | 2 +- docs/api/alerting/enable_rule.asciidoc | 2 +- docs/api/alerting/update_rule.asciidoc | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/api/alerting/create_rule.asciidoc b/docs/api/alerting/create_rule.asciidoc index 6075f50339598..59b17c5c3b5e1 100644 --- a/docs/api/alerting/create_rule.asciidoc +++ b/docs/api/alerting/create_rule.asciidoc @@ -6,7 +6,7 @@ Create {kib} rules. -WARNING: This API only supports <> and does not support <>. +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 888c6455f8db4..112d4bbf61faa 100644 --- a/docs/api/alerting/enable_rule.asciidoc +++ b/docs/api/alerting/enable_rule.asciidoc @@ -6,7 +6,7 @@ Enable a rule. -WARNING: This API only supports <> and does not support <>. +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 400ee460f921c..ec82e60a8e879 100644 --- a/docs/api/alerting/update_rule.asciidoc +++ b/docs/api/alerting/update_rule.asciidoc @@ -6,7 +6,7 @@ Update the attributes for an existing rule. -WARNING: This API only supports <> and does not support <>. +WARNING: This API supports <> only. [[update-rule-api-request]] ==== Request