Skip to content

Commit 68cb039

Browse files
mikecoteelasticmachine
mikecote
and
elasticmachine
committed
Ability to delete alerts even when AAD is out of sync (#56543)
* Ability to delete alerts even when AAD is bad * Small code fixes Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
1 parent dd41917 commit 68cb039

3 files changed

Lines changed: 168 additions & 78 deletions

File tree

‎x-pack/legacy/plugins/alerting/server/alerts_client.test.ts‎

Lines changed: 94 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -1436,97 +1436,120 @@ describe('find()', () => {
14361436
});
14371437

14381438
describe('delete()', () => {
1439-
test('successfully removes an alert', async () => {
1440-
const alertsClient = new AlertsClient(alertsClientParams);
1441-
encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({
1442-
id: '1',
1443-
type: 'alert',
1444-
attributes: {
1445-
alertTypeId: '123',
1446-
schedule: { interval: '10s' },
1447-
params: {
1448-
bar: true,
1449-
},
1450-
scheduledTaskId: 'task-123',
1451-
actions: [
1452-
{
1453-
group: 'default',
1454-
actionRef: 'action_0',
1455-
params: {
1456-
foo: true,
1457-
},
1458-
},
1459-
],
1439+
let alertsClient: AlertsClient;
1440+
const existingAlert = {
1441+
id: '1',
1442+
type: 'alert',
1443+
attributes: {
1444+
alertTypeId: '123',
1445+
schedule: { interval: '10s' },
1446+
params: {
1447+
bar: true,
14601448
},
1461-
references: [
1449+
scheduledTaskId: 'task-123',
1450+
actions: [
14621451
{
1463-
name: 'action_0',
1464-
type: 'action',
1465-
id: '1',
1452+
group: 'default',
1453+
actionRef: 'action_0',
1454+
params: {
1455+
foo: true,
1456+
},
14661457
},
14671458
],
1468-
});
1469-
savedObjectsClient.delete.mockResolvedValueOnce({
1459+
},
1460+
references: [
1461+
{
1462+
name: 'action_0',
1463+
type: 'action',
1464+
id: '1',
1465+
},
1466+
],
1467+
};
1468+
1469+
beforeEach(() => {
1470+
alertsClient = new AlertsClient(alertsClientParams);
1471+
savedObjectsClient.get.mockResolvedValue(existingAlert);
1472+
savedObjectsClient.delete.mockResolvedValue({
14701473
success: true,
14711474
});
1475+
encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue({
1476+
...existingAlert,
1477+
attributes: {
1478+
...existingAlert.attributes,
1479+
apiKey: Buffer.from('123:abc').toString('base64'),
1480+
},
1481+
});
1482+
});
1483+
1484+
test('successfully removes an alert', async () => {
14721485
const result = await alertsClient.delete({ id: '1' });
14731486
expect(result).toEqual({ success: true });
1474-
expect(savedObjectsClient.delete).toHaveBeenCalledTimes(1);
1475-
expect(savedObjectsClient.delete.mock.calls[0]).toMatchInlineSnapshot(`
1476-
Array [
1477-
"alert",
1478-
"1",
1479-
]
1480-
`);
1481-
expect(taskManager.remove).toHaveBeenCalledTimes(1);
1482-
expect(taskManager.remove.mock.calls[0]).toMatchInlineSnapshot(`
1483-
Array [
1484-
"task-123",
1485-
]
1486-
`);
1487+
expect(savedObjectsClient.delete).toHaveBeenCalledWith('alert', '1');
1488+
expect(taskManager.remove).toHaveBeenCalledWith('task-123');
1489+
expect(alertsClientParams.invalidateAPIKey).toHaveBeenCalledWith({ id: '123' });
14871490
});
14881491

1489-
test('swallows error when invalidate API key throws', async () => {
1490-
const alertsClient = new AlertsClient(alertsClientParams);
1491-
alertsClientParams.invalidateAPIKey.mockRejectedValueOnce(new Error('Fail'));
1492-
encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({
1493-
id: '1',
1494-
type: 'alert',
1492+
test(`doesn't remove a task when scheduledTaskId is null`, async () => {
1493+
savedObjectsClient.get.mockResolvedValue({
1494+
...existingAlert,
14951495
attributes: {
1496-
alertTypeId: '123',
1497-
schedule: { interval: '10s' },
1498-
params: {
1499-
bar: true,
1500-
},
1501-
apiKey: Buffer.from('123:abc').toString('base64'),
1502-
scheduledTaskId: 'task-123',
1503-
actions: [
1504-
{
1505-
group: 'default',
1506-
actionRef: 'action_0',
1507-
params: {
1508-
foo: true,
1509-
},
1510-
},
1511-
],
1496+
...existingAlert.attributes,
1497+
scheduledTaskId: null,
15121498
},
1513-
references: [
1514-
{
1515-
name: 'action_0',
1516-
type: 'action',
1517-
id: '1',
1518-
},
1519-
],
15201499
});
1521-
savedObjectsClient.delete.mockResolvedValueOnce({
1522-
success: true,
1500+
1501+
await alertsClient.delete({ id: '1' });
1502+
expect(taskManager.remove).not.toHaveBeenCalled();
1503+
});
1504+
1505+
test(`doesn't invalidate API key when apiKey is null`, async () => {
1506+
encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue({
1507+
...existingAlert,
1508+
attributes: {
1509+
...existingAlert.attributes,
1510+
apiKey: null,
1511+
},
15231512
});
15241513

15251514
await alertsClient.delete({ id: '1' });
1515+
expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled();
1516+
});
1517+
1518+
test('swallows error when invalidate API key throws', async () => {
1519+
alertsClientParams.invalidateAPIKey.mockRejectedValueOnce(new Error('Fail'));
1520+
1521+
await alertsClient.delete({ id: '1' });
1522+
expect(alertsClientParams.invalidateAPIKey).toHaveBeenCalledWith({ id: '123' });
15261523
expect(alertsClientParams.logger.error).toHaveBeenCalledWith(
15271524
'Failed to invalidate API Key: Fail'
15281525
);
15291526
});
1527+
1528+
test('swallows error when getDecryptedAsInternalUser throws an error', async () => {
1529+
encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValue(new Error('Fail'));
1530+
1531+
await alertsClient.delete({ id: '1' });
1532+
expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled();
1533+
expect(alertsClientParams.logger.error).toHaveBeenCalledWith(
1534+
'delete(): Failed to load API key to invalidate on alert 1: Fail'
1535+
);
1536+
});
1537+
1538+
test('throws error when savedObjectsClient.get throws an error', async () => {
1539+
savedObjectsClient.get.mockRejectedValue(new Error('SOC Fail'));
1540+
1541+
await expect(alertsClient.delete({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot(
1542+
`"SOC Fail"`
1543+
);
1544+
});
1545+
1546+
test('throws error when taskManager.remove throws an error', async () => {
1547+
taskManager.remove.mockRejectedValue(new Error('TM Fail'));
1548+
1549+
await expect(alertsClient.delete({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot(
1550+
`"TM Fail"`
1551+
);
1552+
});
15301553
});
15311554

15321555
describe('update()', () => {

‎x-pack/legacy/plugins/alerting/server/alerts_client.ts‎

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -226,14 +226,29 @@ export class AlertsClient {
226226
}
227227

228228
public async delete({ id }: { id: string }) {
229-
const decryptedAlertSavedObject = await this.encryptedSavedObjectsPlugin.getDecryptedAsInternalUser<
230-
RawAlert
231-
>('alert', id, { namespace: this.namespace });
229+
const [taskIdToRemove, apiKeyToInvalidate] = await Promise.all([
230+
this.savedObjectsClient
231+
.get<RawAlert>('alert', id)
232+
.then(result => result.attributes.scheduledTaskId),
233+
// We'll try and load the decrypted saved object but if this fails we'll only log
234+
// and skip invalidating the API key.
235+
this.encryptedSavedObjectsPlugin
236+
.getDecryptedAsInternalUser<RawAlert>('alert', id, { namespace: this.namespace })
237+
.then(result => result.attributes.apiKey)
238+
.catch(e =>
239+
this.logger.error(
240+
`delete(): Failed to load API key to invalidate on alert ${id}: ${e.message}`
241+
)
242+
),
243+
]);
244+
232245
const removeResult = await this.savedObjectsClient.delete('alert', id);
233-
if (decryptedAlertSavedObject.attributes.scheduledTaskId) {
234-
await this.taskManager.remove(decryptedAlertSavedObject.attributes.scheduledTaskId);
235-
}
236-
await this.invalidateApiKey({ apiKey: decryptedAlertSavedObject.attributes.apiKey });
246+
247+
await Promise.all([
248+
taskIdToRemove && this.taskManager.remove(taskIdToRemove),
249+
apiKeyToInvalidate && this.invalidateApiKey({ apiKey: apiKeyToInvalidate }),
250+
]);
251+
237252
return removeResult;
238253
}
239254

‎x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/delete.ts‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,58 @@ export default function createDeleteTests({ getService }: FtrProviderContext) {
108108
throw new Error(`Scenario untested: ${JSON.stringify(scenario)}`);
109109
}
110110
});
111+
112+
it('should still be able to delete alert when AAD is broken', async () => {
113+
const { body: createdAlert } = await supertest
114+
.post(`${getUrlPrefix(space.id)}/api/alert`)
115+
.set('kbn-xsrf', 'foo')
116+
.send(getTestAlertData())
117+
.expect(200);
118+
119+
await supertest
120+
.put(`${getUrlPrefix(space.id)}/api/saved_objects/alert/${createdAlert.id}`)
121+
.set('kbn-xsrf', 'foo')
122+
.send({
123+
attributes: {
124+
name: 'bar',
125+
},
126+
})
127+
.expect(200);
128+
129+
const response = await supertestWithoutAuth
130+
.delete(`${getUrlPrefix(space.id)}/api/alert/${createdAlert.id}`)
131+
.set('kbn-xsrf', 'foo')
132+
.auth(user.username, user.password);
133+
134+
switch (scenario.id) {
135+
case 'no_kibana_privileges at space1':
136+
case 'space_1_all at space2':
137+
case 'global_read at space1':
138+
expect(response.statusCode).to.eql(404);
139+
expect(response.body).to.eql({
140+
statusCode: 404,
141+
error: 'Not Found',
142+
message: 'Not Found',
143+
});
144+
objectRemover.add(space.id, createdAlert.id, 'alert');
145+
// Ensure task still exists
146+
await getScheduledTask(createdAlert.scheduledTaskId);
147+
break;
148+
case 'superuser at space1':
149+
case 'space_1_all at space1':
150+
expect(response.statusCode).to.eql(204);
151+
expect(response.body).to.eql('');
152+
try {
153+
await getScheduledTask(createdAlert.scheduledTaskId);
154+
throw new Error('Should have removed scheduled task');
155+
} catch (e) {
156+
expect(e.status).to.eql(404);
157+
}
158+
break;
159+
default:
160+
throw new Error(`Scenario untested: ${JSON.stringify(scenario)}`);
161+
}
162+
});
111163
});
112164
}
113165
});

0 commit comments

Comments
 (0)