[7.9] Fix alerts unable to create / update when the name has trailing whitepace(s) (#76079) (#76172)

mikecote · elasticmachine · web-flow · commit 1898b38efb93 · 2020-08-31T13:13:40.000-04:00
* Merge with master

* Fix some references

* Fix test failures

Co-authored-by: Elastic Machine &lt;elasticmachine@users.noreply.github.com&gt;
diff --git a/x-pack/plugins/alerts/server/alerts_client.test.ts b/x-pack/plugins/alerts/server/alerts_client.test.ts
@@ -508,6 +508,70 @@ describe('create()', () => {
     expect(taskManager.schedule).toHaveBeenCalledTimes(0);
   });
 
+  test('should trim alert name when creating API key', async () => {
+    const data = getMockData({ name: ' my alert name ' });
+    savedObjectsClient.bulkGet.mockResolvedValueOnce({
+      saved_objects: [
+        {
+          id: '1',
+          type: 'action',
+          attributes: {
+            actions: [],
+            actionTypeId: 'test',
+          },
+          references: [],
+        },
+      ],
+    });
+    savedObjectsClient.create.mockResolvedValueOnce({
+      id: '1',
+      type: 'alert',
+      attributes: {
+        enabled: false,
+        name: ' my alert name ',
+        alertTypeId: '123',
+        schedule: { interval: 10000 },
+        params: {
+          bar: true,
+        },
+        createdAt: new Date().toISOString(),
+        actions: [
+          {
+            group: 'default',
+            actionRef: 'action_0',
+            actionTypeId: 'test',
+            params: {
+              foo: true,
+            },
+          },
+        ],
+      },
+      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,
+    });
+
+    await alertsClient.create({ data });
+    expect(alertsClientParams.createAPIKey).toHaveBeenCalledWith('Alerting: 123/my alert name');
+  });
+
   test('should validate params', async () => {
     const data = getMockData();
     alertTypeRegistry.get.mockReturnValue({
@@ -1864,8 +1928,24 @@ describe('update()', () => {
     type: 'alert',
     attributes: {
       enabled: true,
-      alertTypeId: '123',
+      tags: ['foo'],
+      alertTypeId: 'myType',
+      schedule: { interval: '10s' },
+      consumer: 'myApp',
       scheduledTaskId: 'task-123',
+      params: {},
+      throttle: null,
+      actions: [
+        {
+          group: 'default',
+          id: '1',
+          actionTypeId: '1',
+          actionRef: '1',
+          params: {
+            foo: true,
+          },
+        },
+      ],
     },
     references: [],
     version: '123',
@@ -1883,7 +1963,7 @@ describe('update()', () => {
     savedObjectsClient.get.mockResolvedValue(existingAlert);
     encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue(existingDecryptedAlert);
     alertTypeRegistry.get.mockReturnValue({
-      id: '123',
+      id: 'myType',
       name: 'Test',
       actionGroups: [{ id: 'default', name: 'Default' }],
       defaultActionGroupId: 'default',
@@ -2062,9 +2142,10 @@ describe('update()', () => {
             },
           },
         ],
-        "alertTypeId": "123",
+        "alertTypeId": "myType",
         "apiKey": null,
         "apiKeyOwner": null,
+        "consumer": "myApp",
         "enabled": true,
         "name": "abc",
         "params": Object {
@@ -2217,9 +2298,10 @@ describe('update()', () => {
             },
           },
         ],
-        "alertTypeId": "123",
+        "alertTypeId": "myType",
         "apiKey": "MTIzOmFiYw==",
         "apiKeyOwner": "elastic",
+        "consumer": "myApp",
         "enabled": true,
         "name": "abc",
         "params": Object {
@@ -2366,9 +2448,10 @@ describe('update()', () => {
             },
           },
         ],
-        "alertTypeId": "123",
+        "alertTypeId": "myType",
         "apiKey": null,
         "apiKeyOwner": null,
+        "consumer": "myApp",
         "enabled": false,
         "name": "abc",
         "params": Object {
@@ -2440,6 +2523,64 @@ describe('update()', () => {
     );
   });
 
+  it('should trim alert name in the API key name', async () => {
+    savedObjectsClient.bulkGet.mockResolvedValueOnce({
+      saved_objects: [
+        {
+          id: '1',
+          type: 'action',
+          attributes: {
+            actions: [],
+            actionTypeId: 'test',
+          },
+          references: [],
+        },
+      ],
+    });
+    savedObjectsClient.update.mockResolvedValueOnce({
+      id: '1',
+      type: 'alert',
+      attributes: {
+        enabled: false,
+        name: ' my alert name ',
+        schedule: { interval: '10s' },
+        params: {
+          bar: true,
+        },
+        createdAt: new Date().toISOString(),
+        actions: [
+          {
+            group: 'default',
+            actionRef: 'action_0',
+            actionTypeId: 'test',
+            params: {
+              foo: true,
+            },
+          },
+        ],
+        scheduledTaskId: 'task-123',
+        apiKey: null,
+      },
+      updated_at: new Date().toISOString(),
+      references: [
+        {
+          name: 'action_0',
+          type: 'action',
+          id: '1',
+        },
+      ],
+    });
+    await alertsClient.update({
+      id: '1',
+      data: {
+        ...existingAlert.attributes,
+        name: ' my alert name ',
+      },
+    });
+
+    expect(alertsClientParams.createAPIKey).toHaveBeenCalledWith('Alerting: myType/my alert name');
+  });
+
   it('swallows error when invalidate API key throws', async () => {
     alertsClientParams.invalidateAPIKey.mockRejectedValueOnce(new Error('Fail'));
     savedObjectsClient.bulkGet.mockResolvedValueOnce({
diff --git a/x-pack/plugins/alerts/server/alerts_client.ts b/x-pack/plugins/alerts/server/alerts_client.ts
@@ -5,7 +5,7 @@
  */
 
 import Boom from 'boom';
-import { omit, isEqual, map, truncate } from 'lodash';
+import { omit, isEqual, map, truncate, trim } from 'lodash';
 import { i18n } from '@kbn/i18n';
 import {
   Logger,
@@ -719,6 +719,6 @@ export class AlertsClient {
   }
 
   private generateAPIKeyName(alertTypeId: string, alertName: string) {
-    return truncate(`Alerting: ${alertTypeId}/${alertName}`, { length: 256 });
+    return truncate(`Alerting: ${alertTypeId}/${trim(alertName)}`, { length: 256 });
   }
 }
diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/create.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/create.ts
@@ -153,6 +153,39 @@ export default function createAlertTests({ getService }: FtrProviderContext) {
           }
         });
 
+        it('should handle create alert request appropriately when alert name has leading and trailing whitespaces', async () => {
+          const response = await supertestWithoutAuth
+            .post(`${getUrlPrefix(space.id)}/api/alerts/alert`)
+            .set('kbn-xsrf', 'foo')
+            .auth(user.username, user.password)
+            .send(
+              getTestAlertData({
+                name: ' leading and trailing whitespace ',
+              })
+            );
+
+          switch (scenario.id) {
+            case 'no_kibana_privileges at space1':
+            case 'global_read at space1':
+            case 'space_1_all at space2':
+              expect(response.statusCode).to.eql(404);
+              expect(response.body).to.eql({
+                statusCode: 404,
+                error: 'Not Found',
+                message: 'Not Found',
+              });
+              break;
+            case 'superuser at space1':
+            case 'space_1_all at space1':
+              expect(response.statusCode).to.eql(200);
+              expect(response.body.name).to.eql(' leading and trailing whitespace ');
+              objectRemover.add(space.id, response.body.id, 'alert', 'alerts');
+              break;
+            default:
+              throw new Error(`Scenario untested: ${JSON.stringify(scenario)}`);
+          }
+        });
+
         it('should handle create alert request appropriately when alert type is unregistered', async () => {
           const response = await supertestWithoutAuth
             .post(`${getUrlPrefix(space.id)}/api/alerts/alert`)
diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/update.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/update.ts
@@ -191,6 +191,51 @@ export default function createUpdateTests({ getService }: FtrProviderContext) {
           }
         });
 
+        it('should handle update alert request appropriately when alert name has leading and trailing whitespaces', async () => {
+          const { body: createdAlert } = await supertest
+            .post(`${getUrlPrefix(space.id)}/api/alerts/alert`)
+            .set('kbn-xsrf', 'foo')
+            .send(getTestAlertData())
+            .expect(200);
+          objectRemover.add(space.id, createdAlert.id, 'alert', 'alerts');
+
+          const updatedData = {
+            name: ' leading and trailing whitespace ',
+            tags: ['bar'],
+            params: {
+              foo: true,
+            },
+            schedule: { interval: '12s' },
+            actions: [],
+            throttle: '1m',
+          };
+          const response = await supertestWithoutAuth
+            .put(`${getUrlPrefix(space.id)}/api/alerts/alert/${createdAlert.id}`)
+            .set('kbn-xsrf', 'foo')
+            .auth(user.username, user.password)
+            .send(updatedData);
+
+          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',
+              });
+              break;
+            case 'superuser at space1':
+            case 'space_1_all at space1':
+              expect(response.statusCode).to.eql(200);
+              expect(response.body.name).to.eql(' leading and trailing whitespace ');
+              break;
+            default:
+              throw new Error(`Scenario untested: ${JSON.stringify(scenario)}`);
+          }
+        });
+
         it(`shouldn't update alert from another space`, async () => {
           const { body: createdAlert } = await supertest
             .post(`${getUrlPrefix(space.id)}/api/alerts/alert`)

Original file line number	Diff line number	Diff line change
`@@ -5,7 +5,7 @@`
`5`	`5`	`*/`
`6`	`6`
`7`	`7`	`import Boom from 'boom';`
`8`		`-import { omit, isEqual, map, truncate } from 'lodash';`
	`8`	`+import { omit, isEqual, map, truncate, trim } from 'lodash';`
`9`	`9`	`import { i18n } from '@kbn/i18n';`
`10`	`10`	`import {`
`11`	`11`	`Logger,`
`@@ -719,6 +719,6 @@ export class AlertsClient {`
`719`	`719`	`}`
`720`	`720`
`721`	`721`	`private generateAPIKeyName(alertTypeId: string, alertName: string) {`
`722`		- return truncate(`Alerting: ${alertTypeId}/${alertName}`, { length: 256 });
	`722`	+ return truncate(`Alerting: ${alertTypeId}/${trim(alertName)}`, { length: 256 });
`723`	`723`	`}`
`724`	`724`	`}`