Skip to content

Commit

Permalink
[Connectors] ConnectorTokenClient improvements (#131955)
Browse files Browse the repository at this point in the history 
* Throwing error if service now access token is null. Properly returning rejected promise

* Setting time to calculate token expiration to before the token is created

* Returning null access token if stored access token has invalid expiresAt date

* Adding response interceptor to delete connector token if using access token returns 4xx error

* Adding test for tokenRequestDate

* Handling 4xx errors in the response

* Fixing unit tests

* Fixing types
  • Loading branch information
@ymao1
ymao1 authored May 12, 2022
1 parent e9b1d38 commit b932dca
Show file tree
Hide file tree
Showing 11 changed files with 490 additions and 65 deletions.
107 changes: 103 additions & 4 deletions x-pack/plugins/actions/server/builtin_action_types/lib/connector_token_client.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -23,14 +24,21 @@ 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,
encryptedSavedObjectsClient,
logger,
});
});
afterAll(() => clock.restore());

describe('create()', () => {
test('creates connector_token with all given properties', async () => {
Expand Down Expand Up @@ -131,7 +139,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({
Expand All @@ -145,7 +153,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,
Expand Down Expand Up @@ -178,6 +186,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()', () => {
Expand Down Expand Up @@ -375,12 +424,60 @@ describe('updateOrReplace()', () => {
connectorId: '1',
token: null,
newToken: 'newToken',
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();
Expand Down Expand Up @@ -434,6 +531,7 @@ describe('updateOrReplace()', () => {
connectorId: '1',
token: null,
newToken: 'newToken',
tokenRequestDate: Date.now(),
expiresInSec: 1000,
deleteExisting: true,
});
Expand Down Expand Up @@ -483,6 +581,7 @@ describe('updateOrReplace()', () => {
expiresAt: new Date().toISOString(),
},
newToken: 'newToken',
tokenRequestDate: Date.now(),
expiresInSec: 1000,
deleteExisting: true,
});
Expand Down
35 changes: 25 additions & 10 deletions x-pack/plugins/actions/server/builtin_action_types/lib/connector_token_client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ interface UpdateOrReplaceOptions {
token: ConnectorToken | null;
newToken: string;
expiresInSec: number;
tokenRequestDate: number;
deleteExisting: boolean;
}
export class ConnectorTokenClient {
Expand Down Expand Up @@ -195,6 +196,7 @@ export class ConnectorTokenClient {
return { hasErrors: false, connectorToken: null };
}

let accessToken: string;
try {
const {
attributes: { token },
Expand All @@ -203,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: "${
Expand All @@ -219,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,
},
};
}

/**
Expand Down Expand Up @@ -258,9 +271,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({
Expand All @@ -272,14 +287,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',
});
}
Expand Down
21 changes: 17 additions & 4 deletions ...actions/server/builtin_action_types/lib/get_oauth_client_credentials_access_token.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -20,7 +21,15 @@ const logger = loggingSystemMock.create().get() as jest.Mocked<Logger>;
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,
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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: {
Expand Down Expand Up @@ -147,6 +157,7 @@ describe('getOAuthClientCredentialsAccessToken', () => {
expiresAt,
},
newToken: 'access_token brandnewaccesstoken',
tokenRequestDate: 1609502400000,
expiresInSec: 1000,
deleteExisting: false,
});
Expand Down Expand Up @@ -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'));
Expand Down Expand Up @@ -268,6 +280,7 @@ describe('getOAuthClientCredentialsAccessToken', () => {
(requestOAuthClientCredentialsToken as jest.Mock).mockResolvedValueOnce({
tokenType: 'access_token',
accessToken: 'brandnewaccesstoken',
tokenRequestDate: 1609502400000,
expiresIn: 1000,
});

Expand Down
4 changes: 4 additions & 0 deletions ...gins/actions/server/builtin_action_types/lib/get_oauth_client_credentials_access_token.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -82,6 +85,7 @@ export const getOAuthClientCredentialsAccessToken = async ({
connectorId,
token: connectorToken,
newToken: accessToken,
tokenRequestDate: requestTokenStart,
expiresInSec: tokenResult.expiresIn,
deleteExisting: hasErrors,
});
Expand Down
Loading

0 comments on commit b932dca

Please sign in to comment.