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.

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

[7.x] Add support for custom alert ids (#89814) #89911

Merged
merged 1 commit into from
Feb 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion x-pack/plugins/alerts/server/alerts_client/alerts_client.ts
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
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
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
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
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
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();
}
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