Fix onErrors

Author: hsubox76

packages/app-check-interop-types/index.d.ts

@@ -24,10 +24,10 @@ export interface FirebaseAppCheckInternal {
   // registered at the same time for one or more FirebaseAppAttestation instances. The
   // listeners call back on the UI thread whenever the current token associated with this
   // FirebaseAppAttestation changes.
-  addTokenListener(listener: (token: AppCheckTokenResult) => void): void;
+  addTokenListener(listener: AppCheckTokenListener): void;
 
   // Unregisters a listener to changes in the token state.
-  removeTokenListener(listener: (token: AppCheckTokenResult) => void): void;
+  removeTokenListener(listener: AppCheckTokenListener): void;
 }
 
 type AppCheckTokenListener = (token: AppCheckTokenResult) => void;

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

@@ -36,6 +36,7 @@ import { FirebaseApp } from '@firebase/app-types';
 import * as internalApi from './internal-api';
 import * as client from './client';
 import * as storage from './storage';
+import * as logger from './logger';
 
 describe('api', () => {
   describe('activate()', () => {
@@ -151,31 +152,32 @@ describe('api', () => {
       const errorFn1 = spy();
       const errorFn2 = spy();
 
-      const unSubscribe1 = onTokenChanged(
+      const unsubscribe1 = onTokenChanged(
         app,
         fakePlatformLoggingProvider,
         listener1,
         errorFn1
       );
-      const unSubscribe2 = onTokenChanged(
+      const unsubscribe2 = onTokenChanged(
         app,
         fakePlatformLoggingProvider,
         listener2,
         errorFn2
       );
 
-      expect(getState(app).tokenListeners.length).to.equal(2);
+      expect(getState(app).tokenObservers.length).to.equal(2);
 
-      await getToken(app, fakePlatformLoggingProvider);
+      await internalApi.getToken(app, fakePlatformLoggingProvider);
 
       expect(listener2).to.be.calledWith({
         token: fakeRecaptchaAppCheckToken.token
       });
-      expect(errorFn1).to.be.calledOnce;
+      // onError should not be called on listener errors.
+      expect(errorFn1).to.not.be.called;
       expect(errorFn2).to.not.be.called;
-      unSubscribe1();
-      unSubscribe2();
-      expect(getState(app).tokenListeners.length).to.equal(0);
+      unsubscribe1();
+      unsubscribe2();
+      expect(getState(app).tokenObservers.length).to.equal(0);
     });
 
     it('Listeners work when using Observer pattern', async () => {
@@ -206,27 +208,60 @@ describe('api', () => {
        * Reverse the order of adding the failed and successful handler, for extra
        * testing.
        */
-      const unSubscribe2 = onTokenChanged(app, fakePlatformLoggingProvider, {
+      const unsubscribe2 = onTokenChanged(app, fakePlatformLoggingProvider, {
         next: listener2,
         error: errorFn2
       });
-      const unSubscribe1 = onTokenChanged(app, fakePlatformLoggingProvider, {
+      const unsubscribe1 = onTokenChanged(app, fakePlatformLoggingProvider, {
         next: listener1,
         error: errorFn1
       });
 
-      expect(getState(app).tokenListeners.length).to.equal(2);
+      expect(getState(app).tokenObservers.length).to.equal(2);
 
-      await getToken(app, fakePlatformLoggingProvider);
+      await internalApi.getToken(app, fakePlatformLoggingProvider);
 
       expect(listener2).to.be.calledWith({
         token: fakeRecaptchaAppCheckToken.token
       });
-      expect(errorFn1).to.be.calledOnce;
+      // onError should not be called on listener errors.
+      expect(errorFn1).to.not.be.called;
       expect(errorFn2).to.not.be.called;
-      unSubscribe1();
-      unSubscribe2();
-      expect(getState(app).tokenListeners.length).to.equal(0);
+      unsubscribe1();
+      unsubscribe2();
+      expect(getState(app).tokenObservers.length).to.equal(0);
+    });
+
+    it('onError() catches token errors', async () => {
+      stub(logger.logger, 'error');
+      const app = getFakeApp();
+      activate(app, FAKE_SITE_KEY, false);
+      const fakePlatformLoggingProvider = getFakePlatformLoggingProvider();
+      const fakeRecaptchaToken = 'fake-recaptcha-token';
+      stub(reCAPTCHA, 'getToken').returns(Promise.resolve(fakeRecaptchaToken));
+      stub(client, 'exchangeToken').rejects('exchange error');
+      stub(storage, 'writeTokenToStorage').returns(Promise.resolve(undefined));
+
+      const listener1 = spy();
+
+      const errorFn1 = spy();
+
+      const unsubscribe1 = onTokenChanged(
+        app,
+        fakePlatformLoggingProvider,
+        listener1,
+        errorFn1
+      );
+
+      await internalApi.getToken(app, fakePlatformLoggingProvider);
+
+      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(0);
     });
   });
 });

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

@@ -35,6 +35,7 @@ import {
   defaultTokenErrorData
 } from './internal-api';
 import * as reCAPTCHA from './recaptcha';
+import * as logger from './logger';
 import * as client from './client';
 import * as storage from './storage';
 import { getState, clearState, setState, getDebugState } from './state';
@@ -179,30 +180,21 @@ describe('internal api', () => {
       });
     });
 
-    it('calls optional handler if a listener throws', async () => {
+    it('calls optional error handler if there is an error getting a token', async () => {
+      stub(logger.logger, 'error');
       activate(app, FAKE_SITE_KEY, true);
       stub(reCAPTCHA, 'getToken').returns(Promise.resolve(fakeRecaptchaToken));
-      stub(client, 'exchangeToken').returns(
-        Promise.resolve(fakeRecaptchaAppCheckToken)
-      );
-      const listener1 = (): void => {
-        throw new Error();
-      };
-      const listener2 = spy();
+      stub(client, 'exchangeToken').rejects('exchange error');
+      const listener1 = spy();
 
       const errorFn1 = spy();
-      const errorFn2 = spy();
 
       addTokenListener(app, fakePlatformLoggingProvider, listener1, errorFn1);
-      addTokenListener(app, fakePlatformLoggingProvider, listener2, errorFn2);
 
       await getToken(app, fakePlatformLoggingProvider);
 
-      expect(listener2).to.be.calledWith({
-        token: fakeRecaptchaAppCheckToken.token
-      });
       expect(errorFn1).to.be.calledOnce;
-      expect(errorFn2).to.not.be.called;
+      expect(errorFn1.args[0][0].name).to.include('exchange error');
     });
 
     it('loads persisted token to memory and returns it', async () => {
@@ -297,13 +289,13 @@ describe('internal api', () => {
 
       addTokenListener(app, fakePlatformLoggingProvider, listener);
 
-      expect(getState(app).tokenListeners[0].listener).to.equal(listener);
+      expect(getState(app).tokenObservers[0].next).to.equal(listener);
     });
 
     it('starts proactively refreshing token after adding the first listener', () => {
       const listener = (): void => {};
       setState(app, { ...getState(app), isTokenAutoRefreshEnabled: true });
-      expect(getState(app).tokenListeners.length).to.equal(0);
+      expect(getState(app).tokenObservers.length).to.equal(0);
       expect(getState(app).tokenRefresher).to.equal(undefined);
 
       addTokenListener(app, fakePlatformLoggingProvider, listener);
@@ -391,22 +383,22 @@ describe('internal api', () => {
     it('should remove token listeners', () => {
       const listener = (): void => {};
       addTokenListener(app, fakePlatformLoggingProvider, listener);
-      expect(getState(app).tokenListeners.length).to.equal(1);
+      expect(getState(app).tokenObservers.length).to.equal(1);
 
       removeTokenListener(app, listener);
-      expect(getState(app).tokenListeners.length).to.equal(0);
+      expect(getState(app).tokenObservers.length).to.equal(0);
     });
 
     it('should stop proactively refreshing token after deleting the last listener', () => {
       const listener = (): void => {};
       setState(app, { ...getState(app), isTokenAutoRefreshEnabled: true });
 
       addTokenListener(app, fakePlatformLoggingProvider, listener);
-      expect(getState(app).tokenListeners.length).to.equal(1);
+      expect(getState(app).tokenObservers.length).to.equal(1);
       expect(getState(app).tokenRefresher?.isRunning()).to.be.true;
 
       removeTokenListener(app, listener);
-      expect(getState(app).tokenListeners.length).to.equal(0);
+      expect(getState(app).tokenObservers.length).to.equal(0);
       expect(getState(app).tokenRefresher?.isRunning()).to.be.false;
     });
   });

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

@@ -23,7 +23,7 @@ import {
 } from '@firebase/app-check-interop-types';
 import {
   AppCheckTokenInternal,
-  AppCheckTokenListenerInternal,
+  AppCheckTokenObserver,
   getDebugState,
   getState,
   setState
@@ -172,13 +172,13 @@ export function addTokenListener(
   onError?: (error: Error) => void
 ): void {
   const state = getState(app);
-  const tokenListener: AppCheckTokenListenerInternal = {
-    listener,
-    onError
+  const tokenListener: AppCheckTokenObserver = {
+    next: listener,
+    error: onError
   };
   const newState = {
     ...state,
-    tokenListeners: [...state.tokenListeners, tokenListener]
+    tokenObservers: [...state.tokenObservers, tokenListener]
   };
 
   /**
@@ -191,14 +191,8 @@ export function addTokenListener(
     if (debugState.enabled && debugState.token) {
       debugState.token.promise
         .then(token => listener({ token }))
-        .catch(e => {
-          /**
-           * An error handler will be provided if this is called by the public
-           * API. Internal callers don't care about errors in listeners.
-           */
-          if (onError) {
-            onError(e);
-          }
+        .catch(() => {
+          /** Ignore errors in listeners. */
         });
     }
   } else {
@@ -208,7 +202,11 @@ export function addTokenListener(
      * invoke the listener with the valid token, then start the token refresher
      */
     if (!newState.tokenRefresher) {
-      const tokenRefresher = createTokenRefresher(app, platformLoggerProvider);
+      const tokenRefresher = createTokenRefresher(
+        app,
+        platformLoggerProvider,
+        onError
+      );
       newState.tokenRefresher = tokenRefresher;
     }
 
@@ -226,14 +224,8 @@ export function addTokenListener(
       const validToken = state.token;
       Promise.resolve()
         .then(() => listener({ token: validToken.token }))
-        .catch(e => {
-          /**
-           * An error handler will be provided if this is called by the public
-           * API. Internal callers don't care about errors in listeners.
-           */
-          if (onError) {
-            onError(e);
-          }
+        .catch(() => {
+          /** Ignore errors in listeners. */
         });
     }
   }
@@ -247,11 +239,11 @@ export function removeTokenListener(
 ): void {
   const state = getState(app);
 
-  const newListeners = state.tokenListeners.filter(
-    tokenListener => tokenListener.listener !== listener
+  const newObservers = state.tokenObservers.filter(
+    tokenObserver => tokenObserver.next !== listener
   );
   if (
-    newListeners.length === 0 &&
+    newObservers.length === 0 &&
     state.tokenRefresher &&
     state.tokenRefresher.isRunning()
   ) {
@@ -260,13 +252,14 @@ export function removeTokenListener(
 
   setState(app, {
     ...state,
-    tokenListeners: newListeners
+    tokenObservers: newObservers
   });
 }
 
 function createTokenRefresher(
   app: FirebaseApp,
-  platformLoggerProvider: Provider<'platform-logger'>
+  platformLoggerProvider: Provider<'platform-logger'>,
+  onError?: (error: Error) => void
 ): Refresher {
   return new Refresher(
     // Keep in mind when this fails for any reason other than the ones
@@ -284,7 +277,11 @@ function createTokenRefresher(
 
       // getToken() always resolves. In case the result has an error field defined, it means the operation failed, and we should retry.
       if (result.error) {
-        throw result.error;
+        if (onError) {
+          onError(result.error);
+        } else {
+          throw result.error;
+        }
       }
     },
     () => {
@@ -322,17 +319,24 @@ function notifyTokenListeners(
   app: FirebaseApp,
   token: AppCheckTokenResult
 ): void {
-  const listeners = getState(app).tokenListeners;
+  const observers = getState(app).tokenObservers;
 
-  for (const listener of listeners) {
+  for (const observer of observers) {
     try {
-      listener.listener(token);
-    } catch (e) {
-      // If any listener fails, run any provided error handler,
-      // then run next listener.
-      if (listener.onError) {
-        listener.onError(e);
+      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);
+        }
+      } else {
+        // Otherwise 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.
     }
   }
 }