Update the auth event manager to handle redirects. Add redirect logic to popupredirectresolver (#3409)

* Update event manager and abstract popup redirect operation to handle redirect events

* Missing popup_redirect update

* Tests

* Formatting

* PR feedback

* Formatting

Author: sam-gc

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

@@ -50,26 +50,17 @@ import {
   signInWithCustomToken,
   signInWithEmailAndPassword,
   unlink,
-<<<<<<< HEAD
   updateEmail,
   updatePassword,
   updateProfile,
-  verifyPasswordResetCode
-} from '@firebase/auth-exp';
-=======
-  getMultiFactorResolver,
-  multiFactor,
-  PhoneMultiFactorGenerator,
+  verifyPasswordResetCode,
   OAuthProvider,
   signInWithPopup,
   linkWithPopup,
   reauthenticateWithPopup,
-  BrowserPopupRedirectResolver
+  browserPopupRedirectResolver
 } from '@firebase/auth-exp';
 
-const browserPopupRedirectResolver = new BrowserPopupRedirectResolver();
-
->>>>>>> ad6dfaac7... Popup strategy implementation
 import { config } from './config';
 import {
   alertError,
@@ -1248,8 +1239,8 @@ function signInWithPopupRedirect(provider) {
   const glob = {
     signInWithPopup,
     linkWithPopup,
-    reauthenticateWithPopup,
-  }
+    reauthenticateWithPopup
+  };
   let action = $('input[name=popup-redirect-action]:checked').val();
   let type = $('input[name=popup-redirect-type]:checked').val();
   let method = null;
@@ -1305,14 +1296,18 @@ function signInWithPopupRedirect(provider) {
   console.log('Provider:');
   console.log(provider);
   if (type == 'popup') {
-    glob[method](inst, provider, browserPopupRedirectResolver).then(response => {
-      console.log('Popup response:');
-      console.log(response);
-      alertSuccess(action + ' with ' + provider['providerId'] + ' successful!');
-      logAdditionalUserInfo(response);
-      onAuthSuccess(activeUser());
-    },
-    onAuthError);
+    glob[method](inst, provider, browserPopupRedirectResolver).then(
+      response => {
+        console.log('Popup response:');
+        console.log(response);
+        alertSuccess(
+          action + ' with ' + provider['providerId'] + ' successful!'
+        );
+        logAdditionalUserInfo(response);
+        onAuthSuccess(activeUser());
+      },
+      onAuthError
+    );
   } else {
     alertNotImplemented();
     // try {

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

@@ -34,18 +34,15 @@ describe('src/core/auth/auth_event_manager', () => {
   let manager: AuthEventManager;
 
   function makeConsumer(
-    filter: AuthEventType
+    filter: AuthEventType | AuthEventType[]
   ): sinon.SinonStubbedInstance<AuthEventConsumer> {
     const stub = sinon.stub({
-      filter,
-      isMatchingEvent: () => true,
+      filter: Array.isArray(filter) ? filter : [filter],
       onAuthEvent: () => {},
-      onError: () => {}
+      onError: () => {},
+      eventId: null
     });
 
-    // Make isMatchingEvent call through by default
-    stub.isMatchingEvent.returns(true);
-
     return stub;
   }
 
@@ -94,21 +91,22 @@ describe('src/core/auth/auth_event_manager', () => {
     expect(consumer.onAuthEvent).not.to.have.been.called;
   });
 
-  it('calls isMatchingEvent with the event id', () => {
+  it('does not call through if eventId does not match', () => {
     const consumer = makeConsumer(AuthEventType.REAUTH_VIA_POPUP);
+    consumer.eventId = 'not-event-id';
     manager.registerConsumer(consumer);
+
     manager.onEvent(makeEvent(AuthEventType.REAUTH_VIA_POPUP, 'event-id'));
-    expect(consumer.isMatchingEvent).to.have.been.calledWith('event-id');
+    expect(consumer.onAuthEvent).not.to.have.been.called;
   });
 
-  it('does not call through if isMatchingEvent is false', () => {
+  it('does call through if eventId is null', () => {
     const consumer = makeConsumer(AuthEventType.REAUTH_VIA_POPUP);
+    consumer.eventId = null;
     manager.registerConsumer(consumer);
-    consumer.isMatchingEvent.returns(false);
 
-    manager.onEvent(makeEvent(AuthEventType.REAUTH_VIA_POPUP));
-    expect(consumer.onAuthEvent).not.to.have.been.called;
-    expect(consumer.isMatchingEvent).to.have.been.called;
+    manager.onEvent(makeEvent(AuthEventType.REAUTH_VIA_POPUP, 'event-id'));
+    expect(consumer.onAuthEvent).to.have.been.called;
   });
 
   it('converts errors into FirebaseError if the type matches', () => {
@@ -139,4 +137,59 @@ describe('src/core/auth/auth_event_manager', () => {
     const error = consumer.onError.getCall(0).args[0];
     expect(error.code).to.eq(`auth/${AuthErrorCode.INTERNAL_ERROR}`);
   });
+
+  context('redirect consumers', () => {
+    let consumer: AuthEventConsumer;
+
+    beforeEach(() => {
+      consumer = makeConsumer([
+        AuthEventType.SIGN_IN_VIA_REDIRECT,
+        AuthEventType.LINK_VIA_REDIRECT,
+        AuthEventType.REAUTH_VIA_REDIRECT
+      ]);
+    });
+
+    it('redirect events are queued until the future', () => {
+      const event = makeEvent(AuthEventType.REAUTH_VIA_REDIRECT);
+      expect(manager.onEvent(event)).to.be.true;
+
+      manager.registerConsumer(consumer);
+      expect(consumer.onAuthEvent).to.have.been.calledWith(event);
+    });
+
+    it('queued redirects only work for the first new consumer', () => {
+      const event = makeEvent(AuthEventType.REAUTH_VIA_REDIRECT);
+      expect(manager.onEvent(event)).to.be.true;
+
+      manager.registerConsumer(consumer);
+      expect(consumer.onAuthEvent).to.have.been.calledWith(event);
+
+      const consumerB = makeConsumer(AuthEventType.REAUTH_VIA_REDIRECT);
+      manager.registerConsumer(consumerB);
+      expect(consumerB.onAuthEvent).not.to.have.been.called;
+    });
+
+    it('does not queue a redirect event if it was handled immediately', () => {
+      const event = makeEvent(AuthEventType.REAUTH_VIA_REDIRECT);
+      manager.registerConsumer(consumer);
+
+      expect(manager.onEvent(event)).to.be.true;
+      expect(consumer.onAuthEvent).to.have.been.calledWith(event);
+
+      const consumerB = makeConsumer(AuthEventType.REAUTH_VIA_REDIRECT);
+      manager.registerConsumer(consumerB);
+      expect(consumerB.onAuthEvent).not.to.have.been.called;
+    });
+
+    it('unknown auth error prevents consumption of future redirect 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;
+    });
+  });
 });

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

@@ -18,43 +18,107 @@
 import {
   AuthEvent,
   AuthEventConsumer,
+  AuthEventType,
   EventManager
 } from '../../model/popup_redirect';
 import { AUTH_ERROR_FACTORY, AuthErrorCode } from '../errors';
 
 export class AuthEventManager implements EventManager {
   private readonly consumers: Set<AuthEventConsumer> = new Set();
+  private queuedRedirectEvent: AuthEvent | null = null;
+  private hasHandledPotentialRedirect = false;
 
   constructor(private readonly appName: string) {}
 
   registerConsumer(authEventConsumer: AuthEventConsumer): void {
     this.consumers.add(authEventConsumer);
+
+    if (
+      this.queuedRedirectEvent &&
+      this.isEventForConsumer(this.queuedRedirectEvent, authEventConsumer)
+    ) {
+      this.sendToConsumer(this.queuedRedirectEvent, authEventConsumer);
+      this.queuedRedirectEvent = null;
+    }
   }
 
   unregisterConsumer(authEventConsumer: AuthEventConsumer): void {
     this.consumers.delete(authEventConsumer);
   }
 
-  onEvent(event: AuthEvent): void {
+  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 (
-        consumer.filter === event.type &&
-        consumer.isMatchingEvent(event.eventId)
-      ) {
-        if (event.error) {
-          console.error('ERROR');
-          const code =
-            (event.error.code?.split('auth/')[1] as AuthErrorCode) ||
-            AuthErrorCode.INTERNAL_ERROR;
-          consumer.onError(
-            AUTH_ERROR_FACTORY.create(code, {
-              appName: this.appName
-            })
-          );
-        } else {
-          consumer.onAuthEvent(event);
-        }
+      if (this.isEventForConsumer(event, consumer)) {
+        handled = true;
+        this.sendToConsumer(event, consumer);
       }
     });
+
+    if (this.hasHandledPotentialRedirect || !isRedirectEvent(event.type)) {
+      // If we've already seen a redirect before, or this is a popup event,
+      // bail now
+      return handled;
+    }
+
+    this.hasHandledPotentialRedirect = true;
+
+    // If the redirect wasn't handled, hang on to it
+    if (!handled) {
+      this.queuedRedirectEvent = event;
+      handled = true;
+    }
+
+    return handled;
+  }
+
+  private sendToConsumer(event: AuthEvent, consumer: AuthEventConsumer): void {
+    if (event.error) {
+      const code =
+        (event.error.code?.split('auth/')[1] as AuthErrorCode) ||
+        AuthErrorCode.INTERNAL_ERROR;
+      consumer.onError(
+        AUTH_ERROR_FACTORY.create(code, {
+          appName: this.appName
+        })
+      );
+    } else {
+      consumer.onAuthEvent(event);
+    }
+  }
+
+  private isEventForConsumer(
+    event: AuthEvent,
+    consumer: AuthEventConsumer
+  ): boolean {
+    const eventIdMatches =
+      consumer.eventId === null ||
+      (!!event.eventId && event.eventId === consumer.eventId);
+    return consumer.filter.includes(event.type) && eventIdMatches;
+  }
+}
+
+function isNullRedirectEvent({ type, error }: AuthEvent): boolean {
+  return (
+    type === AuthEventType.UNKNOWN &&
+    error?.code === `auth/${AuthErrorCode.NO_AUTH_EVENT}`
+  );
+}
+
+function isRedirectEvent(type: AuthEventType): boolean {
+  switch (type) {
+    case AuthEventType.SIGN_IN_VIA_REDIRECT:
+    case AuthEventType.LINK_VIA_REDIRECT:
+    case AuthEventType.REAUTH_VIA_REDIRECT:
+      return true;
+    default:
+      return false;
   }
 }

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

@@ -43,15 +43,18 @@ export abstract class AbstractPopupRedirectOperation
   implements AuthEventConsumer {
   private pendingPromise: PendingPromise | null = null;
   private eventManager: EventManager | null = null;
+  readonly filter: AuthEventType[];
 
   abstract eventId: string | null;
 
   constructor(
     protected readonly auth: Auth,
-    readonly filter: AuthEventType,
+    filter: AuthEventType | AuthEventType[],
     protected readonly resolver: PopupRedirectResolver,
     protected user?: User
-  ) {}
+  ) {
+    this.filter = Array.isArray(filter) ? filter : [filter];
+  }
 
   abstract onExecution(): Promise<void>;
 
@@ -96,10 +99,6 @@ export abstract class AbstractPopupRedirectOperation
     this.reject(error);
   }
 
-  isMatchingEvent(eventId: string | null): boolean {
-    return !!eventId && this.eventId === eventId;
-  }
-
   private getIdpTask(type: AuthEventType): IdpTask {
     switch (type) {
       case AuthEventType.SIGN_IN_VIA_POPUP:

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

@@ -414,7 +414,7 @@ describe('core/strategies/phone', () => {
     });
   });
 
-  describe("updatePhoneNumber", () => {
+  describe('updatePhoneNumber', () => {
     let user: User;
     let reloadMock: fetch.Route;
     let signInMock: fetch.Route;

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

@@ -24,6 +24,7 @@ import {
 } from '../../model/popup_redirect';
 import { User } from '../../model/user';
 import { AUTH_ERROR_FACTORY, AuthErrorCode } from '../errors';
+import { debugAssert } from '../util/assert';
 import { Delay } from '../util/delay';
 import { _generateEventId } from '../util/event_id';
 import { _getInstance } from '../util/instantiator';
@@ -100,7 +101,7 @@ class PopupOperation extends AbstractPopupRedirectOperation {
 
   constructor(
     auth: Auth,
-    readonly filter: AuthEventType,
+    filter: AuthEventType,
     private readonly provider: externs.AuthProvider,
     resolver: PopupRedirectResolver,
     user?: User
@@ -114,11 +115,15 @@ class PopupOperation extends AbstractPopupRedirectOperation {
   }
 
   async onExecution(): Promise<void> {
+    debugAssert(
+      this.filter.length === 1,
+      'Popup operations only handle one event'
+    );
     const eventId = _generateEventId();
     this.authWindow = await this.resolver._openPopup(
       this.auth,
       this.provider,
-      this.filter,
+      this.filter[0], // There's always one, see constructor
       eventId
     );
     this.authWindow.associatedEvent = eventId;

packages-exp/auth-exp/src/index.ts

@@ -128,5 +128,4 @@ export { PhoneMultiFactorGenerator } from './mfa/assertions/phone';
 export { getMultiFactorResolver } from './mfa/mfa_resolver';
 export { multiFactor } from './mfa/mfa_user';
 
-// TODO(samhorlbeck): This should be exported as a single const
-export { BrowserPopupRedirectResolver } from './platform_browser/popup_redirect';
+export { browserPopupRedirectResolver } from './platform_browser/popup_redirect';