Skip to content

Commit

Permalink
Add support for custom alert ids (#89814) (#89911)
Browse files Browse the repository at this point in the history 
* Add support for custom alert ids

* UUID v4 also supported

* Change ESO custom id error message

* Update api integration test

* Use errors.createBadRequestError
  • Loading branch information
@mikecote
mikecote authored Feb 2, 2021
1 parent 669dbc5 commit 2273d9e
Show file tree
Hide file tree
Showing 7 changed files with 244 additions and 29 deletions.
3 changes: 2 additions & 1 deletion x-pack/plugins/alerts/server/alerts_client/alerts_client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ export interface CreateOptions<Params extends AlertTypeParams> {
| 'executionStatus'
> & { actions: NormalizedAlertAction[] };
options?: {
id?: string;
migrationVersion?: Record<string, string>;
};
}
Expand Down Expand Up @@ -226,7 +227,7 @@ export class AlertsClient {
data,
options,
}: CreateOptions<Params>): Promise<Alert<Params>> {
const id = SavedObjectsUtils.generateId();
const id = options?.id || SavedObjectsUtils.generateId();

try {
await this.authorization.ensureAuthorized(
Expand Down
67 changes: 67 additions & 0 deletions x-pack/plugins/alerts/server/alerts_client/tests/create.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,73 @@ describe('create()', () => {
expect(actionsClient.isActionTypeEnabled).toHaveBeenCalledWith('test', { notifyUsage: true });
});

test('creates an alert with a custom id', async () => {
const data = getMockData();
const createdAttributes = {
...data,
alertTypeId: '123',
schedule: { interval: '10s' },
params: {
bar: true,
},
createdAt: '2019-02-12T21:01:22.479Z',
createdBy: 'elastic',
updatedBy: 'elastic',
updatedAt: '2019-02-12T21:01:22.479Z',
muteAll: false,
mutedInstanceIds: [],
actions: [
{
group: 'default',
actionRef: 'action_0',
actionTypeId: 'test',
params: {
foo: true,
},
},
],
};
unsecuredSavedObjectsClient.create.mockResolvedValueOnce({
id: '123',
type: 'alert',
attributes: createdAttributes,
references: [
{
name: 'action_0',
type: 'action',
id: '1',
},
],
});
taskManager.schedule.mockResolvedValueOnce({
id: 'task-123',
taskType: 'alerting:123',
scheduledAt: new Date(),
attempts: 1,
status: TaskStatus.Idle,
runAt: new Date(),
startedAt: null,
retryAt: null,
state: {},
params: {},
ownerId: null,
});
const result = await alertsClient.create({ data, options: { id: '123' } });
expect(result.id).toEqual('123');
expect(unsecuredSavedObjectsClient.create.mock.calls[0][2]).toMatchInlineSnapshot(`
Object {
"id": "123",
"references": Array [
Object {
"id": "1",
"name": "action_0",
"type": "action",
},
],
}
`);
});

test('creates an alert with multiple actions', async () => {
const data = getMockData({
actions: [
Expand Down
73 changes: 72 additions & 1 deletion x-pack/plugins/alerts/server/routes/create.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ describe('createAlertRoute', () => {

const [config, handler] = router.post.mock.calls[0];

expect(config.path).toMatchInlineSnapshot(`"/api/alerts/alert"`);
expect(config.path).toMatchInlineSnapshot(`"/api/alerts/alert/{id?}"`);

alertsClient.create.mockResolvedValueOnce(createResult);

Expand Down Expand Up @@ -125,6 +125,9 @@ describe('createAlertRoute', () => {
],
"throttle": "30s",
},
"options": Object {
"id": undefined,
},
},
]
`);
Expand All @@ -134,6 +137,74 @@ describe('createAlertRoute', () => {
});
});

it('allows providing a custom id', async () => {
const expectedResult = {
...createResult,
id: 'custom-id',
};
const licenseState = licenseStateMock.create();
const router = httpServiceMock.createRouter();

createAlertRoute(router, licenseState);

const [config, handler] = router.post.mock.calls[0];

expect(config.path).toMatchInlineSnapshot(`"/api/alerts/alert/{id?}"`);

alertsClient.create.mockResolvedValueOnce(expectedResult);

const [context, req, res] = mockHandlerArguments(
{ alertsClient },
{
params: { id: 'custom-id' },
body: mockedAlert,
},
['ok']
);

expect(await handler(context, req, res)).toEqual({ body: expectedResult });

expect(alertsClient.create).toHaveBeenCalledTimes(1);
expect(alertsClient.create.mock.calls[0]).toMatchInlineSnapshot(`
Array [
Object {
"data": Object {
"actions": Array [
Object {
"group": "default",
"id": "2",
"params": Object {
"foo": true,
},
},
],
"alertTypeId": "1",
"consumer": "bar",
"name": "abc",
"notifyWhen": "onActionGroupChange",
"params": Object {
"bar": true,
},
"schedule": Object {
"interval": "10s",
},
"tags": Array [
"foo",
],
"throttle": "30s",
},
"options": Object {
"id": "custom-id",
},
},
]
`);

expect(res.ok).toHaveBeenCalledWith({
body: expectedResult,
});
});

it('ensures the license allows creating alerts', async () => {
const licenseState = licenseStateMock.create();
const router = httpServiceMock.createRouter();
Expand Down
9 changes: 8 additions & 1 deletion x-pack/plugins/alerts/server/routes/create.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,13 @@ export const bodySchema = schema.object({
export const createAlertRoute = (router: AlertingRouter, licenseState: ILicenseState) => {
router.post(
{
path: `${BASE_ALERT_API_PATH}/alert`,
path: `${BASE_ALERT_API_PATH}/alert/{id?}`,
validate: {
params: schema.maybe(
schema.object({
id: schema.maybe(schema.string()),
})
),
body: bodySchema,
},
},
Expand All @@ -59,10 +64,12 @@ export const createAlertRoute = (router: AlertingRouter, licenseState: ILicenseS
}
const alertsClient = context.alerting.getAlertsClient();
const alert = req.body;
const params = req.params;
const notifyWhen = alert?.notifyWhen ? (alert.notifyWhen as AlertNotifyWhenType) : null;
try {
const alertRes: Alert<AlertTypeParams> = await alertsClient.create<AlertTypeParams>({
data: { ...alert, notifyWhen },
options: { id: params?.id },
});
return res.ok({
body: alertRes,
Expand Down
8 changes: 4 additions & 4 deletions ...crypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,11 @@ describe('#create', () => {
expect(mockBaseClient.create).toHaveBeenCalledWith('unknown-type', attributes, options);
});

it('fails if type is registered and ID is specified', async () => {
it('fails if type is registered and non-UUID ID is specified', async () => {
const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' };

await expect(wrapper.create('known-type', attributes, { id: 'some-id' })).rejects.toThrowError(
'Predefined IDs are not allowed for saved objects with encrypted attributes, unless the ID has been generated using `SavedObjectsUtils.generateId`.'
'Predefined IDs are not allowed for saved objects with encrypted attributes unless the ID is a UUID.'
);

expect(mockBaseClient.create).not.toHaveBeenCalled();
Expand Down Expand Up @@ -310,7 +310,7 @@ describe('#bulkCreate', () => {
);
});

it('fails if ID is specified for registered type', async () => {
it('fails if non-UUID ID is specified for registered type', async () => {
const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' };

const bulkCreateParams = [
Expand All @@ -319,7 +319,7 @@ describe('#bulkCreate', () => {
];

await expect(wrapper.bulkCreate(bulkCreateParams)).rejects.toThrowError(
'Predefined IDs are not allowed for saved objects with encrypted attributes, unless the ID has been generated using `SavedObjectsUtils.generateId`.'
'Predefined IDs are not allowed for saved objects with encrypted attributes unless the ID is a UUID.'
);

expect(mockBaseClient.bulkCreate).not.toHaveBeenCalled();
Expand Down
44 changes: 22 additions & 22 deletions ...ns/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon
return await this.options.baseClient.create(type, attributes, options);
}

const id = getValidId(options.id, options.version, options.overwrite);
const id = this.getValidId(options.id, options.version, options.overwrite);
const namespace = getDescriptorNamespace(
this.options.baseTypeRegistry,
type,
Expand Down Expand Up @@ -93,7 +93,7 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon
return object;
}

const id = getValidId(object.id, object.version, options?.overwrite);
const id = this.getValidId(object.id, object.version, options?.overwrite);
const namespace = getDescriptorNamespace(
this.options.baseTypeRegistry,
object.type,
Expand Down Expand Up @@ -307,27 +307,27 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon

return response;
}
}

// Saved objects with encrypted attributes should have IDs that are hard to guess especially
// since IDs are part of the AAD used during encryption, that's why we control them within this
// wrapper and don't allow consumers to specify their own IDs directly unless overwriting the original document.
function getValidId(
id: string | undefined,
version: string | undefined,
overwrite: boolean | undefined
) {
if (id) {
// only allow a specified ID if we're overwriting an existing ESO with a Version
// this helps us ensure that the document really was previously created using ESO
// and not being used to get around the specified ID limitation
const canSpecifyID = (overwrite && version) || SavedObjectsUtils.isRandomId(id);
if (!canSpecifyID) {
throw new Error(
'Predefined IDs are not allowed for saved objects with encrypted attributes, unless the ID has been generated using `SavedObjectsUtils.generateId`.'
);
// Saved objects with encrypted attributes should have IDs that are hard to guess especially
// since IDs are part of the AAD used during encryption, that's why we control them within this
// wrapper and don't allow consumers to specify their own IDs directly unless overwriting the original document.
private getValidId(
id: string | undefined,
version: string | undefined,
overwrite: boolean | undefined
) {
if (id) {
// only allow a specified ID if we're overwriting an existing ESO with a Version
// this helps us ensure that the document really was previously created using ESO
// and not being used to get around the specified ID limitation
const canSpecifyID = (overwrite && version) || SavedObjectsUtils.isRandomId(id);
if (!canSpecifyID) {
throw this.errors.createBadRequestError(
'Predefined IDs are not allowed for saved objects with encrypted attributes unless the ID is a UUID.'
);
}
return id;
}
return id;
return SavedObjectsUtils.generateId();
}
return SavedObjectsUtils.generateId();
}
69 changes: 69 additions & 0 deletions x-pack/test/alerting_api_integration/spaces_only/tests/alerting/create.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,75 @@ export default function createAlertTests({ getService }: FtrProviderContext) {
});
});

it('should allow providing custom saved object ids (uuid v1)', async () => {
const customId = '09570bb0-6299-11eb-8fde-9fe5ce6ea450';
const response = await supertest
.post(`${getUrlPrefix(Spaces.space1.id)}/api/alerts/alert/${customId}`)
.set('kbn-xsrf', 'foo')
.send(getTestAlertData());

expect(response.status).to.eql(200);
objectRemover.add(Spaces.space1.id, response.body.id, 'alert', 'alerts');
expect(response.body.id).to.eql(customId);
// Ensure AAD isn't broken
await checkAAD({
supertest,
spaceId: Spaces.space1.id,
type: 'alert',
id: customId,
});
});

it('should allow providing custom saved object ids (uuid v4)', async () => {
const customId = 'b3bc6d83-3192-4ffd-9702-ad4fb88617ba';
const response = await supertest
.post(`${getUrlPrefix(Spaces.space1.id)}/api/alerts/alert/${customId}`)
.set('kbn-xsrf', 'foo')
.send(getTestAlertData());

expect(response.status).to.eql(200);
objectRemover.add(Spaces.space1.id, response.body.id, 'alert', 'alerts');
expect(response.body.id).to.eql(customId);
// Ensure AAD isn't broken
await checkAAD({
supertest,
spaceId: Spaces.space1.id,
type: 'alert',
id: customId,
});
});

it('should not allow providing simple custom ids (non uuid)', async () => {
const customId = '1';
const response = await supertest
.post(`${getUrlPrefix(Spaces.space1.id)}/api/alerts/alert/${customId}`)
.set('kbn-xsrf', 'foo')
.send(getTestAlertData());

expect(response.status).to.eql(400);
expect(response.body).to.eql({
statusCode: 400,
error: 'Bad Request',
message:
'Predefined IDs are not allowed for saved objects with encrypted attributes unless the ID is a UUID.: Bad Request',
});
});

it('should return 409 when document with id already exists', async () => {
const customId = '5031f8f0-629a-11eb-b500-d1931a8e5df7';
const createdAlertResponse = await supertest
.post(`${getUrlPrefix(Spaces.space1.id)}/api/alerts/alert/${customId}`)
.set('kbn-xsrf', 'foo')
.send(getTestAlertData())
.expect(200);
objectRemover.add(Spaces.space1.id, createdAlertResponse.body.id, 'alert', 'alerts');
await supertest
.post(`${getUrlPrefix(Spaces.space1.id)}/api/alerts/alert/${customId}`)
.set('kbn-xsrf', 'foo')
.send(getTestAlertData())
.expect(409);
});

it('should handle create alert request appropriately when consumer is unknown', async () => {
const response = await supertest
.post(`${getUrlPrefix(Spaces.space1.id)}/api/alerts/alert`)
Expand Down

0 comments on commit 2273d9e

Please sign in to comment.