Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ability to delete alerts even when AAD is out of sync #56543

Merged
merged 3 commits into from
Feb 3, 2020
Merged

Ability to delete alerts even when AAD is out of sync #56543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e776776
Ability to delete alerts even when AAD is bad
mikecote Jan 31, 2020
8b11a5c
Small code fixes
mikecote Jan 31, 2020
5e70226
Merge branch 'master' into alerting/delete-alerts-broken-aad
elasticmachine Feb 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
165 changes: 94 additions & 71 deletions x-pack/legacy/plugins/alerting/server/alerts_client.test.ts
View file
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,14 +226,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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we should - in a future PR - try to fix this code so we only make one SO get call instead of two. I think, but am not positive, that getDecryptedAsInternalUser() returns the un-encrypted properties also returned from SOC.get(). Not worth figuring out for 7.6 tho.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, so instead of doing two calls in parallel, you only do SOC.get() if ever getDecryptedAsInternalUser() throws an error (in series)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd have to look closer at the impact, but it just doesn't seem right that we're doing two SO lookups, back-to-back. Used to be serially, I think, now it's in parallel, which is better perf-wise for this API, but still hits ES twice, which seems unfortunate, and hopefully, not actually required. If that seems possible, let's open another issue to investigate later. It would be a performance improvement ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have created #56777 to fix this.

.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
Open in desktop
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