Set key in storage before redirect, only open iframe if that key is present

Author: sam-gc

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

@@ -132,6 +132,12 @@ export class AuthImpl implements AuthInternal, _FirebaseService {
         return;
       }
 
+      // Initialize the resolver early if necessary (only applicable to web:
+      // this will cause the iframe to load immediately in certain cases)
+      if (this._popupRedirectResolver?._shouldInitProactively) {
+        await this._popupRedirectResolver._initialize(this);
+      }
+
       await this.initializeCurrentUser(popupRedirectResolver);
 
       if (this._deleted) {

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

@@ -117,6 +117,7 @@ describe('core/auth/initialize', () => {
       cb(true);
     }
     async _originValidation(): Promise<void> {}
+    _shouldInitProactively = false;
     async _completeRedirectFn(
       _auth: Auth,
       _resolver: PopupRedirectResolver,

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

@@ -22,7 +22,8 @@ import {
   ProviderId
 } from '../../model/public_types';
 import * as sinon from 'sinon';
-import { _getInstance } from '../util/instantiator';
+import * as sinonChai from 'sinon-chai';
+import { _clearInstanceMap, _getInstance } from '../util/instantiator';
 import {
   MockPersistenceLayer,
   TestAuth,
@@ -39,24 +40,24 @@ import {
   PopupRedirectResolverInternal
 } from '../../model/popup_redirect';
 import { BASE_AUTH_EVENT } from '../../../test/helpers/iframe_event';
-import { PersistenceInternal } from '../persistence';
-import { InMemoryPersistence } from '../persistence/in_memory';
 import { UserCredentialImpl } from '../user/user_credential_impl';
 import * as idpTasks from '../strategies/idp';
-import { expect } from 'chai';
+import { expect, use } from 'chai';
 import { AuthErrorCode } from '../errors';
+import { RedirectPersistence } from '../../../test/helpers/redirect_persistence';
+
+use(sinonChai);
 
 const MATCHING_EVENT_ID = 'matching-event-id';
 const OTHER_EVENT_ID = 'wrong-id';
 
-class RedirectPersistence extends InMemoryPersistence {}
-
 describe('core/strategies/redirect', () => {
   let auth: AuthInternal;
   let redirectAction: RedirectAction;
   let eventManager: AuthEventManager;
   let resolver: PopupRedirectResolver;
   let idpStubs: sinon.SinonStubbedInstance<typeof idpTasks>;
+  let redirectPersistence: RedirectPersistence;
 
   beforeEach(async () => {
     eventManager = new AuthEventManager(({} as unknown) as TestAuth);
@@ -67,11 +68,16 @@ describe('core/strategies/redirect', () => {
     )._redirectPersistence = RedirectPersistence;
     auth = await testAuth();
     redirectAction = new RedirectAction(auth, _getInstance(resolver), false);
+    redirectPersistence = _getInstance(RedirectPersistence);
+
+    // Default to has redirect for most test
+    redirectPersistence.hasPendingRedirect = true;
   });
 
   afterEach(() => {
     sinon.restore();
     _clearRedirectOutcomes();
+    _clearInstanceMap();
   });
 
   function iframeEvent(event: Partial<AuthEvent>): void {
@@ -86,16 +92,11 @@ describe('core/strategies/redirect', () => {
   }
 
   async function reInitAuthWithRedirectUser(eventId: string): Promise<void> {
-    const redirectPersistence: PersistenceInternal = _getInstance(
-      RedirectPersistence
-    );
     const mainPersistence = new MockPersistenceLayer();
     const oldAuth = await testAuth();
     const user = testUser(oldAuth, 'uid');
     user._redirectEventId = eventId;
-    sinon
-      .stub(redirectPersistence, '_get')
-      .returns(Promise.resolve(user.toJSON()));
+    redirectPersistence.redirectUser = user.toJSON();
     sinon.stub(mainPersistence, '_get').returns(Promise.resolve(user.toJSON()));
 
     auth = await testAuth(resolver, mainPersistence);
@@ -194,4 +195,16 @@ describe('core/strategies/redirect', () => {
     expect(await promise).to.eq(cred);
     expect(await redirectAction.execute()).to.eq(cred);
   });
+
+  it('bypasses initialization if no key set', async () => {
+    await reInitAuthWithRedirectUser(MATCHING_EVENT_ID);
+    const resolverInstance = _getInstance<PopupRedirectResolverInternal>(resolver);
+
+    sinon.spy(resolverInstance, '_initialize');
+    redirectPersistence.hasPendingRedirect = false;
+   
+    expect(await redirectAction.execute()).to.eq(null);
+    expect(await redirectAction.execute()).to.eq(null);
+    expect(resolverInstance._initialize).not.to.have.been.called;
+  });
 });

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

@@ -22,8 +22,13 @@ import {
   PopupRedirectResolverInternal
 } from '../../model/popup_redirect';
 import { UserCredentialInternal } from '../../model/user';
+import { PersistenceInternal } from '../persistence';
+import { _persistenceKeyName } from '../persistence/persistence_user_manager';
+import { _getInstance } from '../util/instantiator';
 import { AbstractPopupRedirectOperation } from './abstract_popup_redirect_operation';
 
+const PENDING_REDIRECT_KEY = 'pendingRedirect';
+
 // We only get one redirect outcome for any one auth, so just store it
 // in here.
 const redirectOutcomeMap: Map<
@@ -61,7 +66,8 @@ export class RedirectAction extends AbstractPopupRedirectOperation {
     let readyOutcome = redirectOutcomeMap.get(this.auth._key());
     if (!readyOutcome) {
       try {
-        const result = await super.execute();
+        const hasPendingRedirect = await _getAndClearPendingRedirectStatus(this.resolver, this.auth);
+        const result = hasPendingRedirect ? await super.execute() : null;
         readyOutcome = () => Promise.resolve(result);
       } catch (e) {
         readyOutcome = () => Promise.reject(e);
@@ -98,6 +104,25 @@ export class RedirectAction extends AbstractPopupRedirectOperation {
   cleanUp(): void {}
 }
 
+export async function _getAndClearPendingRedirectStatus(resolver: PopupRedirectResolverInternal, auth: AuthInternal): Promise<boolean> {
+  const key = pendingRedirectKey(auth);
+  const hasPendingRedirect = await resolverPersistence(resolver)._get(key) === 'true';
+  await resolverPersistence(resolver)._remove(key);
+  return hasPendingRedirect;
+}
+
+export async function _setPendingRedirectStatus(resolver: PopupRedirectResolverInternal, auth: AuthInternal): Promise<void> {
+  return resolverPersistence(resolver)._set(pendingRedirectKey(auth), 'true');
+}
+
 export function _clearRedirectOutcomes(): void {
   redirectOutcomeMap.clear();
 }
+
+function resolverPersistence(resolver: PopupRedirectResolverInternal): PersistenceInternal {
+  return _getInstance(resolver._redirectPersistence);
+}
+
+function pendingRedirectKey(auth: AuthInternal): string {
+  return _persistenceKeyName(PENDING_REDIRECT_KEY, auth.config.apiKey, auth.name);
+}

packages-exp/auth-exp/src/core/util/browser.ts

@@ -92,7 +92,7 @@ export function _isFirefox(ua = getUA()): boolean {
   return /firefox\//i.test(ua);
 }
 
-export function _isSafari(userAgent: string): boolean {
+export function _isSafari(userAgent = getUA()): boolean {
   const ua = userAgent.toLowerCase();
   return (
     ua.includes('safari/') &&

packages-exp/auth-exp/src/core/util/instantiator.ts

@@ -46,3 +46,7 @@ export function _getInstance<T>(cls: unknown): T {
   instanceCache.set(cls, instance);
   return instance;
 }
+
+export function _clearInstanceMap(): void {
+  instanceCache.clear();
+}

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

@@ -80,6 +80,9 @@ export interface EventManager {
 }
 
 export interface PopupRedirectResolverInternal extends PopupRedirectResolver {
+  // Whether or not to initialize the event manager early
+  _shouldInitProactively: boolean;
+
   _initialize(auth: AuthInternal): Promise<EventManager>;
   _openPopup(
     auth: AuthInternal,

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

@@ -46,6 +46,7 @@ import { PopupRedirectResolverInternal } from '../model/popup_redirect';
 import { UserCredentialImpl } from '../core/user/user_credential_impl';
 import { UserInternal } from '../model/user';
 import { _createError } from '../core/util/assert';
+import { makeMockPopupRedirectResolver } from '../../test/helpers/mock_popup_redirect_resolver';
 
 use(sinonChai);
 use(chaiAsPromised);
@@ -191,6 +192,24 @@ describe('core/auth/initializeAuth', () => {
       expect(reload._reloadWithoutSaving).not.to.have.been.called;
     });
 
+    it('does not early-initialize the resolver', async () => {
+      const popupRedirectResolver = makeMockPopupRedirectResolver();
+      const resolverInternal: PopupRedirectResolverInternal = _getInstance(popupRedirectResolver);
+      sinon.stub(resolverInternal, '_shouldInitProactively').value(false);
+      sinon.spy(resolverInternal, '_initialize');
+      await initAndWait(inMemoryPersistence, popupRedirectResolver);
+      expect(resolverInternal._initialize).not.to.have.been.called;
+    });
+
+    it('does early-initialize the resolver', async () => {
+      const popupRedirectResolver = makeMockPopupRedirectResolver();
+      const resolverInternal: PopupRedirectResolverInternal = _getInstance(popupRedirectResolver);
+      sinon.stub(resolverInternal, '_shouldInitProactively').value(true);
+      sinon.spy(resolverInternal, '_initialize');
+      await initAndWait(inMemoryPersistence, popupRedirectResolver);
+      expect(resolverInternal._initialize).to.have.been.called;
+    });
+
     it('reloads non-redirect users', async () => {
       sinon
         .stub(_getInstance<PersistenceInternal>(inMemoryPersistence), '_get')

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

@@ -37,6 +37,7 @@ import { browserSessionPersistence } from './persistence/session_storage';
 import { _open, AuthPopup } from './util/popup';
 import { _getRedirectResult } from './strategies/redirect';
 import { _getRedirectUrl } from '../core/util/handler';
+import { _isIOS, _isMobileBrowser, _isSafari } from '../core/util/browser';
 
 /**
  * The special web storage event
@@ -162,6 +163,11 @@ class BrowserPopupRedirectResolver implements PopupRedirectResolverInternal {
     return this.originValidationPromises[key];
   }
 
+  get _shouldInitProactively(): boolean {
+    // Mobile browsers and Safari need to optimistically initialize
+    return _isMobileBrowser() || _isSafari() || _isIOS();
+  }
+
   _completeRedirectFn = _getRedirectResult;
 }
 

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

@@ -45,11 +45,10 @@ import { UserInternal } from '../../model/user';
 import { AuthEventManager } from '../../core/auth/auth_event_manager';
 import { AuthErrorCode } from '../../core/errors';
 import { PersistenceInternal } from '../../core/persistence';
-import { InMemoryPersistence } from '../../core/persistence/in_memory';
 import { OAuthProvider } from '../../core/providers/oauth';
 import * as link from '../../core/user/link_unlink';
 import { UserCredentialImpl } from '../../core/user/user_credential_impl';
-import { _getInstance } from '../../core/util/instantiator';
+import { _clearInstanceMap, _getInstance } from '../../core/util/instantiator';
 import * as idpTasks from '../../core/strategies/idp';
 import {
   getRedirectResult,
@@ -60,15 +59,14 @@ import {
 } from './redirect';
 import { FirebaseError } from '@firebase/util';
 import { _clearRedirectOutcomes } from '../../core/strategies/redirect';
+import { RedirectPersistence } from '../../../test/helpers/redirect_persistence';
 
 use(sinonChai);
 use(chaiAsPromised);
 
 const MATCHING_EVENT_ID = 'matching-event-id';
 const OTHER_EVENT_ID = 'wrong-id';
 
-class RedirectPersistence extends InMemoryPersistence {}
-
 describe('platform_browser/strategies/redirect', () => {
   let auth: TestAuth;
   let eventManager: AuthEventManager;
@@ -85,11 +83,13 @@ describe('platform_browser/strategies/redirect', () => {
     )._redirectPersistence = RedirectPersistence;
     auth = await testAuth(resolver);
     idpStubs = sinon.stub(idpTasks);
+    _getInstance<RedirectPersistence>(RedirectPersistence).hasPendingRedirect = true;
   });
 
   afterEach(() => {
     sinon.restore();
     _clearRedirectOutcomes();
+    _clearInstanceMap();
   });
 
   context('signInWithRedirect', () => {
@@ -299,16 +299,14 @@ describe('platform_browser/strategies/redirect', () => {
     }
 
     async function reInitAuthWithRedirectUser(eventId: string): Promise<void> {
-      const redirectPersistence: PersistenceInternal = _getInstance(
+      const redirectPersistence: RedirectPersistence = _getInstance(
         RedirectPersistence
       );
       const mainPersistence = new MockPersistenceLayer();
       const oldAuth = await testAuth();
       const user = testUser(oldAuth, 'uid');
       user._redirectEventId = eventId;
-      sinon
-        .stub(redirectPersistence, '_get')
-        .returns(Promise.resolve(user.toJSON()));
+      redirectPersistence.redirectUser = user.toJSON();
       sinon
         .stub(mainPersistence, '_get')
         .returns(Promise.resolve(user.toJSON()));
@@ -427,7 +425,7 @@ describe('platform_browser/strategies/redirect', () => {
         type: AuthEventType.LINK_VIA_REDIRECT
       });
       expect(await promise).to.eq(cred);
-      expect(redirectPersistence._remove).to.have.been.called;
+      expect(redirectPersistence._remove).to.have.been.calledWith('firebase:redirectUser:test-api-key:test-app');
       expect(auth._currentUser?._redirectEventId).to.be.undefined;
       expect(auth.persistenceLayer.lastObjectSet?._redirectEventId).to.be
         .undefined;
@@ -451,7 +449,7 @@ describe('platform_browser/strategies/redirect', () => {
         type: AuthEventType.LINK_VIA_REDIRECT
       });
       expect(await promise).to.eq(cred);
-      expect(redirectPersistence._remove).not.to.have.been.called;
+      expect(redirectPersistence._remove).not.to.have.been.calledWith('firebase:redirectUser:test-api-key:test-app');
       expect(auth._currentUser?._redirectEventId).not.to.be.undefined;
       expect(auth.persistenceLayer.lastObjectSet?._redirectEventId).not.to.be
         .undefined;

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

@@ -32,7 +32,7 @@ import { _generateEventId } from '../../core/util/event_id';
 import { AuthEventType } from '../../model/popup_redirect';
 import { UserInternal } from '../../model/user';
 import { _withDefaultResolver } from '../../core/util/resolver';
-import { RedirectAction } from '../../core/strategies/redirect';
+import { RedirectAction, _setPendingRedirectStatus } from '../../core/strategies/redirect';
 
 /**
  * Authenticates a Firebase client using a full-page redirect flow.
@@ -93,7 +93,10 @@ export async function _signInWithRedirect(
     AuthErrorCode.ARGUMENT_ERROR
   );
 
-  return _withDefaultResolver(authInternal, resolver)._openRedirect(
+  const resolverInternal = _withDefaultResolver(authInternal, resolver);
+  await _setPendingRedirectStatus(resolverInternal, authInternal);
+
+  return resolverInternal._openRedirect(
     authInternal,
     provider,
     AuthEventType.SIGN_IN_VIA_REDIRECT
@@ -153,6 +156,7 @@ export async function _reauthenticateWithRedirect(
 
   // Allow the resolver to error before persisting the redirect user
   const resolverInternal = _withDefaultResolver(userInternal.auth, resolver);
+  await _setPendingRedirectStatus(resolverInternal, userInternal.auth);
 
   const eventId = await prepareUserForRedirect(userInternal);
   return resolverInternal._openRedirect(
@@ -209,8 +213,9 @@ export async function _linkWithRedirect(
 
   // Allow the resolver to error before persisting the redirect user
   const resolverInternal = _withDefaultResolver(userInternal.auth, resolver);
-
   await _assertLinkedStatus(false, userInternal, provider.providerId);
+  await _setPendingRedirectStatus(resolverInternal, userInternal.auth);
+
   const eventId = await prepareUserForRedirect(userInternal);
   return resolverInternal._openRedirect(
     userInternal.auth,

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

@@ -52,6 +52,7 @@ const INITIAL_EVENT_TIMEOUT_MS = 500;
 
 class CordovaPopupRedirectResolver implements PopupRedirectResolverInternal {
   readonly _redirectPersistence = browserSessionPersistence;
+  readonly _shouldInitProactively = true;  // This is lightweight for Cordova
   private readonly eventManagers = new Map<string, CordovaAuthEventManager>();
 
   _completeRedirectFn = _getRedirectResult;

packages-exp/auth-exp/test/helpers/mock_popup_redirect_resolver.ts

@@ -56,6 +56,8 @@ export function makeMockPopupRedirectResolver(
 
     _redirectPersistence?: Persistence;
 
+    _shouldInitProactively = false;
+
     async _completeRedirectFn(): Promise<void> {}
 
     async _originValidation(): Promise<void> {}

packages-exp/auth-exp/test/helpers/redirect_persistence.ts

@@ -0,0 +1,16 @@
+import { PersistenceValue } from '../../src/core/persistence';
+import { InMemoryPersistence } from '../../src/core/persistence/in_memory';
+
+/** Helper class for handling redirect persistence */
+export class RedirectPersistence extends InMemoryPersistence {
+  hasPendingRedirect = false;
+  redirectUser: object|null = null;
+
+  async _get<T extends PersistenceValue>(key: string): Promise<T|null> {
+    if (key.includes('pendingRedirect')) {
+      return this.hasPendingRedirect.toString() as T;
+    }
+
+    return this.redirectUser as T | null;
+  }
+}