Skip to content

Commit 50b8191

Browse files
prameshjprameshj
authored
Await on the auth initialization promise in signIn/link/reauthenticateWithRedirect (#6914)
* Await on auth initialization before signing in with redirect. unit tests * Repro race condition with signInWithRedirect in demo app * changeset
1 parent cd92522 commit 50b8191

File tree

5 files changed

+145
-2
lines changed

5 files changed

+145
-2
lines changed

‎.changeset/tricky-ravens-stare.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@firebase/auth': patch
3+
---
4+
5+
Fix to minimize a potential race condition between auth init and signInWithRedirect

‎packages/auth/demo/src/index.js

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1812,6 +1812,27 @@ function initApp() {
18121812
},
18131813
onAuthError);
18141814

1815+
// Try sign in with redirect once upon page load, not on subsequent loads.
1816+
// This will demonstrate the behavior when signInWithRedirect is called before
1817+
// auth is fully initialized. This will fail on firebase/auth versions 0.21.0 and lower
1818+
// due to https://github.com/firebase/firebase-js-sdk/issues/6827
1819+
/*
1820+
if (sessionStorage.getItem('redirect-race-test') !== 'done') {
1821+
console.log('Starting redirect sign in upon page load.');
1822+
try {
1823+
sessionStorage.setItem('redirect-race-test', 'done');
1824+
signInWithRedirect(
1825+
auth,
1826+
new GoogleAuthProvider(),
1827+
browserPopupRedirectResolver
1828+
).catch(onAuthError);
1829+
} catch (error) {
1830+
console.log('Error while calling signInWithRedirect');
1831+
console.error(error);
1832+
}
1833+
}
1834+
*/
1835+
18151836
// Bootstrap tooltips.
18161837
$('[data-toggle="tooltip"]').tooltip();
18171838

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

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,37 @@ describe('platform_browser/strategies/redirect', () => {
122122
'auth/argument-error'
123123
);
124124
});
125+
126+
it('awaits on the auth initialization promise before opening redirect', async () => {
127+
// Obtain an auth instance which does not await on the initialization promise.
128+
const authWithoutAwait: TestAuth = await testAuth(
129+
resolver,
130+
undefined,
131+
true
132+
);
133+
// completeRedirectFn calls getRedirectResult under the hood.
134+
const getRedirectResultSpy = sinon.spy(
135+
_getInstance<PopupRedirectResolverInternal>(resolver),
136+
'_completeRedirectFn'
137+
);
138+
const openRedirectSpy = sinon.spy(
139+
_getInstance<PopupRedirectResolverInternal>(resolver),
140+
'_openRedirect'
141+
);
142+
await signInWithRedirect(authWithoutAwait, provider);
143+
expect(getRedirectResultSpy).to.have.been.called;
144+
expect(getRedirectResultSpy).to.have.been.calledBefore(openRedirectSpy);
145+
expect(getRedirectResultSpy).to.have.been.calledWith(
146+
authWithoutAwait,
147+
resolver,
148+
true
149+
);
150+
expect(openRedirectSpy).to.have.been.calledWith(
151+
authWithoutAwait,
152+
provider,
153+
AuthEventType.SIGN_IN_VIA_REDIRECT
154+
);
155+
});
125156
});
126157

