Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SDK-1279] getTokenSilently retry logic #336

Merged
merged 12 commits into from
Feb 17, 2020
11 changes: 9 additions & 2 deletions __tests__/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,15 @@ import createAuth0Client, {
PopupConfigOptions,
GetTokenSilentlyOptions
} from '../src/index';

import { AuthenticationError } from '../src/errors';
import version from '../src/version';

import {
DEFAULT_AUTHORIZE_TIMEOUT_IN_SECONDS,
DEFAULT_FETCH_TIMEOUT_MS
} from '../src/constants';

const GET_TOKEN_SILENTLY_LOCK_KEY = 'auth0.lock.getTokenSilently';

const TEST_DOMAIN = 'test.auth0.com';
Expand All @@ -37,7 +44,7 @@ const TEST_TELEMETRY_QUERY_STRING = `&auth0Client=${encodeURIComponent(
)
)}`;

const DEFAULT_POPUP_CONFIG_OPTIONS: PopupConfigOptions = {};
import { DEFAULT_POPUP_CONFIG_OPTIONS } from '../src/constants';

const mockEnclosedCache = {
get: jest.fn(),
Expand Down Expand Up @@ -280,7 +287,7 @@ describe('Auth0', () => {
const { auth0, utils } = await setup({ authorizeTimeoutInSeconds: 1 });
const popup = {};
utils.openPopup.mockReturnValue(popup);
await auth0.loginWithPopup({}, DEFAULT_POPUP_CONFIG_OPTIONS);
await auth0.loginWithPopup({});
expect(utils.runPopup).toHaveBeenCalledWith(
popup,
`https://test.auth0.com/authorize?${TEST_QUERY_PARAMS}${TEST_TELEMETRY_QUERY_STRING}`,
Expand Down
120 changes: 119 additions & 1 deletion __tests__/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,12 @@ import {
getCryptoSubtle,
validateCrypto
} from '../src/utils';
import { DEFAULT_AUTHORIZE_TIMEOUT_IN_SECONDS } from '../src/constants';

import {
DEFAULT_AUTHORIZE_TIMEOUT_IN_SECONDS,
DEFAULT_SILENT_TOKEN_RETRY_COUNT
} from '../src/constants';
import { ExportConverter } from 'typedoc/dist/lib/converter/nodes';

(<any>global).TextEncoder = TextEncoder;

Expand Down Expand Up @@ -254,15 +259,18 @@ describe('utils', () => {
expect(openPopup).toThrowError('Could not open popup');
});
});

describe('oauthToken', () => {
let oauthToken;
let mockUnfetch;

beforeEach(() => {
jest.resetModules();
jest.mock('unfetch');
mockUnfetch = require('unfetch');
oauthToken = require('../src/utils').oauthToken;
});

it('calls oauth/token with the correct url', async () => {
mockUnfetch.mockReturnValue(
new Promise(res =>
Expand All @@ -284,11 +292,13 @@ describe('utils', () => {
method: 'POST'
});
});

it('handles error with error response', async () => {
const theError = {
error: 'the-error',
error_description: 'the-error-description'
};

mockUnfetch.mockReturnValue(
new Promise(res =>
res({
Expand All @@ -297,6 +307,7 @@ describe('utils', () => {
})
)
);

try {
await oauthToken({
baseUrl: 'https://test.com',
Expand All @@ -310,6 +321,7 @@ describe('utils', () => {
expect(error.error_description).toBe(theError.error_description);
}
});

it('handles error without error response', async () => {
mockUnfetch.mockReturnValue(
new Promise(res =>
Expand All @@ -319,6 +331,7 @@ describe('utils', () => {
})
)
);

try {
await oauthToken({
baseUrl: 'https://test.com',
Expand All @@ -336,7 +349,112 @@ describe('utils', () => {
);
}
});

it('retries the request in the event of a network failure', async () => {
// Fetch only fails in the case of a network issue, so should be
// retried here. Failure status (4xx, 5xx, etc) return a resolved Promise
// with the failure in the body.
// https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API
mockUnfetch.mockReturnValue(Promise.reject(new Error('Network failure')));

try {
await oauthToken({
baseUrl: 'https://test.com',
client_id: 'client_idIn',
code: 'codeIn',
code_verifier: 'code_verifierIn'
});
} catch (error) {
expect(error.message).toBe('Network failure');

expect(mockUnfetch).toHaveBeenCalledTimes(
DEFAULT_SILENT_TOKEN_RETRY_COUNT
);
}
});

it('continues the program after failing a couple of times then succeeding', async () => {
// Fetch only fails in the case of a network issue, so should be
// retried here. Failure status (4xx, 5xx, etc) return a resolved Promise
// with the failure in the body.
// https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API
mockUnfetch
.mockReturnValueOnce(Promise.reject(new Error('Network failure')))
.mockReturnValueOnce(Promise.reject(new Error('Network failure')))
.mockReturnValue(
Promise.resolve({
ok: true,
json: () => Promise.resolve({ access_token: 'access-token' })
})
);

const result = await oauthToken({
baseUrl: 'https://test.com',
client_id: 'client_idIn',
code: 'codeIn',
code_verifier: 'code_verifierIn'
});

expect(result.access_token).toBe('access-token');
expect(mockUnfetch).toHaveBeenCalledTimes(3);
});

it('surfaces a timeout error when the fetch continuously times out', async () => {
const createPromise = () =>
new Promise((resolve, _) => {
setTimeout(
() =>
resolve({
ok: true,
json: () => Promise.resolve({ access_token: 'access-token' })
}),
500
);
});

mockUnfetch.mockReturnValue(createPromise());

try {
await oauthToken({
baseUrl: 'https://test.com',
client_id: 'client_idIn',
code: 'codeIn',
code_verifier: 'code_verifierIn',
timeout: 100
});
} catch (e) {
expect(e.message).toBe("Timeout when executing 'fetch'");
expect(mockUnfetch).toHaveBeenCalledTimes(3);
}
});

it('retries the request in the event of a timeout', async () => {
const fetchResult = {
ok: true,
json: () => Promise.resolve({ access_token: 'access-token' })
};

mockUnfetch.mockReturnValueOnce(
new Promise((resolve, _) => {
setTimeout(() => resolve(fetchResult), 1000);
})
);

mockUnfetch.mockReturnValue(Promise.resolve(fetchResult));

const result = await oauthToken({
baseUrl: 'https://test.com',
client_id: 'client_idIn',
code: 'codeIn',
code_verifier: 'code_verifierIn',
timeout: 500
});

expect(result.access_token).toBe('access-token');
expect(mockUnfetch).toHaveBeenCalledTimes(2);
});
});

describe('runPopup', () => {
const TIMEOUT_ERROR = { error: 'timeout', error_description: 'Timeout' };
const setup = customMessage => {
Expand Down
2 changes: 1 addition & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 8 additions & 3 deletions src/Auth0Client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@ import TransactionManager from './transaction-manager';
import { verify as verifyIdToken } from './jwt';
import { AuthenticationError, GenericError } from './errors';
import * as ClientStorage from './storage';
import { DEFAULT_POPUP_CONFIG_OPTIONS } from './constants';
import {
DEFAULT_POPUP_CONFIG_OPTIONS,
DEFAULT_AUTHORIZE_TIMEOUT_IN_SECONDS
} from './constants';
import version from './version';
import {
Auth0ClientOptions,
Expand Down Expand Up @@ -221,7 +224,7 @@ export default class Auth0Client {
*/
public async loginWithPopup(
options: PopupLoginOptions = {},
config: PopupConfigOptions = DEFAULT_POPUP_CONFIG_OPTIONS
config: PopupConfigOptions = {}
) {
const popup = await openPopup();
const { ...authorizeOptions } = options;
Expand All @@ -247,7 +250,9 @@ export default class Auth0Client {
const codeResult = await runPopup(popup, url, {
...config,
timeoutInSeconds:
config.timeoutInSeconds || this.options.authorizeTimeoutInSeconds
config.timeoutInSeconds ||
this.options.authorizeTimeoutInSeconds ||
DEFAULT_AUTHORIZE_TIMEOUT_IN_SECONDS
});

if (stateIn !== codeResult.state) {
Expand Down
15 changes: 14 additions & 1 deletion src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,17 @@ export const DEFAULT_AUTHORIZE_TIMEOUT_IN_SECONDS = 60;
/**
* @ignore
*/
export const DEFAULT_POPUP_CONFIG_OPTIONS: PopupConfigOptions = {};

export const DEFAULT_POPUP_CONFIG_OPTIONS: PopupConfigOptions = {
timeoutInSeconds: DEFAULT_AUTHORIZE_TIMEOUT_IN_SECONDS
};

/**
* @ignore
*/
export const DEFAULT_SILENT_TOKEN_RETRY_COUNT = 3;

/**
* @ignore
*/
export const DEFAULT_FETCH_TIMEOUT_MS = 10000;
1 change: 1 addition & 0 deletions src/global.ts
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,7 @@ export interface TokenEndpointOptions {
baseUrl: string;
client_id: string;
grant_type: string;
timeout?: number;
}

/**
Expand Down
51 changes: 47 additions & 4 deletions src/utils.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,21 @@
import fetch from 'unfetch';

import { DEFAULT_AUTHORIZE_TIMEOUT_IN_SECONDS } from './constants';
import {
AuthenticationResult,
PopupConfigOptions,
TokenEndpointOptions
} from './global';

import {
DEFAULT_AUTHORIZE_TIMEOUT_IN_SECONDS,
DEFAULT_SILENT_TOKEN_RETRY_COUNT,
DEFAULT_FETCH_TIMEOUT_MS
} from './constants';

const dedupe = arr => arr.filter((x, i) => arr.indexOf(x) === i);

const TIMEOUT_ERROR = { error: 'timeout', error_description: 'Timeout' };

export const getUniqueScopes = (...scopes: string[]) => {
const scopeString = scopes.filter(Boolean).join();
return dedupe(scopeString.replace(/\s/g, ',').split(','))
Expand Down Expand Up @@ -182,25 +188,62 @@ export const bufferToBase64UrlEncoded = input => {
);
};

const getJSON = async (url, options) => {
const response = await fetch(url, options);
const fetchWithTimeout = (url, options, timeout = DEFAULT_FETCH_TIMEOUT_MS) => {
// The promise will resolve with one of these two promises (the fetch and the timeout), whichever completes first.
return Promise.race([
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Promise.race ... handy!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this ends the fetch() process so that's not being tried in the background while the app continues?

fetch(url, options),
new Promise((_, reject) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure this works for rejecting the promise early and eventually retrying. But to me, it feels like the timeout should be a feature/option of the node-fetch library. Because, if timeout kicks in, the request should be correctly canceled and that's only something the client can do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are perhaps correct. However, we don't use node-fetch here, it's just the browsers Fetch API (polyfilled using promise-polyfill and that has no protocol whatsoever for requests timing out. It will wait indefinitely for a response or until the connection drops.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then, do we get a way to cancel that fetch(url, options) promise gracefully?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I've done some research into this and we can cancel fetch through the use of AbortController. However, it has no support on IE11 and thus would require a polyfill and would bump the size of the package.

Let me investigate how big that bump would be and come back to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lbalmaceda I've implemented AbortController now and added a polyfill for IE11.

The polyfill bumps the distributed package size up to 16.55kb, a 12.4% increase.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't seem like a huge increment, but I'd leave that to your discretion. Polyfill Q: Is there any way of only importing it on IE or is this something that if added, must be added in a general way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it's all bundled, we have to add it in a generic way.

setTimeout(
() => reject(new Error("Timeout when executing 'fetch'")),
timeout
);
})
]);
};

const getJSON = async (url, timeout, options) => {
let fetchError, response;

for (let i = 0; i < DEFAULT_SILENT_TOKEN_RETRY_COUNT; i++) {
lbalmaceda marked this conversation as resolved.
Show resolved Hide resolved
try {
response = await fetchWithTimeout(url, options, timeout);
fetchError = null;
break;
} catch (e) {
// Fetch only fails in the case of a network issue, so should be
// retried here. Failure status (4xx, 5xx, etc) return a resolved Promise
// with the failure in the body.
// https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API
fetchError = e;
}
}

if (fetchError) {
throw fetchError;
}

const { error, error_description, ...success } = await response.json();

if (!response.ok) {
const errorMessage =
error_description || `HTTP error. Unable to fetch ${url}`;
const e = <any>new Error(errorMessage);

e.error = error || 'request_error';
e.error_description = errorMessage;

throw e;
}

return success;
};

export const oauthToken = async ({
baseUrl,
timeout,
...options
}: TokenEndpointOptions) =>
await getJSON(`${baseUrl}/oauth/token`, {
await getJSON(`${baseUrl}/oauth/token`, timeout, {
method: 'POST',
body: JSON.stringify({
redirect_uri: window.location.origin,
Expand Down