Add auth isolation for oauth flows (#3420)

* Add auth isolation for oauth flows

* Formatting

* Fix bug

* Formatting

* PR feedback

* Formatting

Author: sam-gc

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

@@ -247,6 +247,10 @@ export class AuthImpl implements Auth {
     }
   }
 
+  _key(): string {
+    return `${this.config.authDomain}:${this.config.apiKey}:${this.name}`;
+  }
+
   private notifyAuthListeners(): void {
     if (!this._isInitialized) {
       return;

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

@@ -47,6 +47,7 @@ import { UserCredentialImpl } from '../user/user_credential_impl';
 import { _getInstance } from '../util/instantiator';
 import * as idpTasks from './idp';
 import {
+  _clearOutcomes,
   getRedirectResult,
   linkWithRedirect,
   reauthenticateWithRedirect,
@@ -81,6 +82,7 @@ describe('src/core/strategies/redirect', () => {
 
   afterEach(() => {
     sinon.restore();
+    _clearOutcomes();
   });
 
   context('signInWithRedirect', () => {

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

@@ -107,10 +107,10 @@ async function prepareUserForRedirect(auth: Auth, user: User): Promise<string> {
 
 // We only get one redirect outcome for any one auth, so just store it
 // in here.
-const redirectOutcomeMap: WeakMap<
-  Auth,
+const redirectOutcomeMap: Map<
+  string,
   () => Promise<UserCredential | null>
-> = new WeakMap();
+> = new Map();
 
 class RedirectAction extends AbstractPopupRedirectOperation {
   eventId = null;
@@ -133,7 +133,7 @@ class RedirectAction extends AbstractPopupRedirectOperation {
    * just return it.
    */
   async execute(): Promise<UserCredential | null> {
-    let readyOutcome = redirectOutcomeMap.get(this.auth);
+    let readyOutcome = redirectOutcomeMap.get(this.auth._key());
     if (!readyOutcome) {
       try {
         const result = await super.execute();
@@ -142,7 +142,7 @@ class RedirectAction extends AbstractPopupRedirectOperation {
         readyOutcome = () => Promise.reject(e);
       }
 
-      redirectOutcomeMap.set(this.auth, readyOutcome);
+      redirectOutcomeMap.set(this.auth._key(), readyOutcome);
     }
 
     return readyOutcome();
@@ -172,3 +172,7 @@ class RedirectAction extends AbstractPopupRedirectOperation {
 
   cleanUp(): void {}
 }
+
+export function _clearOutcomes(): void {
+  redirectOutcomeMap.clear();
+}

packages-exp/auth-exp/src/model/auth.d.ts

@@ -49,6 +49,7 @@ export interface Auth extends externs.Auth {
   ): Promise<void>;
   _redirectUserForId(id: string): Promise<User | null>;
   _popupRedirectResolver: PopupRedirectResolver | null;
+  _key(): string;
 }
 
 export interface Dependencies {

packages-exp/auth-exp/src/platform_browser/popup_redirect.test.ts

@@ -81,10 +81,10 @@ describe('src/platform_browser/popup_redirect', () => {
         return {} as Window;
       });
       provider = new OAuthProvider(ProviderId.GOOGLE);
-      await resolver._initialize(auth);
     });
 
     it('builds the correct url', async () => {
+      await resolver._initialize(auth);
       provider.addScope('some-scope-a');
       provider.addScope('some-scope-b');
       provider.setCustomParameters({ foo: 'bar' });
@@ -105,6 +105,7 @@ describe('src/platform_browser/popup_redirect', () => {
 
     it('throws an error if authDomain is unspecified', async () => {
       delete auth.config.authDomain;
+      await resolver._initialize(auth);
 
       await expect(
         resolver._openPopup(auth, provider, event)
@@ -113,6 +114,7 @@ describe('src/platform_browser/popup_redirect', () => {
 
     it('throws an error if apiKey is unspecified', async () => {
       delete auth.config.apiKey;
+      await resolver._initialize(auth);
 
       await expect(
         resolver._openPopup(auth, provider, event)
@@ -176,9 +178,26 @@ describe('src/platform_browser/popup_redirect', () => {
   });
 
   context('#_initialize', () => {
-    it('only registers once, returns same event manager', async () => {
+    it('returns different manager for a different auth', async () => {
       const manager = await resolver._initialize(auth);
       expect(await resolver._initialize(auth)).to.eq(manager);
+
+      const secondAuth = await testAuth();
+      secondAuth.config.authDomain = 'something-else';
+      const secondManager = await resolver._initialize(secondAuth);
+      expect(secondManager).not.to.eq(manager);
+      expect(await resolver._initialize(secondAuth)).to.eq(secondManager);
+    });
+
+    it('initialization promise is cached as well for diff auths', async () => {
+      const promise = resolver._initialize(auth);
+      expect(resolver._initialize(auth)).to.eq(promise);
+
+      const secondAuth = await testAuth();
+      secondAuth.config.authDomain = 'something-else';
+      const secondPromise = resolver._initialize(secondAuth);
+      expect(secondPromise).not.to.eq(promise);
+      expect(resolver._initialize(secondAuth)).to.eq(secondPromise);
     });
 
     it('iframe event goes through to the manager', async () => {

packages-exp/auth-exp/src/platform_browser/popup_redirect.ts

@@ -43,9 +43,13 @@ import { _openIframe } from './iframe/iframe';
  */
 const WIDGET_URL = '__/auth/handler';
 
+interface ManagerOrPromise {
+  manager?: EventManager;
+  promise?: Promise<EventManager>;
+}
+
 class BrowserPopupRedirectResolver implements PopupRedirectResolver {
-  private eventManager: EventManager | null = null;
-  private initializationPromise: Promise<EventManager> | null = null;
+  private readonly eventManagers: Record<string, ManagerOrPromise> = {};
 
   readonly _redirectPersistence = browserSessionPersistence;
 
@@ -58,7 +62,7 @@ class BrowserPopupRedirectResolver implements PopupRedirectResolver {
     eventId?: string
   ): Promise<AuthPopup> {
     debugAssert(
-      this.eventManager,
+      this.eventManagers[auth._key()]?.manager,
       '_initialize() not called before _openPopup()'
     );
     const url = getRedirectUrl(auth, provider, authType, eventId);
@@ -76,32 +80,37 @@ class BrowserPopupRedirectResolver implements PopupRedirectResolver {
   }
 
   _initialize(auth: Auth): Promise<EventManager> {
-    if (this.eventManager) {
-      return Promise.resolve(this.eventManager);
-    }
-
-    if (!this.initializationPromise) {
-      this.initializationPromise = this.initAndGetManager(auth);
+    const key = auth._key();
+    if (this.eventManagers[key]) {
+      const { manager, promise } = this.eventManagers[key];
+      if (manager) {
+        return Promise.resolve(manager);
+      } else {
+        debugAssert(promise, 'If manager is not set, promise should be');
+        return promise;
+      }
     }
 
-    return this.initializationPromise;
+    const promise = this.initAndGetManager(auth);
+    this.eventManagers[key] = { promise };
+    return promise;
   }
 
   private async initAndGetManager(auth: Auth): Promise<EventManager> {
     const iframe = await _openIframe(auth);
-    const eventManager = new AuthEventManager(auth.name);
+    const manager = new AuthEventManager(auth.name);
     iframe.register<GapiAuthEvent>(
       'authEvent',
       ({ authEvent }: GapiAuthEvent) => {
         // TODO: Consider splitting redirect and popup events earlier on
-        const handled = eventManager.onEvent(authEvent);
+        const handled = manager.onEvent(authEvent);
         return { status: handled ? GapiOutcome.ACK : GapiOutcome.ERROR };
       },
       gapi.iframes.CROSS_ORIGIN_IFRAMES_FILTER
     );
 
-    this.eventManager = eventManager;
-    return eventManager;
+    this.eventManagers[auth._key()] = { manager };
+    return manager;
   }
 }