Await on the auth initialization promise in signIn/link/reauthenticateWithRedirect

.changeset/tricky-ravens-stare.md

@@ -0,0 +1,5 @@
+---
+'@firebase/auth': patch
+---
+
+Fix to minimize a potential race condition between auth init and signInWithRedirect

packages/auth/demo/src/index.js

@@ -1812,6 +1812,27 @@ function initApp() {
   },
   onAuthError);
 
+  // Try sign in with redirect once upon page load, not on subsequent loads.
+  // This will demonstrate the behavior when signInWithRedirect is called before
+  // auth is fully initialized. This will fail on firebase/auth versions 0.21.0 and lower
+  // due to https://github.com/firebase/firebase-js-sdk/issues/6827
+  /*
+  if (sessionStorage.getItem('redirect-race-test') !== 'done') {
+    console.log('Starting redirect sign in upon page load.');
+    try {
+      sessionStorage.setItem('redirect-race-test', 'done');
+      signInWithRedirect(
+        auth,
+        new GoogleAuthProvider(),
+        browserPopupRedirectResolver
+      ).catch(onAuthError);
+    } catch (error) {
+      console.log('Error while calling signInWithRedirect');
+      console.error(error);
+    }
+  }
+  */
+
   // Bootstrap tooltips.
   $('[data-toggle="tooltip"]').tooltip();
 

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

@@ -122,6 +122,37 @@ describe('platform_browser/strategies/redirect', () => {
         'auth/argument-error'
       );
     });
