Delete existing tokens when connector is updated (#132061)

* Add code on connector update to remove existing tokens * Fix copy/paste issue in jest test * Log instead of fail when deleting token fn throws
elastic · May 17, 2022 · af3c090 · af3c090
1 parent e0a5617
commit af3c090
Show file tree

Hide file tree

Showing 6 changed files with 73 additions and 33 deletions.
diff --git a/x-pack/plugins/actions/server/actions_client.test.ts b/x-pack/plugins/actions/server/actions_client.test.ts
@@ -115,6 +115,7 @@ beforeEach(() => {
   };
   actionTypeRegistry = new ActionTypeRegistry(actionTypeRegistryParams);
   actionsClient = new ActionsClient({
+    logger,
     actionTypeRegistry,
     unsecuredSavedObjectsClient,
     scopedClusterClient,
@@ -532,6 +533,7 @@ describe('create()', () => {
 
     actionTypeRegistry = new ActionTypeRegistry(localActionTypeRegistryParams);
     actionsClient = new ActionsClient({
+      logger,
       actionTypeRegistry,
       unsecuredSavedObjectsClient,
       scopedClusterClient,
@@ -635,6 +637,7 @@ describe('get()', () => {
 
     test('ensures user is authorised to get preconfigured type of action', async () => {
       actionsClient = new ActionsClient({
+        logger,
         actionTypeRegistry,
         unsecuredSavedObjectsClient,
         scopedClusterClient,
@@ -693,6 +696,7 @@ describe('get()', () => {
 
     test('throws when user is not authorised to get preconfigured of action', async () => {
       actionsClient = new ActionsClient({
+        logger,
         actionTypeRegistry,
         unsecuredSavedObjectsClient,
         scopedClusterClient,
@@ -813,6 +817,7 @@ describe('get()', () => {
 
   test('return predefined action with id', async () => {
     actionsClient = new ActionsClient({
+      logger,
       actionTypeRegistry,
       unsecuredSavedObjectsClient,
       scopedClusterClient,
@@ -886,6 +891,7 @@ describe('getAll()', () => {
       );
 
       actionsClient = new ActionsClient({
+        logger,
         actionTypeRegistry,
         unsecuredSavedObjectsClient,
         scopedClusterClient,
@@ -1026,6 +1032,7 @@ describe('getAll()', () => {
     );
 
     actionsClient = new ActionsClient({
+      logger,
       actionTypeRegistry,
       unsecuredSavedObjectsClient,
       scopedClusterClient,
@@ -1106,6 +1113,7 @@ describe('getBulk()', () => {
       );
 
       actionsClient = new ActionsClient({
+        logger,
         actionTypeRegistry,
         unsecuredSavedObjectsClient,
         scopedClusterClient,
@@ -1240,6 +1248,7 @@ describe('getBulk()', () => {
     );
 
     actionsClient = new ActionsClient({
+      logger,
       actionTypeRegistry,
       unsecuredSavedObjectsClient,
       scopedClusterClient,
@@ -1297,6 +1306,7 @@ describe('getOAuthAccessToken()', () => {
     requestBody: OAuthParams
   ): ReturnType<ActionsClient['getOAuthAccessToken']> {
     actionsClient = new ActionsClient({
+      logger,
       actionTypeRegistry,
       unsecuredSavedObjectsClient,
       scopedClusterClient,
@@ -1321,7 +1331,7 @@ describe('getOAuthAccessToken()', () => {
       ],
       connectorTokenClient: connectorTokenClientMock.create(),
     });
-    return actionsClient.getOAuthAccessToken(requestBody, logger, configurationUtilities);
+    return actionsClient.getOAuthAccessToken(requestBody, configurationUtilities);
   }
 
   describe('authorization', () => {
@@ -1583,7 +1593,6 @@ describe('delete()', () => {
     test('ensures user is authorised to delete actions', async () => {
       await actionsClient.delete({ id: '1' });
       expect(authorization.ensureAuthorized).toHaveBeenCalledWith('delete');
-      expect(connectorTokenClient.deleteConnectorTokens).toHaveBeenCalledTimes(1);
     });
 
     test('throws when user is not authorised to create the type of action', async () => {
@@ -1597,6 +1606,19 @@ describe('delete()', () => {
 
       expect(authorization.ensureAuthorized).toHaveBeenCalledWith('delete');
     });
+
+    test(`deletes any existing authorization tokens`, async () => {
+      await actionsClient.delete({ id: '1' });
+      expect(connectorTokenClient.deleteConnectorTokens).toHaveBeenCalledTimes(1);
+    });
+
+    test(`failing to delete tokens logs error instead of throw`, async () => {
+      connectorTokenClient.deleteConnectorTokens.mockRejectedValueOnce(new Error('Fail'));
+      await expect(actionsClient.delete({ id: '1' })).resolves.toBeUndefined();
+      expect(logger.error).toHaveBeenCalledWith(
+        `Failed to delete auth tokens for connector "1" after delete: Fail`
+      );
+    });
   });
 
   describe('auditLogger', () => {
@@ -1703,6 +1725,19 @@ describe('update()', () => {
 
       expect(authorization.ensureAuthorized).toHaveBeenCalledWith('update');
     });
+
+    test(`deletes any existing authorization tokens`, async () => {
+      await updateOperation();
+      expect(connectorTokenClient.deleteConnectorTokens).toHaveBeenCalledTimes(1);
+    });
+
+    test(`failing to delete tokens logs error instead of throw`, async () => {
+      connectorTokenClient.deleteConnectorTokens.mockRejectedValueOnce(new Error('Fail'));
+      await expect(updateOperation()).resolves.toBeTruthy();
+      expect(logger.error).toHaveBeenCalledWith(
+        `Failed to delete auth tokens for connector "my-action" after update: Fail`
+      );
+    });
   });
 
   describe('auditLogger', () => {
@@ -2305,6 +2340,7 @@ describe('isActionTypeEnabled()', () => {
 describe('isPreconfigured()', () => {
   test('should return true if connector id is in list of preconfigured connectors', () => {
     actionsClient = new ActionsClient({
+      logger,
       actionTypeRegistry,
       unsecuredSavedObjectsClient,
       scopedClusterClient,
@@ -2341,6 +2377,7 @@ describe('isPreconfigured()', () => {
 
   test('should return false if connector id is not in list of preconfigured connectors', () => {
     actionsClient = new ActionsClient({
+      logger,
       actionTypeRegistry,
       unsecuredSavedObjectsClient,
       scopedClusterClient,

diff --git a/x-pack/plugins/actions/server/actions_client.ts b/x-pack/plugins/actions/server/actions_client.ts
@@ -85,6 +85,7 @@ export interface CreateOptions {
 }
 
 interface ConstructorOptions {
+  logger: Logger;
   defaultKibanaIndex: string;
   scopedClusterClient: IScopedClusterClient;
   actionTypeRegistry: ActionTypeRegistry;
@@ -106,6 +107,7 @@ export interface UpdateOptions {
 }
 
 export class ActionsClient {
+  private readonly logger: Logger;
   private readonly defaultKibanaIndex: string;
   private readonly scopedClusterClient: IScopedClusterClient;
   private readonly unsecuredSavedObjectsClient: SavedObjectsClientContract;
@@ -121,6 +123,7 @@ export class ActionsClient {
   private readonly connectorTokenClient: ConnectorTokenClientContract;
 
   constructor({
+    logger,
     actionTypeRegistry,
     defaultKibanaIndex,
     scopedClusterClient,
@@ -135,6 +138,7 @@ export class ActionsClient {
     usageCounter,
     connectorTokenClient,
   }: ConstructorOptions) {
+    this.logger = logger;
     this.actionTypeRegistry = actionTypeRegistry;
     this.unsecuredSavedObjectsClient = unsecuredSavedObjectsClient;
     this.scopedClusterClient = scopedClusterClient;
@@ -283,6 +287,14 @@ export class ActionsClient {
       )
     );
 
+    try {
+      await this.connectorTokenClient.deleteConnectorTokens({ connectorId: id });
+    } catch (e) {
+      this.logger.error(
+        `Failed to delete auth tokens for connector "${id}" after update: ${e.message}`
+      );
+    }
+
     return {
       id,
       actionTypeId: result.attributes.actionTypeId as string,
@@ -468,7 +480,6 @@ export class ActionsClient {
 
   public async getOAuthAccessToken(
     { type, options }: OAuthParams,
-    logger: Logger,
     configurationUtilities: ActionsConfigurationUtilities
   ) {
     // Verify that user has edit access
@@ -502,7 +513,7 @@ export class ActionsClient {
 
       try {
         accessToken = await getOAuthJwtAccessToken({
-          logger,
+          logger: this.logger,
           configurationUtilities,
           credentials: {
             config: tokenOpts.config as GetOAuthJwtConfig,
@@ -511,13 +522,13 @@ export class ActionsClient {
           tokenUrl: tokenOpts.tokenUrl,
         });
 
-        logger.debug(
+        this.logger.debug(
           `Successfully retrieved access token using JWT OAuth with tokenUrl ${
             tokenOpts.tokenUrl
           } and config ${JSON.stringify(tokenOpts.config)}`
         );
       } catch (err) {
-        logger.debug(
+        this.logger.debug(
           `Failed to retrieve access token using JWT OAuth with tokenUrl ${
             tokenOpts.tokenUrl
           } and config ${JSON.stringify(tokenOpts.config)} - ${err.message}`
@@ -528,7 +539,7 @@ export class ActionsClient {
       const tokenOpts = options as OAuthClientCredentialsParams;
       try {
         accessToken = await getOAuthClientCredentialsAccessToken({
-          logger,
+          logger: this.logger,
           configurationUtilities,
           credentials: {
             config: tokenOpts.config as GetOAuthClientCredentialsConfig,
@@ -538,13 +549,13 @@ export class ActionsClient {
           oAuthScope: tokenOpts.scope,
         });
 
-        logger.debug(
+        this.logger.debug(
           `Successfully retrieved access token using Client Credentials OAuth with tokenUrl ${
             tokenOpts.tokenUrl
           }, scope ${tokenOpts.scope} and config ${JSON.stringify(tokenOpts.config)}`
         );
       } catch (err) {
-        logger.debug(
+        this.logger.debug(
           `Failed to retrieved access token using Client Credentials OAuth with tokenUrl ${
             tokenOpts.tokenUrl
           }, scope ${tokenOpts.scope} and config ${JSON.stringify(tokenOpts.config)} - ${
@@ -600,15 +611,12 @@ export class ActionsClient {
 
     try {
       await this.connectorTokenClient.deleteConnectorTokens({ connectorId: id });
-    } catch (error) {
-      this.auditLogger?.log(
-        connectorAuditEvent({
-          action: ConnectorAuditAction.DELETE,
-          savedObject: { type: 'action', id },
-          error,
-        })
+    } catch (e) {
+      this.logger.error(
+        `Failed to delete auth tokens for connector "${id}" after delete: ${e.message}`
       );
     }
+
     return await this.unsecuredSavedObjectsClient.delete('action', id);
   }
 

diff --git a/x-pack/plugins/actions/server/plugin.ts b/x-pack/plugins/actions/server/plugin.ts
@@ -314,7 +314,6 @@ export class ActionsPlugin implements Plugin<PluginSetupContract, PluginStartCon
     defineRoutes({
       router: core.http.createRouter<ActionsRequestHandlerContext>(),
       licenseState: this.licenseState,
-      logger: this.logger,
       actionsConfigUtils,
       usageCounter: this.usageCounter,
     });
@@ -387,6 +386,7 @@ export class ActionsPlugin implements Plugin<PluginSetupContract, PluginStartCon
       );
 
       return new ActionsClient({
+        logger,
         unsecuredSavedObjectsClient,
         actionTypeRegistry: actionTypeRegistry!,
         defaultKibanaIndex: kibanaIndex!,
@@ -578,6 +578,7 @@ export class ActionsPlugin implements Plugin<PluginSetupContract, PluginStartCon
             includedHiddenTypes,
           });
           return new ActionsClient({
+            logger,
             unsecuredSavedObjectsClient,
             actionTypeRegistry: actionTypeRegistry!,
             defaultKibanaIndex,

diff --git a/x-pack/plugins/actions/server/routes/get_oauth_access_token.test.ts b/x-pack/plugins/actions/server/routes/get_oauth_access_token.test.ts
@@ -6,8 +6,7 @@
  */
 
 import { getOAuthAccessToken } from './get_oauth_access_token';
-import { Logger } from '@kbn/core/server';
-import { httpServiceMock, loggingSystemMock } from '@kbn/core/server/mocks';
+import { httpServiceMock } from '@kbn/core/server/mocks';
 import { licenseStateMock } from '../lib/license_state.mock';
 import { mockHandlerArguments } from './legacy/_mock_handler_arguments';
 import { verifyAccessAndContext } from './verify_access_and_context';
@@ -18,7 +17,6 @@ jest.mock('./verify_access_and_context', () => ({
   verifyAccessAndContext: jest.fn(),
 }));
 
-const logger = loggingSystemMock.create().get() as jest.Mocked<Logger>;
 const configurationUtilities = actionsConfigMock.create();
 
 beforeEach(() => {
@@ -31,7 +29,7 @@ describe('getOAuthAccessToken', () => {
     const licenseState = licenseStateMock.create();
     const router = httpServiceMock.createRouter();
 
-    getOAuthAccessToken(router, licenseState, logger, configurationUtilities);
+    getOAuthAccessToken(router, licenseState, configurationUtilities);
 
     const [config, handler] = router.post.mock.calls[0];
 
@@ -77,7 +75,6 @@ describe('getOAuthAccessToken', () => {
     expect(actionsClient.getOAuthAccessToken).toHaveBeenCalledTimes(1);
     expect(actionsClient.getOAuthAccessToken.mock.calls[0]).toEqual([
       requestBody,
-      logger,
       configurationUtilities,
     ]);
 
@@ -92,7 +89,7 @@ describe('getOAuthAccessToken', () => {
     const licenseState = licenseStateMock.create();
     const router = httpServiceMock.createRouter();
 
-    getOAuthAccessToken(router, licenseState, logger, configurationUtilities);
+    getOAuthAccessToken(router, licenseState, configurationUtilities);
 
     const [config, handler] = router.post.mock.calls[0];
 
@@ -137,7 +134,6 @@ describe('getOAuthAccessToken', () => {
     expect(actionsClient.getOAuthAccessToken).toHaveBeenCalledTimes(1);
     expect(actionsClient.getOAuthAccessToken.mock.calls[0]).toEqual([
       requestBody,
-      logger,
       configurationUtilities,
     ]);
 
@@ -152,7 +148,7 @@ describe('getOAuthAccessToken', () => {
     const licenseState = licenseStateMock.create();
     const router = httpServiceMock.createRouter();
 
-    getOAuthAccessToken(router, licenseState, logger, configurationUtilities);
+    getOAuthAccessToken(router, licenseState, configurationUtilities);
 
     const [config, handler] = router.post.mock.calls[0];
 
@@ -193,7 +189,7 @@ describe('getOAuthAccessToken', () => {
       throw new Error('OMG');
     });
 
-    getOAuthAccessToken(router, licenseState, logger, configurationUtilities);
+    getOAuthAccessToken(router, licenseState, configurationUtilities);
 
     const [config, handler] = router.post.mock.calls[0];
 

diff --git a/x-pack/plugins/actions/server/routes/get_oauth_access_token.ts b/x-pack/plugins/actions/server/routes/get_oauth_access_token.ts
@@ -6,7 +6,7 @@
  */
 
 import { schema, TypeOf } from '@kbn/config-schema';
-import { IRouter, Logger } from '@kbn/core/server';
+import { IRouter } from '@kbn/core/server';
 import { ILicenseState } from '../lib';
 import { INTERNAL_BASE_ACTION_API_PATH } from '../../common';
 import { ActionsRequestHandlerContext } from '../types';
@@ -58,7 +58,6 @@ export type OAuthParams = TypeOf<typeof bodySchema>;
 export const getOAuthAccessToken = (
   router: IRouter<ActionsRequestHandlerContext>,
   licenseState: ILicenseState,
-  logger: Logger,
   configurationUtilities: ActionsConfigurationUtilities
 ) => {
   router.post(
@@ -73,7 +72,7 @@ export const getOAuthAccessToken = (
         const actionsClient = (await context.actions).getActionsClient();
 
         return res.ok({
-          body: await actionsClient.getOAuthAccessToken(req.body, logger, configurationUtilities),
+          body: await actionsClient.getOAuthAccessToken(req.body, configurationUtilities),
         });
       })
     )