From 23003d812dee349ea511881a31091f2a36c92d6b Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Thu, 21 Oct 2021 12:33:58 -0700 Subject: [PATCH 01/17] Initial setup --- packages/app-check/src/constants.ts | 5 ++ packages/app-check/src/errors.ts | 7 +- packages/app-check/src/internal-api.ts | 15 +++- packages/app-check/src/providers.ts | 101 +++++++++++++++++++++++-- packages/app-check/src/types.ts | 6 ++ 5 files changed, 122 insertions(+), 12 deletions(-) diff --git a/packages/app-check/src/constants.ts b/packages/app-check/src/constants.ts index ecb7fb49a87..dc8ef26275d 100644 --- a/packages/app-check/src/constants.ts +++ b/packages/app-check/src/constants.ts @@ -38,3 +38,8 @@ export const TOKEN_REFRESH_TIME = { */ RETRIAL_MAX_WAIT: 16 * 60 * 1000 }; + +/** + * One day in millis, for certain error code backoffs. + */ + export const ONE_DAY = 24 * 60 * 60 * 1000; \ No newline at end of file diff --git a/packages/app-check/src/errors.ts b/packages/app-check/src/errors.ts index 05324b639ee..1b23ea46c01 100644 --- a/packages/app-check/src/errors.ts +++ b/packages/app-check/src/errors.ts @@ -26,7 +26,8 @@ export const enum AppCheckError { STORAGE_OPEN = 'storage-open', STORAGE_GET = 'storage-get', STORAGE_WRITE = 'storage-set', - RECAPTCHA_ERROR = 'recaptcha-error' + RECAPTCHA_ERROR = 'recaptcha-error', + THROTTLED = 'throttled' } const ERRORS: ErrorMap = { @@ -52,7 +53,8 @@ const ERRORS: ErrorMap = { 'Error thrown when reading from storage. Original error: {$originalErrorMessage}.', [AppCheckError.STORAGE_WRITE]: 'Error thrown when writing to storage. Original error: {$originalErrorMessage}.', - [AppCheckError.RECAPTCHA_ERROR]: 'ReCAPTCHA error.' + [AppCheckError.RECAPTCHA_ERROR]: 'ReCAPTCHA error.', + [AppCheckError.THROTTLED]: `Requests throttled until {$time} due to {$httpStatus} error.` }; interface ErrorParams { @@ -64,6 +66,7 @@ interface ErrorParams { [AppCheckError.STORAGE_OPEN]: { originalErrorMessage?: string }; [AppCheckError.STORAGE_GET]: { originalErrorMessage?: string }; [AppCheckError.STORAGE_WRITE]: { originalErrorMessage?: string }; + [AppCheckError.THROTTLED]: { time: string, httpStatus: number }; } export const ERROR_FACTORY = new ErrorFactory( diff --git a/packages/app-check/src/internal-api.ts b/packages/app-check/src/internal-api.ts index fee45d72b12..63d77a19392 100644 --- a/packages/app-check/src/internal-api.ts +++ b/packages/app-check/src/internal-api.ts @@ -30,9 +30,10 @@ import { ensureActivated } from './util'; import { exchangeToken, getExchangeDebugTokenRequest } from './client'; import { writeTokenToStorage } from './storage'; import { getDebugToken, isDebugMode } from './debug'; -import { base64 } from '@firebase/util'; +import { base64, FirebaseError } from '@firebase/util'; import { logger } from './logger'; import { AppCheckService } from './factory'; +import { AppCheckError } from './errors'; // Initial hardcoded value agreed upon across platforms for initial launch. // Format left open for possible dynamic error values and other fields in the future. @@ -120,9 +121,15 @@ export async function getToken( // initializeAppCheck() has been called. token = await state.provider!.getToken(); } catch (e) { - // `getToken()` should never throw, but logging error text to console will aid debugging. - logger.error(e); - error = e; + if ((e as FirebaseError).code === AppCheckError.THROTTLED) { + // Warn if throttled, but do not treat it as an error. + logger.warn((e as FirebaseError).message); + } else { + // `getToken()` should never throw, but logging error text to console will aid debugging. + logger.error(e); + } + // Always save error to be added to dummy token. + error = e as FirebaseError; } let interopTokenResult: AppCheckTokenResult | undefined; diff --git a/packages/app-check/src/providers.ts b/packages/app-check/src/providers.ts index ab1009c5148..d9a4656c442 100644 --- a/packages/app-check/src/providers.ts +++ b/packages/app-check/src/providers.ts @@ -17,12 +17,17 @@ import { FirebaseApp, _getProvider } from '@firebase/app'; import { Provider } from '@firebase/component'; -import { issuedAtTime } from '@firebase/util'; +import { + FirebaseError, + issuedAtTime, + calculateBackoffMillis +} from '@firebase/util'; import { exchangeToken, getExchangeRecaptchaEnterpriseTokenRequest, getExchangeRecaptchaV3TokenRequest } from './client'; +import { ONE_DAY } from './constants'; import { AppCheckError, ERROR_FACTORY } from './errors'; import { CustomProviderOptions } from './public-types'; import { @@ -30,7 +35,7 @@ import { initializeV3 as initializeRecaptchaV3, initializeEnterprise as initializeRecaptchaEnterprise } from './recaptcha'; -import { AppCheckProvider, AppCheckTokenInternal } from './types'; +import { AppCheckProvider, AppCheckTokenInternal, ThrottleData } from './types'; /** * App Check provider that can obtain a reCAPTCHA V3 token and exchange it @@ -41,6 +46,11 @@ import { AppCheckProvider, AppCheckTokenInternal } from './types'; export class ReCaptchaV3Provider implements AppCheckProvider { private _app?: FirebaseApp; private _platformLoggerProvider?: Provider<'platform-logger'>; + /** + * Throttle requests on certain error codes to prevent too many retries + * in a short time. + */ + private _throttleData: ThrottleData | null = null; /** * Create a ReCaptchaV3Provider instance. * @param siteKey - ReCAPTCHA V3 siteKey. @@ -52,6 +62,28 @@ export class ReCaptchaV3Provider implements AppCheckProvider { * @internal */ async getToken(): Promise { + if (this._throttleData) { + if (Date.now() - this._throttleData.allowRequestsAfter > 0) { + // If after throttle timestamp, clear throttle data. + this._throttleData = null; + } else { + // If before, throw. + throw ERROR_FACTORY.create(AppCheckError.THROTTLED, { + time: new Date( + this._throttleData.allowRequestsAfter + ).toLocaleString(), + httpStatus: this._throttleData.httpStatus + }); + } + } + if (!this._app || !this._platformLoggerProvider) { + // This should only occur if user has not called initializeAppCheck(). + // We don't have an appName to provide if so. + // This should already be caught in the top level `getToken()` function. + throw ERROR_FACTORY.create(AppCheckError.USE_BEFORE_ACTIVATION, { + appName: '' + }); + } // Top-level `getToken()` has already checked that App Check is initialized // and therefore this._app and this._platformLoggerProvider are available. const attestedClaimsToken = await getReCAPTCHAToken(this._app!).catch( @@ -60,10 +92,67 @@ export class ReCaptchaV3Provider implements AppCheckProvider { throw ERROR_FACTORY.create(AppCheckError.RECAPTCHA_ERROR); } ); - return exchangeToken( - getExchangeRecaptchaV3TokenRequest(this._app!, attestedClaimsToken), - this._platformLoggerProvider! - ); + let result; + try { + result = await exchangeToken( + getExchangeRecaptchaV3TokenRequest(this._app, attestedClaimsToken), + this._platformLoggerProvider + ); + } catch (e) { + if ((e as FirebaseError).code === AppCheckError.FETCH_STATUS_ERROR) { + const throttleData = this._setBackoff( + Number((e as FirebaseError).customData?.httpStatus) + ); + throw ERROR_FACTORY.create(AppCheckError.THROTTLED, { + time: new Date(throttleData.allowRequestsAfter).toLocaleString(), + httpStatus: throttleData.httpStatus + }); + } else { + throw e; + } + } + return result; + } + + /** + * Set throttle data to block requests until after a certain time + * depending on the failed request's status code. + * @param httpStatus Status code of failed request. + * @returns Data about current throttle state and expiration time. + */ + private _setBackoff(httpStatus: number): ThrottleData { + /** + * Block retries for 1 day for the following error codes: + * + * 404: Likely malformed URL. + * + * 403: + * - Attestation failed + * - Wrong API key + * - Project deleted + */ + if (httpStatus === 404 || httpStatus === 403) { + this._throttleData = { + backoffCount: 1, + allowRequestsAfter: Date.now() + ONE_DAY, + httpStatus + }; + } else { + /** + * For all other error codes, the time when it is ok to retry again + * is based on exponential backoff. + */ + const backoffCount = this._throttleData + ? this._throttleData.backoffCount + : 0; + const backoffMillis = calculateBackoffMillis(backoffCount, 1000, 2); + this._throttleData = { + backoffCount: backoffCount + 1, + allowRequestsAfter: Date.now() + backoffMillis, + httpStatus + }; + } + return this._throttleData; } /** diff --git a/packages/app-check/src/types.ts b/packages/app-check/src/types.ts index 21b2aa92622..534509d04d4 100644 --- a/packages/app-check/src/types.ts +++ b/packages/app-check/src/types.ts @@ -75,6 +75,12 @@ export interface AppCheckProvider { */ export type _AppCheckInternalComponentName = 'app-check-internal'; +export interface ThrottleData { + allowRequestsAfter: number; + backoffCount: number; + httpStatus: number; +} + declare module '@firebase/component' { interface NameServiceMapping { 'app-check-internal': FirebaseAppCheckInternal; From 3e45489e0ad39051e06b91d5259e6efccb97db1f Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Thu, 21 Oct 2021 17:09:14 -0700 Subject: [PATCH 02/17] Add initial addTokenListener --- packages/app-check/src/api.ts | 6 ++++++ packages/app-check/src/internal-api.ts | 7 +++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/packages/app-check/src/api.ts b/packages/app-check/src/api.ts index 596b1a402a3..6f5cf31c1bb 100644 --- a/packages/app-check/src/api.ts +++ b/packages/app-check/src/api.ts @@ -97,6 +97,12 @@ export function initializeAppCheck( const appCheck = provider.initialize({ options }); _activate(app, options.provider, options.isTokenAutoRefreshEnabled); + // Adding a listener will start the refresher and fetch a token if needed. + // This gets a token ready and prevents a delay when an internal library + // requests the token. + // Listener function does not need to do anything, its base functionality + // of calling getToken() already fetches token and writes it to memory/storage. + addTokenListener(appCheck, ListenerType.INTERNAL, () => {}); return appCheck; } diff --git a/packages/app-check/src/internal-api.ts b/packages/app-check/src/internal-api.ts index 63d77a19392..be29fedb75d 100644 --- a/packages/app-check/src/internal-api.ts +++ b/packages/app-check/src/internal-api.ts @@ -253,8 +253,11 @@ function createTokenRefresher(appCheck: AppCheckService): Refresher { throw result.error; } }, - () => { - // TODO: when should we retry? + (error: unknown) => { + // Do not queue up retries if it's in a throttled state. + if ((error as FirebaseError).code === AppCheckError.THROTTLED) { + return false; + } return true; }, () => { From 2cb068642d785f845a404d5c5ef69e0f7a9003cb Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Thu, 21 Oct 2021 17:13:06 -0700 Subject: [PATCH 03/17] Fix tests --- packages/app-check/src/api.test.ts | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/packages/app-check/src/api.test.ts b/packages/app-check/src/api.test.ts index 2d869875a28..09681d8b19e 100644 --- a/packages/app-check/src/api.test.ts +++ b/packages/app-check/src/api.test.ts @@ -278,6 +278,9 @@ describe('api', () => { provider: new ReCaptchaV3Provider(FAKE_SITE_KEY), isTokenAutoRefreshEnabled: true }); + + expect(getState(app).tokenObservers.length).to.equal(1); + const fakeRecaptchaToken = 'fake-recaptcha-token'; const fakeRecaptchaAppCheckToken = { token: 'fake-recaptcha-app-check-token', @@ -299,7 +302,7 @@ describe('api', () => { const unsubscribe1 = onTokenChanged(appCheck, listener1, errorFn1); const unsubscribe2 = onTokenChanged(appCheck, listener2, errorFn2); - expect(getState(app).tokenObservers.length).to.equal(2); + expect(getState(app).tokenObservers.length).to.equal(3); await internalApi.getToken(appCheck as AppCheckService); @@ -312,7 +315,7 @@ describe('api', () => { expect(errorFn2).to.not.be.called; unsubscribe1(); unsubscribe2(); - expect(getState(app).tokenObservers.length).to.equal(0); + expect(getState(app).tokenObservers.length).to.equal(1); }); it('Listeners work when using Observer pattern', async () => { @@ -320,6 +323,9 @@ describe('api', () => { provider: new ReCaptchaV3Provider(FAKE_SITE_KEY), isTokenAutoRefreshEnabled: true }); + + expect(getState(app).tokenObservers.length).to.equal(1); + const fakeRecaptchaToken = 'fake-recaptcha-token'; const fakeRecaptchaAppCheckToken = { token: 'fake-recaptcha-app-check-token', @@ -351,7 +357,7 @@ describe('api', () => { error: errorFn1 }); - expect(getState(app).tokenObservers.length).to.equal(2); + expect(getState(app).tokenObservers.length).to.equal(3); await internalApi.getToken(appCheck as AppCheckService); @@ -364,7 +370,7 @@ describe('api', () => { expect(errorFn2).to.not.be.called; unsubscribe1(); unsubscribe2(); - expect(getState(app).tokenObservers.length).to.equal(0); + expect(getState(app).tokenObservers.length).to.equal(1); }); it('onError() catches token errors', async () => { @@ -373,6 +379,9 @@ describe('api', () => { provider: new ReCaptchaV3Provider(FAKE_SITE_KEY), isTokenAutoRefreshEnabled: false }); + + expect(getState(app).tokenObservers.length).to.equal(1); + const fakeRecaptchaToken = 'fake-recaptcha-token'; stub(reCAPTCHA, 'getToken').returns(Promise.resolve(fakeRecaptchaToken)); stub(client, 'exchangeToken').rejects('exchange error'); @@ -386,13 +395,13 @@ describe('api', () => { await internalApi.getToken(appCheck as AppCheckService); - expect(getState(app).tokenObservers.length).to.equal(1); + expect(getState(app).tokenObservers.length).to.equal(2); expect(errorFn1).to.be.calledOnce; expect(errorFn1.args[0][0].name).to.include('exchange error'); unsubscribe1(); - expect(getState(app).tokenObservers.length).to.equal(0); + expect(getState(app).tokenObservers.length).to.equal(1); }); }); }); From 6c4a064bcdacad50edb38f9ecf3f025cd905c6f9 Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Mon, 25 Oct 2021 13:39:03 -0700 Subject: [PATCH 04/17] Start refresh after cache check --- packages/app-check/src/internal-api.ts | 34 ++++++++++++++++---------- packages/app-check/src/providers.ts | 2 +- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/packages/app-check/src/internal-api.ts b/packages/app-check/src/internal-api.ts index be29fedb75d..fb1303fed31 100644 --- a/packages/app-check/src/internal-api.ts +++ b/packages/app-check/src/internal-api.ts @@ -168,19 +168,8 @@ export function addTokenListener( ...state, tokenObservers: [...state.tokenObservers, tokenObserver] }; - /** - * Invoke the listener with the valid token, then start the token refresher - */ - if (!newState.tokenRefresher) { - const tokenRefresher = createTokenRefresher(appCheck); - newState.tokenRefresher = tokenRefresher; - } - // Create the refresher but don't start it if `isTokenAutoRefreshEnabled` - // is not true. - if (!newState.tokenRefresher.isRunning() && state.isTokenAutoRefreshEnabled) { - newState.tokenRefresher.start(); - } + let cacheCheckPromise = Promise.resolve(); // Invoke the listener async immediately if there is a valid token // in memory. @@ -194,7 +183,7 @@ export function addTokenListener( } else if (state.token == null) { // Only check cache if there was no token. If the token was invalid, // skip this and rely on exchange endpoint. - void state + cacheCheckPromise = state .cachedTokenPromise! // Storage token promise. Always populated in `activate()`. .then(cachedToken => { if (cachedToken && isValid(cachedToken)) { @@ -206,6 +195,25 @@ export function addTokenListener( }); } + // Wait for any cached token promise to resolve before starting the token + // refresher. The refresher checks to see if there is an existing token + // in state and calls the exchange endpoint if not. We should first let the + // IndexedDB check have a chance to populate state if it can. + void cacheCheckPromise.then(() => { + if (!newState.tokenRefresher) { + const tokenRefresher = createTokenRefresher(appCheck); + newState.tokenRefresher = tokenRefresher; + } + // Create the refresher but don't start it if `isTokenAutoRefreshEnabled` + // is not true. + if ( + !newState.tokenRefresher.isRunning() && + state.isTokenAutoRefreshEnabled + ) { + newState.tokenRefresher.start(); + } + }); + setState(app, newState); } diff --git a/packages/app-check/src/providers.ts b/packages/app-check/src/providers.ts index d9a4656c442..8f2f4a7912e 100644 --- a/packages/app-check/src/providers.ts +++ b/packages/app-check/src/providers.ts @@ -117,7 +117,7 @@ export class ReCaptchaV3Provider implements AppCheckProvider { /** * Set throttle data to block requests until after a certain time * depending on the failed request's status code. - * @param httpStatus Status code of failed request. + * @param httpStatus - Status code of failed request. * @returns Data about current throttle state and expiration time. */ private _setBackoff(httpStatus: number): ThrottleData { From 82a666fcf4463d0b626e7f7104df42ea0ff547cb Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Mon, 25 Oct 2021 17:07:06 -0700 Subject: [PATCH 05/17] Get rid of duplicate listener calls --- packages/app-check/src/api.ts | 6 +- packages/app-check/src/internal-api.ts | 80 +++++++++++++++----------- packages/app-check/src/state.ts | 1 + 3 files changed, 52 insertions(+), 35 deletions(-) diff --git a/packages/app-check/src/api.ts b/packages/app-check/src/api.ts index 6f5cf31c1bb..1f23a65faec 100644 --- a/packages/app-check/src/api.ts +++ b/packages/app-check/src/api.ts @@ -32,7 +32,8 @@ import { getToken as getTokenInternal, addTokenListener, removeTokenListener, - isValid + isValid, + notifyTokenListeners } from './internal-api'; import { readTokenFromStorage } from './storage'; import { getDebugToken, initializeDebugMode, isDebugMode } from './debug'; @@ -129,6 +130,9 @@ function _activate( newState.cachedTokenPromise = readTokenFromStorage(app).then(cachedToken => { if (cachedToken && isValid(cachedToken)) { setState(app, { ...getState(app), token: cachedToken }); + console.log('notifying token listeners'); + // notify all listeners with the cached token + notifyTokenListeners(app, { token: cachedToken.token }); } return cachedToken; }); diff --git a/packages/app-check/src/internal-api.ts b/packages/app-check/src/internal-api.ts index fb1303fed31..03940e3be24 100644 --- a/packages/app-check/src/internal-api.ts +++ b/packages/app-check/src/internal-api.ts @@ -81,10 +81,6 @@ export async function getToken( const cachedToken = await state.cachedTokenPromise; if (cachedToken && isValid(cachedToken)) { token = cachedToken; - - setState(app, { ...state, token }); - // notify all listeners with the cached token - notifyTokenListeners(app, { token: token.token }); } } @@ -95,16 +91,28 @@ export async function getToken( }; } + let waitedForInFlightRequest = false; + /** * DEBUG MODE * If debug mode is set, and there is no cached token, fetch a new App * Check token using the debug token, and return it directly. */ if (isDebugMode()) { - const tokenFromDebugExchange: AppCheckTokenInternal = await exchangeToken( - getExchangeDebugTokenRequest(app, await getDebugToken()), - appCheck.platformLoggerProvider - ); + // Avoid making another call to the exchange endpoint if one is in flight. + if (!state.exchangeTokenPromise) { + state.exchangeTokenPromise = exchangeToken( + getExchangeDebugTokenRequest(app, await getDebugToken()), + appCheck.platformLoggerProvider + ).then(token => { + state.exchangeTokenPromise = undefined; + return token; + }); + } else { + waitedForInFlightRequest = true; + } + const tokenFromDebugExchange: AppCheckTokenInternal = + await state.exchangeTokenPromise; // Write debug token to indexedDB. await writeTokenToStorage(app, tokenFromDebugExchange); // Write debug token to state. @@ -116,10 +124,19 @@ export async function getToken( * request a new token */ try { - // state.provider is populated in initializeAppCheck() - // ensureActivated() at the top of this function checks that - // initializeAppCheck() has been called. - token = await state.provider!.getToken(); + // Avoid making another call to the exchange endpoint if one is in flight. + if (!state.exchangeTokenPromise) { + // state.provider is populated in initializeAppCheck() + // ensureActivated() at the top of this function checks that + // initializeAppCheck() has been called. + state.exchangeTokenPromise = state.provider!.getToken().then(token => { + state.exchangeTokenPromise = undefined; + return token; + }); + } else { + waitedForInFlightRequest = true; + } + token = await state.exchangeTokenPromise; } catch (e) { if ((e as FirebaseError).code === AppCheckError.THROTTLED) { // Warn if throttled, but do not treat it as an error. @@ -147,7 +164,9 @@ export async function getToken( await writeTokenToStorage(app, token); } - notifyTokenListeners(app, interopTokenResult); + if (!waitedForInFlightRequest) { + notifyTokenListeners(app, interopTokenResult); + } return interopTokenResult; } @@ -169,8 +188,6 @@ export function addTokenListener( tokenObservers: [...state.tokenObservers, tokenObserver] }; - let cacheCheckPromise = Promise.resolve(); - // Invoke the listener async immediately if there is a valid token // in memory. if (state.token && isValid(state.token)) { @@ -180,26 +197,21 @@ export function addTokenListener( .catch(() => { /* we don't care about exceptions thrown in listeners */ }); - } else if (state.token == null) { - // Only check cache if there was no token. If the token was invalid, - // skip this and rely on exchange endpoint. - cacheCheckPromise = state - .cachedTokenPromise! // Storage token promise. Always populated in `activate()`. - .then(cachedToken => { - if (cachedToken && isValid(cachedToken)) { - listener({ token: cachedToken.token }); - } - }) - .catch(() => { - /** Ignore errors in listeners. */ - }); } - // Wait for any cached token promise to resolve before starting the token - // refresher. The refresher checks to see if there is an existing token - // in state and calls the exchange endpoint if not. We should first let the - // IndexedDB check have a chance to populate state if it can. - void cacheCheckPromise.then(() => { + /** + * Wait for any cached token promise to resolve before starting the token + * refresher. The refresher checks to see if there is an existing token + * in state and calls the exchange endpoint if not. We should first let the + * IndexedDB check have a chance to populate state if it can. + * + * We want to call the listener if the cached token check returns something + * but cachedTokenPromise handler already will notify all listeners on the + * first fetch, and we don't want duplicate calls to the listener. + */ + + // state.cachedTokenPromise is always populated in `activate()`. + void state.cachedTokenPromise!.then(() => { if (!newState.tokenRefresher) { const tokenRefresher = createTokenRefresher(appCheck); newState.tokenRefresher = tokenRefresher; @@ -295,7 +307,7 @@ function createTokenRefresher(appCheck: AppCheckService): Refresher { ); } -function notifyTokenListeners( +export function notifyTokenListeners( app: FirebaseApp, token: AppCheckTokenResult ): void { diff --git a/packages/app-check/src/state.ts b/packages/app-check/src/state.ts index 27d9de08816..0a81ba81636 100644 --- a/packages/app-check/src/state.ts +++ b/packages/app-check/src/state.ts @@ -30,6 +30,7 @@ export interface AppCheckState { provider?: AppCheckProvider; token?: AppCheckTokenInternal; cachedTokenPromise?: Promise; + exchangeTokenPromise?: Promise; tokenRefresher?: Refresher; reCAPTCHAState?: ReCAPTCHAState; isTokenAutoRefreshEnabled?: boolean; From f0e9e47a4c284acefba9a13427f68bc696362ebf Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Tue, 26 Oct 2021 10:08:57 -0700 Subject: [PATCH 06/17] clean up in-flight logic --- packages/app-check/src/internal-api.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/app-check/src/internal-api.ts b/packages/app-check/src/internal-api.ts index 03940e3be24..d6b1e674d79 100644 --- a/packages/app-check/src/internal-api.ts +++ b/packages/app-check/src/internal-api.ts @@ -91,7 +91,10 @@ export async function getToken( }; } - let waitedForInFlightRequest = false; + // Only set to true if this `getToken()` call is making the actual + // REST call to the exchange endpoint, versus waiting for an already + // in-flight call (see debug and regular exchange endpoint paths below) + let shouldCallListeners = false; /** * DEBUG MODE @@ -108,8 +111,7 @@ export async function getToken( state.exchangeTokenPromise = undefined; return token; }); - } else { - waitedForInFlightRequest = true; + shouldCallListeners = true; } const tokenFromDebugExchange: AppCheckTokenInternal = await state.exchangeTokenPromise; @@ -133,8 +135,7 @@ export async function getToken( state.exchangeTokenPromise = undefined; return token; }); - } else { - waitedForInFlightRequest = true; + shouldCallListeners = true; } token = await state.exchangeTokenPromise; } catch (e) { @@ -164,7 +165,7 @@ export async function getToken( await writeTokenToStorage(app, token); } - if (!waitedForInFlightRequest) { + if (!shouldCallListeners) { notifyTokenListeners(app, interopTokenResult); } return interopTokenResult; From f8926a1b4a9d313161dd7f405cf65af8ca2141ef Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Tue, 26 Oct 2021 10:43:30 -0700 Subject: [PATCH 07/17] clean up refresher start logic --- packages/app-check/src/internal-api.test.ts | 14 +++++- packages/app-check/src/internal-api.ts | 51 +++++++++++---------- 2 files changed, 40 insertions(+), 25 deletions(-) diff --git a/packages/app-check/src/internal-api.test.ts b/packages/app-check/src/internal-api.test.ts index 779b84fb9fc..5b31fb1867d 100644 --- a/packages/app-check/src/internal-api.test.ts +++ b/packages/app-check/src/internal-api.test.ts @@ -404,7 +404,7 @@ describe('internal api', () => { expect(getState(app).tokenObservers[0].next).to.equal(listener); }); - it('starts proactively refreshing token after adding the first listener', () => { + it('starts proactively refreshing token after adding the first listener', async () => { const listener = (): void => {}; setState(app, { ...getState(app), @@ -420,6 +420,10 @@ describe('internal api', () => { listener ); + // addTokenListener() waits for the result of cachedTokenPromise + // before starting the refresher + await getState(app).cachedTokenPromise; + expect(getState(app).tokenRefresher?.isRunning()).to.be.true; }); @@ -430,6 +434,7 @@ describe('internal api', () => { setState(app, { ...getState(app), + cachedTokenPromise: Promise.resolve(undefined), token: { token: `fake-memory-app-check-token`, expireTimeMillis: Date.now() + 60000, @@ -493,7 +498,7 @@ describe('internal api', () => { expect(getState(app).tokenObservers.length).to.equal(0); }); - it('should stop proactively refreshing token after deleting the last listener', () => { + it('should stop proactively refreshing token after deleting the last listener', async () => { const listener = (): void => {}; setState(app, { ...getState(app), isTokenAutoRefreshEnabled: true }); setState(app, { @@ -506,6 +511,11 @@ describe('internal api', () => { ListenerType.INTERNAL, listener ); + + // addTokenListener() waits for the result of cachedTokenPromise + // before starting the refresher + await getState(app).cachedTokenPromise; + expect(getState(app).tokenObservers.length).to.equal(1); expect(getState(app).tokenRefresher?.isRunning()).to.be.true; diff --git a/packages/app-check/src/internal-api.ts b/packages/app-check/src/internal-api.ts index d6b1e674d79..32604783c3f 100644 --- a/packages/app-check/src/internal-api.ts +++ b/packages/app-check/src/internal-api.ts @@ -165,7 +165,7 @@ export async function getToken( await writeTokenToStorage(app, token); } - if (!shouldCallListeners) { + if (shouldCallListeners) { notifyTokenListeners(app, interopTokenResult); } return interopTokenResult; @@ -184,17 +184,20 @@ export function addTokenListener( error: onError, type }; - const newState = { + setState(app, { ...state, tokenObservers: [...state.tokenObservers, tokenObserver] - }; + }); // Invoke the listener async immediately if there is a valid token // in memory. if (state.token && isValid(state.token)) { const validToken = state.token; Promise.resolve() - .then(() => listener({ token: validToken.token })) + .then(() => { + listener({ token: validToken.token }); + initTokenRefresher(appCheck); + }) .catch(() => { /* we don't care about exceptions thrown in listeners */ }); @@ -206,28 +209,12 @@ export function addTokenListener( * in state and calls the exchange endpoint if not. We should first let the * IndexedDB check have a chance to populate state if it can. * - * We want to call the listener if the cached token check returns something - * but cachedTokenPromise handler already will notify all listeners on the - * first fetch, and we don't want duplicate calls to the listener. + * Listener call isn't needed here because cachedTokenPromise will call any + * listeners that exist when it resolves. */ // state.cachedTokenPromise is always populated in `activate()`. - void state.cachedTokenPromise!.then(() => { - if (!newState.tokenRefresher) { - const tokenRefresher = createTokenRefresher(appCheck); - newState.tokenRefresher = tokenRefresher; - } - // Create the refresher but don't start it if `isTokenAutoRefreshEnabled` - // is not true. - if ( - !newState.tokenRefresher.isRunning() && - state.isTokenAutoRefreshEnabled - ) { - newState.tokenRefresher.start(); - } - }); - - setState(app, newState); + void state.cachedTokenPromise!.then(() => initTokenRefresher(appCheck)); } export function removeTokenListener( @@ -253,6 +240,24 @@ export function removeTokenListener( }); } +/** + * Logic to create and start refresher as needed. + */ +function initTokenRefresher(appCheck: AppCheckService): void { + const { app } = appCheck; + const state = getState(app); + // Create the refresher but don't start it if `isTokenAutoRefreshEnabled` + // is not true. + let refresher: Refresher | undefined = state.tokenRefresher; + if (!refresher) { + refresher = createTokenRefresher(appCheck); + setState(app, { ...state, tokenRefresher: refresher }); + } + if (!refresher.isRunning() && state.isTokenAutoRefreshEnabled) { + refresher.start(); + } +} + function createTokenRefresher(appCheck: AppCheckService): Refresher { const { app } = appCheck; return new Refresher( From e48cc5da5bb6cf0727fe17776cc1e544a90c2aa2 Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Tue, 26 Oct 2021 11:00:15 -0700 Subject: [PATCH 08/17] working on tests --- packages/app-check/src/internal-api.test.ts | 21 +- packages/app-check/src/providers.test.ts | 349 ++++++++++++++++++++ 2 files changed, 369 insertions(+), 1 deletion(-) create mode 100644 packages/app-check/src/providers.test.ts diff --git a/packages/app-check/src/internal-api.test.ts b/packages/app-check/src/internal-api.test.ts index 5b31fb1867d..422e05e5d01 100644 --- a/packages/app-check/src/internal-api.test.ts +++ b/packages/app-check/src/internal-api.test.ts @@ -40,7 +40,7 @@ import * as storage from './storage'; import * as util from './util'; import { getState, clearState, setState, getDebugState } from './state'; import { AppCheckTokenListener } from './public-types'; -import { Deferred } from '@firebase/util'; +import { Deferred, FirebaseError } from '@firebase/util'; import { ReCaptchaEnterpriseProvider, ReCaptchaV3Provider } from './providers'; import { AppCheckService } from './factory'; import { ListenerType } from './types'; @@ -385,6 +385,25 @@ describe('internal api', () => { ); expect(token).to.deep.equal({ token: fakeRecaptchaAppCheckToken.token }); }); + it('throttle', async () => { + const appCheck = initializeAppCheck(app, { + provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) + }); + stub( + client, + 'exchangeToken' + ).returns( + Promise.reject( + new FirebaseError('test-error', 'test error msg', { httpStatus: 503 }) + ) + ); + + const token = await getToken(appCheck as AppCheckService); + + expect(token.error?.message).to.equal( + 'sdfa' + ); + }); }); describe('addTokenListener', () => { diff --git a/packages/app-check/src/providers.test.ts b/packages/app-check/src/providers.test.ts new file mode 100644 index 00000000000..d14fd8c39de --- /dev/null +++ b/packages/app-check/src/providers.test.ts @@ -0,0 +1,349 @@ +/** + * @license + * Copyright 2020 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import '../test/setup'; +import { expect } from 'chai'; +import { SinonStub, spy, stub, useFakeTimers } from 'sinon'; +import { deleteApp, FirebaseApp } from '@firebase/app'; +import { + FAKE_SITE_KEY, + getFullApp, + getFakeCustomTokenProvider, + removegreCAPTCHAScriptsOnPage, + getFakeGreCAPTCHA +} from '../test/util'; +import { initializeAppCheck } from './api'; +import { + getToken, + addTokenListener, + removeTokenListener, + formatDummyToken, + defaultTokenErrorData +} from './internal-api'; +import * as reCAPTCHA from './recaptcha'; +import * as client from './client'; +import * as storage from './storage'; +import * as util from './util'; +import { getState, clearState, setState, getDebugState } from './state'; +import { AppCheckTokenListener } from './public-types'; +import { Deferred, FirebaseError } from '@firebase/util'; +import { ReCaptchaV3Provider } from './providers'; +import { AppCheckService } from './factory'; +import { ListenerType } from './types'; + +const fakeRecaptchaToken = 'fake-recaptcha-token'; +const fakeRecaptchaAppCheckToken = { + token: 'fake-recaptcha-app-check-token', + expireTimeMillis: Date.now() + 60000, + issuedAtTimeMillis: 0 +}; + +const fakeCachedAppCheckToken = { + token: 'fake-cached-app-check-token', + expireTimeMillis: Date.now() + 60000, + issuedAtTimeMillis: 0 +}; + +describe('internal api', () => { + let app: FirebaseApp; + let storageReadStub: SinonStub; + let storageWriteStub: SinonStub; + + beforeEach(() => { + app = getFullApp(); + storageReadStub = stub(storage, 'readTokenFromStorage').resolves(undefined); + storageWriteStub = stub(storage, 'writeTokenToStorage'); + stub(util, 'getRecaptcha').returns(getFakeGreCAPTCHA()); + }); + + afterEach(() => { + clearState(); + removegreCAPTCHAScriptsOnPage(); + return deleteApp(app); + }); + // TODO: test error conditions + describe('getToken()', () => { + it('uses reCAPTCHA token to exchange for AppCheck token', async () => { + const appCheck = initializeAppCheck(app, { + provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) + }); + const exchangeTokenStub: SinonStub = stub( + client, + 'exchangeToken' + ).returns( + Promise.reject( + new FirebaseError('test-error', 'test error msg', { httpStatus: 503 }) + ) + ); + + const token = await getToken(appCheck as AppCheckService); + + expect(exchangeTokenStub.args[0][0].body['recaptcha_token']).to.equal( + fakeRecaptchaToken + ); + expect(token).to.deep.equal({ token: fakeRecaptchaAppCheckToken.token }); + }); + + it('resolves with a dummy token and an error if failed to get a token', async () => { + const errorStub = stub(console, 'error'); + const appCheck = initializeAppCheck(app, { + provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) + }); + + const reCAPTCHASpy = stub(reCAPTCHA, 'getToken').returns( + Promise.resolve(fakeRecaptchaToken) + ); + + const error = new Error('oops, something went wrong'); + stub(client, 'exchangeToken').returns(Promise.reject(error)); + + const token = await getToken(appCheck as AppCheckService); + + expect(reCAPTCHASpy).to.be.called; + expect(token).to.deep.equal({ + token: formatDummyToken(defaultTokenErrorData), + error + }); + expect(errorStub.args[0][1].message).to.include( + 'oops, something went wrong' + ); + errorStub.restore(); + }); + + it('notifies listeners using cached token', async () => { + storageReadStub.resolves(fakeCachedAppCheckToken); + const appCheck = initializeAppCheck(app, { + provider: new ReCaptchaV3Provider(FAKE_SITE_KEY), + isTokenAutoRefreshEnabled: false + }); + + const clock = useFakeTimers(); + + const listener1 = spy(); + const listener2 = spy(); + addTokenListener( + appCheck as AppCheckService, + ListenerType.INTERNAL, + listener1 + ); + addTokenListener( + appCheck as AppCheckService, + ListenerType.INTERNAL, + listener2 + ); + + await getToken(appCheck as AppCheckService); + + clock.tick(1); + + expect(listener1).to.be.calledWith({ + token: fakeCachedAppCheckToken.token + }); + expect(listener2).to.be.calledWith({ + token: fakeCachedAppCheckToken.token + }); + + clock.restore(); + }); + + it('notifies listeners using new token', async () => { + const appCheck = initializeAppCheck(app, { + provider: new ReCaptchaV3Provider(FAKE_SITE_KEY), + isTokenAutoRefreshEnabled: true + }); + + stub(reCAPTCHA, 'getToken').returns(Promise.resolve(fakeRecaptchaToken)); + stub(client, 'exchangeToken').returns( + Promise.resolve(fakeRecaptchaAppCheckToken) + ); + + const listener1 = spy(); + const listener2 = spy(); + addTokenListener( + appCheck as AppCheckService, + ListenerType.INTERNAL, + listener1 + ); + addTokenListener( + appCheck as AppCheckService, + ListenerType.INTERNAL, + listener2 + ); + + await getToken(appCheck as AppCheckService); + + expect(listener1).to.be.calledWith({ + token: fakeRecaptchaAppCheckToken.token + }); + expect(listener2).to.be.calledWith({ + token: fakeRecaptchaAppCheckToken.token + }); + }); + + it('calls 3P error handler if there is an error getting a token', async () => { + stub(console, 'error'); + const appCheck = initializeAppCheck(app, { + provider: new ReCaptchaV3Provider(FAKE_SITE_KEY), + isTokenAutoRefreshEnabled: true + }); + stub(reCAPTCHA, 'getToken').returns(Promise.resolve(fakeRecaptchaToken)); + stub(client, 'exchangeToken').rejects('exchange error'); + const listener1 = spy(); + const errorFn1 = spy(); + + addTokenListener( + appCheck as AppCheckService, + ListenerType.EXTERNAL, + listener1, + errorFn1 + ); + + await getToken(appCheck as AppCheckService); + + expect(errorFn1).to.be.calledOnce; + expect(errorFn1.args[0][0].name).to.include('exchange error'); + }); + + it('ignores listeners that throw', async () => { + stub(console, 'error'); + const appCheck = initializeAppCheck(app, { + provider: new ReCaptchaV3Provider(FAKE_SITE_KEY), + isTokenAutoRefreshEnabled: true + }); + stub(reCAPTCHA, 'getToken').returns(Promise.resolve(fakeRecaptchaToken)); + stub(client, 'exchangeToken').returns( + Promise.resolve(fakeRecaptchaAppCheckToken) + ); + const listener1 = stub().throws(new Error()); + const listener2 = spy(); + + addTokenListener( + appCheck as AppCheckService, + ListenerType.INTERNAL, + listener1 + ); + addTokenListener( + appCheck as AppCheckService, + ListenerType.INTERNAL, + listener2 + ); + + await getToken(appCheck as AppCheckService); + + expect(listener1).to.be.calledWith({ + token: fakeRecaptchaAppCheckToken.token + }); + expect(listener2).to.be.calledWith({ + token: fakeRecaptchaAppCheckToken.token + }); + }); + + it('loads persisted token to memory and returns it', async () => { + const clock = useFakeTimers(); + + storageReadStub.resolves(fakeCachedAppCheckToken); + const appCheck = initializeAppCheck(app, { + provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) + }); + + const clientStub = stub(client, 'exchangeToken'); + + expect(getState(app).token).to.equal(undefined); + expect(await getToken(appCheck as AppCheckService)).to.deep.equal({ + token: fakeCachedAppCheckToken.token + }); + expect(getState(app).token).to.equal(fakeCachedAppCheckToken); + expect(clientStub).has.not.been.called; + + clock.restore(); + }); + + it('persists token to storage', async () => { + const appCheck = initializeAppCheck(app, { + provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) + }); + + stub(reCAPTCHA, 'getToken').returns(Promise.resolve(fakeRecaptchaToken)); + stub(client, 'exchangeToken').returns( + Promise.resolve(fakeRecaptchaAppCheckToken) + ); + storageWriteStub.resetHistory(); + const result = await getToken(appCheck as AppCheckService); + expect(result).to.deep.equal({ token: fakeRecaptchaAppCheckToken.token }); + expect(storageWriteStub).has.been.calledWith( + app, + fakeRecaptchaAppCheckToken + ); + }); + + it('returns the valid token in memory without making network request', async () => { + const clock = useFakeTimers(); + const appCheck = initializeAppCheck(app, { + provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) + }); + setState(app, { ...getState(app), token: fakeRecaptchaAppCheckToken }); + + const clientStub = stub(client, 'exchangeToken'); + expect(await getToken(appCheck as AppCheckService)).to.deep.equal({ + token: fakeRecaptchaAppCheckToken.token + }); + expect(clientStub).to.not.have.been.called; + + clock.restore(); + }); + + it('force to get new token when forceRefresh is true', async () => { + const appCheck = initializeAppCheck(app, { + provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) + }); + setState(app, { ...getState(app), token: fakeRecaptchaAppCheckToken }); + + stub(reCAPTCHA, 'getToken').returns(Promise.resolve(fakeRecaptchaToken)); + stub(client, 'exchangeToken').returns( + Promise.resolve({ + token: 'new-recaptcha-app-check-token', + expireTimeMillis: Date.now() + 60000, + issuedAtTimeMillis: 0 + }) + ); + + expect(await getToken(appCheck as AppCheckService, true)).to.deep.equal({ + token: 'new-recaptcha-app-check-token' + }); + }); + + it('exchanges debug token if in debug mode and there is no cached token', async () => { + const exchangeTokenStub: SinonStub = stub( + client, + 'exchangeToken' + ).returns(Promise.resolve(fakeRecaptchaAppCheckToken)); + const debugState = getDebugState(); + debugState.enabled = true; + debugState.token = new Deferred(); + debugState.token.resolve('my-debug-token'); + const appCheck = initializeAppCheck(app, { + provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) + }); + + const token = await getToken(appCheck as AppCheckService); + expect(exchangeTokenStub.args[0][0].body['debug_token']).to.equal( + 'my-debug-token' + ); + expect(token).to.deep.equal({ token: fakeRecaptchaAppCheckToken.token }); + }); + }); +}); From 9d12268251c92153c20999ee855b62ca49da0bb5 Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Tue, 26 Oct 2021 13:29:34 -0700 Subject: [PATCH 09/17] Add retry code test --- packages/app-check/src/api.ts | 1 - packages/app-check/src/internal-api.test.ts | 41 ++- packages/app-check/src/internal-api.ts | 2 +- packages/app-check/src/providers.test.ts | 349 -------------------- packages/app-check/src/providers.ts | 3 +- packages/app-check/src/util.ts | 27 ++ 6 files changed, 63 insertions(+), 360 deletions(-) delete mode 100644 packages/app-check/src/providers.test.ts diff --git a/packages/app-check/src/api.ts b/packages/app-check/src/api.ts index 1f23a65faec..98d298bd8cb 100644 --- a/packages/app-check/src/api.ts +++ b/packages/app-check/src/api.ts @@ -130,7 +130,6 @@ function _activate( newState.cachedTokenPromise = readTokenFromStorage(app).then(cachedToken => { if (cachedToken && isValid(cachedToken)) { setState(app, { ...getState(app), token: cachedToken }); - console.log('notifying token listeners'); // notify all listeners with the cached token notifyTokenListeners(app, { token: cachedToken.token }); } diff --git a/packages/app-check/src/internal-api.test.ts b/packages/app-check/src/internal-api.test.ts index 422e05e5d01..8ac58770965 100644 --- a/packages/app-check/src/internal-api.test.ts +++ b/packages/app-check/src/internal-api.test.ts @@ -38,12 +38,14 @@ import * as reCAPTCHA from './recaptcha'; import * as client from './client'; import * as storage from './storage'; import * as util from './util'; +import { logger } from './logger'; import { getState, clearState, setState, getDebugState } from './state'; import { AppCheckTokenListener } from './public-types'; import { Deferred, FirebaseError } from '@firebase/util'; import { ReCaptchaEnterpriseProvider, ReCaptchaV3Provider } from './providers'; import { AppCheckService } from './factory'; import { ListenerType } from './types'; +import { AppCheckError } from './errors'; const fakeRecaptchaToken = 'fake-recaptcha-token'; const fakeRecaptchaAppCheckToken = { @@ -385,24 +387,47 @@ describe('internal api', () => { ); expect(token).to.deep.equal({ token: fakeRecaptchaAppCheckToken.token }); }); - it('throttle', async () => { + it('throttles exponentially on 503', async () => { const appCheck = initializeAppCheck(app, { provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) }); - stub( - client, - 'exchangeToken' - ).returns( + const warnStub = stub(logger, 'warn'); + stub(client, 'exchangeToken').returns( Promise.reject( - new FirebaseError('test-error', 'test error msg', { httpStatus: 503 }) + new FirebaseError( + AppCheckError.FETCH_STATUS_ERROR, + 'test error msg', + { httpStatus: 503 } + ) ) ); const token = await getToken(appCheck as AppCheckService); - expect(token.error?.message).to.equal( - 'sdfa' + expect(token.error?.message).to.include('503'); + expect(token.error?.message).to.include('00m'); + expect(warnStub.args[0][0]).to.include('503'); + }); + it('throttles 1d on 403', async () => { + const appCheck = initializeAppCheck(app, { + provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) + }); + const warnStub = stub(logger, 'warn'); + stub(client, 'exchangeToken').returns( + Promise.reject( + new FirebaseError( + AppCheckError.FETCH_STATUS_ERROR, + 'test error msg', + { httpStatus: 403 } + ) + ) ); + + const token = await getToken(appCheck as AppCheckService); + + expect(token.error?.message).to.include('403'); + expect(token.error?.message).to.include('1d'); + expect(warnStub.args[0][0]).to.include('403'); }); }); diff --git a/packages/app-check/src/internal-api.ts b/packages/app-check/src/internal-api.ts index 32604783c3f..e7976d64fb0 100644 --- a/packages/app-check/src/internal-api.ts +++ b/packages/app-check/src/internal-api.ts @@ -139,7 +139,7 @@ export async function getToken( } token = await state.exchangeTokenPromise; } catch (e) { - if ((e as FirebaseError).code === AppCheckError.THROTTLED) { + if ((e as FirebaseError).code === `appCheck/${AppCheckError.THROTTLED}`) { // Warn if throttled, but do not treat it as an error. logger.warn((e as FirebaseError).message); } else { diff --git a/packages/app-check/src/providers.test.ts b/packages/app-check/src/providers.test.ts deleted file mode 100644 index d14fd8c39de..00000000000 --- a/packages/app-check/src/providers.test.ts +++ /dev/null @@ -1,349 +0,0 @@ -/** - * @license - * Copyright 2020 Google LLC - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import '../test/setup'; -import { expect } from 'chai'; -import { SinonStub, spy, stub, useFakeTimers } from 'sinon'; -import { deleteApp, FirebaseApp } from '@firebase/app'; -import { - FAKE_SITE_KEY, - getFullApp, - getFakeCustomTokenProvider, - removegreCAPTCHAScriptsOnPage, - getFakeGreCAPTCHA -} from '../test/util'; -import { initializeAppCheck } from './api'; -import { - getToken, - addTokenListener, - removeTokenListener, - formatDummyToken, - defaultTokenErrorData -} from './internal-api'; -import * as reCAPTCHA from './recaptcha'; -import * as client from './client'; -import * as storage from './storage'; -import * as util from './util'; -import { getState, clearState, setState, getDebugState } from './state'; -import { AppCheckTokenListener } from './public-types'; -import { Deferred, FirebaseError } from '@firebase/util'; -import { ReCaptchaV3Provider } from './providers'; -import { AppCheckService } from './factory'; -import { ListenerType } from './types'; - -const fakeRecaptchaToken = 'fake-recaptcha-token'; -const fakeRecaptchaAppCheckToken = { - token: 'fake-recaptcha-app-check-token', - expireTimeMillis: Date.now() + 60000, - issuedAtTimeMillis: 0 -}; - -const fakeCachedAppCheckToken = { - token: 'fake-cached-app-check-token', - expireTimeMillis: Date.now() + 60000, - issuedAtTimeMillis: 0 -}; - -describe('internal api', () => { - let app: FirebaseApp; - let storageReadStub: SinonStub; - let storageWriteStub: SinonStub; - - beforeEach(() => { - app = getFullApp(); - storageReadStub = stub(storage, 'readTokenFromStorage').resolves(undefined); - storageWriteStub = stub(storage, 'writeTokenToStorage'); - stub(util, 'getRecaptcha').returns(getFakeGreCAPTCHA()); - }); - - afterEach(() => { - clearState(); - removegreCAPTCHAScriptsOnPage(); - return deleteApp(app); - }); - // TODO: test error conditions - describe('getToken()', () => { - it('uses reCAPTCHA token to exchange for AppCheck token', async () => { - const appCheck = initializeAppCheck(app, { - provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) - }); - const exchangeTokenStub: SinonStub = stub( - client, - 'exchangeToken' - ).returns( - Promise.reject( - new FirebaseError('test-error', 'test error msg', { httpStatus: 503 }) - ) - ); - - const token = await getToken(appCheck as AppCheckService); - - expect(exchangeTokenStub.args[0][0].body['recaptcha_token']).to.equal( - fakeRecaptchaToken - ); - expect(token).to.deep.equal({ token: fakeRecaptchaAppCheckToken.token }); - }); - - it('resolves with a dummy token and an error if failed to get a token', async () => { - const errorStub = stub(console, 'error'); - const appCheck = initializeAppCheck(app, { - provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) - }); - - const reCAPTCHASpy = stub(reCAPTCHA, 'getToken').returns( - Promise.resolve(fakeRecaptchaToken) - ); - - const error = new Error('oops, something went wrong'); - stub(client, 'exchangeToken').returns(Promise.reject(error)); - - const token = await getToken(appCheck as AppCheckService); - - expect(reCAPTCHASpy).to.be.called; - expect(token).to.deep.equal({ - token: formatDummyToken(defaultTokenErrorData), - error - }); - expect(errorStub.args[0][1].message).to.include( - 'oops, something went wrong' - ); - errorStub.restore(); - }); - - it('notifies listeners using cached token', async () => { - storageReadStub.resolves(fakeCachedAppCheckToken); - const appCheck = initializeAppCheck(app, { - provider: new ReCaptchaV3Provider(FAKE_SITE_KEY), - isTokenAutoRefreshEnabled: false - }); - - const clock = useFakeTimers(); - - const listener1 = spy(); - const listener2 = spy(); - addTokenListener( - appCheck as AppCheckService, - ListenerType.INTERNAL, - listener1 - ); - addTokenListener( - appCheck as AppCheckService, - ListenerType.INTERNAL, - listener2 - ); - - await getToken(appCheck as AppCheckService); - - clock.tick(1); - - expect(listener1).to.be.calledWith({ - token: fakeCachedAppCheckToken.token - }); - expect(listener2).to.be.calledWith({ - token: fakeCachedAppCheckToken.token - }); - - clock.restore(); - }); - - it('notifies listeners using new token', async () => { - const appCheck = initializeAppCheck(app, { - provider: new ReCaptchaV3Provider(FAKE_SITE_KEY), - isTokenAutoRefreshEnabled: true - }); - - stub(reCAPTCHA, 'getToken').returns(Promise.resolve(fakeRecaptchaToken)); - stub(client, 'exchangeToken').returns( - Promise.resolve(fakeRecaptchaAppCheckToken) - ); - - const listener1 = spy(); - const listener2 = spy(); - addTokenListener( - appCheck as AppCheckService, - ListenerType.INTERNAL, - listener1 - ); - addTokenListener( - appCheck as AppCheckService, - ListenerType.INTERNAL, - listener2 - ); - - await getToken(appCheck as AppCheckService); - - expect(listener1).to.be.calledWith({ - token: fakeRecaptchaAppCheckToken.token - }); - expect(listener2).to.be.calledWith({ - token: fakeRecaptchaAppCheckToken.token - }); - }); - - it('calls 3P error handler if there is an error getting a token', async () => { - stub(console, 'error'); - const appCheck = initializeAppCheck(app, { - provider: new ReCaptchaV3Provider(FAKE_SITE_KEY), - isTokenAutoRefreshEnabled: true - }); - stub(reCAPTCHA, 'getToken').returns(Promise.resolve(fakeRecaptchaToken)); - stub(client, 'exchangeToken').rejects('exchange error'); - const listener1 = spy(); - const errorFn1 = spy(); - - addTokenListener( - appCheck as AppCheckService, - ListenerType.EXTERNAL, - listener1, - errorFn1 - ); - - await getToken(appCheck as AppCheckService); - - expect(errorFn1).to.be.calledOnce; - expect(errorFn1.args[0][0].name).to.include('exchange error'); - }); - - it('ignores listeners that throw', async () => { - stub(console, 'error'); - const appCheck = initializeAppCheck(app, { - provider: new ReCaptchaV3Provider(FAKE_SITE_KEY), - isTokenAutoRefreshEnabled: true - }); - stub(reCAPTCHA, 'getToken').returns(Promise.resolve(fakeRecaptchaToken)); - stub(client, 'exchangeToken').returns( - Promise.resolve(fakeRecaptchaAppCheckToken) - ); - const listener1 = stub().throws(new Error()); - const listener2 = spy(); - - addTokenListener( - appCheck as AppCheckService, - ListenerType.INTERNAL, - listener1 - ); - addTokenListener( - appCheck as AppCheckService, - ListenerType.INTERNAL, - listener2 - ); - - await getToken(appCheck as AppCheckService); - - expect(listener1).to.be.calledWith({ - token: fakeRecaptchaAppCheckToken.token - }); - expect(listener2).to.be.calledWith({ - token: fakeRecaptchaAppCheckToken.token - }); - }); - - it('loads persisted token to memory and returns it', async () => { - const clock = useFakeTimers(); - - storageReadStub.resolves(fakeCachedAppCheckToken); - const appCheck = initializeAppCheck(app, { - provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) - }); - - const clientStub = stub(client, 'exchangeToken'); - - expect(getState(app).token).to.equal(undefined); - expect(await getToken(appCheck as AppCheckService)).to.deep.equal({ - token: fakeCachedAppCheckToken.token - }); - expect(getState(app).token).to.equal(fakeCachedAppCheckToken); - expect(clientStub).has.not.been.called; - - clock.restore(); - }); - - it('persists token to storage', async () => { - const appCheck = initializeAppCheck(app, { - provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) - }); - - stub(reCAPTCHA, 'getToken').returns(Promise.resolve(fakeRecaptchaToken)); - stub(client, 'exchangeToken').returns( - Promise.resolve(fakeRecaptchaAppCheckToken) - ); - storageWriteStub.resetHistory(); - const result = await getToken(appCheck as AppCheckService); - expect(result).to.deep.equal({ token: fakeRecaptchaAppCheckToken.token }); - expect(storageWriteStub).has.been.calledWith( - app, - fakeRecaptchaAppCheckToken - ); - }); - - it('returns the valid token in memory without making network request', async () => { - const clock = useFakeTimers(); - const appCheck = initializeAppCheck(app, { - provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) - }); - setState(app, { ...getState(app), token: fakeRecaptchaAppCheckToken }); - - const clientStub = stub(client, 'exchangeToken'); - expect(await getToken(appCheck as AppCheckService)).to.deep.equal({ - token: fakeRecaptchaAppCheckToken.token - }); - expect(clientStub).to.not.have.been.called; - - clock.restore(); - }); - - it('force to get new token when forceRefresh is true', async () => { - const appCheck = initializeAppCheck(app, { - provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) - }); - setState(app, { ...getState(app), token: fakeRecaptchaAppCheckToken }); - - stub(reCAPTCHA, 'getToken').returns(Promise.resolve(fakeRecaptchaToken)); - stub(client, 'exchangeToken').returns( - Promise.resolve({ - token: 'new-recaptcha-app-check-token', - expireTimeMillis: Date.now() + 60000, - issuedAtTimeMillis: 0 - }) - ); - - expect(await getToken(appCheck as AppCheckService, true)).to.deep.equal({ - token: 'new-recaptcha-app-check-token' - }); - }); - - it('exchanges debug token if in debug mode and there is no cached token', async () => { - const exchangeTokenStub: SinonStub = stub( - client, - 'exchangeToken' - ).returns(Promise.resolve(fakeRecaptchaAppCheckToken)); - const debugState = getDebugState(); - debugState.enabled = true; - debugState.token = new Deferred(); - debugState.token.resolve('my-debug-token'); - const appCheck = initializeAppCheck(app, { - provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) - }); - - const token = await getToken(appCheck as AppCheckService); - expect(exchangeTokenStub.args[0][0].body['debug_token']).to.equal( - 'my-debug-token' - ); - expect(token).to.deep.equal({ token: fakeRecaptchaAppCheckToken.token }); - }); - }); -}); diff --git a/packages/app-check/src/providers.ts b/packages/app-check/src/providers.ts index 8f2f4a7912e..1e924b37ee6 100644 --- a/packages/app-check/src/providers.ts +++ b/packages/app-check/src/providers.ts @@ -36,6 +36,7 @@ import { initializeEnterprise as initializeRecaptchaEnterprise } from './recaptcha'; import { AppCheckProvider, AppCheckTokenInternal, ThrottleData } from './types'; +import { getDurationString } from './util'; /** * App Check provider that can obtain a reCAPTCHA V3 token and exchange it @@ -104,7 +105,7 @@ export class ReCaptchaV3Provider implements AppCheckProvider { Number((e as FirebaseError).customData?.httpStatus) ); throw ERROR_FACTORY.create(AppCheckError.THROTTLED, { - time: new Date(throttleData.allowRequestsAfter).toLocaleString(), + time: getDurationString(throttleData.allowRequestsAfter - Date.now()), httpStatus: throttleData.httpStatus }); } else { diff --git a/packages/app-check/src/util.ts b/packages/app-check/src/util.ts index dd699141da8..2d077961370 100644 --- a/packages/app-check/src/util.ts +++ b/packages/app-check/src/util.ts @@ -47,3 +47,30 @@ export function uuidv4(): string { return v.toString(16); }); } + +export function getDurationString(durationInMillis: number): string { + const totalSeconds = Math.round(durationInMillis / 1000); + const days = Math.floor(totalSeconds / (3600 * 24)); + const hours = Math.floor((totalSeconds - days * 3600 * 24) / 3600); + const minutes = Math.floor( + (totalSeconds - days * 3600 * 24 - hours * 3600) / 60 + ); + const seconds = totalSeconds - days * 3600 * 24 - hours * 3600 - minutes * 60; + + let result = ''; + if (days) { + result += pad(days) + 'd:'; + } + if (hours) { + result += pad(hours) + 'h:'; + } + result += pad(minutes) + 'm:' + pad(seconds) + 's'; + return result; +} + +function pad(value: number): string { + if (value === 0) { + return '00'; + } + return value >= 10 ? value.toString() : '0' + value; +} From b26bbcef5bb231314067c088e435e55fa85ebf98 Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Tue, 26 Oct 2021 14:33:41 -0700 Subject: [PATCH 10/17] Add a provider test --- packages/app-check/src/errors.ts | 4 +- packages/app-check/src/internal-api.test.ts | 2 + packages/app-check/src/providers.test.ts | 96 +++++++++++++++ packages/app-check/src/providers.ts | 123 ++++++++++---------- 4 files changed, 164 insertions(+), 61 deletions(-) create mode 100644 packages/app-check/src/providers.test.ts diff --git a/packages/app-check/src/errors.ts b/packages/app-check/src/errors.ts index 1b23ea46c01..c6f088b371b 100644 --- a/packages/app-check/src/errors.ts +++ b/packages/app-check/src/errors.ts @@ -54,7 +54,7 @@ const ERRORS: ErrorMap = { [AppCheckError.STORAGE_WRITE]: 'Error thrown when writing to storage. Original error: {$originalErrorMessage}.', [AppCheckError.RECAPTCHA_ERROR]: 'ReCAPTCHA error.', - [AppCheckError.THROTTLED]: `Requests throttled until {$time} due to {$httpStatus} error.` + [AppCheckError.THROTTLED]: `Requests throttled due to {$httpStatus} error. Attempts allowed again after {$time}` }; interface ErrorParams { @@ -66,7 +66,7 @@ interface ErrorParams { [AppCheckError.STORAGE_OPEN]: { originalErrorMessage?: string }; [AppCheckError.STORAGE_GET]: { originalErrorMessage?: string }; [AppCheckError.STORAGE_WRITE]: { originalErrorMessage?: string }; - [AppCheckError.THROTTLED]: { time: string, httpStatus: number }; + [AppCheckError.THROTTLED]: { time: string; httpStatus: number }; } export const ERROR_FACTORY = new ErrorFactory( diff --git a/packages/app-check/src/internal-api.test.ts b/packages/app-check/src/internal-api.test.ts index 8ac58770965..fedbc1880e1 100644 --- a/packages/app-check/src/internal-api.test.ts +++ b/packages/app-check/src/internal-api.test.ts @@ -387,6 +387,7 @@ describe('internal api', () => { ); expect(token).to.deep.equal({ token: fakeRecaptchaAppCheckToken.token }); }); + it('throttles exponentially on 503', async () => { const appCheck = initializeAppCheck(app, { provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) @@ -408,6 +409,7 @@ describe('internal api', () => { expect(token.error?.message).to.include('00m'); expect(warnStub.args[0][0]).to.include('503'); }); + it('throttles 1d on 403', async () => { const appCheck = initializeAppCheck(app, { provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) diff --git a/packages/app-check/src/providers.test.ts b/packages/app-check/src/providers.test.ts new file mode 100644 index 00000000000..1245d4f12f5 --- /dev/null +++ b/packages/app-check/src/providers.test.ts @@ -0,0 +1,96 @@ +import '../test/setup'; +import { getFakeGreCAPTCHA, getFullApp } from '../test/util'; +import { ReCaptchaV3Provider } from './providers'; +import * as client from './client'; +import * as reCAPTCHA from './recaptcha'; +import * as util from './util'; +import { stub, useFakeTimers } from 'sinon'; +import { expect } from 'chai'; +import { FirebaseError } from '@firebase/util'; +import { AppCheckError } from './errors'; +import { clearState } from './state'; +import { deleteApp, FirebaseApp } from '@firebase/app'; + +describe('ReCaptchaV3Provider', () => { + let app: FirebaseApp; + let clock = useFakeTimers(); + beforeEach(() => { + clock = useFakeTimers(); + app = getFullApp(); + stub(util, 'getRecaptcha').returns(getFakeGreCAPTCHA()); + stub(reCAPTCHA, 'getToken').returns( + Promise.resolve('fake-recaptcha-token') + ); + }); + + afterEach(() => { + clock.restore(); + clearState(); + return deleteApp(app); + }); + it('getToken() gets a token from the exchange endpoint', async () => { + const app = getFullApp(); + const provider = new ReCaptchaV3Provider('fake-site-key'); + stub(client, 'exchangeToken').resolves({ + token: 'fake-exchange-token', + issuedAtTimeMillis: 0, + expireTimeMillis: 10 + }); + provider.initialize(app); + const token = await provider.getToken(); + expect(token.token).to.equal('fake-exchange-token'); + }); + it('getToken() throttles 1d on 403', async () => { + const app = getFullApp(); + const provider = new ReCaptchaV3Provider('fake-site-key'); + stub(client, 'exchangeToken').rejects( + new FirebaseError(AppCheckError.FETCH_STATUS_ERROR, 'some-message', { + httpStatus: 403 + }) + ); + provider.initialize(app); + await expect(provider.getToken()).to.be.rejectedWith('1d'); + // Wait 10s and try again to see if wait time string decreases. + clock.tick(10000); + await expect(provider.getToken()).to.be.rejectedWith('23h'); + }); + it('getToken() throttles exponentially on 503', async () => { + const app = getFullApp(); + const provider = new ReCaptchaV3Provider('fake-site-key'); + let exchangeTokenStub = stub(client, 'exchangeToken').rejects( + new FirebaseError(AppCheckError.FETCH_STATUS_ERROR, 'some-message', { + httpStatus: 503 + }) + ); + provider.initialize(app); + await expect(provider.getToken()).to.be.rejectedWith('503'); + expect(exchangeTokenStub).to.be.called; + exchangeTokenStub.resetHistory(); + // Try again immediately, should be rejected. + await expect(provider.getToken()).to.be.rejectedWith('503'); + expect(exchangeTokenStub).not.to.be.called; + exchangeTokenStub.resetHistory(); + // Wait for 1.5 seconds to pass, should call exchange endpoint again + // (and be rejected again) + clock.tick(1500); + await expect(provider.getToken()).to.be.rejectedWith('503'); + expect(exchangeTokenStub).to.be.called; + exchangeTokenStub.resetHistory(); + // Wait for 10 seconds to pass, should call exchange endpoint again + // (and be rejected again) + clock.tick(10000); + await expect(provider.getToken()).to.be.rejectedWith('503'); + expect(exchangeTokenStub).to.be.called; + // Wait for 10 seconds to pass, should call exchange endpoint again + // (and succeed) + clock.tick(10000); + exchangeTokenStub.restore(); + exchangeTokenStub = stub(client, 'exchangeToken').resolves({ + token: 'fake-exchange-token', + issuedAtTimeMillis: 0, + expireTimeMillis: 10 + }); + const token = await provider.getToken(); + expect(token.token).to.equal('fake-exchange-token'); + }); +}); diff --git a/packages/app-check/src/providers.ts b/packages/app-check/src/providers.ts index 1e924b37ee6..752ae89d101 100644 --- a/packages/app-check/src/providers.ts +++ b/packages/app-check/src/providers.ts @@ -63,20 +63,8 @@ export class ReCaptchaV3Provider implements AppCheckProvider { * @internal */ async getToken(): Promise { - if (this._throttleData) { - if (Date.now() - this._throttleData.allowRequestsAfter > 0) { - // If after throttle timestamp, clear throttle data. - this._throttleData = null; - } else { - // If before, throw. - throw ERROR_FACTORY.create(AppCheckError.THROTTLED, { - time: new Date( - this._throttleData.allowRequestsAfter - ).toLocaleString(), - httpStatus: this._throttleData.httpStatus - }); - } - } + throwIfThrottled(this._throttleData); + if (!this._app || !this._platformLoggerProvider) { // This should only occur if user has not called initializeAppCheck(). // We don't have an appName to provide if so. @@ -101,61 +89,25 @@ export class ReCaptchaV3Provider implements AppCheckProvider { ); } catch (e) { if ((e as FirebaseError).code === AppCheckError.FETCH_STATUS_ERROR) { - const throttleData = this._setBackoff( - Number((e as FirebaseError).customData?.httpStatus) + this._throttleData = setBackoff( + Number((e as FirebaseError).customData?.httpStatus), + this._throttleData ); throw ERROR_FACTORY.create(AppCheckError.THROTTLED, { - time: getDurationString(throttleData.allowRequestsAfter - Date.now()), - httpStatus: throttleData.httpStatus + time: getDurationString( + this._throttleData.allowRequestsAfter - Date.now() + ), + httpStatus: this._throttleData.httpStatus }); } else { throw e; } } + // If successful, clear throttle data. + this._throttleData = null; return result; } - /** - * Set throttle data to block requests until after a certain time - * depending on the failed request's status code. - * @param httpStatus - Status code of failed request. - * @returns Data about current throttle state and expiration time. - */ - private _setBackoff(httpStatus: number): ThrottleData { - /** - * Block retries for 1 day for the following error codes: - * - * 404: Likely malformed URL. - * - * 403: - * - Attestation failed - * - Wrong API key - * - Project deleted - */ - if (httpStatus === 404 || httpStatus === 403) { - this._throttleData = { - backoffCount: 1, - allowRequestsAfter: Date.now() + ONE_DAY, - httpStatus - }; - } else { - /** - * For all other error codes, the time when it is ok to retry again - * is based on exponential backoff. - */ - const backoffCount = this._throttleData - ? this._throttleData.backoffCount - : 0; - const backoffMillis = calculateBackoffMillis(backoffCount, 1000, 2); - this._throttleData = { - backoffCount: backoffCount + 1, - allowRequestsAfter: Date.now() + backoffMillis, - httpStatus - }; - } - return this._throttleData; - } - /** * @internal */ @@ -290,3 +242,56 @@ export class CustomProvider implements AppCheckProvider { } } } + +/** + * Set throttle data to block requests until after a certain time + * depending on the failed request's status code. + * @param httpStatus - Status code of failed request. + * @returns Data about current throttle state and expiration time. + */ +function setBackoff( + httpStatus: number, + throttleData: ThrottleData | null +): ThrottleData { + /** + * Block retries for 1 day for the following error codes: + * + * 404: Likely malformed URL. + * + * 403: + * - Attestation failed + * - Wrong API key + * - Project deleted + */ + if (httpStatus === 404 || httpStatus === 403) { + return { + backoffCount: 1, + allowRequestsAfter: Date.now() + ONE_DAY, + httpStatus + }; + } else { + /** + * For all other error codes, the time when it is ok to retry again + * is based on exponential backoff. + */ + const backoffCount = throttleData ? throttleData.backoffCount : 0; + const backoffMillis = calculateBackoffMillis(backoffCount, 1000, 2); + return { + backoffCount: backoffCount + 1, + allowRequestsAfter: Date.now() + backoffMillis, + httpStatus + }; + } +} + +function throwIfThrottled(throttleData: ThrottleData | null): void { + if (throttleData) { + if (Date.now() - throttleData.allowRequestsAfter <= 0) { + // If before, throw. + throw ERROR_FACTORY.create(AppCheckError.THROTTLED, { + time: getDurationString(throttleData.allowRequestsAfter - Date.now()), + httpStatus: throttleData.httpStatus + }); + } + } +} From 9518090cfc48c1121ecfffd9062fcb51d210869d Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Thu, 28 Oct 2021 15:11:58 -0700 Subject: [PATCH 11/17] Remove retry blocker --- packages/app-check/src/internal-api.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/app-check/src/internal-api.ts b/packages/app-check/src/internal-api.ts index e7976d64fb0..b7885b1b524 100644 --- a/packages/app-check/src/internal-api.ts +++ b/packages/app-check/src/internal-api.ts @@ -279,11 +279,7 @@ function createTokenRefresher(appCheck: AppCheckService): Refresher { throw result.error; } }, - (error: unknown) => { - // Do not queue up retries if it's in a throttled state. - if ((error as FirebaseError).code === AppCheckError.THROTTLED) { - return false; - } + () => { return true; }, () => { From 7215ba1e9ea41f6df7f3891d5a865e84bcaedbed Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Fri, 29 Oct 2021 15:26:23 -0700 Subject: [PATCH 12/17] Formatting fix --- packages/app-check/src/constants.ts | 2 +- packages/app-check/src/internal-api.test.ts | 2 +- packages/app-check/src/providers.test.ts | 17 +++++++++++++++++ 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/packages/app-check/src/constants.ts b/packages/app-check/src/constants.ts index dc8ef26275d..8e61b3a8745 100644 --- a/packages/app-check/src/constants.ts +++ b/packages/app-check/src/constants.ts @@ -42,4 +42,4 @@ export const TOKEN_REFRESH_TIME = { /** * One day in millis, for certain error code backoffs. */ - export const ONE_DAY = 24 * 60 * 60 * 1000; \ No newline at end of file +export const ONE_DAY = 24 * 60 * 60 * 1000; diff --git a/packages/app-check/src/internal-api.test.ts b/packages/app-check/src/internal-api.test.ts index fedbc1880e1..0bc789f6e2f 100644 --- a/packages/app-check/src/internal-api.test.ts +++ b/packages/app-check/src/internal-api.test.ts @@ -409,7 +409,7 @@ describe('internal api', () => { expect(token.error?.message).to.include('00m'); expect(warnStub.args[0][0]).to.include('503'); }); - + it('throttles 1d on 403', async () => { const appCheck = initializeAppCheck(app, { provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) diff --git a/packages/app-check/src/providers.test.ts b/packages/app-check/src/providers.test.ts index 1245d4f12f5..7821dac621f 100644 --- a/packages/app-check/src/providers.test.ts +++ b/packages/app-check/src/providers.test.ts @@ -1,3 +1,20 @@ +/** + * @license + * Copyright 2021 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + import '../test/setup'; import { getFakeGreCAPTCHA, getFullApp } from '../test/util'; import { ReCaptchaV3Provider } from './providers'; From f63f8c5780c4cadb940fa73c63cc1a1f4de87add Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Mon, 8 Nov 2021 14:08:15 -0800 Subject: [PATCH 13/17] Add changeset --- .changeset/late-bottles-whisper.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/late-bottles-whisper.md diff --git a/.changeset/late-bottles-whisper.md b/.changeset/late-bottles-whisper.md new file mode 100644 index 00000000000..5a0aa106720 --- /dev/null +++ b/.changeset/late-bottles-whisper.md @@ -0,0 +1,5 @@ +--- +'@firebase/app-check': patch +--- + +Block exchange requests for certain periods of time after certain error codes to prevent overwhelming the endpoint. Start token listener when App Check is initialized to avoid extra wait time on first getToken() call. From f9f5e2a9dc6c5591c86e036b0946b635189f8187 Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Wed, 10 Nov 2021 09:59:10 -0800 Subject: [PATCH 14/17] Address PR comments --- packages/app-check/src/api.test.ts | 6 +++--- packages/app-check/src/api.ts | 17 +++++++++++------ packages/app-check/src/internal-api.test.ts | 13 +++++++++++++ packages/app-check/src/providers.test.ts | 10 ++++++---- packages/app-check/src/providers.ts | 3 +++ 5 files changed, 36 insertions(+), 13 deletions(-) diff --git a/packages/app-check/src/api.test.ts b/packages/app-check/src/api.test.ts index 09681d8b19e..724839226af 100644 --- a/packages/app-check/src/api.test.ts +++ b/packages/app-check/src/api.test.ts @@ -380,7 +380,7 @@ describe('api', () => { isTokenAutoRefreshEnabled: false }); - expect(getState(app).tokenObservers.length).to.equal(1); + expect(getState(app).tokenObservers.length).to.equal(0); const fakeRecaptchaToken = 'fake-recaptcha-token'; stub(reCAPTCHA, 'getToken').returns(Promise.resolve(fakeRecaptchaToken)); @@ -395,13 +395,13 @@ describe('api', () => { await internalApi.getToken(appCheck as AppCheckService); - expect(getState(app).tokenObservers.length).to.equal(2); + expect(getState(app).tokenObservers.length).to.equal(1); expect(errorFn1).to.be.calledOnce; expect(errorFn1.args[0][0].name).to.include('exchange error'); unsubscribe1(); - expect(getState(app).tokenObservers.length).to.equal(1); + expect(getState(app).tokenObservers.length).to.equal(0); }); }); }); diff --git a/packages/app-check/src/api.ts b/packages/app-check/src/api.ts index 98d298bd8cb..b885cec1201 100644 --- a/packages/app-check/src/api.ts +++ b/packages/app-check/src/api.ts @@ -98,12 +98,17 @@ export function initializeAppCheck( const appCheck = provider.initialize({ options }); _activate(app, options.provider, options.isTokenAutoRefreshEnabled); - // Adding a listener will start the refresher and fetch a token if needed. - // This gets a token ready and prevents a delay when an internal library - // requests the token. - // Listener function does not need to do anything, its base functionality - // of calling getToken() already fetches token and writes it to memory/storage. - addTokenListener(appCheck, ListenerType.INTERNAL, () => {}); + // If isTokenAutoRefreshEnabled is false, do not send any requests to the + // exchange endpoint without an explicit call from the user either directly + // or through another Firebase library (storage, functions, etc.) + if (getState(app).isTokenAutoRefreshEnabled) { + // Adding a listener will start the refresher and fetch a token if needed. + // This gets a token ready and prevents a delay when an internal library + // requests the token. + // Listener function does not need to do anything, its base functionality + // of calling getToken() already fetches token and writes it to memory/storage. + addTokenListener(appCheck, ListenerType.INTERNAL, () => {}); + } return appCheck; } diff --git a/packages/app-check/src/internal-api.test.ts b/packages/app-check/src/internal-api.test.ts index 0bc789f6e2f..ff99b3fe53a 100644 --- a/packages/app-check/src/internal-api.test.ts +++ b/packages/app-check/src/internal-api.test.ts @@ -405,6 +405,11 @@ describe('internal api', () => { const token = await getToken(appCheck as AppCheckService); + // ReCaptchaV3Provider's _throttleData is private so checking + // the resulting error message to be sure it has roughly the + // correct throttle time. This also tests the time formatter. + // Check both the error itself and that it makes it through to + // console.warn expect(token.error?.message).to.include('503'); expect(token.error?.message).to.include('00m'); expect(warnStub.args[0][0]).to.include('503'); @@ -427,6 +432,12 @@ describe('internal api', () => { const token = await getToken(appCheck as AppCheckService); + + // ReCaptchaV3Provider's _throttleData is private so checking + // the resulting error message to be sure it has roughly the + // correct throttle time. This also tests the time formatter. + // Check both the error itself and that it makes it through to + // console.warn expect(token.error?.message).to.include('403'); expect(token.error?.message).to.include('1d'); expect(warnStub.args[0][0]).to.include('403'); @@ -466,6 +477,8 @@ describe('internal api', () => { listener ); + expect(getState(app).tokenRefresher?.isRunning()).to.be.undefined; + // addTokenListener() waits for the result of cachedTokenPromise // before starting the refresher await getState(app).cachedTokenPromise; diff --git a/packages/app-check/src/providers.test.ts b/packages/app-check/src/providers.test.ts index 7821dac621f..a07871f3aa6 100644 --- a/packages/app-check/src/providers.test.ts +++ b/packages/app-check/src/providers.test.ts @@ -87,20 +87,22 @@ describe('ReCaptchaV3Provider', () => { await expect(provider.getToken()).to.be.rejectedWith('503'); expect(exchangeTokenStub).not.to.be.called; exchangeTokenStub.resetHistory(); + // Times below are max range of each random exponential wait, + // the possible range is 2^(backoff_count) plus or minus 50% // Wait for 1.5 seconds to pass, should call exchange endpoint again // (and be rejected again) clock.tick(1500); await expect(provider.getToken()).to.be.rejectedWith('503'); expect(exchangeTokenStub).to.be.called; exchangeTokenStub.resetHistory(); - // Wait for 10 seconds to pass, should call exchange endpoint again + // Wait for 3 seconds to pass, should call exchange endpoint again // (and be rejected again) - clock.tick(10000); + clock.tick(3000); await expect(provider.getToken()).to.be.rejectedWith('503'); expect(exchangeTokenStub).to.be.called; - // Wait for 10 seconds to pass, should call exchange endpoint again + // Wait for 6 seconds to pass, should call exchange endpoint again // (and succeed) - clock.tick(10000); + clock.tick(6000); exchangeTokenStub.restore(); exchangeTokenStub = stub(client, 'exchangeToken').resolves({ token: 'fake-exchange-token', diff --git a/packages/app-check/src/providers.ts b/packages/app-check/src/providers.ts index 752ae89d101..a97d0c71159 100644 --- a/packages/app-check/src/providers.ts +++ b/packages/app-check/src/providers.ts @@ -93,6 +93,7 @@ export class ReCaptchaV3Provider implements AppCheckProvider { Number((e as FirebaseError).customData?.httpStatus), this._throttleData ); + console.log('next interval', this._throttleData.allowRequestsAfter - Date.now()); throw ERROR_FACTORY.create(AppCheckError.THROTTLED, { time: getDurationString( this._throttleData.allowRequestsAfter - Date.now() @@ -247,6 +248,8 @@ export class CustomProvider implements AppCheckProvider { * Set throttle data to block requests until after a certain time * depending on the failed request's status code. * @param httpStatus - Status code of failed request. + * @param throttleData - `ThrottleData` object containing previous throttle + * data state. * @returns Data about current throttle state and expiration time. */ function setBackoff( From 7ca267ebb0fa64377515d1ef3841ab5666e76023 Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Wed, 10 Nov 2021 10:10:47 -0800 Subject: [PATCH 15/17] Rebase and add recaptcha enterprise --- packages/app-check/src/providers.test.ts | 89 +++++++++++++++++++++++- packages/app-check/src/providers.ts | 53 +++++++++----- 2 files changed, 122 insertions(+), 20 deletions(-) diff --git a/packages/app-check/src/providers.test.ts b/packages/app-check/src/providers.test.ts index a07871f3aa6..78c30811d97 100644 --- a/packages/app-check/src/providers.test.ts +++ b/packages/app-check/src/providers.test.ts @@ -17,7 +17,7 @@ import '../test/setup'; import { getFakeGreCAPTCHA, getFullApp } from '../test/util'; -import { ReCaptchaV3Provider } from './providers'; +import { ReCaptchaEnterpriseProvider, ReCaptchaV3Provider } from './providers'; import * as client from './client'; import * as reCAPTCHA from './recaptcha'; import * as util from './util'; @@ -113,3 +113,90 @@ describe('ReCaptchaV3Provider', () => { expect(token.token).to.equal('fake-exchange-token'); }); }); + +describe('ReCaptchaEnterpriseProvider', () => { + let app: FirebaseApp; + let clock = useFakeTimers(); + beforeEach(() => { + clock = useFakeTimers(); + app = getFullApp(); + stub(util, 'getRecaptcha').returns(getFakeGreCAPTCHA()); + stub(reCAPTCHA, 'getToken').returns( + Promise.resolve('fake-recaptcha-token') + ); + }); + + afterEach(() => { + clock.restore(); + clearState(); + return deleteApp(app); + }); + it('getToken() gets a token from the exchange endpoint', async () => { + const app = getFullApp(); + const provider = new ReCaptchaEnterpriseProvider('fake-site-key'); + stub(client, 'exchangeToken').resolves({ + token: 'fake-exchange-token', + issuedAtTimeMillis: 0, + expireTimeMillis: 10 + }); + provider.initialize(app); + const token = await provider.getToken(); + expect(token.token).to.equal('fake-exchange-token'); + }); + it('getToken() throttles 1d on 403', async () => { + const app = getFullApp(); + const provider = new ReCaptchaEnterpriseProvider('fake-site-key'); + stub(client, 'exchangeToken').rejects( + new FirebaseError(AppCheckError.FETCH_STATUS_ERROR, 'some-message', { + httpStatus: 403 + }) + ); + provider.initialize(app); + await expect(provider.getToken()).to.be.rejectedWith('1d'); + // Wait 10s and try again to see if wait time string decreases. + clock.tick(10000); + await expect(provider.getToken()).to.be.rejectedWith('23h'); + }); + it('getToken() throttles exponentially on 503', async () => { + const app = getFullApp(); + const provider = new ReCaptchaEnterpriseProvider('fake-site-key'); + let exchangeTokenStub = stub(client, 'exchangeToken').rejects( + new FirebaseError(AppCheckError.FETCH_STATUS_ERROR, 'some-message', { + httpStatus: 503 + }) + ); + provider.initialize(app); + await expect(provider.getToken()).to.be.rejectedWith('503'); + expect(exchangeTokenStub).to.be.called; + exchangeTokenStub.resetHistory(); + // Try again immediately, should be rejected. + await expect(provider.getToken()).to.be.rejectedWith('503'); + expect(exchangeTokenStub).not.to.be.called; + exchangeTokenStub.resetHistory(); + // Times below are max range of each random exponential wait, + // the possible range is 2^(backoff_count) plus or minus 50% + // Wait for 1.5 seconds to pass, should call exchange endpoint again + // (and be rejected again) + clock.tick(1500); + await expect(provider.getToken()).to.be.rejectedWith('503'); + expect(exchangeTokenStub).to.be.called; + exchangeTokenStub.resetHistory(); + // Wait for 3 seconds to pass, should call exchange endpoint again + // (and be rejected again) + clock.tick(3000); + await expect(provider.getToken()).to.be.rejectedWith('503'); + expect(exchangeTokenStub).to.be.called; + // Wait for 6 seconds to pass, should call exchange endpoint again + // (and succeed) + clock.tick(6000); + exchangeTokenStub.restore(); + exchangeTokenStub = stub(client, 'exchangeToken').resolves({ + token: 'fake-exchange-token', + issuedAtTimeMillis: 0, + expireTimeMillis: 10 + }); + const token = await provider.getToken(); + expect(token.token).to.equal('fake-exchange-token'); + }); +}); + diff --git a/packages/app-check/src/providers.ts b/packages/app-check/src/providers.ts index a97d0c71159..202b000ef97 100644 --- a/packages/app-check/src/providers.ts +++ b/packages/app-check/src/providers.ts @@ -64,15 +64,7 @@ export class ReCaptchaV3Provider implements AppCheckProvider { */ async getToken(): Promise { throwIfThrottled(this._throttleData); - - if (!this._app || !this._platformLoggerProvider) { - // This should only occur if user has not called initializeAppCheck(). - // We don't have an appName to provide if so. - // This should already be caught in the top level `getToken()` function. - throw ERROR_FACTORY.create(AppCheckError.USE_BEFORE_ACTIVATION, { - appName: '' - }); - } + // Top-level `getToken()` has already checked that App Check is initialized // and therefore this._app and this._platformLoggerProvider are available. const attestedClaimsToken = await getReCAPTCHAToken(this._app!).catch( @@ -84,8 +76,8 @@ export class ReCaptchaV3Provider implements AppCheckProvider { let result; try { result = await exchangeToken( - getExchangeRecaptchaV3TokenRequest(this._app, attestedClaimsToken), - this._platformLoggerProvider + getExchangeRecaptchaV3TokenRequest(this._app!, attestedClaimsToken), + this._platformLoggerProvider! ); } catch (e) { if ((e as FirebaseError).code === AppCheckError.FETCH_STATUS_ERROR) { @@ -93,7 +85,6 @@ export class ReCaptchaV3Provider implements AppCheckProvider { Number((e as FirebaseError).customData?.httpStatus), this._throttleData ); - console.log('next interval', this._throttleData.allowRequestsAfter - Date.now()); throw ERROR_FACTORY.create(AppCheckError.THROTTLED, { time: getDurationString( this._throttleData.allowRequestsAfter - Date.now() @@ -141,6 +132,11 @@ export class ReCaptchaV3Provider implements AppCheckProvider { export class ReCaptchaEnterpriseProvider implements AppCheckProvider { private _app?: FirebaseApp; private _platformLoggerProvider?: Provider<'platform-logger'>; + /** + * Throttle requests on certain error codes to prevent too many retries + * in a short time. + */ + private _throttleData: ThrottleData | null = null; /** * Create a ReCaptchaEnterpriseProvider instance. * @param siteKey - reCAPTCHA Enterprise score-based site key. @@ -152,6 +148,7 @@ export class ReCaptchaEnterpriseProvider implements AppCheckProvider { * @internal */ async getToken(): Promise { + throwIfThrottled(this._throttleData); // Top-level `getToken()` has already checked that App Check is initialized // and therefore this._app and this._platformLoggerProvider are available. const attestedClaimsToken = await getReCAPTCHAToken(this._app!).catch( @@ -160,13 +157,31 @@ export class ReCaptchaEnterpriseProvider implements AppCheckProvider { throw ERROR_FACTORY.create(AppCheckError.RECAPTCHA_ERROR); } ); - return exchangeToken( - getExchangeRecaptchaEnterpriseTokenRequest( - this._app!, - attestedClaimsToken - ), - this._platformLoggerProvider! - ); + let result; + try { + result = await exchangeToken( + getExchangeRecaptchaEnterpriseTokenRequest(this._app!, attestedClaimsToken), + this._platformLoggerProvider! + ); + } catch (e) { + if ((e as FirebaseError).code === AppCheckError.FETCH_STATUS_ERROR) { + this._throttleData = setBackoff( + Number((e as FirebaseError).customData?.httpStatus), + this._throttleData + ); + throw ERROR_FACTORY.create(AppCheckError.THROTTLED, { + time: getDurationString( + this._throttleData.allowRequestsAfter - Date.now() + ), + httpStatus: this._throttleData.httpStatus + }); + } else { + throw e; + } + } + // If successful, clear throttle data. + this._throttleData = null; + return result; } /** From 12d7e28810cd37006c8df9f97b34a4aaeadaf657 Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Wed, 10 Nov 2021 10:13:28 -0800 Subject: [PATCH 16/17] Formatting pass --- packages/app-check/src/internal-api.test.ts | 1 - packages/app-check/src/providers.test.ts | 1 - packages/app-check/src/providers.ts | 7 +++++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/app-check/src/internal-api.test.ts b/packages/app-check/src/internal-api.test.ts index ff99b3fe53a..da450c3e7ba 100644 --- a/packages/app-check/src/internal-api.test.ts +++ b/packages/app-check/src/internal-api.test.ts @@ -432,7 +432,6 @@ describe('internal api', () => { const token = await getToken(appCheck as AppCheckService); - // ReCaptchaV3Provider's _throttleData is private so checking // the resulting error message to be sure it has roughly the // correct throttle time. This also tests the time formatter. diff --git a/packages/app-check/src/providers.test.ts b/packages/app-check/src/providers.test.ts index 78c30811d97..c9734d1ffc3 100644 --- a/packages/app-check/src/providers.test.ts +++ b/packages/app-check/src/providers.test.ts @@ -199,4 +199,3 @@ describe('ReCaptchaEnterpriseProvider', () => { expect(token.token).to.equal('fake-exchange-token'); }); }); - diff --git a/packages/app-check/src/providers.ts b/packages/app-check/src/providers.ts index 202b000ef97..b47b00a02e3 100644 --- a/packages/app-check/src/providers.ts +++ b/packages/app-check/src/providers.ts @@ -64,7 +64,7 @@ export class ReCaptchaV3Provider implements AppCheckProvider { */ async getToken(): Promise { throwIfThrottled(this._throttleData); - + // Top-level `getToken()` has already checked that App Check is initialized // and therefore this._app and this._platformLoggerProvider are available. const attestedClaimsToken = await getReCAPTCHAToken(this._app!).catch( @@ -160,7 +160,10 @@ export class ReCaptchaEnterpriseProvider implements AppCheckProvider { let result; try { result = await exchangeToken( - getExchangeRecaptchaEnterpriseTokenRequest(this._app!, attestedClaimsToken), + getExchangeRecaptchaEnterpriseTokenRequest( + this._app!, + attestedClaimsToken + ), this._platformLoggerProvider! ); } catch (e) { From 5e0d22b14d4e29c90dcea7c209361024897e9029 Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Thu, 11 Nov 2021 12:31:15 -0800 Subject: [PATCH 17/17] Update test and description --- packages/app-check/src/internal-api.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/app-check/src/internal-api.test.ts b/packages/app-check/src/internal-api.test.ts index da450c3e7ba..f5d4954224f 100644 --- a/packages/app-check/src/internal-api.test.ts +++ b/packages/app-check/src/internal-api.test.ts @@ -388,7 +388,8 @@ describe('internal api', () => { expect(token).to.deep.equal({ token: fakeRecaptchaAppCheckToken.token }); }); - it('throttles exponentially on 503', async () => { + it('throttles for a period less than 1d on 503', async () => { + // More detailed check of exponential backoff in providers.test.ts const appCheck = initializeAppCheck(app, { provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) }); @@ -412,6 +413,7 @@ describe('internal api', () => { // console.warn expect(token.error?.message).to.include('503'); expect(token.error?.message).to.include('00m'); + expect(token.error?.message).to.not.include('1d'); expect(warnStub.args[0][0]).to.include('503'); });