Ability to delete alerts even when AAD is out of sync (elastic#56543) (…

…elastic#56686) * Ability to delete alerts even when AAD is bad * Small code fixes Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
kertal · Feb 4, 2020 · c4c0e74 · c4c0e74
1 parent 58536f8
commit c4c0e74
Show file tree

Hide file tree

Showing 3 changed files with 168 additions and 78 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
@@ -1436,97 +1436,120 @@ describe('find()', () => {
 });
 
 describe('delete()', () => {
-  test('successfully removes an alert', async () => {
-    const alertsClient = new AlertsClient(alertsClientParams);
-    encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({
-      id: '1',
-      type: 'alert',
-      attributes: {
-        alertTypeId: '123',
-        schedule: { interval: '10s' },
-        params: {
-          bar: true,
-        },
-        scheduledTaskId: 'task-123',
-        actions: [
-          {
-            group: 'default',
-            actionRef: 'action_0',
-            params: {
-              foo: true,
-            },
-          },
-        ],
+  let alertsClient: AlertsClient;
+  const existingAlert = {
+    id: '1',
+    type: 'alert',
+    attributes: {
+      alertTypeId: '123',
+      schedule: { interval: '10s' },
+      params: {
+        bar: true,
       },
-      references: [
+      scheduledTaskId: 'task-123',
+      actions: [
         {
-          name: 'action_0',
-          type: 'action',
-          id: '1',
+          group: 'default',
+          actionRef: 'action_0',
+          params: {
+            foo: true,
+          },
         },
       ],
-    });
-    savedObjectsClient.delete.mockResolvedValueOnce({
+    },
+    references: [
+      {
+        name: 'action_0',
+        type: 'action',
+        id: '1',
+      },
+    ],
+  };
+
+  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'),
+      },
+    });
+  });
+
+  test('successfully removes an alert', async () => {
     const result = await alertsClient.delete({ id: '1' });
     expect(result).toEqual({ success: true });
-    expect(savedObjectsClient.delete).toHaveBeenCalledTimes(1);
-    expect(savedObjectsClient.delete.mock.calls[0]).toMatchInlineSnapshot(`
-                                                                                                                  Array [
-                                                                                                                    "alert",
-                                                                                                                    "1",
-                                                                                                                  ]
-                                                                            `);
-    expect(taskManager.remove).toHaveBeenCalledTimes(1);
-    expect(taskManager.remove.mock.calls[0]).toMatchInlineSnapshot(`
-                                                                                                                  Array [
-                                                                                                                    "task-123",
-                                                                                                                  ]
-                                                                            `);
+    expect(savedObjectsClient.delete).toHaveBeenCalledWith('alert', '1');
+    expect(taskManager.remove).toHaveBeenCalledWith('task-123');
+    expect(alertsClientParams.invalidateAPIKey).toHaveBeenCalledWith({ id: '123' });
   });
 
-  test('swallows error when invalidate API key throws', async () => {
-    const alertsClient = new AlertsClient(alertsClientParams);
-    alertsClientParams.invalidateAPIKey.mockRejectedValueOnce(new Error('Fail'));
-    encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({
-      id: '1',
-      type: 'alert',
+  test(`doesn't remove a task when scheduledTaskId is null`, async () => {
+    savedObjectsClient.get.mockResolvedValue({
+      ...existingAlert,
       attributes: {
-        alertTypeId: '123',
-        schedule: { interval: '10s' },
-        params: {
-          bar: true,
-        },
-        apiKey: Buffer.from('123:abc').toString('base64'),
-        scheduledTaskId: 'task-123',
-        actions: [
-          {
-            group: 'default',
-            actionRef: 'action_0',
-            params: {
-              foo: true,
-            },
-          },
-        ],
+        ...existingAlert.attributes,
+        scheduledTaskId: null,
       },
-      references: [
-        {
-          name: 'action_0',
-          type: 'action',
-          id: '1',
-        },
-      ],
     });
-    savedObjectsClient.delete.mockResolvedValueOnce({
-      success: true,
+
+    await alertsClient.delete({ id: '1' });
+    expect(taskManager.remove).not.toHaveBeenCalled();
+  });
+
+  test(`doesn't invalidate API key when apiKey is null`, async () => {
+    encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue({
+      ...existingAlert,
+      attributes: {
+        ...existingAlert.attributes,
+        apiKey: null,
+      },
     });
 
     await alertsClient.delete({ id: '1' });
+    expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled();
+  });
+
+  test('swallows error when invalidate API key throws', async () => {
+    alertsClientParams.invalidateAPIKey.mockRejectedValueOnce(new Error('Fail'));
+
+    await alertsClient.delete({ id: '1' });
+    expect(alertsClientParams.invalidateAPIKey).toHaveBeenCalledWith({ id: '123' });
     expect(alertsClientParams.logger.error).toHaveBeenCalledWith(
       'Failed to invalidate API Key: Fail'
     );
   });
+
+  test('swallows error when getDecryptedAsInternalUser throws an error', async () => {
+    encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValue(new Error('Fail'));
+
+    await alertsClient.delete({ id: '1' });
+    expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled();
+    expect(alertsClientParams.logger.error).toHaveBeenCalledWith(
+      'delete(): Failed to load API key to invalidate on alert 1: Fail'
+    );
+  });
+
+  test('throws error when savedObjectsClient.get throws an error', async () => {
+    savedObjectsClient.get.mockRejectedValue(new Error('SOC Fail'));
+
+    await expect(alertsClient.delete({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot(
+      `"SOC Fail"`
+    );
+  });
+
+  test('throws error when taskManager.remove throws an error', async () => {
+    taskManager.remove.mockRejectedValue(new Error('TM Fail'));
+
+    await expect(alertsClient.delete({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot(
+      `"TM Fail"`
+    );
+  });
 });
 
 describe('update()', () => {

diff --git a/x-pack/legacy/plugins/alerting/server/alerts_client.ts b/x-pack/legacy/plugins/alerting/server/alerts_client.ts
@@ -226,14 +226,29 @@ export class AlertsClient {
   }
 
   public async delete({ id }: { id: string }) {
-    const decryptedAlertSavedObject = await this.encryptedSavedObjectsPlugin.getDecryptedAsInternalUser<
-      RawAlert
-    >('alert', id, { namespace: this.namespace });
+    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}`
+          )
+        ),
+    ]);
+
     const removeResult = await this.savedObjectsClient.delete('alert', id);
-    if (decryptedAlertSavedObject.attributes.scheduledTaskId) {
-      await this.taskManager.remove(decryptedAlertSavedObject.attributes.scheduledTaskId);
-    }
-    await this.invalidateApiKey({ apiKey: decryptedAlertSavedObject.attributes.apiKey });
+
+    await Promise.all([
+      taskIdToRemove && this.taskManager.remove(taskIdToRemove),
+      apiKeyToInvalidate && this.invalidateApiKey({ apiKey: apiKeyToInvalidate }),
+    ]);
+
     return removeResult;
   }
 

diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/delete.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/delete.ts
@@ -108,6 +108,58 @@ export default function createDeleteTests({ getService }: FtrProviderContext) {
               throw new Error(`Scenario untested: ${JSON.stringify(scenario)}`);
           }
         });
+
+        it('should still be able to delete alert when AAD is broken', async () => {
+          const { body: createdAlert } = await supertest
+            .post(`${getUrlPrefix(space.id)}/api/alert`)
+            .set('kbn-xsrf', 'foo')
+            .send(getTestAlertData())
+            .expect(200);
+
+          await supertest
+            .put(`${getUrlPrefix(space.id)}/api/saved_objects/alert/${createdAlert.id}`)
+            .set('kbn-xsrf', 'foo')
+            .send({
+              attributes: {
+                name: 'bar',
+              },
+            })
+            .expect(200);
+
+          const response = await supertestWithoutAuth
+            .delete(`${getUrlPrefix(space.id)}/api/alert/${createdAlert.id}`)
+            .set('kbn-xsrf', 'foo')
+            .auth(user.username, user.password);
+
+          switch (scenario.id) {
+            case 'no_kibana_privileges at space1':
+            case 'space_1_all at space2':
+            case 'global_read at space1':
+              expect(response.statusCode).to.eql(404);
+              expect(response.body).to.eql({
+                statusCode: 404,
+                error: 'Not Found',
+                message: 'Not Found',
+              });
+              objectRemover.add(space.id, createdAlert.id, 'alert');
+              // Ensure task still exists
+              await getScheduledTask(createdAlert.scheduledTaskId);
+              break;
+            case 'superuser at space1':
+            case 'space_1_all at space1':
+              expect(response.statusCode).to.eql(204);
+              expect(response.body).to.eql('');
+              try {
+                await getScheduledTask(createdAlert.scheduledTaskId);
+                throw new Error('Should have removed scheduled task');
+              } catch (e) {
+                expect(e.status).to.eql(404);
+              }
+              break;
+            default:
+              throw new Error(`Scenario untested: ${JSON.stringify(scenario)}`);
+          }
+        });
       });
     }
   });