Skip to content

Commit 57d8509

Browse files
mikecotemikecote
authored
Fix alerts unable to create / update when the name has trailing whitepace(s) (#76079) (#76170)
* Trim alert name in API key name * Add API integration tests
1 parent 12579fb commit 57d8509

File tree

4 files changed

+219
-3
lines changed

4 files changed

+219
-3
lines changed

‎x-pack/plugins/alerts/server/alerts_client.test.ts‎

Lines changed: 127 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -652,6 +652,70 @@ describe('create()', () => {
652652
expect(taskManager.schedule).toHaveBeenCalledTimes(0);
653653
});
654654

655+
test('should trim alert name when creating API key', async () => {
656+
const data = getMockData({ name: ' my alert name ' });
657+
unsecuredSavedObjectsClient.bulkGet.mockResolvedValueOnce({
658+
saved_objects: [
659+
{
660+
id: '1',
661+
type: 'action',
662+
attributes: {
663+
actions: [],
664+
actionTypeId: 'test',
665+
},
666+
references: [],
667+
},
668+
],
669+
});
670+
unsecuredSavedObjectsClient.create.mockResolvedValueOnce({
671+
id: '1',
672+
type: 'alert',
673+
attributes: {
674+
enabled: false,
675+
name: ' my alert name ',
676+
alertTypeId: '123',
677+
schedule: { interval: 10000 },
678+
params: {
679+
bar: true,
680+
},
681+
createdAt: new Date().toISOString(),
682+
actions: [
683+
{
684+
group: 'default',
685+
actionRef: 'action_0',
686+
actionTypeId: 'test',
687+
params: {
688+
foo: true,
689+
},
690+
},
691+
],
692+
},
693+
references: [
694+
{
695+
name: 'action_0',
696+
type: 'action',
697+
id: '1',
698+
},
699+
],
700+
});
701+
taskManager.schedule.mockResolvedValueOnce({
702+
id: 'task-123',
703+
taskType: 'alerting:123',
704+
scheduledAt: new Date(),
705+
attempts: 1,
706+
status: TaskStatus.Idle,
707+
runAt: new Date(),
708+
startedAt: null,
709+
retryAt: null,
710+
state: {},
711+
params: {},
712+
ownerId: null,
713+
});
714+
715+
await alertsClient.create({ data });
716+
expect(alertsClientParams.createAPIKey).toHaveBeenCalledWith('Alerting: 123/my alert name');
717+
});
718+
655719
test('should validate params', async () => {
656720
const data = getMockData();
657721
alertTypeRegistry.get.mockReturnValue({
@@ -2896,9 +2960,13 @@ describe('update()', () => {
28962960
type: 'alert',
28972961
attributes: {
28982962
enabled: true,
2963+
tags: ['foo'],
28992964
alertTypeId: 'myType',
2965+
schedule: { interval: '10s' },
29002966
consumer: 'myApp',
29012967
scheduledTaskId: 'task-123',
2968+
params: {},
2969+
throttle: null,
29022970
actions: [
29032971
{
29042972
group: 'default',
@@ -2927,7 +2995,7 @@ describe('update()', () => {
29272995
unsecuredSavedObjectsClient.get.mockResolvedValue(existingAlert);
29282996
encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue(existingDecryptedAlert);
29292997
alertTypeRegistry.get.mockReturnValue({
2930-
id: '123',
2998+
id: 'myType',
29312999
name: 'Test',
29323000
actionGroups: [{ id: 'default', name: 'Default' }],
29333001
defaultActionGroupId: 'default',
@@ -3489,6 +3557,64 @@ describe('update()', () => {
34893557
);
34903558
});
34913559

3560+
it('should trim alert name in the API key name', async () => {
3561+
unsecuredSavedObjectsClient.bulkGet.mockResolvedValueOnce({
3562+
saved_objects: [
3563+
{
3564+
id: '1',
3565+
type: 'action',
3566+
attributes: {
3567+
actions: [],
3568+
actionTypeId: 'test',
3569+
},
3570+
references: [],
3571+
},
3572+
],
3573+
});
3574+
unsecuredSavedObjectsClient.update.mockResolvedValueOnce({
3575+
id: '1',
3576+
type: 'alert',
3577+
attributes: {
3578+
enabled: false,
3579+
name: ' my alert name ',
3580+
schedule: { interval: '10s' },
3581+
params: {
3582+
bar: true,
3583+
},
3584+
createdAt: new Date().toISOString(),
3585+
actions: [
3586+
{
3587+
group: 'default',
3588+
actionRef: 'action_0',
3589+
actionTypeId: 'test',
3590+
params: {
3591+
foo: true,
3592+
},
3593+
},
3594+
],
3595+
scheduledTaskId: 'task-123',
3596+
apiKey: null,
3597+
},
3598+
updated_at: new Date().toISOString(),
3599+
references: [
3600+
{
3601+
name: 'action_0',
3602+
type: 'action',
3603+
id: '1',
3604+
},
3605+
],
3606+
});
3607+
await alertsClient.update({
3608+
id: '1',
3609+
data: {
3610+
...existingAlert.attributes,
3611+
name: ' my alert name ',
3612+
},
3613+
});
3614+
3615+
expect(alertsClientParams.createAPIKey).toHaveBeenCalledWith('Alerting: myType/my alert name');
3616+
});
3617+
34923618
it('swallows error when invalidate API key throws', async () => {
34933619
alertsClientParams.invalidateAPIKey.mockRejectedValueOnce(new Error('Fail'));
34943620
unsecuredSavedObjectsClient.bulkGet.mockResolvedValueOnce({

‎x-pack/plugins/alerts/server/alerts_client.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
*/
66

77
import Boom from 'boom';
8-
import { omit, isEqual, map, uniq, pick, truncate } from 'lodash';
8+
import { omit, isEqual, map, uniq, pick, truncate, trim } from 'lodash';
99
import { i18n } from '@kbn/i18n';
1010
import {
1111
Logger,
@@ -940,7 +940,7 @@ export class AlertsClient {
940940
}
941941

942942
private generateAPIKeyName(alertTypeId: string, alertName: string) {
943-
return truncate(`Alerting: ${alertTypeId}/${alertName}`, { length: 256 });
943+
return truncate(`Alerting: ${alertTypeId}/${trim(alertName)}`, { length: 256 });
944944
}
945945
}
946946

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

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -347,6 +347,45 @@ export default function createAlertTests({ getService }: FtrProviderContext) {
347347
}
348348
});
349349

350+
it('should handle create alert request appropriately when alert name has leading and trailing whitespaces', async () => {
351+
const response = await supertestWithoutAuth
352+
.post(`${getUrlPrefix(space.id)}/api/alerts/alert`)
353+
.set('kbn-xsrf', 'foo')
354+
.auth(user.username, user.password)
355+
.send(
356+
getTestAlertData({
357+
name: ' leading and trailing whitespace ',
358+
})
359+
);
360+
361+
switch (scenario.id) {
362+
case 'no_kibana_privileges at space1':
363+
case 'global_read at space1':
364+
case 'space_1_all at space2':
365+
expect(response.statusCode).to.eql(403);
366+
expect(response.body).to.eql({
367+
error: 'Forbidden',
368+
message: getConsumerUnauthorizedErrorMessage(
369+
'create',
370+
'test.noop',
371+
'alertsFixture'
372+
),
373+
statusCode: 403,
374+
});
375+
break;
376+
case 'superuser at space1':
377+
case 'space_1_all at space1':
378+
case 'space_1_all_alerts_none_actions at space1':
379+
case 'space_1_all_with_restricted_fixture at space1':
380+
expect(response.statusCode).to.eql(200);
381+
expect(response.body.name).to.eql(' leading and trailing whitespace ');
382+
objectRemover.add(space.id, response.body.id, 'alert', 'alerts');
383+
break;
384+
default:
385+
throw new Error(`Scenario untested: ${JSON.stringify(scenario)}`);
386+
}
387+
});
388+
350389
it('should handle create alert request appropriately when alert type is unregistered', async () => {
351390
const response = await supertestWithoutAuth
352391
.post(`${getUrlPrefix(space.id)}/api/alerts/alert`)

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

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -505,6 +505,57 @@ export default function createUpdateTests({ getService }: FtrProviderContext) {
505505
}
506506
});
507507

508+
it('should handle update alert request appropriately when alert name has leading and trailing whitespaces', async () => {
509+
const { body: createdAlert } = await supertest
510+
.post(`${getUrlPrefix(space.id)}/api/alerts/alert`)
511+
.set('kbn-xsrf', 'foo')
512+
.send(getTestAlertData())
513+
.expect(200);
514+
objectRemover.add(space.id, createdAlert.id, 'alert', 'alerts');
515+
516+
const updatedData = {
517+
name: ' leading and trailing whitespace ',
518+
tags: ['bar'],
519+
params: {
520+
foo: true,
521+
},
522+
schedule: { interval: '12s' },
523+
actions: [],
524+
throttle: '1m',
525+
};
526+
const response = await supertestWithoutAuth
527+
.put(`${getUrlPrefix(space.id)}/api/alerts/alert/${createdAlert.id}`)
528+
.set('kbn-xsrf', 'foo')
529+
.auth(user.username, user.password)
530+
.send(updatedData);
531+
532+
switch (scenario.id) {
533+
case 'no_kibana_privileges at space1':
534+
case 'space_1_all at space2':
535+
case 'global_read at space1':
536+
expect(response.statusCode).to.eql(403);
537+
expect(response.body).to.eql({
538+
error: 'Forbidden',
539+
message: getConsumerUnauthorizedErrorMessage(
540+
'update',
541+
'test.noop',
542+
'alertsFixture'
543+
),
544+
statusCode: 403,
545+
});
546+
break;
547+
case 'superuser at space1':
548+
case 'space_1_all at space1':
549+
case 'space_1_all_alerts_none_actions at space1':
550+
case 'space_1_all_with_restricted_fixture at space1':
551+
expect(response.statusCode).to.eql(200);
552+
expect(response.body.name).to.eql(' leading and trailing whitespace ');
553+
break;
554+
default:
555+
throw new Error(`Scenario untested: ${JSON.stringify(scenario)}`);
556+
}
557+
});
558+
508559
it(`shouldn't update alert from another space`, async () => {
509560
const { body: createdAlert } = await supertest
510561
.post(`${getUrlPrefix(space.id)}/api/alerts/alert`)

0 commit comments

Comments
 (0)