Make App Check initialization explicit (#4897)

hsubox76 · web-flow · commit 60e83473940e · 2021-05-12T14:13:39.000-07:00
diff --git a/.changeset/fair-garlics-smoke.md b/.changeset/fair-garlics-smoke.md
@@ -0,0 +1,6 @@
+---
+'@firebase/app': patch
+'@firebase/app-check': patch
+---
+
+Make App Check initialization explicit, to prevent unexpected errors for users who do not intend to use App Check.
diff --git a/packages-exp/app-compat/src/firebaseApp.ts b/packages-exp/app-compat/src/firebaseApp.ts
@@ -20,6 +20,7 @@ import {
   Component,
   ComponentContainer,
   ComponentType,
+  InstantiationMode,
   Name
 } from '@firebase/component';
 import {
@@ -121,8 +122,17 @@ export class FirebaseAppImpl implements Compat<_FirebaseAppExp>, _FirebaseApp {
   ): _FirebaseService {
     this._delegate.checkDestroyed();
 
+    // Initialize instance if InstatiationMode is `EXPLICIT`.
+    const provider = this._delegate.container.getProvider(name as Name);
+    if (
+      !provider.isInitialized() &&
+      provider.getComponent()?.instantiationMode === InstantiationMode.EXPLICIT
+    ) {
+      provider.initialize();
+    }
+
     // getImmediate will always succeed because _getService is only called for registered components.
-    return (this._delegate.container.getProvider(name as Name).getImmediate({
+    return (provider.getImmediate({
       identifier: instanceIdentifier
     }) as unknown) as _FirebaseService;
   }
diff --git a/packages-exp/app-compat/test/firebaseAppCompat.test.ts b/packages-exp/app-compat/test/firebaseAppCompat.test.ts
@@ -21,7 +21,11 @@ import { stub } from 'sinon';
 import { FirebaseNamespace, FirebaseOptions } from '../src/public-types';
 import { _FirebaseApp, _FirebaseNamespace } from '../src/types';
 import { _components, _clearComponents } from '@firebase/app-exp';
-import { ComponentType } from '@firebase/component';
+import {
+  Component,
+  ComponentType,
+  InstantiationMode
+} from '@firebase/component';
 
 import { createFirebaseNamespace } from '../src/firebaseNamespace';
 import { createFirebaseNamespaceLite } from '../src/lite/firebaseNamespaceLite';
@@ -67,6 +71,7 @@ function executeFirebaseTests(): void {
 
       expect(serviceNamespace).to.eq(serviceNamespace2);
       expect(registerStub).to.have.not.thrown();
+      registerStub.restore();
     });
 
     it('returns cached service instances', () => {
@@ -80,6 +85,71 @@ function executeFirebaseTests(): void {
       expect(service).to.eq((firebase as any).test());
     });
 
+    it('does not instantiate explicit components unless called explicitly', () => {
+      firebase.initializeApp({});
+      (firebase as _FirebaseNamespace).INTERNAL.registerComponent(
+        createTestComponent('explicit1').setInstantiationMode(
+          InstantiationMode.EXPLICIT
+        )
+      );
+
+      let explicitService;
+
+      // Expect getImmediate in a consuming component to return null.
+      const consumerComponent = new Component(
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        'consumer' as any,
+        container => {
+          explicitService = container
+            .getProvider('explicit1' as any)
+            .getImmediate({ optional: true });
+          return new TestService(
+            container.getProvider('app-compat').getImmediate()
+          );
+        },
+        ComponentType.PUBLIC
+      );
+      (firebase as _FirebaseNamespace).INTERNAL.registerComponent(
+        consumerComponent
+      );
+
+      (firebase as any).consumer();
+      expect(explicitService).to.be.null;
+    });
+
+    it('does instantiate explicit components when called explicitly', () => {
+      firebase.initializeApp({});
+      (firebase as _FirebaseNamespace).INTERNAL.registerComponent(
+        createTestComponent('explicit2').setInstantiationMode(
+          InstantiationMode.EXPLICIT
+        )
+      );
+
+      let explicitService;
+
+      // Expect getImmediate in a consuming component to return the service.
+      const consumerComponent = new Component(
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        'consumer' as any,
+        container => {
+          explicitService = container
+            .getProvider('explicit2' as any)
+            .getImmediate({ optional: true });
+          return new TestService(
+            container.getProvider('app-compat').getImmediate()
+          );
+        },
+        ComponentType.PUBLIC
+      );
+      (firebase as _FirebaseNamespace).INTERNAL.registerComponent(
+        consumerComponent
+      );
+
+      (firebase as any).explicit2();
+      (firebase as any).consumer();
+      expect(explicitService).to.not.be.null;
+    });
+
     it(`creates a new instance of a service after removing the existing instance`, () => {
       const app = firebase.initializeApp({});
       (firebase as _FirebaseNamespace).INTERNAL.registerComponent(
diff --git a/packages/app-check/src/index.ts b/packages/app-check/src/index.ts
@@ -16,7 +16,11 @@
  */
 import firebase from '@firebase/app';
 import { _FirebaseNamespace } from '@firebase/app-types/private';
-import { Component, ComponentType } from '@firebase/component';
+import {
+  Component,
+  ComponentType,
+  InstantiationMode
+} from '@firebase/component';
 import {
   FirebaseAppCheck,
   AppCheckComponentName
@@ -41,6 +45,26 @@ function registerAppCheck(firebase: _FirebaseNamespace): void {
       },
       ComponentType.PUBLIC
     )
+      /**
+       * AppCheck can only be initialized by explicitly calling firebase.appCheck()
+       * We don't want firebase products that consume AppCheck to gate on AppCheck
+       * if the user doesn't intend them to, just because the AppCheck component
+       * is registered.
+       */
+      .setInstantiationMode(InstantiationMode.EXPLICIT)
+      /**
+       * Because all firebase products that depend on app-check depend on app-check-internal directly,
+       * we need to initialize app-check-internal after app-check is initialized to make it
+       * available to other firebase products.
+       */
+      .setInstanceCreatedCallback(
+        (container, _instanceIdentifier, _instance) => {
+          const appCheckInternalProvider = container.getProvider(
+            APP_CHECK_NAME_INTERNAL
+          );
+          appCheckInternalProvider.initialize();
+        }
+      )
   );
 
   // The internal interface used by other Firebase products
@@ -54,7 +78,7 @@ function registerAppCheck(firebase: _FirebaseNamespace): void {
         return internalFactory(app, platformLoggerProvider);
       },
       ComponentType.PUBLIC
-    )
+    ).setInstantiationMode(InstantiationMode.EXPLICIT)
   );
 
   firebase.registerVersion(name, version);
diff --git a/packages/app/src/firebaseApp.ts b/packages/app/src/firebaseApp.ts
@@ -29,7 +29,8 @@ import {
   ComponentContainer,
   Component,
   ComponentType,
-  Name
+  Name,
+  InstantiationMode
 } from '@firebase/component';
 import { AppError, ERROR_FACTORY } from './errors';
 import { DEFAULT_ENTRY_NAME } from './constants';
@@ -122,8 +123,17 @@ export class FirebaseAppImpl implements FirebaseApp {
   ): FirebaseService {
     this.checkDestroyed_();
 
+    // Initialize instance if InstatiationMode is `EXPLICIT`.
+    const provider = this.container.getProvider(name as Name);
+    if (
+      !provider.isInitialized() &&
+      provider.getComponent()?.instantiationMode === InstantiationMode.EXPLICIT
+    ) {
+      provider.initialize();
+    }
+
     // getImmediate will always succeed because _getService is only called for registered components.
-    return (this.container.getProvider(name as Name).getImmediate({
+    return (provider.getImmediate({
       identifier: instanceIdentifier
     }) as unknown) as FirebaseService;
   }
diff --git a/packages/app/test/clientLogger.test.ts b/packages/app/test/clientLogger.test.ts
@@ -16,7 +16,10 @@
  */
 
 import { FirebaseNamespace, VersionService } from '@firebase/app-types';
-import { _FirebaseNamespace } from '@firebase/app-types/private';
+import {
+  _FirebaseNamespace,
+  FirebaseService
+} from '@firebase/app-types/private';
 import { createFirebaseNamespace } from '../src/firebaseNamespace';
 import { expect } from 'chai';
 import { spy as Spy } from 'sinon';
@@ -29,7 +32,7 @@ declare module '@firebase/component' {
   interface NameServiceMapping {
     'vs1': VersionService;
     'vs2': VersionService;
-    'test-shell': Promise<void>;
+    'test-shell': FirebaseService;
   }
 }
 
@@ -51,7 +54,7 @@ describe('User Log Methods', () => {
       (firebase as _FirebaseNamespace).INTERNAL.registerComponent(
         new Component(
           'test-shell',
-          async () => {
+          () => {
             const logger = new Logger('@firebase/logger-test');
             logger.warn('hello');
             expect(warnSpy.called).to.be.true;
@@ -67,6 +70,7 @@ describe('User Log Methods', () => {
             expect(infoSpy.called).to.be.true;
             logger.log('hi');
             expect(logSpy.called).to.be.true;
+            return {} as FirebaseService;
           },
           ComponentType.PUBLIC
         )
@@ -81,7 +85,7 @@ describe('User Log Methods', () => {
       (firebase as _FirebaseNamespace).INTERNAL.registerComponent(
         new Component(
           'test-shell',
-          async () => {
+          () => {
             const logger = new Logger('@firebase/logger-test');
             (firebase as _FirebaseNamespace).onLog(logData => {
               result = logData;
@@ -92,6 +96,7 @@ describe('User Log Methods', () => {
             expect(result.args).to.deep.equal(['hi']);
             expect(result.type).to.equal('@firebase/logger-test');
             expect(infoSpy.called).to.be.true;
+            return {} as FirebaseService;
           },
           ComponentType.PUBLIC
         )
diff --git a/packages/app/test/firebaseApp.test.ts b/packages/app/test/firebaseApp.test.ts
@@ -29,7 +29,11 @@ import { createFirebaseNamespace } from '../src/firebaseNamespace';
 import { createFirebaseNamespaceLite } from '../src/lite/firebaseNamespaceLite';
 import { expect } from 'chai';
 import { stub } from 'sinon';
-import { Component, ComponentType } from '@firebase/component';
+import {
+  Component,
+  ComponentType,
+  InstantiationMode
+} from '@firebase/component';
 import './setup';
 
 executeFirebaseTests();
@@ -80,6 +84,67 @@ function executeFirebaseTests(): void {
       expect(service).to.eq((firebase as any).test());
     });
 
+    it('does not instantiate explicit components unless called explicitly', () => {
+      firebase.initializeApp({});
+      (firebase as _FirebaseNamespace).INTERNAL.registerComponent(
+        createTestComponent('test').setInstantiationMode(
+          InstantiationMode.EXPLICIT
+        )
+      );
+
+      let explicitService;
+
+      // Expect getImmediate in a consuming component to return null.
+      const consumerComponent = new Component(
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        'consumer' as any,
+        container => {
+          explicitService = container
+            .getProvider('test' as any)
+            .getImmediate({ optional: true });
+          return new TestService(container.getProvider('app').getImmediate());
+        },
+        ComponentType.PUBLIC
+      );
+      (firebase as _FirebaseNamespace).INTERNAL.registerComponent(
+        consumerComponent
+      );
+
+      (firebase as any).consumer();
+      expect(explicitService).to.be.null;
+    });
+
+    it('does instantiate explicit components when called explicitly', () => {
+      firebase.initializeApp({});
+      (firebase as _FirebaseNamespace).INTERNAL.registerComponent(
+        createTestComponent('test').setInstantiationMode(
+          InstantiationMode.EXPLICIT
+        )
+      );
+
+      let explicitService;
+
+      // Expect getImmediate in a consuming component to return the service.
+      const consumerComponent = new Component(
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        'consumer' as any,
+        container => {
+          explicitService = container
+            .getProvider('test' as any)
+            .getImmediate({ optional: true });
+          return new TestService(container.getProvider('app').getImmediate());
+        },
+        ComponentType.PUBLIC
+      );
+      (firebase as _FirebaseNamespace).INTERNAL.registerComponent(
+        consumerComponent
+      );
+
+      (firebase as any).test();
+      (firebase as any).consumer();
+      expect(explicitService).to.not.be.null;
+    });
+
     it(`creates a new instance of a service after removing the existing instance`, () => {
       const app = firebase.initializeApp({});
       (firebase as _FirebaseNamespace).INTERNAL.registerComponent(
diff --git a/packages/app/test/platformLogger.test.ts b/packages/app/test/platformLogger.test.ts
