Redirect strategy updates & test

Author: sam-gc

packages-exp/auth-exp/demo/src/index.js

@@ -1630,7 +1630,8 @@ function initApp() {
   log('Initializing app...');
   app = initializeApp(config);
   auth = initializeAuth(app, {
-    persistence: browserSessionPersistence
+    persistence: browserSessionPersistence,
+    popupRedirectResolver: browserPopupRedirectResolver,
   });
 
   // tempApp = initializeApp({

packages-exp/auth-exp/src/core/auth/auth_event_manager.test.ts

@@ -20,10 +20,7 @@ import * as sinon from 'sinon';
 import * as sinonChai from 'sinon-chai';
 
 import {
-  AuthEvent,
-  AuthEventConsumer,
-  AuthEventError,
-  AuthEventType
+    AuthEvent, AuthEventConsumer, AuthEventError, AuthEventType
 } from '../../model/popup_redirect';
 import { AuthErrorCode } from '../errors';
 import { AuthEventManager } from './auth_event_manager';
@@ -145,7 +142,8 @@ describe('src/core/auth/auth_event_manager', () => {
       consumer = makeConsumer([
         AuthEventType.SIGN_IN_VIA_REDIRECT,
         AuthEventType.LINK_VIA_REDIRECT,
-        AuthEventType.REAUTH_VIA_REDIRECT
+        AuthEventType.REAUTH_VIA_REDIRECT,
+        AuthEventType.UNKNOWN,
       ]);
     });
 
@@ -181,15 +179,13 @@ describe('src/core/auth/auth_event_manager', () => {
       expect(consumerB.onAuthEvent).not.to.have.been.called;
     });
 
-    it('unknown auth error prevents consumption of future redirect events', () => {
+    it('queues unknown events', () => {
       const event = makeEvent(AuthEventType.UNKNOWN);
       event.error = { code: 'auth/no-auth-event' } as AuthEventError;
       expect(manager.onEvent(event)).to.be.true;
-      expect(manager.onEvent(makeEvent(AuthEventType.SIGN_IN_VIA_REDIRECT))).to
-        .be.false;
 
       manager.registerConsumer(consumer);
-      expect(consumer.onAuthEvent).not.to.have.been.called;
+      expect(consumer.onAuthEvent).to.have.been.calledWith(event);
     });
   });
 });

packages-exp/auth-exp/src/core/auth/auth_event_manager.ts

@@ -16,10 +16,7 @@
  */
 
 import {
-  AuthEvent,
-  AuthEventConsumer,
-  AuthEventType,
-  EventManager
+    AuthEvent, AuthEventConsumer, AuthEventType, EventManager
 } from '../../model/popup_redirect';
 import { AUTH_ERROR_FACTORY, AuthErrorCode } from '../errors';
 
