[Auth] set key in storage before redirect, check this key before checking for redirect events (#4550)

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

* Formatting, License

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

Co-authored-by: Yuchen Shi <[email protected]>

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

Co-authored-by: Yuchen Shi <[email protected]>

* PR feedback, fix compat tests

* Formatting

Co-authored-by: Yuchen Shi <[email protected]>

Author: sam-gc

packages-exp/auth-compat-exp/src/platform.ts

@@ -162,8 +162,13 @@ export function _getClientPlatform(): impl.ClientPlatform {
   return impl.ClientPlatform.BROWSER;
 }
 
+/** Quick check that indicates the platform *may* be Cordova */
+export function _isLikelyCordova(): boolean {
+  return _isAndroidOrIosCordovaScheme() && typeof document !== 'undefined';
+}
+
 export async function _isCordova(): Promise<boolean> {
-  if (!_isAndroidOrIosCordovaScheme() || typeof document === 'undefined') {
+  if (!_isLikelyCordova()) {
     return false;
   }
 

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

@@ -126,11 +126,45 @@ describe('popup_redirect/CompatPopupRedirectResolver', () => {
       );
     });
   });
+
+  context('_shouldInitProactively', () => {
+    it('returns true if platform may be cordova', () => {
+      sinon.stub(platform, '_isLikelyCordova').returns(true);
+      expect(compatResolver._shouldInitProactively).to.be.true;
+    });
+
+    it('returns true if cordova is false but browser value is true', () => {
+      sinon
+        .stub(
+          exp._getInstance<exp.PopupRedirectResolverInternal>(
+            exp.browserPopupRedirectResolver
+          ),
+          '_shouldInitProactively'
+        )
+        .value(true);
+      sinon.stub(platform, '_isLikelyCordova').returns(false);
+      expect(compatResolver._shouldInitProactively).to.be.true;
+    });
+
+    it('returns false if not cordova and not browser early init', () => {
+      sinon
+        .stub(
+          exp._getInstance<exp.PopupRedirectResolverInternal>(
+            exp.browserPopupRedirectResolver
+          ),
+          '_shouldInitProactively'
+        )
+        .value(false);
+      sinon.stub(platform, '_isLikelyCordova').returns(false);
+      expect(compatResolver._shouldInitProactively).to.be.false;
+    });
+  });
 });
 
 class FakeResolver implements exp.PopupRedirectResolverInternal {
   _completeRedirectFn = async (): Promise<null> => null;
   _redirectPersistence = exp.inMemoryPersistence;
+  _shouldInitProactively = true;
 
   _initialize(): Promise<exp.EventManager> {
     throw new Error('Method not implemented.');

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

@@ -16,9 +16,15 @@
  */
 
 import * as exp from '@firebase/auth-exp/internal';
-import { _isCordova } from './platform';
+import { _isCordova, _isLikelyCordova } from './platform';
 
 const _assert: typeof exp._assert = exp._assert;
+const BROWSER_RESOLVER: exp.PopupRedirectResolverInternal = exp._getInstance(
+  exp.browserPopupRedirectResolver
+);
+const CORDOVA_RESOLVER: exp.PopupRedirectResolverInternal = exp._getInstance(
+  exp.cordovaPopupRedirectResolver
+);
 
 /** Platform-agnostic popup-redirect resolver */
 export class CompatPopupRedirectResolver
@@ -40,11 +46,7 @@ export class CompatPopupRedirectResolver
     // We haven't yet determined whether or not we're in Cordova; go ahead
     // and determine that state now.
     const isCordova = await _isCordova();
-    this.underlyingResolver = exp._getInstance(
-      isCordova
-        ? exp.cordovaPopupRedirectResolver
-        : exp.browserPopupRedirectResolver
-    );
+    this.underlyingResolver = isCordova ? CORDOVA_RESOLVER : BROWSER_RESOLVER;
     return this.assertedUnderlyingResolver._initialize(auth);
   }
 
@@ -87,6 +89,10 @@ export class CompatPopupRedirectResolver
     return this.assertedUnderlyingResolver._originValidation(auth);
   }
 
+  get _shouldInitProactively(): boolean {
+    return _isLikelyCordova() || BROWSER_RESOLVER._shouldInitProactively;
+  }
+
   private get assertedUnderlyingResolver(): exp.PopupRedirectResolverInternal {
     _assert(this.underlyingResolver, exp.AuthErrorCode.INTERNAL_ERROR);
     return this.underlyingResolver;

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,18 @@ 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,11 @@ 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 +107,38 @@ 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,28 @@ describe('core/auth/initializeAuth', () => {
       expect(reload._reloadWithoutSaving).not.to.have.been.called;
     });
 
+    it('does not early-initialize the resolver if _shouldInitProactively is false', 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('early-initializes the resolver if _shouldInitProactively is true', 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;
 }