diff --git a/x-pack/plugins/actions/server/actions_client.test.ts b/x-pack/plugins/actions/server/actions_client.test.ts index 787b4e450a9e..e5b737456318 100644 --- 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 = 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 index 89156bb56b51..99d8fb32c608 100644 --- 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 index c097b94a8595..131563fd3e73 100644 --- 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(), licenseState: this.licenseState, - logger: this.logger, actionsConfigUtils, usageCounter: this.usageCounter, }); @@ -387,6 +386,7 @@ export class ActionsPlugin implements Plugin ({ verifyAccessAndContext: jest.fn(), })); -const logger = loggingSystemMock.create().get() as jest.Mocked; 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 index e1b612d321bc..1129da9ff8b1 100644 --- 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; export const getOAuthAccessToken = ( router: IRouter, 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), }); }) ) diff --git a/x-pack/plugins/actions/server/routes/index.ts b/x-pack/plugins/actions/server/routes/index.ts index 2822aa366890..501d7045e6d9 100644 --- a/x-pack/plugins/actions/server/routes/index.ts +++ b/x-pack/plugins/actions/server/routes/index.ts @@ -5,7 +5,7 @@ * 2.0. */ -import { IRouter, Logger } from '@kbn/core/server'; +import { IRouter } from '@kbn/core/server'; import { UsageCounter } from '@kbn/usage-collection-plugin/server'; import { ILicenseState } from '../lib'; import { ActionsRequestHandlerContext } from '../types'; @@ -24,13 +24,12 @@ import { ActionsConfigurationUtilities } from '../actions_config'; export interface RouteOptions { router: IRouter; licenseState: ILicenseState; - logger: Logger; actionsConfigUtils: ActionsConfigurationUtilities; usageCounter?: UsageCounter; } export function defineRoutes(opts: RouteOptions) { - const { router, licenseState, logger, actionsConfigUtils, usageCounter } = opts; + const { router, licenseState, actionsConfigUtils, usageCounter } = opts; defineLegacyRoutes(router, licenseState, usageCounter); @@ -42,6 +41,6 @@ export function defineRoutes(opts: RouteOptions) { connectorTypesRoute(router, licenseState); executeActionRoute(router, licenseState); - getOAuthAccessToken(router, licenseState, logger, actionsConfigUtils); + getOAuthAccessToken(router, licenseState, actionsConfigUtils); getWellKnownEmailServiceRoute(router, licenseState); }