diff --git a/.changeset/tricky-ravens-stare.md b/.changeset/tricky-ravens-stare.md new file mode 100644 index 00000000000..b1836bbf0bd --- /dev/null +++ b/.changeset/tricky-ravens-stare.md @@ -0,0 +1,5 @@ +--- +'@firebase/auth': patch +--- + +Fix to minimize a potential race condition between auth init and signInWithRedirect diff --git a/packages/auth/demo/src/index.js b/packages/auth/demo/src/index.js index 86c2af49a65..ea114055d12 100644 --- a/packages/auth/demo/src/index.js +++ b/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(); diff --git a/packages/auth/src/platform_browser/strategies/redirect.test.ts b/packages/auth/src/platform_browser/strategies/redirect.test.ts index cf9ae652b6a..7bf726d6c59 100644 --- a/packages/auth/src/platform_browser/strategies/redirect.test.ts +++ b/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(resolver), + '_completeRedirectFn' + ); + const openRedirectSpy = sinon.spy( + _getInstance(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(resolver), + '_completeRedirectFn' + ); + const openRedirectSpy = sinon.spy( + _getInstance(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(resolver), + '_completeRedirectFn' + ); + const openRedirectSpy = sinon.spy( + _getInstance(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( diff --git a/packages/auth/src/platform_browser/strategies/redirect.ts b/packages/auth/src/platform_browser/strategies/redirect.ts index ea1897f1f44..30722d8cb32 100644 --- a/packages/auth/src/platform_browser/strategies/redirect.ts +++ b/packages/auth/src/platform_browser/strategies/redirect.ts @@ -91,6 +91,10 @@ export async function _signInWithRedirect( ): Promise { 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 { 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 { 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); diff --git a/packages/auth/test/helpers/mock_auth.ts b/packages/auth/test/helpers/mock_auth.ts index 60f204bb6fb..4923ee78e87 100644 --- a/packages/auth/test/helpers/mock_auth.ts +++ b/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 { 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;