127158
context('linkWithRedirect', () => {
@@ -159,6 +190,39 @@ describe('platform_browser/strategies/redirect', () => {
159190
);
160191
});
161192

193+
it('awaits on the auth initialization promise before opening redirect', async () => {
194+
// Obtain an auth instance which does not await on the initialization promise.
195+
const authWithoutAwait: TestAuth = await testAuth(
196+
resolver,
197+
undefined,
198+
true
199+
);
200+
user = testUser(authWithoutAwait, 'uid', 'email', true);
201+
// completeRedirectFn calls getRedirectResult under the hood.
202+
const getRedirectResultSpy = sinon.spy(
203+
_getInstance<PopupRedirectResolverInternal>(resolver),
204+
'_completeRedirectFn'
205+
);
206+
const openRedirectSpy = sinon.spy(
207+
_getInstance<PopupRedirectResolverInternal>(resolver),
208+
'_openRedirect'
209+
);
210+
await authWithoutAwait._updateCurrentUser(user);
211+
await linkWithRedirect(user, provider, resolver);
212+
expect(getRedirectResultSpy).to.have.been.called;
213+
expect(getRedirectResultSpy).to.have.been.calledBefore(openRedirectSpy);
214+
expect(getRedirectResultSpy).to.have.been.calledWith(
215+
authWithoutAwait,
216+
resolver,
217+
true
218+
);
219+
expect(openRedirectSpy).to.have.been.calledWith(
220+
authWithoutAwait,
221+
provider,
222+
AuthEventType.LINK_VIA_REDIRECT
223+
);
224+
});
225+
162226
it('errors if no resolver available', async () => {
163227
auth._popupRedirectResolver = null;
164228
await expect(linkWithRedirect(user, provider)).to.be.rejectedWith(
@@ -236,6 +300,40 @@ describe('platform_browser/strategies/redirect', () => {
236300
);
237301
});
238302

303+
it('awaits on the auth initialization promise before opening redirect', async () => {
304+
// Obtain an auth instance which does not await on the initialization promise.
305+
const authWithoutAwait: TestAuth = await testAuth(
306+
resolver,
307+
undefined,
308+
true
309+
);
310+
user = testUser(authWithoutAwait, 'uid', 'email', true);
311+
// completeRedirectFn calls getRedirectResult under the hood.
312+
const getRedirectResultSpy = sinon.spy(
313+
_getInstance<PopupRedirectResolverInternal>(resolver),
314+
'_completeRedirectFn'
315+
);
316+
const openRedirectSpy = sinon.spy(
317+
_getInstance<PopupRedirectResolverInternal>(resolver),
318+
'_openRedirect'
319+
);
320+
await authWithoutAwait._updateCurrentUser(user);
321+
await signInWithRedirect(authWithoutAwait, provider);
322+
await reauthenticateWithRedirect(user, provider);
323+
expect(getRedirectResultSpy).to.have.been.called;
324+
expect(getRedirectResultSpy).to.have.been.calledBefore(openRedirectSpy);
325+
expect(getRedirectResultSpy).to.have.been.calledWith(
326+
authWithoutAwait,
327+
resolver,
328+
true
329+
);
330+
expect(openRedirectSpy).to.have.been.calledWith(
331+
authWithoutAwait,
332+
provider,
333+
AuthEventType.REAUTH_VIA_REDIRECT
334+
);
335+
});
336+
239337
it('errors if no resolver available', async () => {
240338
auth._popupRedirectResolver = null;
241339
await expect(

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,10 @@ export async function _signInWithRedirect(
9292
): Promise<void | never> {
9393
const authInternal = _castAuth(auth);
9494
_assertInstanceOf(auth, provider, FederatedAuthProvider);
95+
// Wait for auth initialization to complete, this will process pending redirects and clear the
96+
// PENDING_REDIRECT_KEY in persistence. This should be completed before starting a new
97+
// redirect and creating a PENDING_REDIRECT_KEY entry.
98+
await authInternal._initializationPromise;
9599
const resolverInternal = _withDefaultResolver(authInternal, resolver);
96100
await _setPendingRedirectStatus(resolverInternal, authInternal);
97101

@@ -151,6 +155,10 @@ export async function _reauthenticateWithRedirect(
151155
): Promise<void | never> {
152156
const userInternal = getModularInstance(user) as UserInternal;
153157
_assertInstanceOf(userInternal.auth, provider, FederatedAuthProvider);
158+
// Wait for auth initialization to complete, this will process pending redirects and clear the
159+
// PENDING_REDIRECT_KEY in persistence. This should be completed before starting a new
160+
// redirect and creating a PENDING_REDIRECT_KEY entry.
161+
await userInternal.auth._initializationPromise;
154162
// Allow the resolver to error before persisting the redirect user
155163
const resolverInternal = _withDefaultResolver(userInternal.auth, resolver);
156164
await _setPendingRedirectStatus(resolverInternal, userInternal.auth);
@@ -206,6 +214,10 @@ export async function _linkWithRedirect(
206214
): Promise<void | never> {
207215
const userInternal = getModularInstance(user) as UserInternal;
208216
_assertInstanceOf(userInternal.auth, provider, FederatedAuthProvider);
217+
// Wait for auth initialization to complete, this will process pending redirects and clear the
218+
// PENDING_REDIRECT_KEY in persistence. This should be completed before starting a new
219+
// redirect and creating a PENDING_REDIRECT_KEY entry.
220+
await userInternal.auth._initializationPromise;
209221
// Allow the resolver to error before persisting the redirect user
210222
const resolverInternal = _withDefaultResolver(userInternal.auth, resolver);
211223
await _assertLinkedStatus(false, userInternal, provider.providerId);

‎packages/auth/test/helpers/mock_auth.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,8 @@ export class MockPersistenceLayer extends InMemoryPersistence {
7171

7272
export async function testAuth(
7373
popupRedirectResolver?: PopupRedirectResolver,
74-
persistence = new MockPersistenceLayer()
74+
persistence = new MockPersistenceLayer(),
75+
skipAwaitOnInit?: boolean
7576
): Promise<TestAuth> {
7677
const auth: TestAuth = new AuthImpl(
7778
FAKE_APP,
@@ -88,7 +89,13 @@ export async function testAuth(
8889
) as TestAuth;
8990
auth._updateErrorMap(debugErrorMap);
9091

91-
await auth._initializeWithPersistence([persistence], popupRedirectResolver);
92+
if (skipAwaitOnInit) {
93+
// This is used to verify scenarios where auth flows (like signInWithRedirect) are invoked before auth is fully initialized.
94+
// eslint-disable-next-line @typescript-eslint/no-floating-promises
95+
auth._initializeWithPersistence([persistence], popupRedirectResolver);
96+
} else {
97+
await auth._initializeWithPersistence([persistence], popupRedirectResolver);
98+
}
9299
auth.persistenceLayer = persistence;
93100
auth.settings.appVerificationDisabledForTesting = true;
94101
return auth;

0 commit comments

Comments
 (0)