Used SO for saving the API key IDs that should be deleted (elastic#82211

) * Used SO for saving the API key IDs that should be deleted and create a configuration option where can set an execution interval for a TM task which will get the data from this SO and remove marked for delete keys. * removed invalidateApiKey from AlertsClient * Fixed type checks * Fixed jest tests * Removed test code * Changed SO name * fixed type cheks * Moved invalidate logic out of alerts client * fixed type check * Added functional tests * Fixed due to comments * added configurable delay for invalidation task * added interval to the task response * Fixed jest tests * Fixed due to comments * Fixed task * fixed paging * Fixed date filter * Fixed jest tests * fixed due to comments * fixed due to comments * Fixed e2e test * Fixed e2e test * Fixed due to comments. Changed api key invalidation task to use SavedObjectClient * Use encryptedSavedObjectClient * set back flaky test comment
YulNaumenko · Nov 17, 2020 · 24e48a9 · 24e48a9
1 parent 3a29b3b
commit 24e48a9
Show file tree

Hide file tree

Showing 36 changed files with 847 additions and 126 deletions.
diff --git a/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts b/x-pack/plugins/alerts/server/alerts_client/alerts_client.ts
@@ -31,7 +31,6 @@ import {
 } from '../types';
 import { validateAlertTypeParams, alertExecutionStatusFromRaw } from '../lib';
 import {
-  InvalidateAPIKeyParams,
   GrantAPIKeyResult as SecurityPluginGrantAPIKeyResult,
   InvalidateAPIKeyResult as SecurityPluginInvalidateAPIKeyResult,
 } from '../../../security/server';
@@ -48,6 +47,7 @@ import { IEvent } from '../../../event_log/server';
 import { parseDuration } from '../../common/parse_duration';
 import { retryIfConflicts } from '../lib/retry_if_conflicts';
 import { partiallyUpdateAlert } from '../saved_objects';
