Fix update alert API to still work when AAD is out of sync (#57039)

* Ensure update API still works when AAD is broken

* Add API integration test

* Fix ESLint errors

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

x-pack/legacy/plugins/alerting/server/alerts_client.test.ts

@@ -1857,180 +1857,39 @@ describe('delete()', () => {
 });
 
 describe('update()', () => {
-  test('updates given parameters', async () => {
-    const alertsClient = new AlertsClient(alertsClientParams);
-    alertTypeRegistry.get.mockReturnValueOnce({
+  let alertsClient: AlertsClient;
+  const existingAlert = {
+    id: '1',
+    type: 'alert',
+    attributes: {
+      enabled: true,
+      alertTypeId: '123',
+      scheduledTaskId: 'task-123',
+    },
+    references: [],
+    version: '123',
+  };
+  const existingDecryptedAlert = {
+    ...existingAlert,
+    attributes: {
+      ...existingAlert.attributes,
+      apiKey: Buffer.from('123:abc').toString('base64'),
+    },
+  };
+
+  beforeEach(() => {
+    alertsClient = new AlertsClient(alertsClientParams);
+    savedObjectsClient.get.mockResolvedValue(existingAlert);
+    encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue(existingDecryptedAlert);
+    alertTypeRegistry.get.mockReturnValue({
       id: '123',
       name: 'Test',
       actionGroups: ['default'],
       async executor() {},
     });
-    encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({
-      id: '1',
-      type: 'alert',
-      attributes: {
-        enabled: true,
-        alertTypeId: '123',
-        scheduledTaskId: 'task-123',
-      },
-      references: [],
-      version: '123',
-    });
-    savedObjectsClient.bulkGet.mockResolvedValueOnce({
-      saved_objects: [
-        {
-          id: '1',
-          type: 'action',
-          attributes: {
-            actionTypeId: 'test',
-          },
-          references: [],
-        },
-      ],
-    });
-    savedObjectsClient.update.mockResolvedValueOnce({
-      id: '1',
-      type: 'alert',
-      attributes: {
-        enabled: true,
-        schedule: { interval: '10s' },
-        params: {
-          bar: true,
-        },
-        actions: [
-          {
-            group: 'default',
-            actionRef: 'action_0',
-            actionTypeId: 'test',
-            params: {
-              foo: true,
-            },
-          },
-        ],
-        scheduledTaskId: 'task-123',
-        createdAt: new Date().toISOString(),
-      },
-      updated_at: new Date().toISOString(),
-      references: [
-        {
-          name: 'action_0',
-          type: 'action',
-          id: '1',
-        },
-      ],
-    });
-    const result = await alertsClient.update({
-      id: '1',
-      data: {
-        schedule: { interval: '10s' },
-        name: 'abc',
-        tags: ['foo'],
-        params: {
-          bar: true,
-        },
-        actions: [
-          {
-            group: 'default',
-            id: '1',
-            params: {
-              foo: true,
-            },
-          },
-        ],
-      },
-    });
-    expect(result).toMatchInlineSnapshot(`
-      Object {
-        "actions": Array [
-          Object {
-            "actionTypeId": "test",
-            "group": "default",
-            "id": "1",
-            "params": Object {
-              "foo": true,
-            },
-          },
-        ],
-        "createdAt": 2019-02-12T21:01:22.479Z,
-        "enabled": true,
-        "id": "1",
-        "params": Object {
-          "bar": true,
-        },
-        "schedule": Object {
-          "interval": "10s",
-        },
-        "scheduledTaskId": "task-123",
-        "updatedAt": 2019-02-12T21:01:22.479Z,
-      }
-    `);
-    expect(savedObjectsClient.update).toHaveBeenCalledTimes(1);
-    expect(savedObjectsClient.update.mock.calls[0]).toHaveLength(4);
-    expect(savedObjectsClient.update.mock.calls[0][0]).toEqual('alert');
-    expect(savedObjectsClient.update.mock.calls[0][1]).toEqual('1');
-    expect(savedObjectsClient.update.mock.calls[0][2]).toMatchInlineSnapshot(`
-      Object {
-        "actions": Array [
-          Object {
-            "actionRef": "action_0",
-            "actionTypeId": "test",
-            "group": "default",
-            "params": Object {
-              "foo": true,
-            },
-          },
-        ],
-        "alertTypeId": "123",
-        "apiKey": null,
-        "apiKeyOwner": null,
-        "enabled": true,
-        "name": "abc",
-        "params": Object {
-          "bar": true,
-        },
-        "schedule": Object {
-          "interval": "10s",
-        },
-        "scheduledTaskId": "task-123",
-        "tags": Array [
-          "foo",
-        ],
-        "updatedBy": "elastic",
-      }
-    `);
-    expect(savedObjectsClient.update.mock.calls[0][3]).toMatchInlineSnapshot(`
-                                                                                                                  Object {
-                                                                                                                    "references": Array [
-                                                                                                                      Object {
-                                                                                                                        "id": "1",
-                                                                                                                        "name": "action_0",
-                                                                                                                        "type": "action",
-                                                                                                                      },
-                                                                                                                    ],
-                                                                                                                    "version": "123",
-                                                                                                                  }
-                                                                            `);
   });
 
-  it('updates with multiple actions', async () => {
-    const alertsClient = new AlertsClient(alertsClientParams);
-    alertTypeRegistry.get.mockReturnValueOnce({
-      id: '123',
-      name: 'Test',
-      actionGroups: ['default'],
-      async executor() {},
-    });
-    encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({
-      id: '1',
-      type: 'alert',
-      attributes: {
-        enabled: true,
-        alertTypeId: '123',
-        scheduledTaskId: 'task-123',
-      },
-      references: [],
-      version: '123',
-    });
+  test('updates given parameters', async () => {
     savedObjectsClient.bulkGet.mockResolvedValueOnce({
       saved_objects: [
         {
@@ -2060,7 +1919,6 @@ describe('update()', () => {
         params: {
           bar: true,
         },
-        createdAt: new Date().toISOString(),
         actions: [
           {
             group: 'default',
@@ -2088,6 +1946,7 @@ describe('update()', () => {
           },
         ],
         scheduledTaskId: 'task-123',
+        createdAt: new Date().toISOString(),
       },
       updated_at: new Date().toISOString(),
       references: [
@@ -2183,37 +2042,85 @@ describe('update()', () => {
         "updatedAt": 2019-02-12T21:01:22.479Z,
       }
     `);
-    expect(savedObjectsClient.bulkGet).toHaveBeenCalledWith([
-      {
-        id: '1',
-        type: 'action',
-      },
-      {
-        id: '2',
-        type: 'action',
-      },
-    ]);
+    expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', {
+      namespace: 'default',
+    });
+    expect(savedObjectsClient.get).not.toHaveBeenCalled();
+    expect(savedObjectsClient.update).toHaveBeenCalledTimes(1);
+    expect(savedObjectsClient.update.mock.calls[0]).toHaveLength(4);
+    expect(savedObjectsClient.update.mock.calls[0][0]).toEqual('alert');
+    expect(savedObjectsClient.update.mock.calls[0][1]).toEqual('1');
+    expect(savedObjectsClient.update.mock.calls[0][2]).toMatchInlineSnapshot(`
+      Object {
+        "actions": Array [
+          Object {
+            "actionRef": "action_0",
+            "actionTypeId": "test",
+            "group": "default",
+            "params": Object {
+              "foo": true,
+            },
+          },
+          Object {
+            "actionRef": "action_1",
+            "actionTypeId": "test",
+            "group": "default",
+            "params": Object {
+              "foo": true,
+            },
+          },
+          Object {
+            "actionRef": "action_2",
+            "actionTypeId": "test2",
+            "group": "default",
+            "params": Object {
+              "foo": true,
+            },
+          },
+        ],
+        "alertTypeId": "123",
+        "apiKey": null,
+        "apiKeyOwner": null,
+        "enabled": true,
+        "name": "abc",
+        "params": Object {
+          "bar": true,
+        },
+        "schedule": Object {
+          "interval": "10s",
+        },
+        "scheduledTaskId": "task-123",
+        "tags": Array [
+          "foo",
+        ],
+        "updatedBy": "elastic",
+      }
+    `);
+    expect(savedObjectsClient.update.mock.calls[0][3]).toMatchInlineSnapshot(`
+      Object {
+        "references": Array [
+          Object {
+            "id": "1",
+            "name": "action_0",
+            "type": "action",
+          },
+          Object {
+            "id": "1",
+            "name": "action_1",
+            "type": "action",
+          },
+          Object {
+            "id": "2",
+            "name": "action_2",
+            "type": "action",
+          },
+        ],
+        "version": "123",
+      }
+    `);
   });
 
   it('calls the createApiKey function', async () => {
-    const alertsClient = new AlertsClient(alertsClientParams);
-    alertTypeRegistry.get.mockReturnValueOnce({
-      id: '123',
-      name: 'Test',
-      actionGroups: ['default'],
-      async executor() {},
-    });
-    encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({
-      id: '1',
-      type: 'alert',
-      attributes: {
-        enabled: true,
-        alertTypeId: '123',
-        scheduledTaskId: 'task-123',
-      },
-      references: [],
-      version: '123',
-    });
     savedObjectsClient.bulkGet.mockResolvedValueOnce({
       saved_objects: [
         {
@@ -2357,7 +2264,6 @@ describe('update()', () => {
   });
 
   it('should validate params', async () => {
-    const alertsClient = new AlertsClient(alertsClientParams);
     alertTypeRegistry.get.mockReturnValueOnce({
       id: '123',
       name: 'Test',
@@ -2369,14 +2275,6 @@ describe('update()', () => {
       },
       async executor() {},
     });
-    encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({
-      id: '1',
-      type: 'alert',
-      attributes: {
-        alertTypeId: '123',
-      },
-      references: [],
-    });
     await expect(
       alertsClient.update({
         id: '1',
@@ -2404,26 +2302,7 @@ describe('update()', () => {
   });
 
   it('swallows error when invalidate API key throws', async () => {
-    const alertsClient = new AlertsClient(alertsClientParams);
     alertsClientParams.invalidateAPIKey.mockRejectedValueOnce(new Error('Fail'));
-    alertTypeRegistry.get.mockReturnValueOnce({
-      id: '123',
-      name: 'Test',
-      actionGroups: ['default'],
-      async executor() {},
-    });
-    encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({
-      id: '1',
-      type: 'alert',
-      attributes: {
-        enabled: true,
-        alertTypeId: '123',
-        scheduledTaskId: 'task-123',
-        apiKey: Buffer.from('123:abc').toString('base64'),
-      },
-      references: [],
-      version: '123',
-    });
     savedObjectsClient.bulkGet.mockResolvedValueOnce({
       saved_objects: [
         {
@@ -2490,6 +2369,125 @@ describe('update()', () => {
     );
   });
 
+  it('swallows error when getDecryptedAsInternalUser throws', async () => {
+    encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValue(new Error('Fail'));
+    savedObjectsClient.bulkGet.mockResolvedValueOnce({
+      saved_objects: [
+        {
+          id: '1',
+          type: 'action',
+          attributes: {
+            actionTypeId: 'test',
+          },
+          references: [],
+        },
+        {
+          id: '2',
+          type: 'action',
+          attributes: {
+            actionTypeId: 'test2',
+          },
+          references: [],
+        },
+      ],
+    });
+    savedObjectsClient.update.mockResolvedValueOnce({
+      id: '1',
+      type: 'alert',
+      attributes: {
+        enabled: true,
+        schedule: { interval: '10s' },
+        params: {
+          bar: true,
+        },
+        actions: [
+          {
+            group: 'default',
+            actionRef: 'action_0',
+            actionTypeId: 'test',
+            params: {
+              foo: true,
+            },
+          },
+          {
+            group: 'default',
+            actionRef: 'action_1',
+            actionTypeId: 'test',
+            params: {
+              foo: true,
+            },
+          },
+          {
+            group: 'default',
+            actionRef: 'action_2',
+            actionTypeId: 'test2',
+            params: {
+              foo: true,
+            },
+          },
+        ],
+        scheduledTaskId: 'task-123',
+        createdAt: new Date().toISOString(),
+      },
+      updated_at: new Date().toISOString(),
+      references: [
+        {
+          name: 'action_0',
+          type: 'action',
+          id: '1',
+        },
+        {
+          name: 'action_1',
+          type: 'action',
+          id: '1',
+        },
+        {
+          name: 'action_2',
+          type: 'action',
+          id: '2',
+        },
+      ],
+    });
+    await alertsClient.update({
+      id: '1',
+      data: {
+        schedule: { interval: '10s' },
+        name: 'abc',
+        tags: ['foo'],
+        params: {
+          bar: true,
+        },
+        actions: [
+          {
+            group: 'default',
+            id: '1',
+            params: {
+              foo: true,
+            },
+          },
+          {
+            group: 'default',
+            id: '1',
+            params: {
+              foo: true,
+            },
+          },
+          {
+            group: 'default',
+            id: '2',
+            params: {
+              foo: true,
+            },
+          },
+        ],
+      },
+    });
+    expect(savedObjectsClient.get).toHaveBeenCalledWith('alert', '1');
+    expect(alertsClientParams.logger.error).toHaveBeenCalledWith(
+      'update(): Failed to load API key to invalidate on alert 1: Fail'
+    );
+  });
+
   describe('updating an alert schedule', () => {
     function mockApiCalls(
       alertId: string,
@@ -2575,7 +2573,6 @@ describe('update()', () => {
     test('updating the alert schedule should rerun the task immediately', async () => {
       const alertId = uuid.v4();
       const taskId = uuid.v4();
-      const alertsClient = new AlertsClient(alertsClientParams);
 
       mockApiCalls(alertId, taskId, { interval: '60m' }, { interval: '10s' });
 
@@ -2606,7 +2603,6 @@ describe('update()', () => {
     test('updating the alert without changing the schedule should not rerun the task', async () => {
       const alertId = uuid.v4();
       const taskId = uuid.v4();
-      const alertsClient = new AlertsClient(alertsClientParams);
 
       mockApiCalls(alertId, taskId, { interval: '10s' }, { interval: '10s' });
 
@@ -2637,7 +2633,6 @@ describe('update()', () => {
     test('updating the alert should not wait for the rerun the task to complete', async done => {
       const alertId = uuid.v4();
       const taskId = uuid.v4();
-      const alertsClient = new AlertsClient(alertsClientParams);
 
       mockApiCalls(alertId, taskId, { interval: '10s' }, { interval: '30s' });
 
@@ -2676,7 +2671,6 @@ describe('update()', () => {
     test('logs when the rerun of an alerts underlying task fails', async () => {
       const alertId = uuid.v4();
       const taskId = uuid.v4();
-      const alertsClient = new AlertsClient(alertsClientParams);
 
       mockApiCalls(alertId, taskId, { interval: '10s' }, { interval: '30s' });
 

x-pack/legacy/plugins/alerting/server/alerts_client.ts

@@ -269,22 +269,41 @@ export class AlertsClient {
   }
 
   public async update({ id, data }: UpdateOptions): Promise<PartialAlert> {
-    const decryptedAlertSavedObject = await this.encryptedSavedObjectsPlugin.getDecryptedAsInternalUser<
-      RawAlert
-    >('alert', id, { namespace: this.namespace });
-    const updateResult = await this.updateAlert({ id, data }, decryptedAlertSavedObject);
+    let alertSavedObject: SavedObject<RawAlert>;
 
-    if (
-      updateResult.scheduledTaskId &&
-      !isEqual(decryptedAlertSavedObject.attributes.schedule, updateResult.schedule)
-    ) {
-      this.taskManager.runNow(updateResult.scheduledTaskId).catch((err: Error) => {
-        this.logger.error(
-          `Alert update failed to run its underlying task. TaskManager runNow failed with Error: ${err.message}`
-        );
-      });
+    try {
+      alertSavedObject = await this.encryptedSavedObjectsPlugin.getDecryptedAsInternalUser<
+        RawAlert
+      >('alert', id, { namespace: this.namespace });
+    } catch (e) {
+      // We'll skip invalidating the API key since we failed to load the decrypted saved object
+      this.logger.error(
+        `update(): Failed to load API key to invalidate on alert ${id}: ${e.message}`
+      );
+      // Still attempt to load the object using SOC
+      alertSavedObject = await this.savedObjectsClient.get<RawAlert>('alert', id);
     }
 
+    const updateResult = await this.updateAlert({ id, data }, alertSavedObject);
+
+    await Promise.all([
+      alertSavedObject.attributes.apiKey
+        ? this.invalidateApiKey({ apiKey: alertSavedObject.attributes.apiKey })
+        : null,
+      (async () => {
+        if (
+          updateResult.scheduledTaskId &&
+          !isEqual(alertSavedObject.attributes.schedule, updateResult.schedule)
+        ) {
+          this.taskManager.runNow(updateResult.scheduledTaskId).catch((err: Error) => {
+            this.logger.error(
+              `Alert update failed to run its underlying task. TaskManager runNow failed with Error: ${err.message}`
+            );
+          });
+        }
+      })(),
+    ]);
+
     return updateResult;
   }
 
@@ -319,8 +338,6 @@ export class AlertsClient {
       }
     );
 
-    await this.invalidateApiKey({ apiKey: attributes.apiKey });
-
     return this.getPartialAlertFromRaw(
       id,
       updatedObject.attributes,

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

@@ -108,6 +108,87 @@ export default function createUpdateTests({ getService }: FtrProviderContext) {
           }
         });
 
+        it('should still be able to update when AAD is broken', async () => {
+          const { body: createdAlert } = await supertest
+            .post(`${getUrlPrefix(space.id)}/api/alert`)
+            .set('kbn-xsrf', 'foo')
+            .send(getTestAlertData())
+            .expect(200);
+          objectRemover.add(space.id, createdAlert.id, 'alert');
+
+          await supertest
+            .put(`${getUrlPrefix(space.id)}/api/saved_objects/alert/${createdAlert.id}`)
+            .set('kbn-xsrf', 'foo')
+            .send({
+              attributes: {
+                name: 'bar',
+              },
+            })
+            .expect(200);
+
+          const updatedData = {
+            name: 'bcd',
+            tags: ['bar'],
+            params: {
+              foo: true,
+            },
+            schedule: { interval: '12s' },
+            actions: [],
+            throttle: '2m',
+          };
+          const response = await supertestWithoutAuth
+            .put(`${getUrlPrefix(space.id)}/api/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).to.eql({
+                ...updatedData,
+                id: createdAlert.id,
+                alertTypeId: 'test.noop',
+                consumer: 'bar',
+                createdBy: 'elastic',
+                enabled: true,
+                updatedBy: user.username,
+                apiKeyOwner: user.username,
+                muteAll: false,
+                mutedInstanceIds: [],
+                scheduledTaskId: createdAlert.scheduledTaskId,
+                createdAt: response.body.createdAt,
+                updatedAt: response.body.updatedAt,
+              });
+              expect(Date.parse(response.body.createdAt)).to.be.greaterThan(0);
+              expect(Date.parse(response.body.updatedAt)).to.be.greaterThan(0);
+              expect(Date.parse(response.body.updatedAt)).to.be.greaterThan(
+                Date.parse(response.body.createdAt)
+              );
+              // Ensure AAD isn't broken
+              await checkAAD({
+                supertest,
+                spaceId: space.id,
+                type: 'alert',
+                id: createdAlert.id,
+              });
+              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/alert`)