Update appcheck to distinguish between 3P/2P listeners (#5084)

firebase · Jun 30, 2021 · 5d007b8 · 5d007b8
1 parent 580b354
commit 5d007b8
Show file tree

Hide file tree

Showing 7 changed files with 132 additions and 30 deletions.
diff --git a/.changeset/gold-turtles-tell.md 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -34,6 +34,12 @@ export interface AppCheckTokenObserver
   extends PartialObserver<AppCheckTokenResult> {
   // required
   next: AppCheckTokenListener;
+  type: ListenerType;
+}
+
+export const enum ListenerType {
+  'INTERNAL' = 'INTERNAL',
+  'EXTERNAL' = 'EXTERNAL'
 }
 
 export interface AppCheckState {