Avoid the possibility to do simultaneous calls to the token endpoint (#…

…664)

* First attempt to avoid simultaneous calls

* Rework fix to not use a new class and static methods

* Add comments to matcher

* Fix locking

* Replace while with retry

* Move singlePromise and promiseRetry to promise-utils

* Add tests for promise utils

* Add concurrency test

* Add locking retry tests

* Remove unused variable

* Add client ID to the singlePromise key

* Rollback changes to index.html

* Update test for hitting the cache for subsequent requests that occur before the response

* Fix review comments

* Fix implicit any errors

__mocks__/browser-tabs-lock/index.ts

@@ -1,12 +1,14 @@
 const Lock = jest.requireActual('browser-tabs-lock').default;
 
-export const acquireLockSpy = jest.fn();
+export const acquireLockSpy = jest.fn().mockResolvedValue(true);
 export const releaseLockSpy = jest.fn();
 
 export default class extends Lock {
-  acquireLock(...args) {
-    acquireLockSpy(...args);
-    return super.acquireLock(...args);
+  async acquireLock(...args) {
+    const canProceed = await acquireLockSpy(...args);
+    if (canProceed) {
+      return super.acquireLock(...args);
+    }
   }
   releaseLock(...args) {
     releaseLockSpy(...args);

__tests__/Auth0Client/getTokenSilently.test.ts

@@ -4,6 +4,7 @@ import unfetch from 'unfetch';
 import { verify } from '../../src/jwt';
 import { MessageChannel } from 'worker_threads';
 import * as utils from '../../src/utils';
+import * as promiseUtils from '../../src/promise-utils';
 import * as scope from '../../src/scope';
 import * as api from '../../src/api';
 
@@ -98,6 +99,7 @@ describe('Auth0Client', () => {
 
   afterEach(() => {
     mockFetch.mockReset();
+    acquireLockSpy.mockResolvedValue(true);
     jest.clearAllMocks();
     window.location = oldWindowLocation;
   });
@@ -664,6 +666,68 @@ describe('Auth0Client', () => {
       );
     });
 
+    describe('concurrency', () => {
+      it('should call _getTokenSilently multiple times when no call in flight concurrently', async () => {
+        const client = setup();
+
+        jest.spyOn(<any>utils, 'runIframe').mockResolvedValue({
+          access_token: TEST_ACCESS_TOKEN,
+          state: TEST_STATE,
+          code: TEST_CODE
+        });
+
+        jest.spyOn(client as any, '_getTokenSilently');
+
+        await getTokenSilently(client);
+        await getTokenSilently(client);
+
+        expect(client['_getTokenSilently']).toHaveBeenCalledTimes(2);
+      });
+
+      it('should not call _getTokenSilently if a call is already in flight', async () => {
+        const client = setup();
+
+        jest.spyOn(<any>utils, 'runIframe').mockResolvedValue({
+          access_token: TEST_ACCESS_TOKEN,
+          state: TEST_STATE,
+          code: TEST_CODE
+        });
+
+        jest.spyOn(client as any, '_getTokenSilently');
+
+        const tokens = await Promise.all([
+          getTokenSilently(client),
+          getTokenSilently(client)
+        ]);
+
+        expect(client['_getTokenSilently']).toHaveBeenCalledTimes(1);
+        expect(tokens[0]).toEqual(tokens[1]);
+      });
+
+      it('should not call _getTokenSilently if a call is already in flight (cross instance)', async () => {
+        const client1 = setup();
+        const client2 = setup();
+
+        jest.spyOn(<any>utils, 'runIframe').mockResolvedValue({
+          access_token: TEST_ACCESS_TOKEN,
+          state: TEST_STATE,
+          code: TEST_CODE
+        });
+
+        jest.spyOn(client1 as any, '_getTokenSilently');
+        jest.spyOn(client2 as any, '_getTokenSilently');
+
+        const tokens = await Promise.all([
+          getTokenSilently(client1),
+          getTokenSilently(client2)
+        ]);
+
+        expect(client1['_getTokenSilently']).toHaveBeenCalledTimes(1);
+        expect(client2['_getTokenSilently']).not.toHaveBeenCalled();
+        expect(tokens[0]).toEqual(tokens[1]);
+      });
+    });
+
     it('handles fetch errors from the worker', async () => {
       const auth0 = setup({
         useRefreshTokens: true
@@ -916,27 +980,35 @@ describe('Auth0Client', () => {
     });
 
     it('uses the cache for subsequent requests that occur before the response', async () => {
-      const auth0 = setup();
-      await loginWithRedirect(auth0);
-      (auth0 as any).cache.clear();
-      jest.spyOn(<any>utils, 'runIframe').mockResolvedValue({
-        access_token: TEST_ACCESS_TOKEN,
-        state: TEST_STATE
-      });
-      mockFetch.mockResolvedValue(
-        fetchResponse(true, {
-          id_token: TEST_ID_TOKEN,
+      let singlePromiseSpy = jest
+        .spyOn(promiseUtils, 'singlePromise')
+        .mockImplementation(cb => cb());
+
+      try {
+        const auth0 = setup();
+        await loginWithRedirect(auth0);
+        (auth0 as any).cache.clear();
+        jest.spyOn(<any>utils, 'runIframe').mockResolvedValue({
           access_token: TEST_ACCESS_TOKEN,
-          expires_in: 86400
-        })
-      );
-      let [access_token] = await Promise.all([
-        auth0.getTokenSilently(),
-        auth0.getTokenSilently(),
-        auth0.getTokenSilently()
-      ]);
-      expect(access_token).toEqual(TEST_ACCESS_TOKEN);
-      expect(utils.runIframe).toHaveBeenCalledTimes(1);
+          state: TEST_STATE
+        });
+        mockFetch.mockResolvedValue(
+          fetchResponse(true, {
+            id_token: TEST_ID_TOKEN,
+            access_token: TEST_ACCESS_TOKEN,
+            expires_in: 86400
+          })
+        );
+        let [access_token] = await Promise.all([
+          auth0.getTokenSilently(),
+          auth0.getTokenSilently(),
+          auth0.getTokenSilently()
+        ]);
+        expect(access_token).toEqual(TEST_ACCESS_TOKEN);
+        expect(utils.runIframe).toHaveBeenCalledTimes(1);
+      } finally {
+        singlePromiseSpy.mockRestore();
+      }
     });
 
     it('uses the cache for multiple token requests with audience and scope', async () => {
@@ -1011,6 +1083,45 @@ describe('Auth0Client', () => {
       expect(releaseLockSpy).toHaveBeenCalledWith(GET_TOKEN_SILENTLY_LOCK_KEY);
     });
 
+    it('should retry acquiring a lock', async () => {
+      const auth0 = setup();
+
+      jest.spyOn(<any>utils, 'runIframe').mockResolvedValue({
+        access_token: TEST_ACCESS_TOKEN,
+        state: TEST_STATE
+      });
+
+      let i = 1;
+
+      acquireLockSpy.mockImplementation(() => {
+        if (i === 3) {
+          return Promise.resolve(true);
+        } else {
+          i++;
+          return Promise.resolve(false);
+        }
+      });
+
+      await getTokenSilently(auth0);
+
+      expect(acquireLockSpy).toHaveBeenCalledTimes(3);
+    });
+
+    it('should trow a Timeout error if it can not acquire a lock after retrying', async () => {
+      const auth0 = setup();
+
+      jest.spyOn(<any>utils, 'runIframe').mockResolvedValue({
+        access_token: TEST_ACCESS_TOKEN,
+        state: TEST_STATE
+      });
+
+      acquireLockSpy.mockResolvedValue(false);
+
+      await expect(getTokenSilently(auth0)).rejects.toThrow('Timeout');
+
+      expect(acquireLockSpy).toHaveBeenCalledTimes(10);
+    });
+
     it('should release a browser lock when an error occurred', async () => {
       const auth0 = setup();
       let error;

__tests__/promise-utils.test.ts

@@ -0,0 +1,76 @@
+/**
+ * @jest-environment node
+ */
+import { retryPromise, singlePromise } from '../src/promise-utils';
+
+describe('Promise Utils', () => {
+  describe('singlePromise', () => {
+    it('reuses the same promise when the key matches', async () => {
+      const cb = jest.fn().mockResolvedValue({});
+
+      await Promise.all([
+        singlePromise(cb as any, 'test-key'),
+        singlePromise(cb as any, 'test-key')
+      ]);
+
+      expect(cb).toHaveBeenCalledTimes(1);
+    });
+
+    it('does not reuse the same promise when the key is different', async () => {
+      const cb = jest.fn().mockResolvedValue({});
+
+      await Promise.all([
+        singlePromise(cb as any, 'test-key'),
+        singlePromise(cb as any, 'test-key2')
+      ]);
+
+      expect(cb).toHaveBeenCalledTimes(2);
+    });
+
+    it('does not reuse the same promise when the key matches but the first promise resolves before calling the second', async () => {
+      const cb = jest.fn().mockResolvedValue({});
+
+      await singlePromise(cb as any, 'test-key');
+      await singlePromise(cb as any, 'test-key');
+
+      expect(cb).toHaveBeenCalledTimes(2);
+    });
+  });
+
+  describe('retryPromise', () => {
+    it('does not retry promise when it resolves to true', async () => {
+      const cb = jest.fn().mockResolvedValue(true);
+
+      const value = await retryPromise(cb as any);
+
+      expect(value).toBe(true);
+      expect(cb).toHaveBeenCalledTimes(1);
+    });
+
+    it('retries promise until it resolves to true', async () => {
+      let i = 1;
+      const cb = jest.fn().mockImplementation(() => {
+        if (i === 3) {
+          return Promise.resolve(true);
+        }
+
+        i++;
+        return Promise.resolve(false);
+      });
+
+      const value = await retryPromise(cb as any);
+
+      expect(value).toBe(true);
+      expect(cb).toHaveBeenCalledTimes(3);
+    });
+
+    it('resolves to false when all retries resolve to false', async () => {
+      const cb = jest.fn().mockResolvedValue(false);
+
+      const value = await retryPromise(cb as any, 5);
+
+      expect(value).toBe(false);
+      expect(cb).toHaveBeenCalledTimes(5);
+    });
+  });
+});

src/Auth0Client.ts

@@ -18,7 +18,7 @@ import { getUniqueScopes } from './scope';
 import { InMemoryCache, ICache, LocalStorageCache } from './cache';
 import TransactionManager from './transaction-manager';
 import { verify as verifyIdToken } from './jwt';
-import { AuthenticationError } from './errors';
+import { AuthenticationError, TimeoutError } from './errors';
 
 import {
   ClientStorage,
@@ -61,6 +61,7 @@ import {
 // @ts-ignore
 import TokenWorker from './worker/token.worker.ts';
 import { isIE11, isSafari10, isSafari11, isSafari12_0 } from './user-agent';
+import { singlePromise, retryPromise } from './promise-utils';
 
 /**
  * @ignore
@@ -612,6 +613,19 @@ export default class Auth0Client {
       scope: getUniqueScopes(this.defaultScope, this.scope, options.scope)
     };
 
+    return singlePromise(
+      () =>
+        this._getTokenSilently({
+          ignoreCache,
+          ...getTokenOptions
+        }),
+      `${this.options.client_id}::${getTokenOptions.audience}::${getTokenOptions.scope}`
+    );
+  }
+
+  private async _getTokenSilently(options: GetTokenSilentlyOptions = {}) {
+    const { ignoreCache, ...getTokenOptions } = options;
+
     const getAccessTokenFromCache = () => {
       const cache = this.cache.get(
         {
@@ -634,32 +648,38 @@ export default class Auth0Client {
       }
     }
 
-    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) {
-        let accessToken = getAccessTokenFromCache();
-        if (accessToken) {
-          return accessToken;
+    if (
+      await retryPromise(
+        () => lock.acquireLock(GET_TOKEN_SILENTLY_LOCK_KEY, 5000),
+        10
+      )
+    ) {
+      try {
+        // 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) {
+          let accessToken = getAccessTokenFromCache();
+          if (accessToken) {
+            return accessToken;
+          }
         }
-      }
 
-      const authResult = this.options.useRefreshTokens
-        ? await this._getTokenUsingRefreshToken(getTokenOptions)
-        : await this._getTokenFromIFrame(getTokenOptions);
+        const authResult = this.options.useRefreshTokens
+          ? await this._getTokenUsingRefreshToken(getTokenOptions)
+          : await this._getTokenFromIFrame(getTokenOptions);
 
-      this.cache.save({ client_id: this.options.client_id, ...authResult });
+        this.cache.save({ client_id: this.options.client_id, ...authResult });
 
-      this.cookieStorage.save('auth0.is.authenticated', true, {
-        daysUntilExpire: this.sessionCheckExpiryDays
-      });
+        this.cookieStorage.save('auth0.is.authenticated', true, {
+          daysUntilExpire: this.sessionCheckExpiryDays
+        });
 
-      return authResult.access_token;
-    } catch (e) {
-      throw e;
-    } finally {
-      await lock.releaseLock(GET_TOKEN_SILENTLY_LOCK_KEY);
+        return authResult.access_token;
+      } finally {
+        await lock.releaseLock(GET_TOKEN_SILENTLY_LOCK_KEY);
+      }
+    } else {
+      throw new TimeoutError();
     }
   }
 

src/promise-utils.ts

@@ -0,0 +1,26 @@
+const singlePromiseMap: Record<string, Promise<any>> = {};
+
+export const singlePromise = <T>(cb: () => Promise<T>, key: string) => {
+  let promise = singlePromiseMap[key];
+  if (!promise) {
+    promise = cb().finally(() => {
+      delete singlePromiseMap[key];
+      promise = null;
+    });
+    singlePromiseMap[key] = promise;
+  }
+  return promise;
+};
+
+export const retryPromise = async (
+  cb: () => Promise<boolean>,
+  maxNumberOfRetries = 3
+) => {
+  for (let i = 0; i < maxNumberOfRetries; i++) {
+    if (await cb()) {
+      return true;
+    }
+  }
+
+  return false;
+};