@@ -47,13 +44,6 @@ export class AuthEventManager implements EventManager {
   }
 
   onEvent(event: AuthEvent): boolean {
-    if (isNullRedirectEvent(event)) {
-      // A null redirect event comes through as the first event when no pending
-      // redirect actions are in the auth domain
-      this.hasHandledPotentialRedirect = true;
-      return true;
-    }
-
     let handled = false;
     this.consumers.forEach(consumer => {
       if (this.isEventForConsumer(event, consumer)) {
@@ -62,7 +52,7 @@ export class AuthEventManager implements EventManager {
       }
     });
 
-    if (this.hasHandledPotentialRedirect || !isRedirectEvent(event.type)) {
+    if (this.hasHandledPotentialRedirect || !isRedirectEvent(event)) {
       // If we've already seen a redirect before, or this is a popup event,
       // bail now
       return handled;
@@ -80,7 +70,7 @@ export class AuthEventManager implements EventManager {
   }
 
   private sendToConsumer(event: AuthEvent, consumer: AuthEventConsumer): void {
-    if (event.error) {
+    if (event.error && !isNullRedirectEvent(event)) {
       const code =
         (event.error.code?.split('auth/')[1] as AuthErrorCode) ||
         AuthErrorCode.INTERNAL_ERROR;
@@ -112,12 +102,14 @@ function isNullRedirectEvent({ type, error }: AuthEvent): boolean {
   );
 }
 
-function isRedirectEvent(type: AuthEventType): boolean {
-  switch (type) {
+function isRedirectEvent(event: AuthEvent): boolean {
+  switch (event.type) {
     case AuthEventType.SIGN_IN_VIA_REDIRECT:
     case AuthEventType.LINK_VIA_REDIRECT:
     case AuthEventType.REAUTH_VIA_REDIRECT:
       return true;
+    case AuthEventType.UNKNOWN:
+      return isNullRedirectEvent(event);
     default:
       return false;
   }

packages-exp/auth-exp/src/core/auth/auth_impl.ts

@@ -19,13 +19,7 @@ import { getApp } from '@firebase/app-exp';
 import { FirebaseApp } from '@firebase/app-types-exp';
 import * as externs from '@firebase/auth-types-exp';
 import {
-  CompleteFn,
-  createSubscribe,
-  ErrorFn,
-  NextFn,
-  Observer,
-  Subscribe,
-  Unsubscribe
+    CompleteFn, createSubscribe, ErrorFn, NextFn, Observer, Subscribe, Unsubscribe
 } from '@firebase/util';
 
 import { Auth, Dependencies } from '../../model/auth';
@@ -34,8 +28,7 @@ import { User } from '../../model/user';
 import { AuthErrorCode } from '../errors';
 import { Persistence } from '../persistence';
 import {
-  _REDIRECT_USER_KEY_NAME,
-  PersistenceUserManager
+    _REDIRECT_USER_KEY_NAME, PersistenceUserManager
 } from '../persistence/persistence_user_manager';
 import { _reloadWithoutSaving } from '../user/reload';
 import { assert } from '../util/assert';
@@ -60,6 +53,7 @@ export class AuthImpl implements Auth {
   private idTokenSubscription = new Subscription<User>(this);
   private redirectUser: User | null = null;
   _isInitialized = false;
+  _popupRedirectResolver: PopupRedirectResolver | null = null;
 
   // Tracks the last notified UID for state change listeners to prevent
   // repeated calls to the callbacks
@@ -79,21 +73,23 @@ export class AuthImpl implements Auth {
     popupRedirectResolver?: externs.PopupRedirectResolver
   ): Promise<void> {
     return this.queue(async () => {
+      if (popupRedirectResolver) {
+        this._popupRedirectResolver = _getInstance(popupRedirectResolver);
+      }
+
       this.persistenceManager = await PersistenceUserManager.create(
         this,
         persistenceHierarchy
       );
 
-      await this.initializeCurrentUser(popupRedirectResolver);
+      await this.initializeCurrentUser();
 
       this._isInitialized = true;
       this.notifyAuthListeners();
     });
   }
 
-  private async initializeCurrentUser(
-    popupRedirectResolver?: externs.PopupRedirectResolver
-  ): Promise<void> {
+  private async initializeCurrentUser(): Promise<void> {
     const storedUser = await this.assertedPersistence.getCurrentUser();
     if (!storedUser) {
       return this.directlySetCurrentUser(storedUser);
@@ -104,16 +100,8 @@ export class AuthImpl implements Auth {
       return this.reloadAndSetCurrentUserOrClear(storedUser);
     }
 
-    assert(popupRedirectResolver, this.name, AuthErrorCode.ARGUMENT_ERROR);
-    const resolver: PopupRedirectResolver = _getInstance(popupRedirectResolver);
-
-    this.redirectPersistenceManager = await PersistenceUserManager.create(
-      this,
-      [_getInstance(resolver._redirectPersistence)],
-      _REDIRECT_USER_KEY_NAME
-    );
-
-    this.redirectUser = await this.redirectPersistenceManager.getCurrentUser();
+    assert(this._popupRedirectResolver, this.name, AuthErrorCode.ARGUMENT_ERROR);
+    await this.getOrInitRedirectPersistenceManager();
 
     // If the redirect user's event ID matches the current user's event ID,
     // DO NOT reload the current user, otherwise they'll be cleared from storage.
@@ -189,11 +177,30 @@ export class AuthImpl implements Auth {
     );
   }
 
-  async _setRedirectUser(user: User): Promise<void> {
-    return this.redirectPersistenceManager?.setCurrentUser(user);
+  async _setRedirectUser(user: User|null, popupRedirectResolver?: externs.PopupRedirectResolver): Promise<void> {
+    const redirectManager = await this.getOrInitRedirectPersistenceManager(popupRedirectResolver);
+    return user === null ? redirectManager.removeCurrentUser() : redirectManager.setCurrentUser(user);
   }
 
-  _redirectUserForId(id: string): User | null {
+  private async getOrInitRedirectPersistenceManager(popupRedirectResolver?: externs.PopupRedirectResolver): Promise<PersistenceUserManager> {
+    if (!this.redirectPersistenceManager) {
+      const resolver: PopupRedirectResolver | null = (popupRedirectResolver && _getInstance(popupRedirectResolver) || this._popupRedirectResolver);
+      assert(resolver, this.name, AuthErrorCode.ARGUMENT_ERROR);
+      this.redirectPersistenceManager = await PersistenceUserManager.create(
+        this,
+        [_getInstance(resolver._redirectPersistence)],
+        _REDIRECT_USER_KEY_NAME
+      );
+      this.redirectUser = await this.redirectPersistenceManager.getCurrentUser();
+    }
+
+    return this.redirectPersistenceManager;
+  }
+
+  async _redirectUserForId(id: string): Promise<User | null> {
+    // Make sure we've cleared any pending ppersistence actions
+    await this.queue(async () => {});
+
     if (this.currentUser?._redirectEventId === id) {
       return this.currentUser;
     }

packages-exp/auth-exp/src/core/strategies/abstract_popup_redirect_operation.test.ts

@@ -29,10 +29,7 @@ import { testAuth, testUser } from '../../../test/mock_auth';
 import { makeMockPopupRedirectResolver } from '../../../test/mock_popup_redirect_resolver';
 import { Auth } from '../../model/auth';
 import {
-  AuthEvent,
-  AuthEventType,
-  EventManager,
-  PopupRedirectResolver
+    AuthEvent, AuthEventType, EventManager, PopupRedirectResolver
 } from '../../model/popup_redirect';
 import { AuthEventManager } from '../auth/auth_event_manager';
 import { AUTH_ERROR_FACTORY, AuthErrorCode } from '../errors';
@@ -144,7 +141,7 @@ describe('src/core/strategies/abstract_popup_redirect_operation', () => {
 
     it('emits the user credential returned from idp task', async () => {
       finishPromise(authEvent());
-      const cred = await operation.execute();
+      const cred = (await operation.execute())!;
       expect(cred.user.uid).to.eq('uid');
       expect(cred.credential).to.be.null;
       expect(cred.operationType).to.eq(OperationType.SIGN_IN);

packages-exp/auth-exp/src/core/strategies/abstract_popup_redirect_operation.ts

@@ -19,19 +19,15 @@ import { FirebaseError } from '@firebase/util';
 
 import { Auth } from '../../model/auth';
 import {
-  AuthEvent,
-  AuthEventConsumer,
-  AuthEventType,
-  EventManager,
-  PopupRedirectResolver
+    AuthEvent, AuthEventConsumer, AuthEventType, EventManager, PopupRedirectResolver
 } from '../../model/popup_redirect';
 import { User, UserCredential } from '../../model/user';
 import { AuthErrorCode } from '../errors';
 import { debugAssert, fail } from '../util/assert';
 import { _link, _reauth, _signIn, IdpTask, IdpTaskParams } from './idp';
 
 interface PendingPromise {
-  resolve: (cred: UserCredential) => void;
+  resolve: (cred: UserCredential|null) => void;
   reject: (error: Error) => void;
 }
 
@@ -58,8 +54,8 @@ export abstract class AbstractPopupRedirectOperation
 
   abstract onExecution(): Promise<void>;
 
-  execute(): Promise<UserCredential> {
-    return new Promise<UserCredential>(async (resolve, reject) => {
+  execute(): Promise<UserCredential|null> {
+    return new Promise<UserCredential|null>(async (resolve, reject) => {
       this.pendingPromise = { resolve, reject };
 
       try {
@@ -115,7 +111,7 @@ export abstract class AbstractPopupRedirectOperation
     }
   }
 
-  protected resolve(cred: UserCredential): void {
+  protected resolve(cred: UserCredential|null): void {
     debugAssert(this.pendingPromise, 'Pending promise was never set');
     this.pendingPromise.resolve(cred);
     this.unregisterAndCleanUp();

packages-exp/auth-exp/src/core/strategies/popup.test.ts

@@ -20,11 +20,7 @@ import * as chaiAsPromised from 'chai-as-promised';
 import * as sinon from 'sinon';
 import * as sinonChai from 'sinon-chai';
 
-import {
-  OperationType,
-  PopupRedirectResolver,
-  ProviderId
-} from '@firebase/auth-types-exp';
+import { OperationType, PopupRedirectResolver, ProviderId } from '@firebase/auth-types-exp';
 import { FirebaseError } from '@firebase/util';
 
 import { delay } from '../../../test/delay';
@@ -43,11 +39,8 @@ import * as eid from '../util/event_id';
 import { AuthPopup } from '../util/popup';
 import * as idpTasks from './idp';
 import {
-  _AUTH_EVENT_TIMEOUT,
-  _POLL_WINDOW_CLOSE_TIMEOUT,
-  linkWithPopup,
-  reauthenticateWithPopup,
-  signInWithPopup
+    _AUTH_EVENT_TIMEOUT, _POLL_WINDOW_CLOSE_TIMEOUT, linkWithPopup, reauthenticateWithPopup,
+    signInWithPopup
 } from './popup';
 
 use(sinonChai);

packages-exp/auth-exp/src/core/strategies/popup.ts

@@ -18,13 +18,10 @@
 import * as externs from '@firebase/auth-types-exp';
 
 import { Auth } from '../../model/auth';
-import {
-  AuthEventType,
-  PopupRedirectResolver
-} from '../../model/popup_redirect';
+import { AuthEventType, PopupRedirectResolver } from '../../model/popup_redirect';
 import { User } from '../../model/user';
 import { AUTH_ERROR_FACTORY, AuthErrorCode } from '../errors';
-import { debugAssert } from '../util/assert';
+import { assert, debugAssert } from '../util/assert';
 import { Delay } from '../util/delay';
 import { _generateEventId } from '../util/event_id';
 import { _getInstance } from '../util/instantiator';
@@ -49,7 +46,7 @@ export async function signInWithPopup(
     provider,
     resolver
   );
-  return action.execute();
+  return action.executeNotNull();
 }
 
 export async function reauthenticateWithPopup(
@@ -67,7 +64,7 @@ export async function reauthenticateWithPopup(
     resolver,
     user
   );
-  return action.execute();
+  return action.executeNotNull();
 }
 
 export async function linkWithPopup(
@@ -85,7 +82,7 @@ export async function linkWithPopup(
     resolver,
     user
   );
-  return action.execute();
+  return action.executeNotNull();
 }
 
 /**
@@ -114,6 +111,12 @@ class PopupOperation extends AbstractPopupRedirectOperation {
     PopupOperation.currentPopupAction = this;
   }
 
+  async executeNotNull(): Promise<externs.UserCredential> {
+    const result = await this.execute();
+    assert(result, this.auth.name);
+    return result;
+  }
+
   async onExecution(): Promise<void> {
     debugAssert(
       this.filter.length === 1,