+
+    it('awaits on the auth initialization promise before opening redirect', async () => {
+      // Obtain an auth instance which does not await on the initialization promise.
+      const authWithoutAwait: TestAuth = await testAuth(
+        resolver,
+        undefined,
+        true
+      );
+      // completeRedirectFn calls getRedirectResult under the hood.
+      const getRedirectResultSpy = sinon.spy(
+        _getInstance<PopupRedirectResolverInternal>(resolver),
+        '_completeRedirectFn'
+      );
+      const openRedirectSpy = sinon.spy(
+        _getInstance<PopupRedirectResolverInternal>(resolver),
+        '_openRedirect'
+      );
+      await signInWithRedirect(authWithoutAwait, provider);
+      expect(getRedirectResultSpy).to.have.been.called;
+      expect(getRedirectResultSpy).to.have.been.calledBefore(openRedirectSpy);
+      expect(getRedirectResultSpy).to.have.been.calledWith(
+        authWithoutAwait,
+        resolver,
+        true
+      );
+      expect(openRedirectSpy).to.have.been.calledWith(
+        authWithoutAwait,
+        provider,
+        AuthEventType.SIGN_IN_VIA_REDIRECT
+      );
+    });
   });
 
   context('linkWithRedirect', () => {
@@ -159,6 +190,39 @@ describe('platform_browser/strategies/redirect', () => {
       );
     });
 
+    it('awaits on the auth initialization promise before opening redirect', async () => {
+      // Obtain an auth instance which does not await on the initialization promise.
+      const authWithoutAwait: TestAuth = await testAuth(
+        resolver,
+        undefined,
+        true
+      );
+      user = testUser(authWithoutAwait, 'uid', 'email', true);
+      // completeRedirectFn calls getRedirectResult under the hood.
+      const getRedirectResultSpy = sinon.spy(
+        _getInstance<PopupRedirectResolverInternal>(resolver),
+        '_completeRedirectFn'
+      );
+      const openRedirectSpy = sinon.spy(
+        _getInstance<PopupRedirectResolverInternal>(resolver),
+        '_openRedirect'
+      );
+      await authWithoutAwait._updateCurrentUser(user);
+      await linkWithRedirect(user, provider, resolver);
+      expect(getRedirectResultSpy).to.have.been.called;
+      expect(getRedirectResultSpy).to.have.been.calledBefore(openRedirectSpy);
+      expect(getRedirectResultSpy).to.have.been.calledWith(
+        authWithoutAwait,
+        resolver,
+        true
+      );
+      expect(openRedirectSpy).to.have.been.calledWith(
+        authWithoutAwait,
+        provider,
+        AuthEventType.LINK_VIA_REDIRECT
+      );
+    });
+
     it('errors if no resolver available', async () => {
       auth._popupRedirectResolver = null;
       await expect(linkWithRedirect(user, provider)).to.be.rejectedWith(
@@ -236,6 +300,40 @@ describe('platform_browser/strategies/redirect', () => {
       );
     });
 
+    it('awaits on the auth initialization promise before opening redirect', async () => {
+      // Obtain an auth instance which does not await on the initialization promise.
+      const authWithoutAwait: TestAuth = await testAuth(
+        resolver,
+        undefined,
+        true
+      );
+      user = testUser(authWithoutAwait, 'uid', 'email', true);
+      // completeRedirectFn calls getRedirectResult under the hood.
+      const getRedirectResultSpy = sinon.spy(
+        _getInstance<PopupRedirectResolverInternal>(resolver),
+        '_completeRedirectFn'
+      );
+      const openRedirectSpy = sinon.spy(
+        _getInstance<PopupRedirectResolverInternal>(resolver),
+        '_openRedirect'
+      );
+      await authWithoutAwait._updateCurrentUser(user);
+      await signInWithRedirect(authWithoutAwait, provider);
+      await reauthenticateWithRedirect(user, provider);
+      expect(getRedirectResultSpy).to.have.been.called;
+      expect(getRedirectResultSpy).to.have.been.calledBefore(openRedirectSpy);
+      expect(getRedirectResultSpy).to.have.been.calledWith(
+        authWithoutAwait,
+        resolver,
+        true
+      );
+      expect(openRedirectSpy).to.have.been.calledWith(
+        authWithoutAwait,
+        provider,
+        AuthEventType.REAUTH_VIA_REDIRECT
+      );
+    });
+
     it('errors if no resolver available', async () => {
       auth._popupRedirectResolver = null;
       await expect(

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

@@ -91,6 +91,10 @@ export async function _signInWithRedirect(
 ): Promise<void | never> {
   const authInternal = _castAuth(auth);
   _assertInstanceOf(auth, provider, FederatedAuthProvider);
+  // Wait for auth initialization to complete, this will process pending redirects and clear the
+  // PENDING_REDIRECT_KEY in persistence. This should be completed before starting a new
+  // redirect and creating a PENDING_REDIRECT_KEY entry.
+  await authInternal._initializationPromise;
   const resolverInternal = _withDefaultResolver(authInternal, resolver);
   await _setPendingRedirectStatus(resolverInternal, authInternal);
 
@@ -147,6 +151,10 @@ export async function _reauthenticateWithRedirect(
 ): Promise<void | never> {
   const userInternal = getModularInstance(user) as UserInternal;
   _assertInstanceOf(userInternal.auth, provider, FederatedAuthProvider);
+  // Wait for auth initialization to complete, this will process pending redirects and clear the
+  // PENDING_REDIRECT_KEY in persistence. This should be completed before starting a new
+  // redirect and creating a PENDING_REDIRECT_KEY entry.
+  await userInternal.auth._initializationPromise;
   // Allow the resolver to error before persisting the redirect user
   const resolverInternal = _withDefaultResolver(userInternal.auth, resolver);
   await _setPendingRedirectStatus(resolverInternal, userInternal.auth);
@@ -199,6 +207,10 @@ export async function _linkWithRedirect(
 ): Promise<void | never> {
   const userInternal = getModularInstance(user) as UserInternal;
   _assertInstanceOf(userInternal.auth, provider, FederatedAuthProvider);
+  // Wait for auth initialization to complete, this will process pending redirects and clear the
+  // PENDING_REDIRECT_KEY in persistence. This should be completed before starting a new
+  // redirect and creating a PENDING_REDIRECT_KEY entry.
+  await userInternal.auth._initializationPromise;
   // Allow the resolver to error before persisting the redirect user
   const resolverInternal = _withDefaultResolver(userInternal.auth, resolver);
   await _assertLinkedStatus(false, userInternal, provider.providerId);

packages/auth/test/helpers/mock_auth.ts

@@ -71,7 +71,8 @@ export class MockPersistenceLayer extends InMemoryPersistence {
 
 export async function testAuth(
   popupRedirectResolver?: PopupRedirectResolver,
-  persistence = new MockPersistenceLayer()
+  persistence = new MockPersistenceLayer(),
+  skipAwaitOnInit?: boolean
 ): Promise<TestAuth> {
   const auth: TestAuth = new AuthImpl(
     FAKE_APP,
@@ -88,7 +89,13 @@ export async function testAuth(
   ) as TestAuth;
   auth._updateErrorMap(debugErrorMap);
 
-  await auth._initializeWithPersistence([persistence], popupRedirectResolver);
+  if (skipAwaitOnInit) {
+    // This is used to verify scenarios where auth flows (like signInWithRedirect) are invoked before auth is fully initialized.
+    // eslint-disable-next-line @typescript-eslint/no-floating-promises
+    auth._initializeWithPersistence([persistence], popupRedirectResolver);
+  } else {
+    await auth._initializeWithPersistence([persistence], popupRedirectResolver);
+  }
   auth.persistenceLayer = persistence;
   auth.settings.appVerificationDisabledForTesting = true;
   return auth;