Load alert on delete with fallback instead of doubling the calls (ela…

…stic#57004) (elastic#57031)
thomasneirynck · Feb 6, 2020 · 1a49357 · 1a49357
1 parent 31dbd88
commit 1a49357
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 27 deletions.
diff --git a/x-pack/legacy/plugins/alerting/server/alerts_client.test.ts b/x-pack/legacy/plugins/alerting/server/alerts_client.test.ts
@@ -1465,20 +1465,21 @@ describe('delete()', () => {
       },
     ],
   };
+  const existingDecryptedAlert = {
+    ...existingAlert,
+    attributes: {
+      ...existingAlert.attributes,
+      apiKey: Buffer.from('123:abc').toString('base64'),
+    },
+  };
 
   beforeEach(() => {
     alertsClient = new AlertsClient(alertsClientParams);
     savedObjectsClient.get.mockResolvedValue(existingAlert);
     savedObjectsClient.delete.mockResolvedValue({
       success: true,
     });
-    encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue({
-      ...existingAlert,
-      attributes: {
-        ...existingAlert.attributes,
-        apiKey: Buffer.from('123:abc').toString('base64'),
-      },
-    });
+    encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue(existingDecryptedAlert);
   });
 
   test('successfully removes an alert', async () => {
@@ -1487,13 +1488,31 @@ describe('delete()', () => {
     expect(savedObjectsClient.delete).toHaveBeenCalledWith('alert', '1');
     expect(taskManager.remove).toHaveBeenCalledWith('task-123');
     expect(alertsClientParams.invalidateAPIKey).toHaveBeenCalledWith({ id: '123' });
+    expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', {
+      namespace: 'default',
+    });
+    expect(savedObjectsClient.get).not.toHaveBeenCalled();
+  });
+
+  test('falls back to SOC.get when getDecryptedAsInternalUser throws an error', async () => {
+    encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValue(new Error('Fail'));
+
+    const result = await alertsClient.delete({ id: '1' });
+    expect(result).toEqual({ success: true });
+    expect(savedObjectsClient.delete).toHaveBeenCalledWith('alert', '1');
+    expect(taskManager.remove).toHaveBeenCalledWith('task-123');
+    expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled();
+    expect(savedObjectsClient.get).toHaveBeenCalledWith('alert', '1');
+    expect(alertsClientParams.logger.error).toHaveBeenCalledWith(
+      'delete(): Failed to load API key to invalidate on alert 1: Fail'
+    );
   });
 
   test(`doesn't remove a task when scheduledTaskId is null`, async () => {
-    savedObjectsClient.get.mockResolvedValue({
-      ...existingAlert,
+    encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue({
+      ...existingDecryptedAlert,
       attributes: {
-        ...existingAlert.attributes,
+        ...existingDecryptedAlert.attributes,
         scheduledTaskId: null,
       },
     });
@@ -1536,6 +1555,7 @@ describe('delete()', () => {
   });
 
   test('throws error when savedObjectsClient.get throws an error', async () => {
+    encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValue(new Error('Fail'));
     savedObjectsClient.get.mockRejectedValue(new Error('SOC Fail'));
 
     await expect(alertsClient.delete({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot(

diff --git a/x-pack/legacy/plugins/alerting/server/alerts_client.ts b/x-pack/legacy/plugins/alerting/server/alerts_client.ts
@@ -226,27 +226,30 @@ export class AlertsClient {
   }
 
   public async delete({ id }: { id: string }) {
-    const [taskIdToRemove, apiKeyToInvalidate] = await Promise.all([
-      this.savedObjectsClient
-        .get<RawAlert>('alert', id)
-        .then(result => result.attributes.scheduledTaskId),
-      // We'll try and load the decrypted saved object but if this fails we'll only log
-      // and skip invalidating the API key.
-      this.encryptedSavedObjectsPlugin
-        .getDecryptedAsInternalUser<RawAlert>('alert', id, { namespace: this.namespace })
-        .then(result => result.attributes.apiKey)
-        .catch(e =>
-          this.logger.error(
-            `delete(): Failed to load API key to invalidate on alert ${id}: ${e.message}`
-          )
-        ),
-    ]);
+    let taskIdToRemove: string | undefined;
+    let apiKeyToInvalidate: string | null = null;
+
+    try {
+      const decryptedAlert = await this.encryptedSavedObjectsPlugin.getDecryptedAsInternalUser<
+        RawAlert
+      >('alert', id, { namespace: this.namespace });
+      apiKeyToInvalidate = decryptedAlert.attributes.apiKey;
+      taskIdToRemove = decryptedAlert.attributes.scheduledTaskId;
+    } catch (e) {
+      // We'll skip invalidating the API key since we failed to load the decrypted saved object
+      this.logger.error(
+        `delete(): Failed to load API key to invalidate on alert ${id}: ${e.message}`
+      );
+      // Still attempt to load the scheduledTaskId using SOC
+      const alert = await this.savedObjectsClient.get<RawAlert>('alert', id);
+      taskIdToRemove = alert.attributes.scheduledTaskId;
+    }
 
     const removeResult = await this.savedObjectsClient.delete('alert', id);
 
     await Promise.all([
-      taskIdToRemove && this.taskManager.remove(taskIdToRemove),
-      apiKeyToInvalidate && this.invalidateApiKey({ apiKey: apiKeyToInvalidate }),
+      taskIdToRemove ? this.taskManager.remove(taskIdToRemove) : null,
+      apiKeyToInvalidate ? this.invalidateApiKey({ apiKey: apiKeyToInvalidate }) : null,
     ]);
 
     return removeResult;