From 4cc33864b8276c4de92b1c761c7347e14cc6bb5c Mon Sep 17 00:00:00 2001 From: Ying Mao Date: Mon, 9 May 2022 15:01:34 -0400 Subject: [PATCH 1/8] Throwing error if service now access token is null. Properly returning rejected promise --- .../servicenow/utils.test.ts | 72 ++++++++++++++++--- .../builtin_action_types/servicenow/utils.ts | 5 +- 2 files changed, 65 insertions(+), 12 deletions(-) diff --git a/x-pack/plugins/actions/server/builtin_action_types/servicenow/utils.test.ts b/x-pack/plugins/actions/server/builtin_action_types/servicenow/utils.test.ts index 64a80977709e5..4ee93bc1865c7 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/servicenow/utils.test.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/servicenow/utils.test.ts @@ -191,17 +191,6 @@ describe('utils', () => { }); test('creates axios instance with interceptor when isOAuth is true and OAuth fields are defined', async () => { - connectorTokenClient.get.mockResolvedValueOnce({ - hasErrors: false, - connectorToken: { - id: '1', - connectorId: '123', - tokenType: 'access_token', - token: 'testtokenvalue', - createdAt: new Date().toISOString(), - expiresAt: new Date(Date.now() + 10000000000).toISOString(), - }, - }); getAxiosInstance({ connectorId: '123', logger, @@ -259,5 +248,66 @@ describe('utils', () => { connectorTokenClient, }); }); + + test('throws expected error if getOAuthJwtAccessToken returns null access token', async () => { + getAxiosInstance({ + connectorId: '123', + logger, + configurationUtilities, + credentials: { + config: { + apiUrl: 'https://servicenow', + usesTableApi: true, + isOAuth: true, + clientId: 'clientId', + jwtKeyId: 'jwtKeyId', + userIdentifierValue: 'userIdentifierValue', + }, + secrets: { + clientSecret: 'clientSecret', + privateKey: 'privateKey', + privateKeyPassword: null, + username: null, + password: null, + }, + }, + snServiceUrl: 'https://dev23432523.service-now.com', + connectorTokenClient, + }); + expect(createAxiosInstanceMock).toHaveBeenCalledTimes(1); + expect(createAxiosInstanceMock).toHaveBeenCalledWith(); + expect(axiosInstanceMock.interceptors.request.use).toHaveBeenCalledTimes(1); + + (getOAuthJwtAccessToken as jest.Mock).mockResolvedValueOnce(null); + + const mockRequestCallback = (axiosInstanceMock.interceptors.request.use as jest.Mock).mock + .calls[0][0]; + + await expect(() => + mockRequestCallback({ headers: {} }) + ).rejects.toThrowErrorMatchingInlineSnapshot( + `"Unable to retrieve access token for connectorId: 123"` + ); + + expect(getOAuthJwtAccessToken as jest.Mock).toHaveBeenCalledWith({ + connectorId: '123', + logger, + configurationUtilities, + credentials: { + config: { + clientId: 'clientId', + jwtKeyId: 'jwtKeyId', + userIdentifierValue: 'userIdentifierValue', + }, + secrets: { + clientSecret: 'clientSecret', + privateKey: 'privateKey', + privateKeyPassword: null, + }, + }, + tokenUrl: 'https://dev23432523.service-now.com/oauth_token.do', + connectorTokenClient, + }); + }); }); }); diff --git a/x-pack/plugins/actions/server/builtin_action_types/servicenow/utils.ts b/x-pack/plugins/actions/server/builtin_action_types/servicenow/utils.ts index 538967269b1ea..b8b707d84d860 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/servicenow/utils.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/servicenow/utils.ts @@ -134,11 +134,14 @@ export const getAxiosInstance = ({ tokenUrl: `${snServiceUrl}/oauth_token.do`, connectorTokenClient, }); + if (!accessToken) { + throw new Error(`Unable to retrieve access token for connectorId: ${connectorId}`); + } axiosConfig.headers.Authorization = accessToken; return axiosConfig; }, (error) => { - Promise.reject(error); + return Promise.reject(error); } ); } From 4d197fbdc61c1ccfafbf5a0af0d953b83d45b7f5 Mon Sep 17 00:00:00 2001 From: Ying Mao Date: Mon, 9 May 2022 15:30:54 -0400 Subject: [PATCH 2/8] Setting time to calculate token expiration to before the token is created --- .../lib/connector_token_client.test.ts | 3 +++ .../lib/connector_token_client.ts | 7 +++++-- ...th_client_credentials_access_token.test.ts | 21 +++++++++++++++---- ...t_oauth_client_credentials_access_token.ts | 4 ++++ .../lib/get_oauth_jwt_access_token.test.ts | 19 +++++++++++++---- .../lib/get_oauth_jwt_access_token.ts | 4 ++++ 6 files changed, 48 insertions(+), 10 deletions(-) diff --git a/x-pack/plugins/actions/server/builtin_action_types/lib/connector_token_client.test.ts b/x-pack/plugins/actions/server/builtin_action_types/lib/connector_token_client.test.ts index 54765b9e01b8f..d55a97dccacca 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/lib/connector_token_client.test.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/lib/connector_token_client.test.ts @@ -375,6 +375,7 @@ describe('updateOrReplace()', () => { connectorId: '1', token: null, newToken: 'newToken', + tokenRequestDate: Date.now(), expiresInSec: 1000, deleteExisting: false, }); @@ -434,6 +435,7 @@ describe('updateOrReplace()', () => { connectorId: '1', token: null, newToken: 'newToken', + tokenRequestDate: Date.now(), expiresInSec: 1000, deleteExisting: true, }); @@ -483,6 +485,7 @@ describe('updateOrReplace()', () => { expiresAt: new Date().toISOString(), }, newToken: 'newToken', + tokenRequestDate: Date.now(), expiresInSec: 1000, deleteExisting: true, }); diff --git a/x-pack/plugins/actions/server/builtin_action_types/lib/connector_token_client.ts b/x-pack/plugins/actions/server/builtin_action_types/lib/connector_token_client.ts index 6ce91fad94546..af0ebffa30160 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/lib/connector_token_client.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/lib/connector_token_client.ts @@ -38,6 +38,7 @@ interface UpdateOrReplaceOptions { token: ConnectorToken | null; newToken: string; expiresInSec: number; + tokenRequestDate: number; deleteExisting: boolean; } export class ConnectorTokenClient { @@ -258,9 +259,11 @@ export class ConnectorTokenClient { token, newToken, expiresInSec, + tokenRequestDate, deleteExisting, }: UpdateOrReplaceOptions) { expiresInSec = expiresInSec ?? 3600; + tokenRequestDate = tokenRequestDate ?? Date.now(); if (token === null) { if (deleteExisting) { await this.deleteConnectorTokens({ @@ -272,14 +275,14 @@ export class ConnectorTokenClient { await this.create({ connectorId, token: newToken, - expiresAtMillis: new Date(Date.now() + expiresInSec * 1000).toISOString(), + expiresAtMillis: new Date(tokenRequestDate + expiresInSec * 1000).toISOString(), tokenType: 'access_token', }); } else { await this.update({ id: token.id!.toString(), token: newToken, - expiresAtMillis: new Date(Date.now() + expiresInSec * 1000).toISOString(), + expiresAtMillis: new Date(tokenRequestDate + expiresInSec * 1000).toISOString(), tokenType: 'access_token', }); } diff --git a/x-pack/plugins/actions/server/builtin_action_types/lib/get_oauth_client_credentials_access_token.test.ts b/x-pack/plugins/actions/server/builtin_action_types/lib/get_oauth_client_credentials_access_token.test.ts index 2efa79cf09c48..c3464a11e557e 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/lib/get_oauth_client_credentials_access_token.test.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/lib/get_oauth_client_credentials_access_token.test.ts @@ -4,6 +4,7 @@ * 2.0; you may not use this file except in compliance with the Elastic License * 2.0. */ +import sinon from 'sinon'; import { Logger } from '@kbn/core/server'; import { asyncForEach } from '@kbn/std'; import { loggingSystemMock } from '@kbn/core/server/mocks'; @@ -20,7 +21,15 @@ const logger = loggingSystemMock.create().get() as jest.Mocked; const configurationUtilities = actionsConfigMock.create(); const connectorTokenClient = connectorTokenClientMock.create(); +let clock: sinon.SinonFakeTimers; + describe('getOAuthClientCredentialsAccessToken', () => { + beforeAll(() => { + clock = sinon.useFakeTimers(new Date('2021-01-01T12:00:00.000Z')); + }); + beforeEach(() => clock.reset()); + afterAll(() => clock.restore()); + const getOAuthClientCredentialsAccessTokenOpts = { connectorId: '123', logger, @@ -52,8 +61,8 @@ describe('getOAuthClientCredentialsAccessToken', () => { connectorId: '123', tokenType: 'access_token', token: 'testtokenvalue', - createdAt: new Date().toISOString(), - expiresAt: new Date(Date.now() + 10000000000).toISOString(), + createdAt: new Date('2021-01-01T08:00:00.000Z').toISOString(), + expiresAt: new Date('2021-01-02T13:00:00.000Z').toISOString(), }, }); const accessToken = await getOAuthClientCredentialsAccessToken( @@ -95,14 +104,15 @@ describe('getOAuthClientCredentialsAccessToken', () => { connectorId: '123', token: null, newToken: 'access_token brandnewaccesstoken', + tokenRequestDate: 1609502400000, expiresInSec: 1000, deleteExisting: false, }); }); test('creates new assertion if stored access token exists but is expired', async () => { - const createdAt = new Date().toISOString(); - const expiresAt = new Date(Date.now() - 100).toISOString(); + const createdAt = new Date('2021-01-01T08:00:00.000Z').toISOString(); + const expiresAt = new Date('2021-01-01T09:00:00.000Z').toISOString(); connectorTokenClient.get.mockResolvedValueOnce({ hasErrors: false, connectorToken: { @@ -147,6 +157,7 @@ describe('getOAuthClientCredentialsAccessToken', () => { expiresAt, }, newToken: 'access_token brandnewaccesstoken', + tokenRequestDate: 1609502400000, expiresInSec: 1000, deleteExisting: false, }); @@ -210,6 +221,7 @@ describe('getOAuthClientCredentialsAccessToken', () => { (requestOAuthClientCredentialsToken as jest.Mock).mockResolvedValueOnce({ tokenType: 'access_token', accessToken: 'brandnewaccesstoken', + tokenRequestDate: 1609502400000, expiresIn: 1000, }); connectorTokenClient.updateOrReplace.mockRejectedValueOnce(new Error('updateOrReplace error')); @@ -268,6 +280,7 @@ describe('getOAuthClientCredentialsAccessToken', () => { (requestOAuthClientCredentialsToken as jest.Mock).mockResolvedValueOnce({ tokenType: 'access_token', accessToken: 'brandnewaccesstoken', + tokenRequestDate: 1609502400000, expiresIn: 1000, }); diff --git a/x-pack/plugins/actions/server/builtin_action_types/lib/get_oauth_client_credentials_access_token.ts b/x-pack/plugins/actions/server/builtin_action_types/lib/get_oauth_client_credentials_access_token.ts index 803cce2db7668..1cce245a154c2 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/lib/get_oauth_client_credentials_access_token.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/lib/get_oauth_client_credentials_access_token.ts @@ -62,6 +62,9 @@ export const getOAuthClientCredentialsAccessToken = async ({ } if (connectorToken === null || Date.parse(connectorToken.expiresAt) <= Date.now()) { + // Save the time before requesting token so we can use it to calculate expiration + const requestTokenStart = Date.now(); + // request access token with jwt assertion const tokenResult = await requestOAuthClientCredentialsToken( tokenUrl, @@ -82,6 +85,7 @@ export const getOAuthClientCredentialsAccessToken = async ({ connectorId, token: connectorToken, newToken: accessToken, + tokenRequestDate: requestTokenStart, expiresInSec: tokenResult.expiresIn, deleteExisting: hasErrors, }); diff --git a/x-pack/plugins/actions/server/builtin_action_types/lib/get_oauth_jwt_access_token.test.ts b/x-pack/plugins/actions/server/builtin_action_types/lib/get_oauth_jwt_access_token.test.ts index b48456ddd2a8c..0fe837fc0581a 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/lib/get_oauth_jwt_access_token.test.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/lib/get_oauth_jwt_access_token.test.ts @@ -4,6 +4,7 @@ * 2.0; you may not use this file except in compliance with the Elastic License * 2.0. */ +import sinon from 'sinon'; import { Logger } from '@kbn/core/server'; import { asyncForEach } from '@kbn/std'; import { loggingSystemMock } from '@kbn/core/server/mocks'; @@ -24,7 +25,15 @@ const logger = loggingSystemMock.create().get() as jest.Mocked; const configurationUtilities = actionsConfigMock.create(); const connectorTokenClient = connectorTokenClientMock.create(); +let clock: sinon.SinonFakeTimers; + describe('getOAuthJwtAccessToken', () => { + beforeAll(() => { + clock = sinon.useFakeTimers(new Date('2021-01-01T12:00:00.000Z')); + }); + beforeEach(() => clock.reset()); + afterAll(() => clock.restore()); + const getOAuthJwtAccessTokenOpts = { connectorId: '123', logger, @@ -58,8 +67,8 @@ describe('getOAuthJwtAccessToken', () => { connectorId: '123', tokenType: 'access_token', token: 'testtokenvalue', - createdAt: new Date().toISOString(), - expiresAt: new Date(Date.now() + 10000000000).toISOString(), + createdAt: new Date('2021-01-01T08:00:00.000Z').toISOString(), + expiresAt: new Date('2021-01-02T13:00:00.000Z').toISOString(), }, }); const accessToken = await getOAuthJwtAccessToken(getOAuthJwtAccessTokenOpts); @@ -105,14 +114,15 @@ describe('getOAuthJwtAccessToken', () => { connectorId: '123', token: null, newToken: 'access_token brandnewaccesstoken', + tokenRequestDate: 1609502400000, expiresInSec: 1000, deleteExisting: false, }); }); test('creates new assertion if stored access token exists but is expired', async () => { - const createdAt = new Date().toISOString(); - const expiresAt = new Date(Date.now() - 100).toISOString(); + const createdAt = new Date('2021-01-01T08:00:00.000Z').toISOString(); + const expiresAt = new Date('2021-01-01T09:00:00.000Z').toISOString(); connectorTokenClient.get.mockResolvedValueOnce({ hasErrors: false, connectorToken: { @@ -161,6 +171,7 @@ describe('getOAuthJwtAccessToken', () => { createdAt, expiresAt, }, + tokenRequestDate: 1609502400000, newToken: 'access_token brandnewaccesstoken', expiresInSec: 1000, deleteExisting: false, diff --git a/x-pack/plugins/actions/server/builtin_action_types/lib/get_oauth_jwt_access_token.ts b/x-pack/plugins/actions/server/builtin_action_types/lib/get_oauth_jwt_access_token.ts index a4867d99556e7..1233a61c0f3c8 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/lib/get_oauth_jwt_access_token.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/lib/get_oauth_jwt_access_token.ts @@ -72,6 +72,9 @@ export const getOAuthJwtAccessToken = async ({ keyId: jwtKeyId, }); + // Save the time before requesting token so we can use it to calculate expiration + const requestTokenStart = Date.now(); + // request access token with jwt assertion const tokenResult = await requestOAuthJWTToken( tokenUrl, @@ -92,6 +95,7 @@ export const getOAuthJwtAccessToken = async ({ connectorId, token: connectorToken, newToken: accessToken, + tokenRequestDate: requestTokenStart, expiresInSec: tokenResult.expiresIn, deleteExisting: hasErrors, }); From e2ec8df8c940a02a1a470edc72e308a306ba302d Mon Sep 17 00:00:00 2001 From: Ying Mao Date: Mon, 9 May 2022 15:50:46 -0400 Subject: [PATCH 3/8] Returning null access token if stored access token has invalid expiresAt date --- .../lib/connector_token_client.test.ts | 45 ++++++++++++++++++- .../lib/connector_token_client.ts | 28 ++++++++---- 2 files changed, 63 insertions(+), 10 deletions(-) diff --git a/x-pack/plugins/actions/server/builtin_action_types/lib/connector_token_client.test.ts b/x-pack/plugins/actions/server/builtin_action_types/lib/connector_token_client.test.ts index d55a97dccacca..74dae360980a7 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/lib/connector_token_client.test.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/lib/connector_token_client.test.ts @@ -131,7 +131,7 @@ describe('get()', () => { expect(result).toEqual({ connectorToken: null, hasErrors: false }); }); - test('return null and log the error if unsecuredSavedObjectsClient thows an error', async () => { + test('return null and log the error if unsecuredSavedObjectsClient throws an error', async () => { unsecuredSavedObjectsClient.find.mockRejectedValueOnce(new Error('Fail')); const result = await connectorTokenClient.get({ @@ -145,7 +145,7 @@ describe('get()', () => { expect(result).toEqual({ connectorToken: null, hasErrors: true }); }); - test('return null and log the error if encryptedSavedObjectsClient decrypt method thows an error', async () => { + test('return null and log the error if encryptedSavedObjectsClient decrypt method throws an error', async () => { const expectedResult = { total: 1, per_page: 10, @@ -178,6 +178,47 @@ describe('get()', () => { ]); expect(result).toEqual({ connectorToken: null, hasErrors: true }); }); + + test('return null and log the error if expiresAt is NaN', async () => { + const expectedResult = { + total: 1, + per_page: 10, + page: 1, + saved_objects: [ + { + id: '1', + type: 'connector_token', + attributes: { + connectorId: '123', + tokenType: 'access_token', + createdAt: new Date().toISOString(), + expiresAt: 'yo', + }, + score: 1, + references: [], + }, + ], + }; + unsecuredSavedObjectsClient.find.mockResolvedValueOnce(expectedResult); + encryptedSavedObjectsClient.getDecryptedAsInternalUser.mockResolvedValueOnce({ + id: '1', + type: 'connector_token', + references: [], + attributes: { + token: 'testtokenvalue', + }, + }); + + const result = await connectorTokenClient.get({ + connectorId: '123', + tokenType: 'access_token', + }); + + expect(logger.error.mock.calls[0]).toMatchObject([ + `Failed to get connector_token for connectorId "123" and tokenType: "access_token". Error: expiresAt is not a valid Date "yo"`, + ]); + expect(result).toEqual({ connectorToken: null, hasErrors: true }); + }); }); describe('update()', () => { diff --git a/x-pack/plugins/actions/server/builtin_action_types/lib/connector_token_client.ts b/x-pack/plugins/actions/server/builtin_action_types/lib/connector_token_client.ts index af0ebffa30160..df1615d503329 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/lib/connector_token_client.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/lib/connector_token_client.ts @@ -196,6 +196,7 @@ export class ConnectorTokenClient { return { hasErrors: false, connectorToken: null }; } + let accessToken: string; try { const { attributes: { token }, @@ -204,14 +205,7 @@ export class ConnectorTokenClient { connectorTokensResult[0].id ); - return { - hasErrors: false, - connectorToken: { - id: connectorTokensResult[0].id, - ...connectorTokensResult[0].attributes, - token, - }, - }; + accessToken = token; } catch (err) { this.logger.error( `Failed to decrypt connector_token for connectorId "${connectorId}" and tokenType: "${ @@ -220,6 +214,24 @@ export class ConnectorTokenClient { ); return { hasErrors: true, connectorToken: null }; } + + if (isNaN(Date.parse(connectorTokensResult[0].attributes.expiresAt))) { + this.logger.error( + `Failed to get connector_token for connectorId "${connectorId}" and tokenType: "${ + tokenType ?? 'access_token' + }". Error: expiresAt is not a valid Date "${connectorTokensResult[0].attributes.expiresAt}"` + ); + return { hasErrors: true, connectorToken: null }; + } + + return { + hasErrors: false, + connectorToken: { + id: connectorTokensResult[0].id, + ...connectorTokensResult[0].attributes, + token: accessToken, + }, + }; } /** From 787376ae0078a11f9326de013f262dd5a8a4286b Mon Sep 17 00:00:00 2001 From: Ying Mao Date: Tue, 10 May 2022 12:52:58 -0400 Subject: [PATCH 4/8] Adding response interceptor to delete connector token if using access token returns 4xx error --- .../lib/send_email.test.ts | 122 +++++++++++++++--- .../builtin_action_types/lib/send_email.ts | 19 ++- .../lib/send_email_graph_api.ts | 8 +- .../servicenow/utils.test.ts | 103 +++++++++++++++ .../builtin_action_types/servicenow/utils.ts | 20 ++- 5 files changed, 246 insertions(+), 26 deletions(-) diff --git a/x-pack/plugins/actions/server/builtin_action_types/lib/send_email.test.ts b/x-pack/plugins/actions/server/builtin_action_types/lib/send_email.test.ts index fbf0d90541659..5c7831832b926 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/lib/send_email.test.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/lib/send_email.test.ts @@ -5,10 +5,21 @@ * 2.0. */ +import axios from 'axios'; +import { Logger } from '@kbn/core/server'; +import { sendEmail } from './send_email'; +import { loggingSystemMock } from '@kbn/core/server/mocks'; +import nodemailer from 'nodemailer'; +import { ProxySettings } from '../../types'; +import { actionsConfigMock } from '../../actions_config.mock'; +import { CustomHostSettings } from '../../config'; +import { sendEmailGraphApi } from './send_email_graph_api'; +import { getOAuthClientCredentialsAccessToken } from './get_oauth_client_credentials_access_token'; +import { connectorTokenClientMock } from './connector_token_client.mock'; + jest.mock('nodemailer', () => ({ createTransport: jest.fn(), })); - jest.mock('./send_email_graph_api', () => ({ sendEmailGraphApi: jest.fn(), })); @@ -16,36 +27,32 @@ jest.mock('./get_oauth_client_credentials_access_token', () => ({ getOAuthClientCredentialsAccessToken: jest.fn(), })); -import { Logger } from '@kbn/core/server'; -import { sendEmail } from './send_email'; -import { loggingSystemMock, savedObjectsClientMock } from '@kbn/core/server/mocks'; -import nodemailer from 'nodemailer'; -import { ProxySettings } from '../../types'; -import { actionsConfigMock } from '../../actions_config.mock'; -import { CustomHostSettings } from '../../config'; -import { sendEmailGraphApi } from './send_email_graph_api'; -import { getOAuthClientCredentialsAccessToken } from './get_oauth_client_credentials_access_token'; -import { ConnectorTokenClient } from './connector_token_client'; -import { encryptedSavedObjectsMock } from '@kbn/encrypted-saved-objects-plugin/server/mocks'; +jest.mock('axios'); +const mockAxiosInstanceInterceptor = { + request: { eject: jest.fn(), use: jest.fn() }, + response: { eject: jest.fn(), use: jest.fn() }, +}; const createTransportMock = nodemailer.createTransport as jest.Mock; const sendMailMockResult = { result: 'does not matter' }; const sendMailMock = jest.fn(); const mockLogger = loggingSystemMock.create().get() as jest.Mocked; -const unsecuredSavedObjectsClient = savedObjectsClientMock.create(); -const encryptedSavedObjectsClient = encryptedSavedObjectsMock.createClient(); -const connectorTokenClient = new ConnectorTokenClient({ - unsecuredSavedObjectsClient, - encryptedSavedObjectsClient, - logger: mockLogger, -}); +const connectorTokenClient = connectorTokenClientMock.create(); describe('send_email module', () => { beforeEach(() => { jest.resetAllMocks(); createTransportMock.mockReturnValue({ sendMail: sendMailMock }); sendMailMock.mockResolvedValue(sendMailMockResult); + + axios.create = jest.fn(() => { + const actual = jest.requireActual('axios'); + return { + ...actual.create, + interceptors: mockAxiosInstanceInterceptor, + }; + }); }); test('handles authenticated email using service', async () => { @@ -647,6 +654,83 @@ describe('send_email module', () => { ] `); }); + + test('deletes saved access tokens if 4xx response received', async () => { + const createAxiosInstanceMock = axios.create as jest.Mock; + const sendEmailOptions = getSendEmailOptions({ + transport: { + service: 'exchange_server', + clientId: '123456', + tenantId: '98765', + clientSecret: 'sdfhkdsjhfksdjfh', + }, + }); + (getOAuthClientCredentialsAccessToken as jest.Mock).mockResolvedValueOnce( + 'Bearer clienttokentokentoken' + ); + + await sendEmail(mockLogger, sendEmailOptions, connectorTokenClient); + expect(createAxiosInstanceMock).toHaveBeenCalledTimes(1); + expect(createAxiosInstanceMock).toHaveBeenCalledWith(); + expect(mockAxiosInstanceInterceptor.response.use).toHaveBeenCalledTimes(1); + + const mockResponseCallback = (mockAxiosInstanceInterceptor.response.use as jest.Mock).mock + .calls[0][1]; + + const errorResponse = { + response: { + status: 403, + statusText: 'Forbidden', + data: { + error: { + message: 'Insufficient rights to query records', + detail: 'Field(s) present in the query do not have permission to be read', + }, + status: 'failure', + }, + }, + }; + + await expect(() => mockResponseCallback(errorResponse)).rejects.toEqual(errorResponse); + + expect(connectorTokenClient.deleteConnectorTokens).toHaveBeenCalledWith({ + connectorId: '1', + }); + }); + + test('does not delete saved access token if not 4xx error response received', async () => { + const createAxiosInstanceMock = axios.create as jest.Mock; + const sendEmailOptions = getSendEmailOptions({ + transport: { + service: 'exchange_server', + clientId: '123456', + tenantId: '98765', + clientSecret: 'sdfhkdsjhfksdjfh', + }, + }); + (getOAuthClientCredentialsAccessToken as jest.Mock).mockResolvedValueOnce( + 'Bearer clienttokentokentoken' + ); + + await sendEmail(mockLogger, sendEmailOptions, connectorTokenClient); + expect(createAxiosInstanceMock).toHaveBeenCalledTimes(1); + expect(createAxiosInstanceMock).toHaveBeenCalledWith(); + expect(mockAxiosInstanceInterceptor.response.use).toHaveBeenCalledTimes(1); + + const mockResponseCallback = (mockAxiosInstanceInterceptor.response.use as jest.Mock).mock + .calls[0][1]; + + const errorResponse = { + response: { + status: 500, + statusText: 'Server error', + }, + }; + + await expect(() => mockResponseCallback(errorResponse)).rejects.toEqual(errorResponse); + + expect(connectorTokenClient.deleteConnectorTokens).not.toHaveBeenCalled(); + }); }); function getSendEmailOptions( diff --git a/x-pack/plugins/actions/server/builtin_action_types/lib/send_email.ts b/x-pack/plugins/actions/server/builtin_action_types/lib/send_email.ts index f2b059e51e0d6..db1151b6c7bac 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/lib/send_email.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/lib/send_email.ts @@ -5,6 +5,7 @@ * 2.0. */ +import axios, { AxiosResponse } from 'axios'; // info on nodemailer: https://nodemailer.com/about/ import nodemailer from 'nodemailer'; import { default as MarkdownIt } from 'markdown-it'; @@ -77,7 +78,7 @@ export async function sendEmail( } // send an email using MS Exchange Graph API -async function sendEmailWithExchange( +export async function sendEmailWithExchange( logger: Logger, options: SendEmailOptions, messageHTML: string, @@ -113,6 +114,22 @@ async function sendEmailWithExchange( Authorization: accessToken, }; + const axiosInstance = axios.create(); + axiosInstance.interceptors.response.use( + (response: AxiosResponse) => response, + async (error) => { + const statusCode = error?.response?.status; + + // Look for 4xx errors that indicate something is wrong with the request + // We don't know for sure that it is an access token issue but remove saved + // token just to be sure + if (statusCode >= 400 && statusCode < 500) { + await connectorTokenClient.deleteConnectorTokens({ connectorId }); + } + return Promise.reject(error); + } + ); + return await sendEmailGraphApi( { options, diff --git a/x-pack/plugins/actions/server/builtin_action_types/lib/send_email_graph_api.ts b/x-pack/plugins/actions/server/builtin_action_types/lib/send_email_graph_api.ts index c16cd884cb753..6475426143af7 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/lib/send_email_graph_api.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/lib/send_email_graph_api.ts @@ -7,7 +7,7 @@ // @ts-expect-error missing type def import stringify from 'json-stringify-safe'; -import axios, { AxiosResponse } from 'axios'; +import axios, { AxiosInstance, AxiosResponse } from 'axios'; import { Logger } from '@kbn/core/server'; import { request } from './axios_utils'; import { ActionsConfigurationUtilities } from '../../actions_config'; @@ -25,11 +25,13 @@ const MICROSOFT_GRAPH_API_HOST = 'https://graph.microsoft.com/v1.0'; export async function sendEmailGraphApi( sendEmailOptions: SendEmailGraphApiOptions, logger: Logger, - configurationUtilities: ActionsConfigurationUtilities + configurationUtilities: ActionsConfigurationUtilities, + axiosInstance?: AxiosInstance ): Promise { const { options, headers, messageHTML, graphApiUrl } = sendEmailOptions; - const axiosInstance = axios.create(); + // Create a new axios instance if one is not provided + axiosInstance = axiosInstance ?? axios.create(); // POST /users/{id | userPrincipalName}/sendMail const res = await request({ diff --git a/x-pack/plugins/actions/server/builtin_action_types/servicenow/utils.test.ts b/x-pack/plugins/actions/server/builtin_action_types/servicenow/utils.test.ts index 4ee93bc1865c7..40e121864a71f 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/servicenow/utils.test.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/servicenow/utils.test.ts @@ -309,5 +309,108 @@ describe('utils', () => { connectorTokenClient, }); }); + + test('deletes saved access tokens if 4xx response received', async () => { + getAxiosInstance({ + connectorId: '123', + logger, + configurationUtilities, + credentials: { + config: { + apiUrl: 'https://servicenow', + usesTableApi: true, + isOAuth: true, + clientId: 'clientId', + jwtKeyId: 'jwtKeyId', + userIdentifierValue: 'userIdentifierValue', + }, + secrets: { + clientSecret: 'clientSecret', + privateKey: 'privateKey', + privateKeyPassword: null, + username: null, + password: null, + }, + }, + snServiceUrl: 'https://dev23432523.service-now.com', + connectorTokenClient, + }); + expect(createAxiosInstanceMock).toHaveBeenCalledTimes(1); + expect(createAxiosInstanceMock).toHaveBeenCalledWith(); + expect(axiosInstanceMock.interceptors.request.use).toHaveBeenCalledTimes(1); + expect(axiosInstanceMock.interceptors.response.use).toHaveBeenCalledTimes(1); + + (getOAuthJwtAccessToken as jest.Mock).mockResolvedValueOnce('Bearer tokentokentoken'); + + const mockResponseCallback = (axiosInstanceMock.interceptors.response.use as jest.Mock).mock + .calls[0][1]; + + const errorResponse = { + response: { + status: 403, + statusText: 'Forbidden', + data: { + error: { + message: 'Insufficient rights to query records', + detail: 'Field(s) present in the query do not have permission to be read', + }, + status: 'failure', + }, + }, + }; + + await expect(() => mockResponseCallback(errorResponse)).rejects.toEqual(errorResponse); + + expect(connectorTokenClient.deleteConnectorTokens).toHaveBeenCalledWith({ + connectorId: '123', + }); + }); + + test('does not delete saved access token if not 4xx error response received', async () => { + getAxiosInstance({ + connectorId: '123', + logger, + configurationUtilities, + credentials: { + config: { + apiUrl: 'https://servicenow', + usesTableApi: true, + isOAuth: true, + clientId: 'clientId', + jwtKeyId: 'jwtKeyId', + userIdentifierValue: 'userIdentifierValue', + }, + secrets: { + clientSecret: 'clientSecret', + privateKey: 'privateKey', + privateKeyPassword: null, + username: null, + password: null, + }, + }, + snServiceUrl: 'https://dev23432523.service-now.com', + connectorTokenClient, + }); + expect(createAxiosInstanceMock).toHaveBeenCalledTimes(1); + expect(createAxiosInstanceMock).toHaveBeenCalledWith(); + expect(axiosInstanceMock.interceptors.request.use).toHaveBeenCalledTimes(1); + expect(axiosInstanceMock.interceptors.response.use).toHaveBeenCalledTimes(1); + + (getOAuthJwtAccessToken as jest.Mock).mockResolvedValueOnce('Bearer tokentokentoken'); + + const mockResponseCallback = (axiosInstanceMock.interceptors.response.use as jest.Mock).mock + .calls[0][1]; + + const errorResponse = { + response: { + status: 500, + statusText: 'Server error', + }, + }; + + await expect(() => mockResponseCallback(errorResponse)).rejects.toEqual(errorResponse); + + expect(connectorTokenClient.deleteConnectorTokens).not.toHaveBeenCalled(); + }); }); }); diff --git a/x-pack/plugins/actions/server/builtin_action_types/servicenow/utils.ts b/x-pack/plugins/actions/server/builtin_action_types/servicenow/utils.ts index b8b707d84d860..42fb96dd76ade 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/servicenow/utils.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/servicenow/utils.ts @@ -5,7 +5,7 @@ * 2.0. */ -import axios, { AxiosInstance, AxiosRequestConfig } from 'axios'; +import axios, { AxiosInstance, AxiosRequestConfig, AxiosResponse } from 'axios'; import { Logger } from '@kbn/core/server'; import { ExternalServiceCredentials, @@ -83,12 +83,12 @@ export const throwIfSubActionIsNotSupported = ({ }; export interface GetAxiosInstanceOpts { - connectorId?: string; + connectorId: string; logger: Logger; configurationUtilities: ActionsConfigurationUtilities; credentials: ExternalServiceCredentials; snServiceUrl: string; - connectorTokenClient?: ConnectorTokenClientContract; + connectorTokenClient: ConnectorTokenClientContract; } export const getAxiosInstance = ({ @@ -144,6 +144,20 @@ export const getAxiosInstance = ({ return Promise.reject(error); } ); + axiosInstance.interceptors.response.use( + (response: AxiosResponse) => response, + async (error) => { + const statusCode = error?.response?.status; + + // Look for 4xx errors that indicate something is wrong with the request + // We don't know for sure that it is an access token issue but remove saved + // token just to be sure + if (statusCode >= 400 && statusCode < 500) { + await connectorTokenClient.deleteConnectorTokens({ connectorId }); + } + return Promise.reject(error); + } + ); } return axiosInstance; From 3673c23672e3d59a2f925381a54a6f3103e3cae1 Mon Sep 17 00:00:00 2001 From: Ying Mao Date: Wed, 11 May 2022 08:55:35 -0400 Subject: [PATCH 5/8] Adding test for tokenRequestDate --- .../lib/connector_token_client.test.ts | 61 ++++++++++++++++++- 1 file changed, 58 insertions(+), 3 deletions(-) diff --git a/x-pack/plugins/actions/server/builtin_action_types/lib/connector_token_client.test.ts b/x-pack/plugins/actions/server/builtin_action_types/lib/connector_token_client.test.ts index 74dae360980a7..dfa307ca3cd91 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/lib/connector_token_client.test.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/lib/connector_token_client.test.ts @@ -5,6 +5,7 @@ * 2.0. */ +import sinon from 'sinon'; import { loggingSystemMock, savedObjectsClientMock } from '@kbn/core/server/mocks'; import { encryptedSavedObjectsMock } from '@kbn/encrypted-saved-objects-plugin/server/mocks'; import { ConnectorTokenClient } from './connector_token_client'; @@ -23,7 +24,13 @@ const encryptedSavedObjectsClient = encryptedSavedObjectsMock.createClient(); let connectorTokenClient: ConnectorTokenClient; +let clock: sinon.SinonFakeTimers; + +beforeAll(() => { + clock = sinon.useFakeTimers(new Date('2021-01-01T12:00:00.000Z')); +}); beforeEach(() => { + clock.reset(); jest.resetAllMocks(); connectorTokenClient = new ConnectorTokenClient({ unsecuredSavedObjectsClient, @@ -31,6 +38,7 @@ beforeEach(() => { logger, }); }); +afterAll(() => clock.restore()); describe('create()', () => { test('creates connector_token with all given properties', async () => { @@ -416,13 +424,60 @@ describe('updateOrReplace()', () => { connectorId: '1', token: null, newToken: 'newToken', - tokenRequestDate: Date.now(), + tokenRequestDate: undefined as unknown as number, expiresInSec: 1000, deleteExisting: false, }); expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(1); - expect((unsecuredSavedObjectsClient.create.mock.calls[0][1] as ConnectorToken).token).toBe( - 'newToken' + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledWith( + 'connector_token', + { + connectorId: '1', + createdAt: '2021-01-01T12:00:00.000Z', + expiresAt: '2021-01-01T12:16:40.000Z', + token: 'newToken', + tokenType: 'access_token', + updatedAt: '2021-01-01T12:00:00.000Z', + }, + { id: 'mock-saved-object-id' } + ); + + expect(unsecuredSavedObjectsClient.find).not.toHaveBeenCalled(); + expect(unsecuredSavedObjectsClient.delete).not.toHaveBeenCalled(); + }); + + test('uses tokenRequestDate to determine expire time if provided', async () => { + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ + id: '1', + type: 'connector_token', + attributes: { + connectorId: '123', + tokenType: 'access_token', + token: 'testtokenvalue', + expiresAt: new Date('2021-01-01T08:00:00.000Z').toISOString(), + }, + references: [], + }); + await connectorTokenClient.updateOrReplace({ + connectorId: '1', + token: null, + newToken: 'newToken', + tokenRequestDate: new Date('2021-03-03T00:00:00.000Z').getTime(), + expiresInSec: 1000, + deleteExisting: false, + }); + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(1); + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledWith( + 'connector_token', + { + connectorId: '1', + createdAt: '2021-01-01T12:00:00.000Z', + expiresAt: '2021-03-03T00:16:40.000Z', + token: 'newToken', + tokenType: 'access_token', + updatedAt: '2021-01-01T12:00:00.000Z', + }, + { id: 'mock-saved-object-id' } ); expect(unsecuredSavedObjectsClient.find).not.toHaveBeenCalled(); From 603a206e1760a55d5066e2ddfad85ecba2e2f48b Mon Sep 17 00:00:00 2001 From: Ying Mao Date: Wed, 11 May 2022 12:16:55 -0400 Subject: [PATCH 6/8] Handling 4xx errors in the response --- .../server/builtin_action_types/lib/send_email.ts | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/actions/server/builtin_action_types/lib/send_email.ts b/x-pack/plugins/actions/server/builtin_action_types/lib/send_email.ts index db1151b6c7bac..2fee4dd8b377d 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/lib/send_email.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/lib/send_email.ts @@ -116,7 +116,15 @@ export async function sendEmailWithExchange( const axiosInstance = axios.create(); axiosInstance.interceptors.response.use( - (response: AxiosResponse) => response, + async (response: AxiosResponse) => { + // Look for 4xx errors that indicate something is wrong with the request + // We don't know for sure that it is an access token issue but remove saved + // token just to be sure + if (response.status >= 400 && response.status < 500) { + await connectorTokenClient.deleteConnectorTokens({ connectorId }); + } + return response; + }, async (error) => { const statusCode = error?.response?.status; @@ -138,7 +146,8 @@ export async function sendEmailWithExchange( graphApiUrl: configurationUtilities.getMicrosoftGraphApiUrl(), }, logger, - configurationUtilities + configurationUtilities, + axiosInstance ); } From a9f3c34710d98078a869bdede949d0889f53ae5e Mon Sep 17 00:00:00 2001 From: Ying Mao Date: Wed, 11 May 2022 12:24:12 -0400 Subject: [PATCH 7/8] Fixing unit tests --- .../actions/server/builtin_action_types/lib/send_email.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/x-pack/plugins/actions/server/builtin_action_types/lib/send_email.test.ts b/x-pack/plugins/actions/server/builtin_action_types/lib/send_email.test.ts index 5c7831832b926..fe6fc3492492a 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/lib/send_email.test.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/lib/send_email.test.ts @@ -132,6 +132,7 @@ describe('send_email module', () => { delete sendEmailGraphApiMock.mock.calls[0][0].options.configurationUtilities; sendEmailGraphApiMock.mock.calls[0].pop(); + sendEmailGraphApiMock.mock.calls[0].pop(); expect(sendEmailGraphApiMock.mock.calls[0]).toMatchInlineSnapshot(` Array [ Object { From ab56dc56952ff80fb21cedf73efec377ab6d19f9 Mon Sep 17 00:00:00 2001 From: Ying Mao Date: Thu, 12 May 2022 11:27:32 -0400 Subject: [PATCH 8/8] Fixing types --- .../actions/server/builtin_action_types/servicenow/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/actions/server/builtin_action_types/servicenow/utils.ts b/x-pack/plugins/actions/server/builtin_action_types/servicenow/utils.ts index 42fb96dd76ade..92fd13d86e608 100644 --- a/x-pack/plugins/actions/server/builtin_action_types/servicenow/utils.ts +++ b/x-pack/plugins/actions/server/builtin_action_types/servicenow/utils.ts @@ -137,7 +137,7 @@ export const getAxiosInstance = ({ if (!accessToken) { throw new Error(`Unable to retrieve access token for connectorId: ${connectorId}`); } - axiosConfig.headers.Authorization = accessToken; + axiosConfig.headers = { ...axiosConfig.headers, Authorization: accessToken }; return axiosConfig; }, (error) => {