+import { markApiKeyForInvalidation } from '../invalidate_pending_api_keys/mark_api_key_for_invalidation';
 
 export interface RegistryAlertTypeWithAuth extends RegistryAlertType {
   authorizedConsumers: string[];
@@ -72,7 +72,6 @@ export interface ConstructorOptions {
   namespace?: string;
   getUserName: () => Promise<string | null>;
   createAPIKey: (name: string) => Promise<CreateAPIKeyResult>;
-  invalidateAPIKey: (params: InvalidateAPIKeyParams) => Promise<InvalidateAPIKeyResult>;
   getActionsClient: () => Promise<ActionsClient>;
   getEventLogClient: () => Promise<IEventLogClient>;
   kibanaVersion: PluginInitializerContext['env']['packageInfo']['version'];
@@ -172,9 +171,6 @@ export class AlertsClient {
   private readonly authorization: AlertsAuthorization;
   private readonly alertTypeRegistry: AlertTypeRegistry;
   private readonly createAPIKey: (name: string) => Promise<CreateAPIKeyResult>;
-  private readonly invalidateAPIKey: (
-    params: InvalidateAPIKeyParams
-  ) => Promise<InvalidateAPIKeyResult>;
   private readonly getActionsClient: () => Promise<ActionsClient>;
   private readonly actionsAuthorization: ActionsAuthorization;
   private readonly getEventLogClient: () => Promise<IEventLogClient>;
@@ -191,7 +187,6 @@ export class AlertsClient {
     namespace,
     getUserName,
     createAPIKey,
-    invalidateAPIKey,
     encryptedSavedObjectsClient,
     getActionsClient,
     actionsAuthorization,
@@ -207,7 +202,6 @@ export class AlertsClient {
     this.unsecuredSavedObjectsClient = unsecuredSavedObjectsClient;
     this.authorization = authorization;
     this.createAPIKey = createAPIKey;
-    this.invalidateAPIKey = invalidateAPIKey;
     this.encryptedSavedObjectsClient = encryptedSavedObjectsClient;
     this.getActionsClient = getActionsClient;
     this.actionsAuthorization = actionsAuthorization;
@@ -263,7 +257,11 @@ export class AlertsClient {
       );
     } catch (e) {
       // Avoid unused API key
-      this.invalidateApiKey({ apiKey: rawAlert.apiKey });
+      markApiKeyForInvalidation(
+        { apiKey: rawAlert.apiKey },
+        this.logger,
+        this.unsecuredSavedObjectsClient
+      );
       throw e;
     }
     if (data.enabled) {
@@ -487,7 +485,13 @@ export class AlertsClient {
 
     await Promise.all([
       taskIdToRemove ? deleteTaskIfItExists(this.taskManager, taskIdToRemove) : null,
-      apiKeyToInvalidate ? this.invalidateApiKey({ apiKey: apiKeyToInvalidate }) : null,
+      apiKeyToInvalidate
+        ? markApiKeyForInvalidation(
+            { apiKey: apiKeyToInvalidate },
+            this.logger,
+            this.unsecuredSavedObjectsClient
+          )
+        : null,
     ]);
 
     return removeResult;
@@ -526,7 +530,11 @@ export class AlertsClient {
 
     await Promise.all([
       alertSavedObject.attributes.apiKey
-        ? this.invalidateApiKey({ apiKey: alertSavedObject.attributes.apiKey })
+        ? markApiKeyForInvalidation(
+            { apiKey: alertSavedObject.attributes.apiKey },
+            this.logger,
+            this.unsecuredSavedObjectsClient
+          )
         : null,
       (async () => {
         if (
@@ -591,7 +599,11 @@ export class AlertsClient {
       );
     } catch (e) {
       // Avoid unused API key
-      this.invalidateApiKey({ apiKey: createAttributes.apiKey });
+      markApiKeyForInvalidation(
+        { apiKey: createAttributes.apiKey },
+        this.logger,
+        this.unsecuredSavedObjectsClient
+      );
       throw e;
     }
 
@@ -671,28 +683,20 @@ export class AlertsClient {
       await this.unsecuredSavedObjectsClient.update('alert', id, updateAttributes, { version });
     } catch (e) {
       // Avoid unused API key
-      this.invalidateApiKey({ apiKey: updateAttributes.apiKey });
+      markApiKeyForInvalidation(
+        { apiKey: updateAttributes.apiKey },
+        this.logger,
+        this.unsecuredSavedObjectsClient
+      );
       throw e;
     }
 
     if (apiKeyToInvalidate) {
-      await this.invalidateApiKey({ apiKey: apiKeyToInvalidate });
-    }
-  }
-
-  private async invalidateApiKey({ apiKey }: { apiKey: string | null }): Promise<void> {
-    if (!apiKey) {
-      return;
-    }
-
-    try {
-      const apiKeyId = Buffer.from(apiKey, 'base64').toString().split(':')[0];
-      const response = await this.invalidateAPIKey({ id: apiKeyId });
-      if (response.apiKeysEnabled === true && response.result.error_count > 0) {
-        this.logger.error(`Failed to invalidate API Key [id="${apiKeyId}"]`);
-      }
-    } catch (e) {
-      this.logger.error(`Failed to invalidate API Key: ${e.message}`);
+      await markApiKeyForInvalidation(
+        { apiKey: apiKeyToInvalidate },
+        this.logger,
+        this.unsecuredSavedObjectsClient
+      );
     }
   }
 
@@ -752,7 +756,11 @@ export class AlertsClient {
         await this.unsecuredSavedObjectsClient.update('alert', id, updateAttributes, { version });
       } catch (e) {
         // Avoid unused API key
-        this.invalidateApiKey({ apiKey: updateAttributes.apiKey });
+        markApiKeyForInvalidation(
+          { apiKey: updateAttributes.apiKey },
+          this.logger,
+          this.unsecuredSavedObjectsClient
+        );
         throw e;
       }
       const scheduledTask = await this.scheduleAlert(
@@ -764,7 +772,11 @@ export class AlertsClient {
         scheduledTaskId: scheduledTask.id,
       });
       if (apiKeyToInvalidate) {
-        await this.invalidateApiKey({ apiKey: apiKeyToInvalidate });
+        await markApiKeyForInvalidation(
+          { apiKey: apiKeyToInvalidate },
+          this.logger,
+          this.unsecuredSavedObjectsClient
+        );
       }
     }
   }
@@ -825,7 +837,13 @@ export class AlertsClient {
         attributes.scheduledTaskId
           ? deleteTaskIfItExists(this.taskManager, attributes.scheduledTaskId)
           : null,
-        apiKeyToInvalidate ? this.invalidateApiKey({ apiKey: apiKeyToInvalidate }) : null,
+        apiKeyToInvalidate
+          ? await markApiKeyForInvalidation(
+              { apiKey: apiKeyToInvalidate },
+              this.logger,
+              this.unsecuredSavedObjectsClient
+            )
+          : null,
       ]);
     }
   }

diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/aggregate.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/aggregate.test.ts
@@ -34,7 +34,6 @@ const alertsClientParams: jest.Mocked<ConstructorOptions> = {
   namespace: 'default',
   getUserName: jest.fn(),
   createAPIKey: jest.fn(),
-  invalidateAPIKey: jest.fn(),
   logger: loggingSystemMock.create().get(),
   encryptedSavedObjectsClient: encryptedSavedObjects,
   getActionsClient: jest.fn(),

diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/create.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/create.test.ts
@@ -34,7 +34,6 @@ const alertsClientParams: jest.Mocked<ConstructorOptions> = {
   namespace: 'default',
   getUserName: jest.fn(),
   createAPIKey: jest.fn(),
-  invalidateAPIKey: jest.fn(),
   logger: loggingSystemMock.create().get(),
   encryptedSavedObjectsClient: encryptedSavedObjects,
   getActionsClient: jest.fn(),
@@ -711,7 +710,7 @@ describe('create()', () => {
     expect(taskManager.schedule).not.toHaveBeenCalled();
   });
 
-  test('throws error and invalidates API key when create saved object fails', async () => {
+  test('throws error and add API key to invalidatePendingApiKey SO when create saved object fails', async () => {
     const data = getMockData();
     alertsClientParams.createAPIKey.mockResolvedValueOnce({
       apiKeysEnabled: true,
@@ -731,11 +730,25 @@ describe('create()', () => {
       ],
     });
     unsecuredSavedObjectsClient.create.mockRejectedValueOnce(new Error('Test failure'));
+    const createdAt = new Date().toISOString();
+    unsecuredSavedObjectsClient.create.mockResolvedValueOnce({
+      id: '1',
+      type: 'api_key_pending_invalidation',
+      attributes: {
+        apiKeyId: '123',
+        createdAt,
+      },
+      references: [],
+    });
     await expect(alertsClient.create({ data })).rejects.toThrowErrorMatchingInlineSnapshot(
       `"Test failure"`
     );
     expect(taskManager.schedule).not.toHaveBeenCalled();
-    expect(alertsClientParams.invalidateAPIKey).toHaveBeenCalledWith({ id: '123' });
+    expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(2);
+    expect(unsecuredSavedObjectsClient.create.mock.calls[1][1]).toStrictEqual({
+      apiKeyId: '123',
+      createdAt,
+    });
   });
 
   test('attempts to remove saved object if scheduling failed', async () => {

diff --git a/x-pack/plugins/alerts/server/alerts_client/tests/delete.test.ts b/x-pack/plugins/alerts/server/alerts_client/tests/delete.test.ts
@@ -32,7 +32,6 @@ const alertsClientParams: jest.Mocked<ConstructorOptions> = {
   namespace: 'default',
   getUserName: jest.fn(),
   createAPIKey: jest.fn(),
-  invalidateAPIKey: jest.fn(),
   logger: loggingSystemMock.create().get(),
   encryptedSavedObjectsClient: encryptedSavedObjects,
   getActionsClient: jest.fn(),
@@ -94,11 +93,22 @@ describe('delete()', () => {
   });
 
   test('successfully removes an alert', async () => {
+    unsecuredSavedObjectsClient.create.mockResolvedValueOnce({
+      id: '1',
+      type: 'api_key_pending_invalidation',
+      attributes: {
+        apiKeyId: '123',
+        createdAt: '2019-02-12T21:01:22.479Z',
+      },
+      references: [],
+    });
     const result = await alertsClient.delete({ id: '1' });
     expect(result).toEqual({ success: true });
     expect(unsecuredSavedObjectsClient.delete).toHaveBeenCalledWith('alert', '1');
     expect(taskManager.remove).toHaveBeenCalledWith('task-123');
-    expect(alertsClientParams.invalidateAPIKey).toHaveBeenCalledWith({ id: '123' });
+    expect(unsecuredSavedObjectsClient.create.mock.calls[0][0]).toBe(
+      'api_key_pending_invalidation'
+    );
     expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', {
       namespace: 'default',
     });
@@ -107,12 +117,21 @@ describe('delete()', () => {
 
   test('falls back to SOC.get when getDecryptedAsInternalUser throws an error', async () => {
     encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValue(new Error('Fail'));
+    unsecuredSavedObjectsClient.create.mockResolvedValueOnce({
+      id: '1',
+      type: 'api_key_pending_invalidation',
+      attributes: {
+        apiKeyId: '123',
+        createdAt: '2019-02-12T21:01:22.479Z',
+      },
+      references: [],
+    });
 
     const result = await alertsClient.delete({ id: '1' });
     expect(result).toEqual({ success: true });
     expect(unsecuredSavedObjectsClient.delete).toHaveBeenCalledWith('alert', '1');
     expect(taskManager.remove).toHaveBeenCalledWith('task-123');
-    expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled();
+    expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
     expect(unsecuredSavedObjectsClient.get).toHaveBeenCalledWith('alert', '1');
     expect(alertsClientParams.logger.error).toHaveBeenCalledWith(
       'delete(): Failed to load API key to invalidate on alert 1: Fail'
@@ -133,6 +152,15 @@ describe('delete()', () => {
   });
 
   test(`doesn't invalidate API key when apiKey is null`, async () => {
+    unsecuredSavedObjectsClient.create.mockResolvedValueOnce({
+      id: '1',
+      type: 'api_key_pending_invalidation',
+      attributes: {
+        apiKeyId: '123',
+        createdAt: '2019-02-12T21:01:22.479Z',
+      },
+      references: [],
+    });
     encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValue({
       ...existingAlert,
       attributes: {
@@ -142,24 +170,34 @@ describe('delete()', () => {
     });
 
     await alertsClient.delete({ id: '1' });
-    expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled();
+    expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
   });
 
   test('swallows error when invalidate API key throws', async () => {
-    alertsClientParams.invalidateAPIKey.mockRejectedValueOnce(new Error('Fail'));
-
+    unsecuredSavedObjectsClient.create.mockRejectedValueOnce(new Error('Fail'));
     await alertsClient.delete({ id: '1' });
-    expect(alertsClientParams.invalidateAPIKey).toHaveBeenCalledWith({ id: '123' });
+    expect(unsecuredSavedObjectsClient.create.mock.calls[0][0]).toBe(
+      'api_key_pending_invalidation'
+    );
     expect(alertsClientParams.logger.error).toHaveBeenCalledWith(
-      'Failed to invalidate API Key: Fail'
+      'Failed to mark for API key [id="MTIzOmFiYw=="] for invalidation: Fail'
     );
   });
 
   test('swallows error when getDecryptedAsInternalUser throws an error', async () => {
+    unsecuredSavedObjectsClient.create.mockResolvedValueOnce({
+      id: '1',
+      type: 'api_key_pending_invalidation',
+      attributes: {
+        apiKeyId: '123',
+        createdAt: '2019-02-12T21:01:22.479Z',
+      },
+      references: [],
+    });
     encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValue(new Error('Fail'));
 
     await alertsClient.delete({ id: '1' });
-    expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled();
+    expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
     expect(alertsClientParams.logger.error).toHaveBeenCalledWith(
       'delete(): Failed to load API key to invalidate on alert 1: Fail'
     );