Skip to content

Commit 6f0049e

Browse files
hsubox76hsubox76
authored
Add retry logic to app check (#5676)
1 parent acc5810 commit 6f0049e

12 files changed

+561
-72
lines changed

‎.changeset/late-bottles-whisper.md

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@firebase/app-check': patch
3+
---
4+
5+
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.

‎packages/app-check/src/api.test.ts

+13-4
Original file line numberDiff line numberDiff line change
@@ -278,6 +278,9 @@ describe('api', () => {
278278
provider: new ReCaptchaV3Provider(FAKE_SITE_KEY),
279279
isTokenAutoRefreshEnabled: true
280280
});
281+
282+
expect(getState(app).tokenObservers.length).to.equal(1);
283+
281284
const fakeRecaptchaToken = 'fake-recaptcha-token';
282285
const fakeRecaptchaAppCheckToken = {
283286
token: 'fake-recaptcha-app-check-token',
@@ -299,7 +302,7 @@ describe('api', () => {
299302
const unsubscribe1 = onTokenChanged(appCheck, listener1, errorFn1);
300303
const unsubscribe2 = onTokenChanged(appCheck, listener2, errorFn2);
301304

302-
expect(getState(app).tokenObservers.length).to.equal(2);
305+
expect(getState(app).tokenObservers.length).to.equal(3);
303306

304307
await internalApi.getToken(appCheck as AppCheckService);
305308

@@ -312,14 +315,17 @@ describe('api', () => {
312315
expect(errorFn2).to.not.be.called;
313316
unsubscribe1();
314317
unsubscribe2();
315-
expect(getState(app).tokenObservers.length).to.equal(0);
318+
expect(getState(app).tokenObservers.length).to.equal(1);
316319
});
317320

318321
it('Listeners work when using Observer pattern', async () => {
319322
const appCheck = initializeAppCheck(app, {
320323
provider: new ReCaptchaV3Provider(FAKE_SITE_KEY),
321324
isTokenAutoRefreshEnabled: true
322325
});
326+
327+
expect(getState(app).tokenObservers.length).to.equal(1);
328+
323329
const fakeRecaptchaToken = 'fake-recaptcha-token';
324330
const fakeRecaptchaAppCheckToken = {
325331
token: 'fake-recaptcha-app-check-token',
@@ -351,7 +357,7 @@ describe('api', () => {
351357
error: errorFn1
352358
});
353359

354-
expect(getState(app).tokenObservers.length).to.equal(2);
360+
expect(getState(app).tokenObservers.length).to.equal(3);
355361

356362
await internalApi.getToken(appCheck as AppCheckService);
357363

@@ -364,7 +370,7 @@ describe('api', () => {
364370
expect(errorFn2).to.not.be.called;
365371
unsubscribe1();
366372
unsubscribe2();
367-
expect(getState(app).tokenObservers.length).to.equal(0);
373+
expect(getState(app).tokenObservers.length).to.equal(1);
368374
});
369375

370376
it('onError() catches token errors', async () => {
@@ -373,6 +379,9 @@ describe('api', () => {
373379
provider: new ReCaptchaV3Provider(FAKE_SITE_KEY),
374380
isTokenAutoRefreshEnabled: false
375381
});
382+
383+
expect(getState(app).tokenObservers.length).to.equal(0);
384+
376385
const fakeRecaptchaToken = 'fake-recaptcha-token';
377386
stub(reCAPTCHA, 'getToken').returns(Promise.resolve(fakeRecaptchaToken));
378387
stub(client, 'exchangeToken').rejects('exchange error');

‎packages/app-check/src/api.ts

+15-1
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,8 @@ import {
3232
getToken as getTokenInternal,
3333
addTokenListener,
3434
removeTokenListener,
35-
isValid
35+
isValid,
36+
notifyTokenListeners
3637
} from './internal-api';
3738
import { readTokenFromStorage } from './storage';
3839
import { getDebugToken, initializeDebugMode, isDebugMode } from './debug';
@@ -97,6 +98,17 @@ export function initializeAppCheck(
9798

9899
const appCheck = provider.initialize({ options });
99100
_activate(app, options.provider, options.isTokenAutoRefreshEnabled);
101+
// If isTokenAutoRefreshEnabled is false, do not send any requests to the
102+
// exchange endpoint without an explicit call from the user either directly
103+
// or through another Firebase library (storage, functions, etc.)
104+
if (getState(app).isTokenAutoRefreshEnabled) {
105+
// Adding a listener will start the refresher and fetch a token if needed.
106+
// This gets a token ready and prevents a delay when an internal library
107+
// requests the token.
108+
// Listener function does not need to do anything, its base functionality
109+
// of calling getToken() already fetches token and writes it to memory/storage.
110+
addTokenListener(appCheck, ListenerType.INTERNAL, () => {});
111+
}
100112

101113
return appCheck;
102114
}
@@ -123,6 +135,8 @@ function _activate(
123135
newState.cachedTokenPromise = readTokenFromStorage(app).then(cachedToken => {
124136
if (cachedToken && isValid(cachedToken)) {
125137
setState(app, { ...getState(app), token: cachedToken });
138+
// notify all listeners with the cached token
139+
notifyTokenListeners(app, { token: cachedToken.token });
126140
}
127141
return cachedToken;
128142
});

‎packages/app-check/src/constants.ts

+5
Original file line numberDiff line numberDiff line change
@@ -38,3 +38,8 @@ export const TOKEN_REFRESH_TIME = {
3838
*/
3939
RETRIAL_MAX_WAIT: 16 * 60 * 1000
4040
};
41+
42+
/**
43+
* One day in millis, for certain error code backoffs.
44+
*/
45+
export const ONE_DAY = 24 * 60 * 60 * 1000;

‎packages/app-check/src/errors.ts

+5-2
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,8 @@ export const enum AppCheckError {
2626
STORAGE_OPEN = 'storage-open',
2727
STORAGE_GET = 'storage-get',
2828
STORAGE_WRITE = 'storage-set',
29-
RECAPTCHA_ERROR = 'recaptcha-error'
29+
RECAPTCHA_ERROR = 'recaptcha-error',
30+
THROTTLED = 'throttled'
3031
}
3132

3233
const ERRORS: ErrorMap<AppCheckError> = {
@@ -52,7 +53,8 @@ const ERRORS: ErrorMap<AppCheckError> = {
5253
'Error thrown when reading from storage. Original error: {$originalErrorMessage}.',
5354
[AppCheckError.STORAGE_WRITE]:
5455
'Error thrown when writing to storage. Original error: {$originalErrorMessage}.',
55-
[AppCheckError.RECAPTCHA_ERROR]: 'ReCAPTCHA error.'
56+
[AppCheckError.RECAPTCHA_ERROR]: 'ReCAPTCHA error.',
57+
[AppCheckError.THROTTLED]: `Requests throttled due to {$httpStatus} error. Attempts allowed again after {$time}`
5658
};
5759

5860
interface ErrorParams {
@@ -64,6 +66,7 @@ interface ErrorParams {
6466
[AppCheckError.STORAGE_OPEN]: { originalErrorMessage?: string };
6567
[AppCheckError.STORAGE_GET]: { originalErrorMessage?: string };
6668
[AppCheckError.STORAGE_WRITE]: { originalErrorMessage?: string };
69+
[AppCheckError.THROTTLED]: { time: string; httpStatus: number };
6770
}
6871

6972
export const ERROR_FACTORY = new ErrorFactory<AppCheckError, ErrorParams>(

‎packages/app-check/src/internal-api.test.ts

+73-3
Original file line numberDiff line numberDiff line change
@@ -38,12 +38,14 @@ import * as reCAPTCHA from './recaptcha';
3838
import * as client from './client';
3939
import * as storage from './storage';
4040
import * as util from './util';
41+
import { logger } from './logger';
4142
import { getState, clearState, setState, getDebugState } from './state';
4243
import { AppCheckTokenListener } from './public-types';
43-
import { Deferred } from '@firebase/util';
44+
import { Deferred, FirebaseError } from '@firebase/util';
4445
import { ReCaptchaEnterpriseProvider, ReCaptchaV3Provider } from './providers';
4546
import { AppCheckService } from './factory';
4647
import { ListenerType } from './types';
48+
import { AppCheckError } from './errors';
4749

4850
const fakeRecaptchaToken = 'fake-recaptcha-token';
4951
const fakeRecaptchaAppCheckToken = {
@@ -385,6 +387,62 @@ describe('internal api', () => {
385387
);
386388
expect(token).to.deep.equal({ token: fakeRecaptchaAppCheckToken.token });
387389
});
390+
391+
it('throttles for a period less than 1d on 503', async () => {
392+
// More detailed check of exponential backoff in providers.test.ts
393+
const appCheck = initializeAppCheck(app, {
394+
provider: new ReCaptchaV3Provider(FAKE_SITE_KEY)
395+
});
396+
const warnStub = stub(logger, 'warn');
397+
stub(client, 'exchangeToken').returns(
398+
Promise.reject(
399+
new FirebaseError(
400+
AppCheckError.FETCH_STATUS_ERROR,
401+
'test error msg',
402+
{ httpStatus: 503 }
403+
)
404+
)
405+
);
406+
407+
const token = await getToken(appCheck as AppCheckService);
408+
409+
// ReCaptchaV3Provider's _throttleData is private so checking
410+
// the resulting error message to be sure it has roughly the
411+
// correct throttle time. This also tests the time formatter.
412+
// Check both the error itself and that it makes it through to
413+
// console.warn
414+
expect(token.error?.message).to.include('503');
415+
expect(token.error?.message).to.include('00m');
416+
expect(token.error?.message).to.not.include('1d');
417+
expect(warnStub.args[0][0]).to.include('503');
418+
});
419+
420+
it('throttles 1d on 403', async () => {
421+
const appCheck = initializeAppCheck(app, {
422+
provider: new ReCaptchaV3Provider(FAKE_SITE_KEY)
423+
});
424+
const warnStub = stub(logger, 'warn');
425+
stub(client, 'exchangeToken').returns(
426+
Promise.reject(
427+
new FirebaseError(
428+
AppCheckError.FETCH_STATUS_ERROR,
429+
'test error msg',
430+
{ httpStatus: 403 }
431+
)
432+
)
433+
);
434+
435+
const token = await getToken(appCheck as AppCheckService);
436+
437+
// ReCaptchaV3Provider's _throttleData is private so checking
438+
// the resulting error message to be sure it has roughly the
439+
// correct throttle time. This also tests the time formatter.
440+
// Check both the error itself and that it makes it through to
441+
// console.warn
442+
expect(token.error?.message).to.include('403');
443+
expect(token.error?.message).to.include('1d');
444+
expect(warnStub.args[0][0]).to.include('403');
445+
});
388446
});
389447

390448
describe('addTokenListener', () => {
@@ -404,7 +462,7 @@ describe('internal api', () => {
404462
expect(getState(app).tokenObservers[0].next).to.equal(listener);
405463
});
406464

407-
it('starts proactively refreshing token after adding the first listener', () => {
465+
it('starts proactively refreshing token after adding the first listener', async () => {
408466
const listener = (): void => {};
409467
setState(app, {
410468
...getState(app),
@@ -420,6 +478,12 @@ describe('internal api', () => {
420478
listener
421479
);
422480

481+
expect(getState(app).tokenRefresher?.isRunning()).to.be.undefined;
482+
483+
// addTokenListener() waits for the result of cachedTokenPromise
484+
// before starting the refresher
485+
await getState(app).cachedTokenPromise;
486+
423487
expect(getState(app).tokenRefresher?.isRunning()).to.be.true;
424488
});
425489

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

431495
setState(app, {
432496
...getState(app),
497+
cachedTokenPromise: Promise.resolve(undefined),
433498
token: {
434499
token: `fake-memory-app-check-token`,
435500
expireTimeMillis: Date.now() + 60000,
@@ -493,7 +558,7 @@ describe('internal api', () => {
493558
expect(getState(app).tokenObservers.length).to.equal(0);
494559
});
495560

496-
it('should stop proactively refreshing token after deleting the last listener', () => {
561+
it('should stop proactively refreshing token after deleting the last listener', async () => {
497562
const listener = (): void => {};
498563
setState(app, { ...getState(app), isTokenAutoRefreshEnabled: true });
499564
setState(app, {
@@ -506,6 +571,11 @@ describe('internal api', () => {
506571
ListenerType.INTERNAL,
507572
listener
508573
);
574+
575+
// addTokenListener() waits for the result of cachedTokenPromise
576+
// before starting the refresher
577+
await getState(app).cachedTokenPromise;
578+
509579
expect(getState(app).tokenObservers.length).to.equal(1);
510580
expect(getState(app).tokenRefresher?.isRunning()).to.be.true;
511581

0 commit comments

Comments
 (0)