Skip to content

Commit

Permalink
Ability to delete alerts even when AAD is out of sync (#56543) (#56687)
Browse files Browse the repository at this point in the history 
* Ability to delete alerts even when AAD is bad

* Small code fixes

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
  • Loading branch information
@mikecote @elasticmachine
mikecote and elasticmachine committed Feb 4, 2020
1 parent 0835738 commit 6d76951
Show file tree
Hide file tree
Showing 3 changed files with 168 additions and 78 deletions.
165 changes: 94 additions & 71 deletions x-pack/legacy/plugins/alerting/server/alerts_client.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1436,97 +1436,120 @@ describe('find()', () => {
});

describe('delete()', () => {
test('successfully removes an alert', async () => {
const alertsClient = new AlertsClient(alertsClientParams);
encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({
id: '1',
type: 'alert',
attributes: {
alertTypeId: '123',
schedule: { interval: '10s' },
params: {
bar: true,
},
scheduledTaskId: 'task-123',
actions: [
{
group: 'default',
actionRef: 'action_0',
params: {
foo: true,
},
},
],
let alertsClient: AlertsClient;
const existingAlert = {
id: '1',
type: 'alert',
attributes: {
alertTypeId: '123',
schedule: { interval: '10s' },
params: {
bar: true,
},
references: [
scheduledTaskId: 'task-123',
actions: [
{
name: 'action_0',
type: 'action',
id: '1',
group: 'default',
actionRef: 'action_0',
params: {
foo: true,
},
},
],
});
savedObjectsClient.delete.mockResolvedValueOnce({
},
references: [
{
name: 'action_0',
type: 'action',
id: '1',
},
],
};

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'),
},
});
});

test('successfully removes an alert', async () => {
const result = await alertsClient.delete({ id: '1' });
expect(result).toEqual({ success: true });
expect(savedObjectsClient.delete).toHaveBeenCalledTimes(1);
expect(savedObjectsClient.delete.mock.calls[0]).toMatchInlineSnapshot(`
Array [
"alert",
"1",
]
`);
expect(taskManager.remove).toHaveBeenCalledTimes(1);
expect(taskManager.remove.mock.calls[0]).toMatchInlineSnapshot(`
Array [
"task-123",
]
`);
expect(savedObjectsClient.delete).toHaveBeenCalledWith('alert', '1');
expect(taskManager.remove).toHaveBeenCalledWith('task-123');
expect(alertsClientParams.invalidateAPIKey).toHaveBeenCalledWith({ id: '123' });
});

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(`doesn't remove a task when scheduledTaskId is null`, async () => {
savedObjectsClient.get.mockResolvedValue({
...existingAlert,
attributes: {
alertTypeId: '123',
schedule: { interval: '10s' },
params: {
bar: true,
},
apiKey: Buffer.from('123:abc').toString('base64'),
scheduledTaskId: 'task-123',
actions: [
{
group: 'default',
actionRef: 'action_0',
params: {
foo: true,
},
},
],
...existingAlert.attributes,
scheduledTaskId: null,
},
references: [
{
name: 'action_0',
type: 'action',
id: '1',
},
],
});
savedObjectsClient.delete.mockResolvedValueOnce({
success: true,

await alertsClient.delete({ id: '1' });
expect(taskManager.remove).not.toHaveBeenCalled();
});

test(`doesn't invalidate API key when apiKey is null`, async () => {
encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue({
...existingAlert,
attributes: {
...existingAlert.attributes,
apiKey: null,
},
});

await alertsClient.delete({ id: '1' });
expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled();
});

test('swallows error when invalidate API key throws', async () => {
alertsClientParams.invalidateAPIKey.mockRejectedValueOnce(new Error('Fail'));

await alertsClient.delete({ id: '1' });
expect(alertsClientParams.invalidateAPIKey).toHaveBeenCalledWith({ id: '123' });
expect(alertsClientParams.logger.error).toHaveBeenCalledWith(
'Failed to invalidate API Key: Fail'
);
});

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

await alertsClient.delete({ id: '1' });
expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled();
expect(alertsClientParams.logger.error).toHaveBeenCalledWith(
'delete(): Failed to load API key to invalidate on alert 1: Fail'
);
});

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

await expect(alertsClient.delete({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot(
`"SOC Fail"`
);
});

test('throws error when taskManager.remove throws an error', async () => {
taskManager.remove.mockRejectedValue(new Error('TM Fail'));

await expect(alertsClient.delete({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot(
`"TM Fail"`
);
});
});

describe('update()', () => {
Expand Down
29 changes: 22 additions & 7 deletions x-pack/legacy/plugins/alerting/server/alerts_client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,14 +225,29 @@ export class AlertsClient {
}

public async delete({ id }: { id: string }) {
const decryptedAlertSavedObject = await this.encryptedSavedObjectsPlugin.getDecryptedAsInternalUser<
RawAlert
>('alert', id, { namespace: this.namespace });
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}`
)
),
]);

const removeResult = await this.savedObjectsClient.delete('alert', id);
if (decryptedAlertSavedObject.attributes.scheduledTaskId) {
await this.taskManager.remove(decryptedAlertSavedObject.attributes.scheduledTaskId);
}
await this.invalidateApiKey({ apiKey: decryptedAlertSavedObject.attributes.apiKey });

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

return removeResult;
}

Expand Down
52 changes: 52 additions & 0 deletions x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/delete.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,58 @@ export default function createDeleteTests({ getService }: FtrProviderContext) {
throw new Error(`Scenario untested: ${JSON.stringify(scenario)}`);
}
});

it('should still be able to delete alert when AAD is broken', async () => {
const { body: createdAlert } = await supertest
.post(`${getUrlPrefix(space.id)}/api/alert`)
.set('kbn-xsrf', 'foo')
.send(getTestAlertData())
.expect(200);

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 supertestWithoutAuth
.delete(`${getUrlPrefix(space.id)}/api/alert/${createdAlert.id}`)
.set('kbn-xsrf', 'foo')
.auth(user.username, user.password);

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',
});
objectRemover.add(space.id, createdAlert.id, 'alert');
// 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);
}
break;
default:
throw new Error(`Scenario untested: ${JSON.stringify(scenario)}`);
}
});
});
}
});
Expand Down

0 comments on commit 6d76951

Please sign in to comment.