From 4f7f28da3cc0719954a48221137f0cfae67ce66f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mike=20C=C3=B4t=C3=A9?= Date: Mon, 3 Feb 2020 16:20:00 -0500 Subject: [PATCH] Ability to delete alerts even when AAD is out of sync (#56543) * Ability to delete alerts even when AAD is bad * Small code fixes Co-authored-by: Elastic Machine --- .../alerting/server/alerts_client.test.ts | 165 ++++++++++-------- .../plugins/alerting/server/alerts_client.ts | 29 ++- .../tests/alerting/delete.ts | 52 ++++++ 3 files changed, 168 insertions(+), 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 index 9b8cfe9b69ed04..a5d9f6fb6e6a60 100644 --- 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 index 7801e8f478712e..211f158a33a2d7 100644 --- a/x-pack/legacy/plugins/alerting/server/alerts_client.ts +++ b/x-pack/legacy/plugins/alerting/server/alerts_client.ts @@ -225,14 +225,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('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('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 index aab683df097404..3da9552df976c9 100644 --- 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)}`); + } + }); }); } });