Skip to content

Commit

Permalink
Add retry logic to app check (#5676)
Browse files Browse the repository at this point in the history
  • Loading branch information
hsubox76 authored Nov 16, 2021
1 parent acc5810 commit 6f0049e
Show file tree
Hide file tree
Showing 12 changed files with 561 additions and 72 deletions.
5 changes: 5 additions & 0 deletions .changeset/late-bottles-whisper.md
Original file line number Diff line number Diff line change
@@ -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.
17 changes: 13 additions & 4 deletions packages/app-check/src/api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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);

Expand All @@ -312,14 +315,17 @@ 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 () => {
const appCheck = initializeAppCheck(app, {
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',
Expand Down Expand Up @@ -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);

Expand All @@ -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 () => {
Expand All @@ -373,6 +379,9 @@ describe('api', () => {
provider: new ReCaptchaV3Provider(FAKE_SITE_KEY),
isTokenAutoRefreshEnabled: false
});

expect(getState(app).tokenObservers.length).to.equal(0);

const fakeRecaptchaToken = 'fake-recaptcha-token';
stub(reCAPTCHA, 'getToken').returns(Promise.resolve(fakeRecaptchaToken));
stub(client, 'exchangeToken').rejects('exchange error');
Expand Down
16 changes: 15 additions & 1 deletion packages/app-check/src/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -97,6 +98,17 @@ export function initializeAppCheck(

const appCheck = provider.initialize({ options });
_activate(app, options.provider, options.isTokenAutoRefreshEnabled);
// 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;
}
Expand All @@ -123,6 +135,8 @@ function _activate(
newState.cachedTokenPromise = readTokenFromStorage(app).then(cachedToken => {
if (cachedToken && isValid(cachedToken)) {
setState(app, { ...getState(app), token: cachedToken });
// notify all listeners with the cached token
notifyTokenListeners(app, { token: cachedToken.token });
}
return cachedToken;
});
Expand Down
5 changes: 5 additions & 0 deletions packages/app-check/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
7 changes: 5 additions & 2 deletions packages/app-check/src/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<AppCheckError> = {
Expand All @@ -52,7 +53,8 @@ const ERRORS: ErrorMap<AppCheckError> = {
'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 due to {$httpStatus} error. Attempts allowed again after {$time}`
};

interface ErrorParams {
Expand All @@ -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<AppCheckError, ErrorParams>(
Expand Down
76 changes: 73 additions & 3 deletions packages/app-check/src/internal-api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 } from '@firebase/util';
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 = {
Expand Down Expand Up @@ -385,6 +387,62 @@ describe('internal api', () => {
);
expect(token).to.deep.equal({ token: fakeRecaptchaAppCheckToken.token });
});

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)
});
const warnStub = stub(logger, 'warn');
stub(client, 'exchangeToken').returns(
Promise.reject(
new FirebaseError(
AppCheckError.FETCH_STATUS_ERROR,
'test error msg',
{ httpStatus: 503 }
)
)
);

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(token.error?.message).to.not.include('1d');
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);

// 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');
});
});

describe('addTokenListener', () => {
Expand All @@ -404,7 +462,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),
Expand All @@ -420,6 +478,12 @@ 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;

expect(getState(app).tokenRefresher?.isRunning()).to.be.true;
});

Expand All @@ -430,6 +494,7 @@ describe('internal api', () => {

setState(app, {
...getState(app),
cachedTokenPromise: Promise.resolve(undefined),
token: {
token: `fake-memory-app-check-token`,
expireTimeMillis: Date.now() + 60000,
Expand Down Expand Up @@ -493,7 +558,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, {
Expand All @@ -506,6 +571,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;

Expand Down
Loading

0 comments on commit 6f0049e

Please sign in to comment.