Retain APIKey when disabling/enabling a rule

docs/user/alerting/alerting-setup.asciidoc

@@ -65,10 +65,9 @@ Rules and connectors are isolated to the {kib} space in which they were created.
 Rules are authorized using an <<api-keys,API key>> associated with the last user to edit the rule. This API key captures a snapshot of the user's privileges at the time of edit and is subsequently used to run all background tasks associated with the rule, including condition checks like {es} queries and triggered actions. The following rule actions will re-generate the API key:
 
 * Creating a rule
-* Enabling a disabled rule
 * Updating a rule
 
 [IMPORTANT]
 ==============================================
-If a rule requires certain privileges, such as index privileges, to run, and a user without those privileges updates, disables, or re-enables the rule, the rule will no longer function. Conversely, if a user with greater or administrator privileges modifies the rule, it will begin running with increased privileges.
+If a rule requires certain privileges, such as index privileges, to run, and a user without those privileges updates the rule, the rule will no longer function. Conversely, if a user with greater or administrator privileges modifies the rule, it will begin running with increased privileges.
 ==============================================

x-pack/plugins/alerting/server/rules_client/rules_client.ts

@@ -1828,7 +1828,7 @@ export class RulesClient {
   }
 
   private async enableWithOCC({ id }: { id: string }) {
-    let apiKeyToInvalidate: string | null = null;
+    let existingApiKey: string | null = null;
     let attributes: RawRule;
     let version: string | undefined;
 
@@ -1837,14 +1837,11 @@ export class RulesClient {
         await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser<RawRule>('alert', id, {
           namespace: this.namespace,
         });
-      apiKeyToInvalidate = decryptedAlert.attributes.apiKey;
+      existingApiKey = decryptedAlert.attributes.apiKey;
       attributes = decryptedAlert.attributes;
       version = decryptedAlert.version;
     } catch (e) {
-      // We'll skip invalidating the API key since we failed to load the decrypted saved object
-      this.logger.error(
-        `enable(): Failed to load API key to invalidate on alert ${id}: ${e.message}`
-      );
+      this.logger.error(`enable(): Failed to load API key of alert ${id}: ${e.message}`);
       // Still attempt to load the attributes and version using SOC
       const alert = await this.unsecuredSavedObjectsClient.get<RawRule>('alert', id);
       attributes = alert.attributes;
@@ -1886,19 +1883,10 @@ export class RulesClient {
     if (attributes.enabled === false) {
       const username = await this.getUserName();
 
-      let createdAPIKey = null;
-      try {
-        createdAPIKey = await this.createAPIKey(
-          this.generateAPIKeyName(attributes.alertTypeId, attributes.name)
-        );
-      } catch (error) {
-        throw Boom.badRequest(`Error enabling rule: could not create API key - ${error.message}`);
-      }
-
       const updateAttributes = this.updateMeta({
         ...attributes,
+        ...(!existingApiKey && (await this.createNewAPIKeySet({ attributes, username }))),
         enabled: true,
-        ...this.apiKeyAsAlertAttributes(createdAPIKey, username),
         updatedBy: username,
         updatedAt: new Date().toISOString(),
         executionStatus: {
@@ -1909,15 +1897,10 @@ export class RulesClient {
           warning: null,
         },
       });
+
       try {
         await this.unsecuredSavedObjectsClient.update('alert', id, updateAttributes, { version });
       } catch (e) {
-        // Avoid unused API key
-        await bulkMarkApiKeysForInvalidation(
-          { apiKeys: updateAttributes.apiKey ? [updateAttributes.apiKey] : [] },
-          this.logger,
-          this.unsecuredSavedObjectsClient
-        );
         throw e;
       }
       const scheduledTask = await this.scheduleRule({
@@ -1930,16 +1913,28 @@ export class RulesClient {
       await this.unsecuredSavedObjectsClient.update('alert', id, {
         scheduledTaskId: scheduledTask.id,
       });
-      if (apiKeyToInvalidate) {
-        await bulkMarkApiKeysForInvalidation(
-          { apiKeys: [apiKeyToInvalidate] },
-          this.logger,
-          this.unsecuredSavedObjectsClient
-        );
-      }
     }
   }
 
+  private async createNewAPIKeySet({
+    attributes,
+    username,
+  }: {
+    attributes: RawRule;
+    username: string | null;
+  }): Promise<Pick<RawRule, 'apiKey' | 'apiKeyOwner'>> {
+    let createdAPIKey = null;
+    try {
+      createdAPIKey = await this.createAPIKey(
+        this.generateAPIKeyName(attributes.alertTypeId, attributes.name)
+      );
+    } catch (error) {
+      throw Boom.badRequest(`Error creating API key for rule: ${error.message}`);
+    }
+
+    return this.apiKeyAsAlertAttributes(createdAPIKey, username);
+  }
+
   public async disable({ id }: { id: string }): Promise<void> {
     return await retryIfConflicts(
       this.logger,
@@ -1949,7 +1944,6 @@ export class RulesClient {
   }
 
   private async disableWithOCC({ id }: { id: string }) {
-    let apiKeyToInvalidate: string | null = null;
     let attributes: RawRule;
     let version: string | undefined;
 
@@ -1958,14 +1952,10 @@ export class RulesClient {
         await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser<RawRule>('alert', id, {
           namespace: this.namespace,
         });
-      apiKeyToInvalidate = decryptedAlert.attributes.apiKey;
       attributes = decryptedAlert.attributes;
       version = decryptedAlert.version;
     } catch (e) {
-      // We'll skip invalidating the API key since we failed to load the decrypted saved object
-      this.logger.error(
-        `disable(): Failed to load API key to invalidate on alert ${id}: ${e.message}`
-      );
+      this.logger.error(`disable(): Failed to load API key of alert ${id}: ${e.message}`);
       // Still attempt to load the attributes and version using SOC
       const alert = await this.unsecuredSavedObjectsClient.get<RawRule>('alert', id);
       attributes = alert.attributes;
@@ -2058,26 +2048,14 @@ export class RulesClient {
           ...attributes,
           enabled: false,
           scheduledTaskId: null,
-          apiKey: null,
-          apiKeyOwner: null,
           updatedBy: await this.getUserName(),
           updatedAt: new Date().toISOString(),
         }),
         { version }
       );
-
-      await Promise.all([
-        attributes.scheduledTaskId
-          ? this.taskManager.removeIfExists(attributes.scheduledTaskId)
-          : null,
-        apiKeyToInvalidate
-          ? await bulkMarkApiKeysForInvalidation(
-              { apiKeys: [apiKeyToInvalidate] },
-              this.logger,
-              this.unsecuredSavedObjectsClient
-            )
-          : null,
-      ]);
+      if (attributes.scheduledTaskId) {
+        await this.taskManager.removeIfExists(attributes.scheduledTaskId);
+      }
     }
   }
 

x-pack/plugins/alerting/server/rules_client/tests/disable.test.ts

@@ -18,7 +18,6 @@ import { auditLoggerMock } from '@kbn/security-plugin/server/audit/mocks';
 import { getBeforeSetup, setGlobalDate } from './lib';
 import { eventLoggerMock } from '@kbn/event-log-plugin/server/event_logger.mock';
 import { TaskStatus } from '@kbn/task-manager-plugin/server';
-import { bulkMarkApiKeysForInvalidation } from '../../invalidate_pending_api_keys/bulk_mark_api_keys_for_invalidation';
 
 jest.mock('../../invalidate_pending_api_keys/bulk_mark_api_keys_for_invalidation', () => ({
   bulkMarkApiKeysForInvalidation: jest.fn(),
@@ -111,6 +110,7 @@ describe('disable()', () => {
     attributes: {
       ...existingAlert.attributes,
       apiKey: Buffer.from('123:abc').toString('base64'),
+      apiKeyOwner: 'elastic',
     },
     version: '123',
     references: [],
@@ -206,11 +206,11 @@ describe('disable()', () => {
         alertTypeId: 'myType',
         enabled: false,
         meta: {
-          versionApiKeyLastmodified: kibanaVersion,
+          versionApiKeyLastmodified: 'v7.10.0',
         },
         scheduledTaskId: null,
-        apiKey: null,
-        apiKeyOwner: null,
+        apiKey: 'MTIzOmFiYw==',
+        apiKeyOwner: 'elastic',
         updatedAt: '2019-02-12T21:01:22.479Z',
         updatedBy: 'elastic',
         actions: [
@@ -230,12 +230,6 @@ describe('disable()', () => {
       }
     );
     expect(taskManager.removeIfExists).toHaveBeenCalledWith('task-123');
-    expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledTimes(1);
-    expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledWith(
-      { apiKeys: ['MTIzOmFiYw=='] },
-      expect.any(Object),
-      expect.any(Object)
-    );
   });
 
   test('disables the rule with calling event log to "recover" the alert instances from the task state', async () => {
@@ -282,11 +276,11 @@ describe('disable()', () => {
         alertTypeId: 'myType',
         enabled: false,
         meta: {
-          versionApiKeyLastmodified: kibanaVersion,
+          versionApiKeyLastmodified: 'v7.10.0',
         },
         scheduledTaskId: null,
-        apiKey: null,
-        apiKeyOwner: null,
+        apiKey: 'MTIzOmFiYw==',
+        apiKeyOwner: 'elastic',
         updatedAt: '2019-02-12T21:01:22.479Z',
         updatedBy: 'elastic',
         actions: [
@@ -306,12 +300,6 @@ describe('disable()', () => {
       }
     );
     expect(taskManager.removeIfExists).toHaveBeenCalledWith('task-123');
-    expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledTimes(1);
-    expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledWith(
-      { apiKeys: ['MTIzOmFiYw=='] },
-      expect.any(Object),
-      expect.any(Object)
-    );
 
     expect(eventLogger.logEvent).toHaveBeenCalledTimes(1);
     expect(eventLogger.logEvent.mock.calls[0][0]).toStrictEqual({
@@ -369,11 +357,11 @@ describe('disable()', () => {
         alertTypeId: 'myType',
         enabled: false,
         meta: {
-          versionApiKeyLastmodified: kibanaVersion,
+          versionApiKeyLastmodified: 'v7.10.0',
         },
         scheduledTaskId: null,
-        apiKey: null,
-        apiKeyOwner: null,
+        apiKey: 'MTIzOmFiYw==',
+        apiKeyOwner: 'elastic',
         updatedAt: '2019-02-12T21:01:22.479Z',
         updatedBy: 'elastic',
         actions: [
@@ -393,12 +381,6 @@ describe('disable()', () => {
       }
     );
     expect(taskManager.removeIfExists).toHaveBeenCalledWith('task-123');
-    expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledTimes(1);
-    expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledWith(
-      { apiKeys: ['MTIzOmFiYw=='] },
-      expect.any(Object),
-      expect.any(Object)
-    );
 
     expect(eventLogger.logEvent).toHaveBeenCalledTimes(0);
     expect(rulesClientParams.logger.warn).toHaveBeenCalledWith(
@@ -408,7 +390,6 @@ describe('disable()', () => {
 
   test('falls back when getDecryptedAsInternalUser throws an error', async () => {
     encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValueOnce(new Error('Fail'));
-
     await rulesClient.disable({ id: '1' });
     expect(unsecuredSavedObjectsClient.get).toHaveBeenCalledWith('alert', '1');
     expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', {
@@ -422,12 +403,7 @@ describe('disable()', () => {
         schedule: { interval: '10s' },
         alertTypeId: 'myType',
         enabled: false,
-        meta: {
-          versionApiKeyLastmodified: kibanaVersion,
-        },
         scheduledTaskId: null,
-        apiKey: null,
-        apiKeyOwner: null,
         updatedAt: '2019-02-12T21:01:22.479Z',
         updatedBy: 'elastic',
         actions: [
@@ -447,7 +423,6 @@ describe('disable()', () => {
       }
     );
     expect(taskManager.removeIfExists).toHaveBeenCalledWith('task-123');
-    expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
   });
 
   test(`doesn't disable already disabled alerts`, async () => {
@@ -463,14 +438,6 @@ describe('disable()', () => {
     await rulesClient.disable({ id: '1' });
     expect(unsecuredSavedObjectsClient.update).not.toHaveBeenCalled();
     expect(taskManager.removeIfExists).not.toHaveBeenCalled();
-    expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
-  });
-
-  test(`doesn't invalidate when no API key is used`, async () => {
-    encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce(existingAlert);
-
-    await rulesClient.disable({ id: '1' });
-    expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
   });
 
   test('swallows error when failing to load decrypted saved object', async () => {
@@ -479,9 +446,8 @@ describe('disable()', () => {
     await rulesClient.disable({ id: '1' });
     expect(unsecuredSavedObjectsClient.update).toHaveBeenCalled();
     expect(taskManager.removeIfExists).toHaveBeenCalled();
-    expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
     expect(rulesClientParams.logger.error).toHaveBeenCalledWith(
-      'disable(): Failed to load API key to invalidate on alert 1: Fail'
+      'disable(): Failed to load API key of alert 1: Fail'
     );
   });
 
@@ -493,17 +459,6 @@ describe('disable()', () => {
     );
   });
 
-  test('swallows error when invalidate API key throws', async () => {
-    unsecuredSavedObjectsClient.create.mockRejectedValueOnce(new Error('Fail'));
-    await rulesClient.disable({ id: '1' });
-    expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledTimes(1);
-    expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledWith(
-      { apiKeys: ['MTIzOmFiYw=='] },
-      expect.any(Object),
-      expect.any(Object)
-    );
-  });
-
   test('throws when failing to remove task from task manager', async () => {
     taskManager.removeIfExists.mockRejectedValueOnce(new Error('Failed to remove task'));