Skip to content

Commit

Permalink
Load alert on delete with fallback instead of doubling the calls (#57004
Browse files Browse the repository at this point in the history
)
  • Loading branch information
mikecote authored Feb 6, 2020
1 parent 3a35190 commit 8aa2b61
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 27 deletions.
40 changes: 30 additions & 10 deletions x-pack/legacy/plugins/alerting/server/alerts_client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1465,20 +1465,21 @@ describe('delete()', () => {
},
],
};
const existingDecryptedAlert = {
...existingAlert,
attributes: {
...existingAlert.attributes,
apiKey: Buffer.from('123:abc').toString('base64'),
},
};

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'),
},
});
encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue(existingDecryptedAlert);
});

test('successfully removes an alert', async () => {
Expand All @@ -1487,13 +1488,31 @@ describe('delete()', () => {
expect(savedObjectsClient.delete).toHaveBeenCalledWith('alert', '1');
expect(taskManager.remove).toHaveBeenCalledWith('task-123');
expect(alertsClientParams.invalidateAPIKey).toHaveBeenCalledWith({ id: '123' });
expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', {
namespace: 'default',
});
expect(savedObjectsClient.get).not.toHaveBeenCalled();
});

test('falls back to SOC.get when getDecryptedAsInternalUser throws an error', async () => {
encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValue(new Error('Fail'));

const result = await alertsClient.delete({ id: '1' });
expect(result).toEqual({ success: true });
expect(savedObjectsClient.delete).toHaveBeenCalledWith('alert', '1');
expect(taskManager.remove).toHaveBeenCalledWith('task-123');
expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled();
expect(savedObjectsClient.get).toHaveBeenCalledWith('alert', '1');
expect(alertsClientParams.logger.error).toHaveBeenCalledWith(
'delete(): Failed to load API key to invalidate on alert 1: Fail'
);
});

test(`doesn't remove a task when scheduledTaskId is null`, async () => {
savedObjectsClient.get.mockResolvedValue({
...existingAlert,
encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue({
...existingDecryptedAlert,
attributes: {
...existingAlert.attributes,
...existingDecryptedAlert.attributes,
scheduledTaskId: null,
},
});
Expand Down Expand Up @@ -1536,6 +1555,7 @@ describe('delete()', () => {
});

test('throws error when savedObjectsClient.get throws an error', async () => {
encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValue(new Error('Fail'));
savedObjectsClient.get.mockRejectedValue(new Error('SOC Fail'));

await expect(alertsClient.delete({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot(
Expand Down
37 changes: 20 additions & 17 deletions x-pack/legacy/plugins/alerting/server/alerts_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -226,27 +226,30 @@ export class AlertsClient {
}

public async delete({ id }: { id: string }) {
const [taskIdToRemove, apiKeyToInvalidate] = await Promise.all([
this.savedObjectsClient
.get<RawAlert>('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<RawAlert>('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}`
)
),
]);
let taskIdToRemove: string | undefined;
let apiKeyToInvalidate: string | null = null;

try {
const decryptedAlert = await this.encryptedSavedObjectsPlugin.getDecryptedAsInternalUser<
RawAlert
>('alert', id, { namespace: this.namespace });
apiKeyToInvalidate = decryptedAlert.attributes.apiKey;
taskIdToRemove = decryptedAlert.attributes.scheduledTaskId;
} catch (e) {
// We'll skip invalidating the API key since we failed to load the decrypted saved object
this.logger.error(
`delete(): Failed to load API key to invalidate on alert ${id}: ${e.message}`
);
// Still attempt to load the scheduledTaskId using SOC
const alert = await this.savedObjectsClient.get<RawAlert>('alert', id);
taskIdToRemove = alert.attributes.scheduledTaskId;
}

const removeResult = await this.savedObjectsClient.delete('alert', id);

await Promise.all([
taskIdToRemove && this.taskManager.remove(taskIdToRemove),
apiKeyToInvalidate && this.invalidateApiKey({ apiKey: apiKeyToInvalidate }),
taskIdToRemove ? this.taskManager.remove(taskIdToRemove) : null,
apiKeyToInvalidate ? this.invalidateApiKey({ apiKey: apiKeyToInvalidate }) : null,
]);

return removeResult;
Expand Down

0 comments on commit 8aa2b61

Please sign in to comment.