From 968e8c0fa44f45599b11cccc8a5004b0529c324f Mon Sep 17 00:00:00 2001 From: adamjmcgrath Date: Wed, 26 Aug 2020 10:44:52 +0100 Subject: [PATCH] Check the cache before acquiring lock to avoid latency --- __mocks__/browser-tabs-lock/index.ts | 18 +++++--- __tests__/Auth0Client.test.ts | 29 ++++++++++++- __tests__/index.test.ts | 65 +++++++++++++++------------- src/Auth0Client.ts | 40 +++++++++++------ 4 files changed, 101 insertions(+), 51 deletions(-) diff --git a/__mocks__/browser-tabs-lock/index.ts b/__mocks__/browser-tabs-lock/index.ts index ea23563c6..be5dfc513 100644 --- a/__mocks__/browser-tabs-lock/index.ts +++ b/__mocks__/browser-tabs-lock/index.ts @@ -1,7 +1,15 @@ -export const acquireLockMock = jest.fn(); -export const releaseLockMock = jest.fn(); +const Lock = jest.requireActual('browser-tabs-lock').default; -export default class FakeLock { - acquireLock = acquireLockMock; - releaseLock = releaseLockMock; +export const acquireLockSpy = jest.fn(); +export const releaseLockSpy = jest.fn(); + +export default class extends Lock { + acquireLock(...args) { + acquireLockSpy(...args); + return super.acquireLock(...args); + } + releaseLock(...args) { + releaseLockSpy(...args); + return super.releaseLock(...args); + } } diff --git a/__tests__/Auth0Client.test.ts b/__tests__/Auth0Client.test.ts index c5bd7cdce..4bc083027 100644 --- a/__tests__/Auth0Client.test.ts +++ b/__tests__/Auth0Client.test.ts @@ -8,14 +8,14 @@ import * as utils from '../src/utils'; import { Auth0ClientOptions, IdToken } from '../src'; import * as scope from '../src/scope'; import { expectToHaveBeenCalledWithAuth0ClientParam } from './helpers'; +// @ts-ignore +import { acquireLockSpy } from 'browser-tabs-lock'; jest.mock('unfetch'); jest.mock('es-cookie'); jest.mock('../src/jwt'); jest.mock('../src/token.worker'); -jest.unmock('browser-tabs-lock'); - const mockWindow = global; const mockFetch = (mockWindow.fetch = unfetch); const mockVerify = verify; @@ -684,6 +684,31 @@ describe('Auth0Client', () => { expect(utils.runIframe).not.toHaveBeenCalled(); }); + it('should not acquire a browser lock when cache is populated', async () => { + const auth0 = setup(); + await login(auth0); + jest.spyOn(utils, 'runIframe').mockResolvedValue({ + access_token: 'my_access_token', + state: 'MTIz' + }); + mockFetch.mockResolvedValue( + fetchResponse(true, { + id_token: 'my_id_token', + refresh_token: 'my_refresh_token', + access_token: 'my_access_token', + expires_in: 86400 + }) + ); + let access_token = await auth0.getTokenSilently({ audience: 'foo' }); + expect(access_token).toEqual('my_access_token'); + expect(acquireLockSpy).toHaveBeenCalled(); + acquireLockSpy.mockClear(); + // This request will hit the cache, so should not acquire the lock + access_token = await auth0.getTokenSilently({ audience: 'foo' }); + expect(access_token).toEqual('my_access_token'); + expect(acquireLockSpy).not.toHaveBeenCalled(); + }); + it('sends custom options through to the token endpoint when using an iframe', async () => { const auth0 = setup({ custom_param: 'foo', diff --git a/__tests__/index.test.ts b/__tests__/index.test.ts index 912470950..dcf8db875 100644 --- a/__tests__/index.test.ts +++ b/__tests__/index.test.ts @@ -1,13 +1,14 @@ -jest.mock('browser-tabs-lock'); +import { expectToHaveBeenCalledWithAuth0ClientParam } from './helpers'; +import { CacheLocation, Auth0ClientOptions } from '../src/global'; +import * as scope from '../src/scope'; +// @ts-ignore +import { acquireLockSpy, releaseLockSpy } from 'browser-tabs-lock'; + jest.mock('../src/jwt'); jest.mock('../src/storage'); jest.mock('../src/transaction-manager'); jest.mock('../src/utils'); -import { expectToHaveBeenCalledWithAuth0ClientParam } from './helpers'; -import { CacheLocation, Auth0ClientOptions } from '../src/global'; -import * as scope from '../src/scope'; - import createAuth0Client, { Auth0Client, GetTokenSilentlyOptions @@ -64,12 +65,6 @@ const webWorkerMatcher = expect.objectContaining({ }); const setup = async (clientOptions: Partial = {}) => { - const auth0 = await createAuth0Client({ - domain: TEST_DOMAIN, - client_id: TEST_CLIENT_ID, - ...clientOptions - }); - const getDefaultInstance = m => require(m).default.mock.instances[0]; const storage = { @@ -78,11 +73,9 @@ const setup = async (clientOptions: Partial = {}) => { remove: require('../src/storage').remove }; - const lock = require('browser-tabs-lock'); const cache = mockEnclosedCache; const tokenVerifier = require('../src/jwt').verify; - const transactionManager = getDefaultInstance('../src/transaction-manager'); const utils = require('../src/utils'); utils.createQueryParams.mockReturnValue(TEST_QUERY_PARAMS); @@ -126,6 +119,13 @@ const setup = async (clientOptions: Partial = {}) => { close: jest.fn() }; + const auth0 = await createAuth0Client({ + domain: TEST_DOMAIN, + client_id: TEST_CLIENT_ID, + ...clientOptions + }); + const transactionManager = getDefaultInstance('../src/transaction-manager'); + return { auth0, storage, @@ -133,7 +133,6 @@ const setup = async (clientOptions: Partial = {}) => { tokenVerifier, transactionManager, utils, - lock, popup }; }; @@ -155,7 +154,7 @@ describe('Auth0', () => { }); afterEach(() => { - jest.resetAllMocks(); + jest.clearAllMocks(); getUniqueScopesSpy.mockRestore(); }); @@ -216,6 +215,7 @@ describe('Auth0', () => { 'access_denied' ]; for (let error of recoverableErrors) { + utils.runIframe.mockClear(); utils.runIframe.mockRejectedValue({ error }); const auth0 = await createAuth0Client({ domain: TEST_DOMAIN, @@ -223,7 +223,6 @@ describe('Auth0', () => { }); expect(auth0).toBeInstanceOf(Auth0Client); expect(utils.runIframe).toHaveBeenCalledTimes(1); - utils.runIframe.mockClear(); } }); @@ -1527,7 +1526,8 @@ describe('Auth0', () => { }); it('continues method execution when there is no cache available', async () => { - const { auth0, utils } = await setup(); + const { auth0, utils, cache } = await setup(); + cache.get.mockReturnValue(undefined); await auth0.getTokenSilently(); @@ -1718,18 +1718,20 @@ describe('Auth0', () => { }; it('releases the lock when there is an error', async () => { - const { auth0, lock, utils } = await setup(); + const { auth0, utils } = await setup(); - utils.runIframe.mockReturnValue(Promise.reject(new Error('Failed'))); + utils.runIframe.mockImplementation(() => + Promise.reject(new Error('Failed')) + ); await expect(auth0.getTokenSilently()).rejects.toThrowError('Failed'); - expect(lock.acquireLockMock).toHaveBeenCalledWith( + expect(acquireLockSpy).toHaveBeenCalledWith( GET_TOKEN_SILENTLY_LOCK_KEY, 5000 ); - expect(lock.releaseLockMock).toHaveBeenCalledWith( + expect(releaseLockSpy).toHaveBeenCalledWith( GET_TOKEN_SILENTLY_LOCK_KEY ); }); @@ -1892,7 +1894,7 @@ describe('Auth0', () => { }); it('throws error if state from popup response is different from the provided state', async () => { - const { auth0, utils, lock } = await setup(); + const { auth0, utils } = await setup(); utils.runIframe.mockReturnValue( Promise.resolve({ @@ -1904,7 +1906,7 @@ describe('Auth0', () => { auth0.getTokenSilently(defaultOptionsIgnoreCacheTrue) ).rejects.toThrowError('Invalid state'); - expect(lock.releaseLockMock).toHaveBeenCalledWith( + expect(releaseLockSpy).toHaveBeenCalledWith( GET_TOKEN_SILENTLY_LOCK_KEY ); }); @@ -1969,15 +1971,15 @@ describe('Auth0', () => { ); }); it('acquires and releases lock', async () => { - const { auth0, lock } = await setup(); + const { auth0 } = await setup(); await auth0.getTokenSilently(defaultOptionsIgnoreCacheTrue); - expect(lock.acquireLockMock).toHaveBeenCalledWith( + expect(acquireLockSpy).toHaveBeenCalledWith( GET_TOKEN_SILENTLY_LOCK_KEY, 5000 ); - expect(lock.releaseLockMock).toHaveBeenCalledWith( + expect(releaseLockSpy).toHaveBeenCalledWith( GET_TOKEN_SILENTLY_LOCK_KEY ); }); @@ -2179,17 +2181,18 @@ describe('Auth0', () => { }); describe('default creation function', () => { - it('does nothing if there is nothing storage', async () => { - Auth0Client.prototype.getTokenSilently = jest.fn(); + it('does nothing if there is nothing in storage', async () => { + jest.spyOn(Auth0Client.prototype, 'getTokenSilently'); + const getSpy = jest + .spyOn(require('../src/storage'), 'get') + .mockReturnValueOnce(false); const auth0 = await createAuth0Client({ domain: TEST_DOMAIN, client_id: TEST_CLIENT_ID }); - expect(require('../src/storage').get).toHaveBeenCalledWith( - 'auth0.is.authenticated' - ); + expect(getSpy).toHaveBeenCalledWith('auth0.is.authenticated'); expect(auth0.getTokenSilently).not.toHaveBeenCalled(); }); diff --git a/src/Auth0Client.ts b/src/Auth0Client.ts index fa965afd1..e1fb7641c 100644 --- a/src/Auth0Client.ts +++ b/src/Auth0Client.ts @@ -582,22 +582,36 @@ export default class Auth0Client { scope: getUniqueScopes(this.defaultScope, this.scope, options.scope) }; + const getAccessTokenFromCache = () => { + const cache = this.cache.get( + { + scope: getTokenOptions.scope, + audience: getTokenOptions.audience || 'default', + client_id: this.options.client_id + }, + 60 // get a new token if within 60 seconds of expiring + ); + + return cache && cache.access_token; + }; + + // Check the cache before acquiring the lock to avoid the latency of + // `lock.acquireLock` when the cache is populated. + if (!ignoreCache) { + let accessToken = getAccessTokenFromCache(); + if (accessToken) { + return accessToken; + } + } + try { await lock.acquireLock(GET_TOKEN_SILENTLY_LOCK_KEY, 5000); - + // Check the cache a second time, because it may have been populated + // by a previous call while this call was waiting to acquire the lock. if (!ignoreCache) { - const cache = this.cache.get( - { - scope: getTokenOptions.scope, - audience: getTokenOptions.audience || 'default', - client_id: this.options.client_id - }, - 60 // get a new token if within 60 seconds of expiring - ); - - if (cache && cache.access_token) { - await lock.releaseLock(GET_TOKEN_SILENTLY_LOCK_KEY); - return cache.access_token; + let accessToken = getAccessTokenFromCache(); + if (accessToken) { + return accessToken; } }