[Alerting] Return 400 Bad Request errors when creating/enabling/upd…

…ating 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 <[email protected]> Co-authored-by: ymao1 <[email protected]>
elastic · Apr 27, 2021 · cb59315 · cb59315
1 parent 4984676
commit cb59315
Show file tree

Hide file tree

Showing 8 changed files with 136 additions and 17 deletions.
diff --git 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 <<token-api-authentication>> only.
+
 [[create-rule-api-request]]
 ==== Request
 

diff --git 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 <<token-api-authentication>> only.
+
 [[enable-rule-api-request]]
 ==== Request
 

diff --git 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 <<token-api-authentication>> 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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',