From a3dd282588888c3ced8a37b0fd36d27e144e3928 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mike=20C=C3=B4t=C3=A9?= Date: Mon, 10 Feb 2020 14:48:29 -0500 Subject: [PATCH] Make the update alert API key API work when AAD is out of sync (#56640) * Make the update API key API work when AAD is out of sync * Make updateAPIKey only load SOC where possible Co-authored-by: Elastic Machine --- .../alerting/server/alerts_client.test.ts | 106 +++++++++++++----- .../plugins/alerting/server/alerts_client.ts | 31 +++-- .../tests/alerting/update_api_key.ts | 54 +++++++++ 3 files changed, 154 insertions(+), 37 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 e5c4daef88e94..a7f1a0e8c6dc9 100644 --- a/x-pack/legacy/plugins/alerting/server/alerts_client.test.ts +++ b/x-pack/legacy/plugins/alerting/server/alerts_client.test.ts @@ -2714,25 +2714,42 @@ describe('update()', () => { }); describe('updateApiKey()', () => { - test('updates the API key for the alert', async () => { - const alertsClient = new AlertsClient(alertsClientParams); - encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ - id: '1', - type: 'alert', - attributes: { - schedule: { interval: '10s' }, - alertTypeId: '2', - enabled: true, - }, - version: '123', - references: [], - }); + let alertsClient: AlertsClient; + const existingAlert = { + id: '1', + type: 'alert', + attributes: { + schedule: { interval: '10s' }, + alertTypeId: '2', + enabled: true, + }, + version: '123', + references: [], + }; + const existingEncryptedAlert = { + ...existingAlert, + attributes: { + ...existingAlert.attributes, + apiKey: Buffer.from('123:abc').toString('base64'), + }, + }; + + beforeEach(() => { + alertsClient = new AlertsClient(alertsClientParams); + savedObjectsClient.get.mockResolvedValue(existingAlert); + encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue(existingEncryptedAlert); alertsClientParams.createAPIKey.mockResolvedValueOnce({ apiKeysEnabled: true, - result: { id: '123', api_key: 'abc' }, + result: { id: '234', api_key: 'abc' }, }); + }); + test('updates the API key for the alert', async () => { await alertsClient.updateApiKey({ id: '1' }); + expect(savedObjectsClient.get).not.toHaveBeenCalled(); + expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', { + namespace: 'default', + }); expect(savedObjectsClient.update).toHaveBeenCalledWith( 'alert', '1', @@ -2740,37 +2757,66 @@ describe('updateApiKey()', () => { schedule: { interval: '10s' }, alertTypeId: '2', enabled: true, - apiKey: Buffer.from('123:abc').toString('base64'), + apiKey: Buffer.from('234:abc').toString('base64'), apiKeyOwner: 'elastic', updatedBy: 'elastic', }, { version: '123' } ); + expect(alertsClientParams.invalidateAPIKey).toHaveBeenCalledWith({ id: '123' }); }); - test('swallows error when invalidate API key throws', async () => { - const alertsClient = new AlertsClient(alertsClientParams); - alertsClientParams.invalidateAPIKey.mockRejectedValue(new Error('Fail')); - encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ - id: '1', - type: 'alert', - attributes: { + test('falls back to SOC when getDecryptedAsInternalUser throws an error', async () => { + encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValueOnce(new Error('Fail')); + + await alertsClient.updateApiKey({ id: '1' }); + expect(savedObjectsClient.get).toHaveBeenCalledWith('alert', '1'); + expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', { + namespace: 'default', + }); + expect(savedObjectsClient.update).toHaveBeenCalledWith( + 'alert', + '1', + { schedule: { interval: '10s' }, alertTypeId: '2', enabled: true, - apiKey: Buffer.from('123:abc').toString('base64'), + apiKey: Buffer.from('234:abc').toString('base64'), + apiKeyOwner: 'elastic', + updatedBy: 'elastic', }, - version: '123', - references: [], - }); - alertsClientParams.createAPIKey.mockResolvedValueOnce({ - apiKeysEnabled: true, - result: { id: '123', api_key: 'abc' }, - }); + { version: '123' } + ); + expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled(); + }); + + test('swallows error when invalidate API key throws', async () => { + alertsClientParams.invalidateAPIKey.mockRejectedValue(new Error('Fail')); await alertsClient.updateApiKey({ id: '1' }); expect(alertsClientParams.logger.error).toHaveBeenCalledWith( 'Failed to invalidate API Key: Fail' ); + expect(savedObjectsClient.update).toHaveBeenCalled(); + }); + + test('swallows error when getting decrypted object throws', async () => { + encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValueOnce(new Error('Fail')); + + await alertsClient.updateApiKey({ id: '1' }); + expect(alertsClientParams.logger.error).toHaveBeenCalledWith( + 'updateApiKey(): Failed to load API key to invalidate on alert 1: Fail' + ); + expect(savedObjectsClient.update).toHaveBeenCalled(); + expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled(); + }); + + test('throws when savedObjectsClient update fails', async () => { + savedObjectsClient.update.mockRejectedValueOnce(new Error('Fail')); + + await expect(alertsClient.updateApiKey({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot( + `"Fail"` + ); + expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled(); }); }); diff --git a/x-pack/legacy/plugins/alerting/server/alerts_client.ts b/x-pack/legacy/plugins/alerting/server/alerts_client.ts index d54560ff5b776..97f556be04957 100644 --- a/x-pack/legacy/plugins/alerting/server/alerts_client.ts +++ b/x-pack/legacy/plugins/alerting/server/alerts_client.ts @@ -345,12 +345,27 @@ export class AlertsClient { } public async updateApiKey({ id }: { id: string }) { - const { - version, - attributes, - } = await this.encryptedSavedObjectsPlugin.getDecryptedAsInternalUser('alert', id, { - namespace: this.namespace, - }); + let apiKeyToInvalidate: string | null = null; + let attributes: RawAlert; + let version: string | undefined; + + try { + const decryptedAlert = await this.encryptedSavedObjectsPlugin.getDecryptedAsInternalUser< + RawAlert + >('alert', id, { namespace: this.namespace }); + apiKeyToInvalidate = decryptedAlert.attributes.apiKey; + attributes = decryptedAlert.attributes; + version = decryptedAlert.version; + } catch (e) { + // We'll skip invalidating the API key since we failed to load the decrypted saved object + this.logger.error( + `updateApiKey(): Failed to load API key to invalidate on alert ${id}: ${e.message}` + ); + // Still attempt to load the attributes and version using SOC + const alert = await this.savedObjectsClient.get('alert', id); + attributes = alert.attributes; + version = alert.version; + } const username = await this.getUserName(); await this.savedObjectsClient.update( @@ -364,7 +379,9 @@ export class AlertsClient { { version } ); - await this.invalidateApiKey({ apiKey: attributes.apiKey }); + if (apiKeyToInvalidate) { + await this.invalidateApiKey({ apiKey: apiKeyToInvalidate }); + } } private async invalidateApiKey({ apiKey }: { apiKey: string | null }): Promise { diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/update_api_key.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/update_api_key.ts index b54147348d9a3..cd821a739a9eb 100644 --- a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/update_api_key.ts +++ b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/update_api_key.ts @@ -74,6 +74,60 @@ export default function createUpdateApiKeyTests({ getService }: FtrProviderConte } }); + it('should still be able to update API key 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 response = await alertUtils.getUpdateApiKeyRequest(createdAlert.id); + + 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(204); + expect(response.body).to.eql(''); + const { body: updatedAlert } = await supertestWithoutAuth + .get(`${getUrlPrefix(space.id)}/api/alert/${createdAlert.id}`) + .set('kbn-xsrf', 'foo') + .auth(user.username, user.password) + .expect(200); + expect(updatedAlert.apiKeyOwner).to.eql(user.username); + // 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 api key from another space`, async () => { const { body: createdAlert } = await supertest .post(`${getUrlPrefix('other')}/api/alert`)