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 f9d1d97a521fe..52b0cf1def3f6 100644 --- a/x-pack/legacy/plugins/alerting/server/alerts_client.test.ts +++ b/x-pack/legacy/plugins/alerting/server/alerts_client.test.ts @@ -848,20 +848,27 @@ describe('create()', () => { }); describe('enable()', () => { - test('enables an alert', async () => { - const alertsClient = new AlertsClient(alertsClientParams); - encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ - id: '1', - type: 'alert', - attributes: { - schedule: { interval: '10s' }, - alertTypeId: '2', - enabled: false, - }, - version: '123', - references: [], + let alertsClient: AlertsClient; + const existingAlert = { + id: '1', + type: 'alert', + attributes: { + schedule: { interval: '10s' }, + alertTypeId: '2', + enabled: false, + }, + version: '123', + references: [], + }; + + beforeEach(() => { + alertsClient = new AlertsClient(alertsClientParams); + encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue(existingAlert); + savedObjectsClient.get.mockResolvedValue(existingAlert); + alertsClientParams.createAPIKey.mockResolvedValue({ + apiKeysEnabled: false, }); - taskManager.schedule.mockResolvedValueOnce({ + taskManager.schedule.mockResolvedValue({ id: 'task-123', scheduledAt: new Date(), attempts: 0, @@ -874,8 +881,16 @@ describe('enable()', () => { retryAt: null, ownerId: null, }); + }); + test('enables an alert', async () => { await alertsClient.enable({ id: '1' }); + expect(savedObjectsClient.get).not.toHaveBeenCalled(); + expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', { + namespace: 'default', + }); + expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled(); + expect(alertsClientParams.createAPIKey).toHaveBeenCalled(); expect(savedObjectsClient.update).toHaveBeenCalledWith( 'alert', '1', @@ -891,9 +906,6 @@ describe('enable()', () => { version: '123', } ); - expect(savedObjectsClient.update).toHaveBeenCalledWith('alert', '1', { - scheduledTaskId: 'task-123', - }); expect(taskManager.schedule).toHaveBeenCalledWith({ taskType: `alerting:2`, params: { @@ -907,52 +919,45 @@ describe('enable()', () => { }, scope: ['alerting'], }); + expect(savedObjectsClient.update).toHaveBeenCalledWith('alert', '1', { + scheduledTaskId: 'task-123', + }); }); - test(`doesn't enable already enabled alerts`, async () => { - const alertsClient = new AlertsClient(alertsClientParams); - encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ - id: '1', - type: 'alert', + test('invalidates API key if ever one existed prior to updating', async () => { + encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue({ + ...existingAlert, attributes: { - schedule: { interval: '10s' }, - alertTypeId: '2', - enabled: true, + ...existingAlert.attributes, + apiKey: Buffer.from('123:abc').toString('base64'), }, - references: [], }); await alertsClient.enable({ id: '1' }); - expect(taskManager.schedule).toHaveBeenCalledTimes(0); - expect(savedObjectsClient.update).toHaveBeenCalledTimes(0); + expect(savedObjectsClient.get).not.toHaveBeenCalled(); + expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', { + namespace: 'default', + }); + expect(alertsClientParams.invalidateAPIKey).toHaveBeenCalledWith({ id: '123' }); }); - test('calls the API key function', async () => { - const alertsClient = new AlertsClient(alertsClientParams); + test(`doesn't enable already enabled alerts`, async () => { encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ - id: '1', - type: 'alert', + ...existingAlert, attributes: { - schedule: { interval: '10s' }, - alertTypeId: '2', - enabled: false, + ...existingAlert.attributes, + enabled: true, }, - version: '123', - references: [], - }); - taskManager.schedule.mockResolvedValueOnce({ - id: 'task-123', - scheduledAt: new Date(), - attempts: 0, - status: TaskStatus.Idle, - runAt: new Date(), - state: {}, - params: {}, - taskType: '', - startedAt: null, - retryAt: null, - ownerId: null, }); + + await alertsClient.enable({ id: '1' }); + expect(alertsClientParams.getUserName).not.toHaveBeenCalled(); + expect(alertsClientParams.createAPIKey).not.toHaveBeenCalled(); + expect(savedObjectsClient.update).not.toHaveBeenCalled(); + expect(taskManager.schedule).not.toHaveBeenCalled(); + }); + + test('sets API key when createAPIKey returns one', async () => { alertsClientParams.createAPIKey.mockResolvedValueOnce({ apiKeysEnabled: true, result: { id: '123', api_key: 'abc' }, @@ -974,77 +979,136 @@ describe('enable()', () => { version: '123', } ); - expect(savedObjectsClient.update).toHaveBeenCalledWith('alert', '1', { - scheduledTaskId: 'task-123', - }); - expect(taskManager.schedule).toHaveBeenCalledWith({ - taskType: `alerting:2`, - params: { - alertId: '1', - spaceId: 'default', - }, - state: { - alertInstances: {}, - alertTypeState: {}, - previousStartedAt: null, - }, - scope: ['alerting'], - }); }); - 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('falls back when failing to getDecryptedAsInternalUser', async () => { + encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValue(new Error('Fail')); + + await alertsClient.enable({ id: '1' }); + expect(savedObjectsClient.get).toHaveBeenCalledWith('alert', '1'); + expect(alertsClientParams.logger.error).toHaveBeenCalledWith( + 'enable(): Failed to load API key to invalidate on alert 1: Fail' + ); + }); + + test('throws error when failing to load the saved object using SOC', async () => { + encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValue(new Error('Fail')); + savedObjectsClient.get.mockRejectedValueOnce(new Error('Fail to get')); + + await expect(alertsClient.enable({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot( + `"Fail to get"` + ); + expect(alertsClientParams.getUserName).not.toHaveBeenCalled(); + expect(alertsClientParams.createAPIKey).not.toHaveBeenCalled(); + expect(savedObjectsClient.update).not.toHaveBeenCalled(); + expect(taskManager.schedule).not.toHaveBeenCalled(); + }); + + test('throws error when failing to update the first time', async () => { + savedObjectsClient.update.mockRejectedValueOnce(new Error('Fail to update')); + + await expect(alertsClient.enable({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot( + `"Fail to update"` + ); + expect(alertsClientParams.getUserName).toHaveBeenCalled(); + expect(alertsClientParams.createAPIKey).toHaveBeenCalled(); + expect(savedObjectsClient.update).toHaveBeenCalledTimes(1); + expect(taskManager.schedule).not.toHaveBeenCalled(); + }); + + test('throws error when failing to update the second time', async () => { + savedObjectsClient.update.mockResolvedValueOnce({ + ...existingAlert, attributes: { - schedule: { interval: '10s' }, - alertTypeId: '2', - enabled: false, - apiKey: Buffer.from('123:abc').toString('base64'), + ...existingAlert.attributes, + enabled: true, }, - version: '123', - references: [], - }); - taskManager.schedule.mockResolvedValueOnce({ - id: 'task-123', - scheduledAt: new Date(), - attempts: 0, - status: TaskStatus.Idle, - runAt: new Date(), - state: {}, - params: {}, - taskType: '', - startedAt: null, - retryAt: null, - ownerId: null, }); + savedObjectsClient.update.mockRejectedValueOnce(new Error('Fail to update second time')); - await alertsClient.enable({ id: '1' }); - expect(alertsClientParams.logger.error).toHaveBeenCalledWith( - 'Failed to invalidate API Key: Fail' + await expect(alertsClient.enable({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot( + `"Fail to update second time"` ); + expect(alertsClientParams.getUserName).toHaveBeenCalled(); + expect(alertsClientParams.createAPIKey).toHaveBeenCalled(); + expect(savedObjectsClient.update).toHaveBeenCalledTimes(2); + expect(taskManager.schedule).toHaveBeenCalled(); + }); + + test('throws error when failing to schedule task', async () => { + taskManager.schedule.mockRejectedValueOnce(new Error('Fail to schedule')); + + await expect(alertsClient.enable({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot( + `"Fail to schedule"` + ); + expect(alertsClientParams.getUserName).toHaveBeenCalled(); + expect(alertsClientParams.createAPIKey).toHaveBeenCalled(); + expect(savedObjectsClient.update).toHaveBeenCalled(); }); }); describe('disable()', () => { + let alertsClient: AlertsClient; + const existingAlert = { + id: '1', + type: 'alert', + attributes: { + schedule: { interval: '10s' }, + alertTypeId: '2', + enabled: true, + scheduledTaskId: 'task-123', + }, + version: '123', + references: [], + }; + 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); + }); + test('disables an alert', async () => { - const alertsClient = new AlertsClient(alertsClientParams); - savedObjectsClient.get.mockResolvedValueOnce({ - id: '1', - type: 'alert', - attributes: { + await alertsClient.disable({ id: '1' }); + expect(savedObjectsClient.get).not.toHaveBeenCalled(); + expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', { + namespace: 'default', + }); + expect(savedObjectsClient.update).toHaveBeenCalledWith( + 'alert', + '1', + { schedule: { interval: '10s' }, alertTypeId: '2', - enabled: true, - scheduledTaskId: 'task-123', + apiKey: null, + apiKeyOwner: null, + enabled: false, + scheduledTaskId: null, + updatedBy: 'elastic', }, - version: '123', - references: [], - }); + { + version: '123', + } + ); + expect(taskManager.remove).toHaveBeenCalledWith('task-123'); + expect(alertsClientParams.invalidateAPIKey).toHaveBeenCalledWith({ id: '123' }); + }); + + test('falls back when getDecryptedAsInternalUser throws an error', async () => { + encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValueOnce(new Error('Fail')); await alertsClient.disable({ id: '1' }); + expect(savedObjectsClient.get).toHaveBeenCalledWith('alert', '1'); + expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', { + namespace: 'default', + }); expect(savedObjectsClient.update).toHaveBeenCalledWith( 'alert', '1', @@ -1062,25 +1126,66 @@ describe('disable()', () => { } ); expect(taskManager.remove).toHaveBeenCalledWith('task-123'); + expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled(); }); test(`doesn't disable already disabled alerts`, async () => { - const alertsClient = new AlertsClient(alertsClientParams); - savedObjectsClient.get.mockResolvedValueOnce({ - id: '1', - type: 'alert', + encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ + ...existingDecryptedAlert, attributes: { - schedule: { interval: '10s' }, - alertTypeId: '2', + ...existingDecryptedAlert.attributes, enabled: false, - scheduledTaskId: 'task-123', }, - references: [], }); await alertsClient.disable({ id: '1' }); - expect(savedObjectsClient.update).toHaveBeenCalledTimes(0); - expect(taskManager.remove).toHaveBeenCalledTimes(0); + expect(savedObjectsClient.update).not.toHaveBeenCalled(); + expect(taskManager.remove).not.toHaveBeenCalled(); + expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled(); + }); + + test(`doesn't invalidate when no API key is used`, async () => { + encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce(existingAlert); + + await alertsClient.disable({ id: '1' }); + expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled(); + }); + + test('swallows error when failing to load decrypted saved object', async () => { + encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValueOnce(new Error('Fail')); + + await alertsClient.disable({ id: '1' }); + expect(savedObjectsClient.update).toHaveBeenCalled(); + expect(taskManager.remove).toHaveBeenCalled(); + expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled(); + expect(alertsClientParams.logger.error).toHaveBeenCalledWith( + 'disable(): Failed to load API key to invalidate on alert 1: Fail' + ); + }); + + test('throws when savedObjectsClient update fails', async () => { + savedObjectsClient.update.mockRejectedValueOnce(new Error('Failed to update')); + + await expect(alertsClient.disable({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot( + `"Failed to update"` + ); + }); + + test('swallows error when invalidate API key throws', async () => { + alertsClientParams.invalidateAPIKey.mockRejectedValueOnce(new Error('Fail')); + + await alertsClient.disable({ id: '1' }); + expect(alertsClientParams.logger.error).toHaveBeenCalledWith( + 'Failed to invalidate API Key: Fail' + ); + }); + + test('throws when failing to remove task from task manager', async () => { + taskManager.remove.mockRejectedValueOnce(new Error('Failed to remove task')); + + await expect(alertsClient.disable({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot( + `"Failed to remove task"` + ); }); }); diff --git a/x-pack/legacy/plugins/alerting/server/alerts_client.ts b/x-pack/legacy/plugins/alerting/server/alerts_client.ts index f6841ed5a0e46..1b4fe89212b79 100644 --- a/x-pack/legacy/plugins/alerting/server/alerts_client.ts +++ b/x-pack/legacy/plugins/alerting/server/alerts_client.ts @@ -385,12 +385,27 @@ export class AlertsClient { } public async enable({ 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( + `enable(): 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; + } if (attributes.enabled === false) { const username = await this.getUserName(); @@ -407,12 +422,35 @@ export class AlertsClient { ); const scheduledTask = await this.scheduleAlert(id, attributes.alertTypeId); await this.savedObjectsClient.update('alert', id, { scheduledTaskId: scheduledTask.id }); - await this.invalidateApiKey({ apiKey: attributes.apiKey }); + if (apiKeyToInvalidate) { + await this.invalidateApiKey({ apiKey: apiKeyToInvalidate }); + } } } public async disable({ id }: { id: string }) { - const { attributes, version } = await this.savedObjectsClient.get('alert', id); + 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( + `disable(): 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; + } + if (attributes.enabled === true) { await this.savedObjectsClient.update( 'alert', @@ -427,7 +465,11 @@ export class AlertsClient { }, { version } ); - await this.taskManager.remove(attributes.scheduledTaskId); + + await Promise.all([ + attributes.scheduledTaskId ? this.taskManager.remove(attributes.scheduledTaskId) : null, + apiKeyToInvalidate ? this.invalidateApiKey({ apiKey: apiKeyToInvalidate }) : null, + ]); } } diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/disable.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/disable.ts index bafca30abf28a..5007cfa6cf044 100644 --- a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/disable.ts +++ b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/disable.ts @@ -84,6 +84,62 @@ export default function createDisableAlertTests({ getService }: FtrProviderConte } }); + it('should still be able to disable alert when AAD is broken', async () => { + const { body: createdAlert } = await supertest + .post(`${getUrlPrefix(space.id)}/api/alert`) + .set('kbn-xsrf', 'foo') + .send(getTestAlertData({ enabled: true })) + .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.getDisableRequest(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', + }); + // 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); + } + // 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 disable alert from another space`, async () => { const { body: createdAlert } = await supertest .post(`${getUrlPrefix('other')}/api/alert`) diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/enable.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/enable.ts index 9df1f955232b1..d89172515757b 100644 --- a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/enable.ts +++ b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/enable.ts @@ -89,6 +89,67 @@ export default function createEnableAlertTests({ getService }: FtrProviderContex } }); + it('should still be able to enable alert when AAD is broken', async () => { + const { body: createdAlert } = await supertest + .post(`${getUrlPrefix(space.id)}/api/alert`) + .set('kbn-xsrf', 'foo') + .send(getTestAlertData({ enabled: false })) + .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.getEnableRequest(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(typeof updatedAlert.scheduledTaskId).to.eql('string'); + const { _source: taskRecord } = await getScheduledTask(updatedAlert.scheduledTaskId); + expect(taskRecord.type).to.eql('task'); + expect(taskRecord.task.taskType).to.eql('alerting:test.noop'); + expect(JSON.parse(taskRecord.task.params)).to.eql({ + alertId: createdAlert.id, + spaceId: space.id, + }); + // 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 enable alert from another space`, async () => { const { body: createdAlert } = await supertest .post(`${getUrlPrefix('other')}/api/alert`)