Handle app deletion a little more gracefully in auth-exp and auth-com…

…pat-exp (#3917) * Handle app deletion a little more gracefully in auth-exp and auth-compat-exp * Formatting * Add some tests * Formatting
firebase · Oct 13, 2020 · 9e92e91 · 9e92e91
1 parent df27732
commit 9e92e91
Show file tree

Hide file tree

Showing 3 changed files with 72 additions and 2 deletions.
diff --git a/packages-exp/auth-compat-exp/src/auth.ts b/packages-exp/auth-compat-exp/src/auth.ts
@@ -16,6 +16,7 @@
  */
 
 import { FirebaseApp } from '@firebase/app-types';
+import { _FirebaseService } from '@firebase/app-types-exp';
 import * as impl from '@firebase/auth-exp/internal';
 import * as compat from '@firebase/auth-types';
 import * as externs from '@firebase/auth-types-exp';
@@ -35,11 +36,15 @@ import {
 } from './user_credential';
 import { unwrap, Wrapper } from './wrap';
 
-export class Auth implements compat.FirebaseAuth, Wrapper<externs.Auth> {
+export class Auth
+  implements compat.FirebaseAuth, Wrapper<externs.Auth>, _FirebaseService {
   // private readonly auth: impl.AuthImpl;
 
   constructor(readonly app: FirebaseApp, private readonly auth: impl.AuthImpl) {
     const { apiKey } = app.options;
+    if (this.auth._deleted) {
+      return;
+    }
 
     // TODO(avolkovi): Implement proper persistence fallback
     const hierarchy = [impl.indexedDBLocalPersistence].map<impl.Persistence>(
@@ -289,6 +294,9 @@ export class Auth implements compat.FirebaseAuth, Wrapper<externs.Auth> {
   unwrap(): externs.Auth {
     return this.auth;
   }
+  _delete(): Promise<void> {
+    return this.auth._delete();
+  }
 }
 
 function wrapObservers(

diff --git a/packages-exp/auth-exp/src/core/auth/auth_impl.test.ts b/packages-exp/auth-exp/src/core/auth/auth_impl.test.ts
@@ -33,6 +33,7 @@ import { Persistence } from '../persistence';
 import { inMemoryPersistence } from '../persistence/in_memory';
 import { _getInstance } from '../util/instantiator';
 import * as navigator from '../util/navigator';
+import * as reload from '../user/reload';
 import {
   _castAuth,
   AuthImpl,
@@ -410,6 +411,41 @@ describe('core/auth/auth_impl', () => {
       });
     });
   });
+
+  context('#_delete', () => {
+    beforeEach(async () => {
+      sinon.stub(reload, '_reloadWithoutSaving').returns(Promise.resolve());
+    });
+
+    it('prevents initialization from completing', async () => {
+      const authImpl = new AuthImpl(FAKE_APP, {
+        apiKey: FAKE_APP.options.apiKey!,
+        apiHost: DEFAULT_API_HOST,
+        apiScheme: DEFAULT_API_SCHEME,
+        tokenApiHost: DEFAULT_TOKEN_API_HOST,
+        sdkClientVersion: 'v'
+      });
+
+      persistenceStub._get.returns(
+        Promise.resolve(testUser(auth, 'uid').toJSON())
+      );
+      await authImpl._delete();
+      await authImpl._initializeWithPersistence([
+        persistenceStub as Persistence
+      ]);
+      expect(authImpl.currentUser).to.be.null;
+    });
+
+    it('no longer calls listeners', async () => {
+      const spy = sinon.spy();
+      auth.onAuthStateChanged(spy);
+      await Promise.resolve();
+      spy.resetHistory();
+      await (auth as AuthImpl)._delete();
+      await auth.updateCurrentUser(testUser(auth, 'blah'));
+      expect(spy).not.to.have.been.called;
+    });
+  });
 });
 
 // These tests are separate because they are using a different auth with

diff --git a/packages-exp/auth-exp/src/core/auth/auth_impl.ts b/packages-exp/auth-exp/src/core/auth/auth_impl.ts
@@ -63,6 +63,7 @@ export class AuthImpl implements Auth, _FirebaseService {
   // initialization
   _canInitEmulator = true;
   _isInitialized = false;
+  _deleted = false;
   _initializationPromise: Promise<void> | null = null;
   _popupRedirectResolver: PopupRedirectResolver | null = null;
   readonly name: string;
@@ -86,7 +87,13 @@ export class AuthImpl implements Auth, _FirebaseService {
     persistenceHierarchy: Persistence[],
     popupRedirectResolver?: externs.PopupRedirectResolver
   ): Promise<void> {
+    // Have to check for app deletion throughout initialization (after each
+    // promise resolution)
     this._initializationPromise = this.queue(async () => {
+      if (this._deleted) {
+        return;
+      }
+
       if (popupRedirectResolver) {
         this._popupRedirectResolver = _getInstance(popupRedirectResolver);
       }
@@ -96,8 +103,16 @@ export class AuthImpl implements Auth, _FirebaseService {
         persistenceHierarchy
       );
 
+      if (this._deleted) {
+        return;
+      }
+
       await this.initializeCurrentUser();
 
+      if (this._deleted) {
+        return;
+      }
+
       this._isInitialized = true;
       this.notifyAuthListeners();
     });
@@ -109,6 +124,10 @@ export class AuthImpl implements Auth, _FirebaseService {
    * If the persistence is changed in another window, the user manager will let us know
    */
   async _onStorageEvent(): Promise<void> {
+    if (this._deleted) {
+      return;
+    }
+
     const user = await this.assertedPersistence.getCurrentUser();
 
     if (!this.currentUser && !user) {
@@ -189,10 +208,13 @@ export class AuthImpl implements Auth, _FirebaseService {
   }
 
   async _delete(): Promise<void> {
-    // TODO: Determine what we want to do in this case
+    this._deleted = true;
   }
 
   async updateCurrentUser(user: externs.User | null): Promise<void> {
+    if (this._deleted) {
+      return;
+    }
     if (user) {
       assert(
         this.tenantId === user.tenantId,
@@ -357,6 +379,10 @@ export class AuthImpl implements Auth, _FirebaseService {
     error?: ErrorFn,
     completed?: CompleteFn
   ): Unsubscribe {
+    if (this._deleted) {
+      return () => {};
+    }
+
     const cb =
       typeof nextOrObserver === 'function'
         ? nextOrObserver