diff --git a/.changeset/gold-turtles-tell.md b/.changeset/gold-turtles-tell.md new file mode 100644 index 0000000000..f061a51ff4 --- /dev/null +++ b/.changeset/gold-turtles-tell.md @@ -0,0 +1,6 @@ +--- +'@firebase/app-check': patch +'@firebase/app-check-types': patch +--- + +Fixed so token listeners added through public API call the error handler while internal token listeners return the error as a token field. diff --git a/packages/app-check-types/index.d.ts b/packages/app-check-types/index.d.ts index aa5e49d039..4a46a68579 100644 --- a/packages/app-check-types/index.d.ts +++ b/packages/app-check-types/index.d.ts @@ -16,8 +16,12 @@ */ import { PartialObserver, Unsubscribe } from '@firebase/util'; +import { FirebaseApp } from '@firebase/app-types'; export interface FirebaseAppCheck { + /** The `FirebaseApp` associated with this instance. */ + app: FirebaseApp; + /** * Activate AppCheck * @param siteKeyOrProvider - reCAPTCHA sitekey or custom token provider diff --git a/packages/app-check/src/api.ts b/packages/app-check/src/api.ts index 225c78917b..05ebd9cdea 100644 --- a/packages/app-check/src/api.ts +++ b/packages/app-check/src/api.ts @@ -22,7 +22,7 @@ import { import { FirebaseApp } from '@firebase/app-types'; import { ERROR_FACTORY, AppCheckError } from './errors'; import { initialize as initializeRecaptcha } from './recaptcha'; -import { getState, setState, AppCheckState } from './state'; +import { getState, setState, AppCheckState, ListenerType } from './state'; import { getToken as getTokenInternal, addTokenListener, @@ -162,6 +162,12 @@ export function onTokenChanged( } else if (onError) { errorFn = onError; } - addTokenListener(app, platformLoggerProvider, nextFn, errorFn); + addTokenListener( + app, + platformLoggerProvider, + ListenerType.EXTERNAL, + nextFn, + errorFn + ); return () => removeTokenListener(app, nextFn); } diff --git a/packages/app-check/src/factory.ts b/packages/app-check/src/factory.ts index 4173813b52..240b69b8ee 100644 --- a/packages/app-check/src/factory.ts +++ b/packages/app-check/src/factory.ts @@ -37,7 +37,7 @@ import { Provider } from '@firebase/component'; import { PartialObserver } from '@firebase/util'; import { FirebaseService } from '@firebase/app-types/private'; -import { getState } from './state'; +import { getState, ListenerType } from './state'; export function factory( app: FirebaseApp, @@ -92,7 +92,12 @@ export function internalFactory( getToken: forceRefresh => getTokenInternal(app, platformLoggerProvider, forceRefresh), addTokenListener: listener => - addTokenListener(app, platformLoggerProvider, listener), + addTokenListener( + app, + platformLoggerProvider, + ListenerType.INTERNAL, + listener + ), removeTokenListener: listener => removeTokenListener(app, listener) }; } diff --git a/packages/app-check/src/internal-api.test.ts b/packages/app-check/src/internal-api.test.ts index c0bf48fcfa..bd512d8670 100644 --- a/packages/app-check/src/internal-api.test.ts +++ b/packages/app-check/src/internal-api.test.ts @@ -40,7 +40,13 @@ import * as logger from './logger'; import * as client from './client'; import * as storage from './storage'; import * as util from './util'; -import { getState, clearState, setState, getDebugState } from './state'; +import { + getState, + clearState, + setState, + getDebugState, + ListenerType +} from './state'; import { Deferred } from '@firebase/util'; import { AppCheckTokenResult } from '../../app-check-interop-types'; @@ -143,8 +149,18 @@ describe('internal api', () => { const listener1 = spy(); const listener2 = spy(); - addTokenListener(app, fakePlatformLoggingProvider, listener1); - addTokenListener(app, fakePlatformLoggingProvider, listener2); + addTokenListener( + app, + fakePlatformLoggingProvider, + ListenerType.INTERNAL, + listener1 + ); + addTokenListener( + app, + fakePlatformLoggingProvider, + ListenerType.INTERNAL, + listener2 + ); await getToken(app, fakePlatformLoggingProvider); @@ -164,8 +180,18 @@ describe('internal api', () => { const listener1 = spy(); const listener2 = spy(); - addTokenListener(app, fakePlatformLoggingProvider, listener1); - addTokenListener(app, fakePlatformLoggingProvider, listener2); + addTokenListener( + app, + fakePlatformLoggingProvider, + ListenerType.INTERNAL, + listener1 + ); + addTokenListener( + app, + fakePlatformLoggingProvider, + ListenerType.INTERNAL, + listener2 + ); await getToken(app, fakePlatformLoggingProvider); @@ -177,7 +203,7 @@ describe('internal api', () => { }); }); - it('calls optional error handler if there is an error getting a token', async () => { + it('calls 3P error handler if there is an error getting a token', async () => { stub(logger.logger, 'error'); activate(app, FAKE_SITE_KEY, false); stub(reCAPTCHA, 'getToken').resolves(fakeRecaptchaToken); @@ -186,7 +212,13 @@ describe('internal api', () => { const errorFn1 = spy(); - addTokenListener(app, fakePlatformLoggingProvider, listener1, errorFn1); + addTokenListener( + app, + fakePlatformLoggingProvider, + ListenerType.EXTERNAL, + listener1, + errorFn1 + ); await getToken(app, fakePlatformLoggingProvider); @@ -203,8 +235,19 @@ describe('internal api', () => { const errorFn1 = spy(); - addTokenListener(app, fakePlatformLoggingProvider, listener1, errorFn1); - addTokenListener(app, fakePlatformLoggingProvider, listener2); + addTokenListener( + app, + fakePlatformLoggingProvider, + ListenerType.INTERNAL, + listener1, + errorFn1 + ); + addTokenListener( + app, + fakePlatformLoggingProvider, + ListenerType.INTERNAL, + listener2 + ); await getToken(app, fakePlatformLoggingProvider); @@ -298,7 +341,12 @@ describe('internal api', () => { const listener = (): void => {}; stub(client, 'exchangeToken').resolves(fakeRecaptchaAppCheckToken); - addTokenListener(app, fakePlatformLoggingProvider, listener); + addTokenListener( + app, + fakePlatformLoggingProvider, + ListenerType.INTERNAL, + listener + ); expect(getState(app).tokenObservers[0].next).to.equal(listener); }); @@ -310,7 +358,12 @@ describe('internal api', () => { expect(getState(app).tokenObservers.length).to.equal(0); expect(getState(app).tokenRefresher).to.equal(undefined); - addTokenListener(app, fakePlatformLoggingProvider, listener); + addTokenListener( + app, + fakePlatformLoggingProvider, + ListenerType.INTERNAL, + listener + ); expect(getState(app).tokenRefresher?.isRunning()).to.be.true; @@ -330,7 +383,12 @@ describe('internal api', () => { } }); - addTokenListener(app, fakePlatformLoggingProvider, listener); + addTokenListener( + app, + fakePlatformLoggingProvider, + ListenerType.INTERNAL, + listener + ); await clock.runAllAsync(); expect(listener).to.be.calledWith({ token: 'fake-memory-app-check-token' @@ -355,7 +413,12 @@ describe('internal api', () => { done(); }; - addTokenListener(app, fakePlatformLoggingProvider, fakeListener); + addTokenListener( + app, + fakePlatformLoggingProvider, + ListenerType.INTERNAL, + fakeListener + ); }); }); @@ -369,7 +432,12 @@ describe('internal api', () => { it('should remove token listeners', () => { stub(client, 'exchangeToken').resolves(fakeRecaptchaAppCheckToken); const listener = (): void => {}; - addTokenListener(app, fakePlatformLoggingProvider, listener); + addTokenListener( + app, + fakePlatformLoggingProvider, + ListenerType.INTERNAL, + listener + ); expect(getState(app).tokenObservers.length).to.equal(1); removeTokenListener(app, listener); @@ -381,7 +449,12 @@ describe('internal api', () => { const listener = (): void => {}; setState(app, { ...getState(app), isTokenAutoRefreshEnabled: true }); - addTokenListener(app, fakePlatformLoggingProvider, listener); + addTokenListener( + app, + fakePlatformLoggingProvider, + ListenerType.INTERNAL, + listener + ); 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 ebbe70fe4f..9c2dac052f 100644 --- a/packages/app-check/src/internal-api.ts +++ b/packages/app-check/src/internal-api.ts @@ -25,6 +25,7 @@ import { AppCheckTokenInternal, AppCheckTokenObserver, getState, + ListenerType, setState } from './state'; import { TOKEN_REFRESH_TIME } from './constants'; @@ -176,13 +177,15 @@ export async function getToken( export function addTokenListener( app: FirebaseApp, platformLoggerProvider: Provider<'platform-logger'>, + type: ListenerType, listener: AppCheckTokenListener, onError?: (error: Error) => void ): void { const state = getState(app); const tokenListener: AppCheckTokenObserver = { next: listener, - error: onError + error: onError, + type }; const newState = { ...state, @@ -304,20 +307,19 @@ function notifyTokenListeners( for (const observer of observers) { try { - if (observer.error) { - // If this listener has an error handler, handle errors differently - // from successes. - if (token.error) { - observer.error(token.error); - } else { - observer.next(token); - } + if (observer.type === ListenerType.EXTERNAL && token.error != null) { + // If this listener was added by a 3P call, send any token error to + // the supplied error handler. A 3P observer always has an error + // handler. + observer.error!(token.error); } else { - // Otherwise return the token, whether or not it has an error field. + // If the token has no error field, always return the token. + // If this is a 2P listener, return the token, whether or not it + // has an error field. observer.next(token); } } catch (ignored) { - // If any handler fails, ignore and run next handler. + // Errors in the listener function itself are always ignored. } } } diff --git a/packages/app-check/src/state.ts b/packages/app-check/src/state.ts index d8fe52b046..51bdec2b36 100644 --- a/packages/app-check/src/state.ts +++ b/packages/app-check/src/state.ts @@ -34,6 +34,12 @@ export interface AppCheckTokenObserver extends PartialObserver { // required next: AppCheckTokenListener; + type: ListenerType; +} + +export const enum ListenerType { + 'INTERNAL' = 'INTERNAL', + 'EXTERNAL' = 'EXTERNAL' } export interface AppCheckState {