From 773d7ec5ae15ab1703b4adf51ea0073400a79bf0 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Thu, 6 Feb 2020 15:42:27 -0500 Subject: [PATCH 1/3] Ensure update API still works when AAD is broken --- .../alerting/server/alerts_client.test.ts | 432 +++++++++--------- .../plugins/alerting/server/alerts_client.ts | 49 +- 2 files changed, 248 insertions(+), 233 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..c67c2f508857ec 100644 --- a/x-pack/legacy/plugins/alerting/server/alerts_client.test.ts +++ b/x-pack/legacy/plugins/alerting/server/alerts_client.test.ts @@ -1530,180 +1530,39 @@ describe('delete()', () => { }); describe('update()', () => { - test('updates given parameters', async () => { - const alertsClient = new AlertsClient(alertsClientParams); - alertTypeRegistry.get.mockReturnValueOnce({ + let alertsClient: AlertsClient; + const existingAlert = { + id: '1', + type: 'alert', + attributes: { + enabled: true, + alertTypeId: '123', + scheduledTaskId: 'task-123', + }, + references: [], + version: '123', + }; + const existingDecryptedAlert = { + ...existingAlert, + attributes: { + ...existingAlert.attributes, + apiKey: Buffer.from('123:abc').toString('base64'), + }, + }; + + beforeEach(() => { + alertsClient = new AlertsClient(alertsClientParams); + savedObjectsClient.get.mockResolvedValue(existingAlert); + encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue(existingDecryptedAlert); + alertTypeRegistry.get.mockReturnValue({ id: '123', name: 'Test', actionGroups: ['default'], async executor() {}, }); - encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ - id: '1', - type: 'alert', - attributes: { - enabled: true, - alertTypeId: '123', - scheduledTaskId: 'task-123', - }, - references: [], - version: '123', - }); - savedObjectsClient.bulkGet.mockResolvedValueOnce({ - saved_objects: [ - { - id: '1', - type: 'action', - attributes: { - actionTypeId: 'test', - }, - references: [], - }, - ], - }); - savedObjectsClient.update.mockResolvedValueOnce({ - id: '1', - type: 'alert', - attributes: { - enabled: true, - schedule: { interval: '10s' }, - params: { - bar: true, - }, - actions: [ - { - group: 'default', - actionRef: 'action_0', - actionTypeId: 'test', - params: { - foo: true, - }, - }, - ], - scheduledTaskId: 'task-123', - createdAt: new Date().toISOString(), - }, - updated_at: new Date().toISOString(), - references: [ - { - name: 'action_0', - type: 'action', - id: '1', - }, - ], - }); - const result = await alertsClient.update({ - id: '1', - data: { - schedule: { interval: '10s' }, - name: 'abc', - tags: ['foo'], - params: { - bar: true, - }, - actions: [ - { - group: 'default', - id: '1', - params: { - foo: true, - }, - }, - ], - }, - }); - expect(result).toMatchInlineSnapshot(` - Object { - "actions": Array [ - Object { - "actionTypeId": "test", - "group": "default", - "id": "1", - "params": Object { - "foo": true, - }, - }, - ], - "createdAt": 2019-02-12T21:01:22.479Z, - "enabled": true, - "id": "1", - "params": Object { - "bar": true, - }, - "schedule": Object { - "interval": "10s", - }, - "scheduledTaskId": "task-123", - "updatedAt": 2019-02-12T21:01:22.479Z, - } - `); - expect(savedObjectsClient.update).toHaveBeenCalledTimes(1); - expect(savedObjectsClient.update.mock.calls[0]).toHaveLength(4); - expect(savedObjectsClient.update.mock.calls[0][0]).toEqual('alert'); - expect(savedObjectsClient.update.mock.calls[0][1]).toEqual('1'); - expect(savedObjectsClient.update.mock.calls[0][2]).toMatchInlineSnapshot(` - Object { - "actions": Array [ - Object { - "actionRef": "action_0", - "actionTypeId": "test", - "group": "default", - "params": Object { - "foo": true, - }, - }, - ], - "alertTypeId": "123", - "apiKey": null, - "apiKeyOwner": null, - "enabled": true, - "name": "abc", - "params": Object { - "bar": true, - }, - "schedule": Object { - "interval": "10s", - }, - "scheduledTaskId": "task-123", - "tags": Array [ - "foo", - ], - "updatedBy": "elastic", - } - `); - expect(savedObjectsClient.update.mock.calls[0][3]).toMatchInlineSnapshot(` - Object { - "references": Array [ - Object { - "id": "1", - "name": "action_0", - "type": "action", - }, - ], - "version": "123", - } - `); }); - it('updates with multiple actions', async () => { - const alertsClient = new AlertsClient(alertsClientParams); - alertTypeRegistry.get.mockReturnValueOnce({ - id: '123', - name: 'Test', - actionGroups: ['default'], - async executor() {}, - }); - encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ - id: '1', - type: 'alert', - attributes: { - enabled: true, - alertTypeId: '123', - scheduledTaskId: 'task-123', - }, - references: [], - version: '123', - }); + test('updates given parameters', async () => { savedObjectsClient.bulkGet.mockResolvedValueOnce({ saved_objects: [ { @@ -1733,7 +1592,6 @@ describe('update()', () => { params: { bar: true, }, - createdAt: new Date().toISOString(), actions: [ { group: 'default', @@ -1761,6 +1619,7 @@ describe('update()', () => { }, ], scheduledTaskId: 'task-123', + createdAt: new Date().toISOString(), }, updated_at: new Date().toISOString(), references: [ @@ -1856,37 +1715,85 @@ describe('update()', () => { "updatedAt": 2019-02-12T21:01:22.479Z, } `); - expect(savedObjectsClient.bulkGet).toHaveBeenCalledWith([ - { - id: '1', - type: 'action', - }, - { - id: '2', - type: 'action', - }, - ]); + expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', { + namespace: 'default', + }); + expect(savedObjectsClient.get).not.toHaveBeenCalled(); + expect(savedObjectsClient.update).toHaveBeenCalledTimes(1); + expect(savedObjectsClient.update.mock.calls[0]).toHaveLength(4); + expect(savedObjectsClient.update.mock.calls[0][0]).toEqual('alert'); + expect(savedObjectsClient.update.mock.calls[0][1]).toEqual('1'); + expect(savedObjectsClient.update.mock.calls[0][2]).toMatchInlineSnapshot(` + Object { + "actions": Array [ + Object { + "actionRef": "action_0", + "actionTypeId": "test", + "group": "default", + "params": Object { + "foo": true, + }, + }, + Object { + "actionRef": "action_1", + "actionTypeId": "test", + "group": "default", + "params": Object { + "foo": true, + }, + }, + Object { + "actionRef": "action_2", + "actionTypeId": "test2", + "group": "default", + "params": Object { + "foo": true, + }, + }, + ], + "alertTypeId": "123", + "apiKey": null, + "apiKeyOwner": null, + "enabled": true, + "name": "abc", + "params": Object { + "bar": true, + }, + "schedule": Object { + "interval": "10s", + }, + "scheduledTaskId": "task-123", + "tags": Array [ + "foo", + ], + "updatedBy": "elastic", + } + `); + expect(savedObjectsClient.update.mock.calls[0][3]).toMatchInlineSnapshot(` + Object { + "references": Array [ + Object { + "id": "1", + "name": "action_0", + "type": "action", + }, + Object { + "id": "1", + "name": "action_1", + "type": "action", + }, + Object { + "id": "2", + "name": "action_2", + "type": "action", + }, + ], + "version": "123", + } + `); }); it('calls the createApiKey function', async () => { - const alertsClient = new AlertsClient(alertsClientParams); - alertTypeRegistry.get.mockReturnValueOnce({ - id: '123', - name: 'Test', - actionGroups: ['default'], - async executor() {}, - }); - encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ - id: '1', - type: 'alert', - attributes: { - enabled: true, - alertTypeId: '123', - scheduledTaskId: 'task-123', - }, - references: [], - version: '123', - }); savedObjectsClient.bulkGet.mockResolvedValueOnce({ saved_objects: [ { @@ -2030,7 +1937,6 @@ describe('update()', () => { }); it('should validate params', async () => { - const alertsClient = new AlertsClient(alertsClientParams); alertTypeRegistry.get.mockReturnValueOnce({ id: '123', name: 'Test', @@ -2042,14 +1948,6 @@ describe('update()', () => { }, async executor() {}, }); - encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ - id: '1', - type: 'alert', - attributes: { - alertTypeId: '123', - }, - references: [], - }); await expect( alertsClient.update({ id: '1', @@ -2077,26 +1975,75 @@ describe('update()', () => { }); it('swallows error when invalidate API key throws', async () => { - const alertsClient = new AlertsClient(alertsClientParams); alertsClientParams.invalidateAPIKey.mockRejectedValueOnce(new Error('Fail')); - alertTypeRegistry.get.mockReturnValueOnce({ - id: '123', - name: 'Test', - actionGroups: ['default'], - async executor() {}, + savedObjectsClient.bulkGet.mockResolvedValueOnce({ + saved_objects: [ + { + id: '1', + type: 'action', + attributes: { + actionTypeId: 'test', + }, + references: [], + }, + ], }); - encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ + savedObjectsClient.update.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { enabled: true, - alertTypeId: '123', + schedule: { interval: '10s' }, + params: { + bar: true, + }, + actions: [ + { + group: 'default', + actionRef: 'action_0', + actionTypeId: 'test', + params: { + foo: true, + }, + }, + ], scheduledTaskId: 'task-123', - apiKey: Buffer.from('123:abc').toString('base64'), }, - references: [], - version: '123', + references: [ + { + name: 'action_0', + type: 'action', + id: '1', + }, + ], }); + await alertsClient.update({ + id: '1', + data: { + schedule: { interval: '10s' }, + name: 'abc', + tags: ['foo'], + params: { + bar: true, + }, + actions: [ + { + group: 'default', + id: '1', + params: { + foo: true, + }, + }, + ], + }, + }); + expect(alertsClientParams.logger.error).toHaveBeenCalledWith( + 'Failed to invalidate API Key: Fail' + ); + }); + + it('swallows error when getDecryptedAsInternalUser throws', async () => { + encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValue(new Error('Fail')); savedObjectsClient.bulkGet.mockResolvedValueOnce({ saved_objects: [ { @@ -2107,6 +2054,14 @@ describe('update()', () => { }, references: [], }, + { + id: '2', + type: 'action', + attributes: { + actionTypeId: 'test2', + }, + references: [], + }, ], }); savedObjectsClient.update.mockResolvedValueOnce({ @@ -2127,15 +2082,43 @@ describe('update()', () => { foo: true, }, }, + { + group: 'default', + actionRef: 'action_1', + actionTypeId: 'test', + params: { + foo: true, + }, + }, + { + group: 'default', + actionRef: 'action_2', + actionTypeId: 'test2', + params: { + foo: true, + }, + }, ], scheduledTaskId: 'task-123', + createdAt: new Date().toISOString(), }, + updated_at: new Date().toISOString(), references: [ { name: 'action_0', type: 'action', id: '1', }, + { + name: 'action_1', + type: 'action', + id: '1', + }, + { + name: 'action_2', + type: 'action', + id: '2', + }, ], }); await alertsClient.update({ @@ -2155,11 +2138,26 @@ describe('update()', () => { foo: true, }, }, + { + group: 'default', + id: '1', + params: { + foo: true, + }, + }, + { + group: 'default', + id: '2', + params: { + foo: true, + }, + }, ], }, }); + expect(savedObjectsClient.get).toHaveBeenCalledWith('alert', '1'); expect(alertsClientParams.logger.error).toHaveBeenCalledWith( - 'Failed to invalidate API Key: Fail' + 'update(): Failed to load API key to invalidate on alert 1: Fail' ); }); diff --git a/x-pack/legacy/plugins/alerting/server/alerts_client.ts b/x-pack/legacy/plugins/alerting/server/alerts_client.ts index 1346d403edda1a..dc74a83017cfa5 100644 --- a/x-pack/legacy/plugins/alerting/server/alerts_client.ts +++ b/x-pack/legacy/plugins/alerting/server/alerts_client.ts @@ -238,22 +238,41 @@ export class AlertsClient { } public async update({ id, data }: UpdateOptions): Promise { - const decryptedAlertSavedObject = await this.encryptedSavedObjectsPlugin.getDecryptedAsInternalUser< - RawAlert - >('alert', id, { namespace: this.namespace }); - const updateResult = await this.updateAlert({ id, data }, decryptedAlertSavedObject); - - if ( - updateResult.scheduledTaskId && - !isEqual(decryptedAlertSavedObject.attributes.schedule, updateResult.schedule) - ) { - this.taskManager.runNow(updateResult.scheduledTaskId).catch((err: Error) => { - this.logger.error( - `Alert update failed to run its underlying task. TaskManager runNow failed with Error: ${err.message}` - ); - }); + let alertSavedObject: SavedObject; + + try { + alertSavedObject = await this.encryptedSavedObjectsPlugin.getDecryptedAsInternalUser< + RawAlert + >('alert', id, { namespace: this.namespace }); + } catch (e) { + // We'll skip invalidating the API key since we failed to load the decrypted saved object + this.logger.error( + `update(): Failed to load API key to invalidate on alert ${id}: ${e.message}` + ); + // Still attempt to load the object using SOC + alertSavedObject = await this.savedObjectsClient.get('alert', id); } + const updateResult = await this.updateAlert({ id, data }, alertSavedObject); + + await Promise.all([ + alertSavedObject.attributes.apiKey + ? this.invalidateApiKey({ apiKey: alertSavedObject.attributes.apiKey }) + : null, + (async () => { + if ( + updateResult.scheduledTaskId && + !isEqual(alertSavedObject.attributes.schedule, updateResult.schedule) + ) { + this.taskManager.runNow(updateResult.scheduledTaskId).catch((err: Error) => { + this.logger.error( + `Alert update failed to run its underlying task. TaskManager runNow failed with Error: ${err.message}` + ); + }); + } + })(), + ]); + return updateResult; } @@ -288,8 +307,6 @@ export class AlertsClient { } ); - await this.invalidateApiKey({ apiKey: attributes.apiKey }); - return this.getPartialAlertFromRaw( id, updatedObject.attributes, From c02533cf3b13d203145607be5a6a3416db46d84c Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Thu, 6 Feb 2020 15:51:16 -0500 Subject: [PATCH 2/3] Add API integration test --- .../tests/alerting/update.ts | 81 +++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/update.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/update.ts index 2a7e0b22038248..7a4f73885107c4 100644 --- a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/update.ts +++ b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/update.ts @@ -108,6 +108,87 @@ export default function createUpdateTests({ getService }: FtrProviderContext) { } }); + it('should still be able to update when AAD is broken', async () => { + const { body: createdAlert } = await supertest + .post(`${getUrlPrefix(space.id)}/api/alert`) + .set('kbn-xsrf', 'foo') + .send(getTestAlertData()) + .expect(200); + objectRemover.add(space.id, createdAlert.id, 'alert'); + + await supertest + .put(`${getUrlPrefix(space.id)}/api/saved_objects/alert/${createdAlert.id}`) + .set('kbn-xsrf', 'foo') + .send({ + attributes: { + name: 'bar', + }, + }) + .expect(200); + + const updatedData = { + name: 'bcd', + tags: ['bar'], + params: { + foo: true, + }, + schedule: { interval: '12s' }, + actions: [], + throttle: '2m', + }; + const response = await supertestWithoutAuth + .put(`${getUrlPrefix(space.id)}/api/alert/${createdAlert.id}`) + .set('kbn-xsrf', 'foo') + .auth(user.username, user.password) + .send(updatedData); + + 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', + }); + break; + case 'superuser at space1': + case 'space_1_all at space1': + expect(response.statusCode).to.eql(200); + expect(response.body).to.eql({ + ...updatedData, + id: createdAlert.id, + alertTypeId: 'test.noop', + consumer: 'bar', + createdBy: 'elastic', + enabled: true, + updatedBy: user.username, + apiKeyOwner: user.username, + muteAll: false, + mutedInstanceIds: [], + scheduledTaskId: createdAlert.scheduledTaskId, + createdAt: response.body.createdAt, + updatedAt: response.body.updatedAt, + }); + expect(Date.parse(response.body.createdAt)).to.be.greaterThan(0); + expect(Date.parse(response.body.updatedAt)).to.be.greaterThan(0); + expect(Date.parse(response.body.updatedAt)).to.be.greaterThan( + Date.parse(response.body.createdAt) + ); + // Ensure AAD isn't broken + await checkAAD({ + supertest, + spaceId: space.id, + type: 'alert', + id: createdAlert.id, + }); + break; + default: + throw new Error(`Scenario untested: ${JSON.stringify(scenario)}`); + } + }); + it(`shouldn't update alert from another space`, async () => { const { body: createdAlert } = await supertest .post(`${getUrlPrefix(space.id)}/api/alert`) From d8152151e15d920c1d3c1a1e06f980de5e63f9e3 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Thu, 6 Feb 2020 16:38:51 -0500 Subject: [PATCH 3/3] Fix ESLint errors --- x-pack/legacy/plugins/alerting/server/alerts_client.test.ts | 4 ---- 1 file changed, 4 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 db8d2226fed472..cfacef7c1d0ca9 100644 --- a/x-pack/legacy/plugins/alerting/server/alerts_client.test.ts +++ b/x-pack/legacy/plugins/alerting/server/alerts_client.test.ts @@ -2289,7 +2289,6 @@ describe('update()', () => { test('updating the alert schedule should rerun the task immediately', async () => { const alertId = uuid.v4(); const taskId = uuid.v4(); - const alertsClient = new AlertsClient(alertsClientParams); mockApiCalls(alertId, taskId, { interval: '60m' }, { interval: '10s' }); @@ -2320,7 +2319,6 @@ describe('update()', () => { test('updating the alert without changing the schedule should not rerun the task', async () => { const alertId = uuid.v4(); const taskId = uuid.v4(); - const alertsClient = new AlertsClient(alertsClientParams); mockApiCalls(alertId, taskId, { interval: '10s' }, { interval: '10s' }); @@ -2351,7 +2349,6 @@ describe('update()', () => { test('updating the alert should not wait for the rerun the task to complete', async done => { const alertId = uuid.v4(); const taskId = uuid.v4(); - const alertsClient = new AlertsClient(alertsClientParams); mockApiCalls(alertId, taskId, { interval: '10s' }, { interval: '30s' }); @@ -2390,7 +2387,6 @@ describe('update()', () => { test('logs when the rerun of an alerts underlying task fails', async () => { const alertId = uuid.v4(); const taskId = uuid.v4(); - const alertsClient = new AlertsClient(alertsClientParams); mockApiCalls(alertId, taskId, { interval: '10s' }, { interval: '30s' });