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..a9c2f9f28613c5 100644 --- a/x-pack/legacy/plugins/alerting/server/alerts_client.test.ts +++ b/x-pack/legacy/plugins/alerting/server/alerts_client.test.ts @@ -2387,25 +2387,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, + 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).toHaveBeenCalledWith('alert', '1'); + expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', { + namespace: 'default', + }); expect(savedObjectsClient.update).toHaveBeenCalledWith( 'alert', '1', @@ -2413,37 +2430,42 @@ 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: { - schedule: { interval: '10s' }, - alertTypeId: '2', - enabled: true, - apiKey: Buffer.from('123:abc').toString('base64'), - }, - version: '123', - references: [], - }); - alertsClientParams.createAPIKey.mockResolvedValueOnce({ - apiKeysEnabled: true, - result: { id: '123', api_key: 'abc' }, - }); 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: 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 1346d403edda1a..98c1ad8d71a374 100644 --- a/x-pack/legacy/plugins/alerting/server/alerts_client.ts +++ b/x-pack/legacy/plugins/alerting/server/alerts_client.ts @@ -314,12 +314,20 @@ export class AlertsClient { } public async updateApiKey({ id }: { id: string }) { - const { - version, - attributes, - } = await this.encryptedSavedObjectsPlugin.getDecryptedAsInternalUser('alert', id, { - namespace: this.namespace, - }); + const [{ attributes, version }, apiKeyToInvalidate] = await Promise.all< + SavedObject, + string | null | void + >([ + this.savedObjectsClient.get('alert', id), + // 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(`updateApiKey(): Failed to load API key to invalidate: ${e.message}`) + ), + ]); const username = await this.getUserName(); await this.savedObjectsClient.update( @@ -333,7 +341,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 b54147348d9a33..cd821a739a9eba 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`)