Skip to content

Commit 0dfef5d

Browse files
sam-gcsam-gc
authored
Make the resolver parameter for all popup/redirect methods optional (#3889)
* Make the popup redirect resolver optional * Formatting * PR feedback
1 parent b9087b9 commit 0dfef5d

File tree

5 files changed

+193
-19
lines changed

5 files changed

+193
-19
lines changed

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import { SDK_VERSION } from '@firebase/app-exp';
1919
import * as externs from '@firebase/auth-types-exp';
2020
import { isEmpty, querystring } from '@firebase/util';
21+
import { _getInstance } from '../core/util/instantiator';
2122

2223
import { AuthEventManager } from '../core/auth/auth_event_manager';
2324
import { AuthErrorCode } from '../core/errors';
@@ -64,6 +65,26 @@ interface ManagerOrPromise {
6465
promise?: Promise<EventManager>;
6566
}
6667

68+
/**
69+
* Chooses a popup/redirect resolver to use. This prefers the override (which
70+
* is directly passed in), and falls back to the property set on the auth
71+
* object. If neither are available, this function errors w/ an argument error.
72+
*/
73+
export function _withDefaultResolver(
74+
auth: Auth,
75+
resolverOverride: externs.PopupRedirectResolver | undefined
76+
): PopupRedirectResolver {
77+
if (resolverOverride) {
78+
return _getInstance(resolverOverride);
79+
}
80+
81+
assert(auth._popupRedirectResolver, AuthErrorCode.ARGUMENT_ERROR, {
82+
appName: auth.name
83+
});
84+
85+
return auth._popupRedirectResolver;
86+
}
87+
6788
class BrowserPopupRedirectResolver implements PopupRedirectResolver {
6889
private readonly eventManagers: Record<string, ManagerOrPromise> = {};
6990
private readonly iframes: Record<string, gapi.iframes.Iframe> = {};

‎packages-exp/auth-exp/src/platform_browser/strategies/popup.test.ts

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,34 @@ describe('src/core/strategies/popup', () => {
109109
expect(await promise).to.eq(cred);
110110
});
111111

112+
it('completes the full flow with default resolver', async () => {
113+
const cred = new UserCredentialImpl({
114+
user: testUser(auth, 'uid'),
115+
providerId: ProviderId.GOOGLE,
116+
operationType: OperationType.SIGN_IN
117+
});
118+
auth._popupRedirectResolver = _getInstance(resolver);
119+
idpStubs._signIn.returns(Promise.resolve(cred));
120+
const promise = signInWithPopup(auth, provider);
121+
iframeEvent({
122+
type: AuthEventType.SIGN_IN_VIA_POPUP
123+
});
124+
expect(await promise).to.eq(cred);
125+
});
126+
127+
it('errors if resolver not provided and not on auth', async () => {
128+
const cred = new UserCredentialImpl({
129+
user: testUser(auth, 'uid'),
130+
providerId: ProviderId.GOOGLE,
131+
operationType: OperationType.SIGN_IN
132+
});
133+
idpStubs._signIn.returns(Promise.resolve(cred));
134+
await expect(signInWithPopup(auth, provider)).to.be.rejectedWith(
135+
FirebaseError,
136+
'auth/argument-error'
137+
);
138+
});
139+
112140
it('ignores events for another event id', async () => {
113141
const cred = new UserCredentialImpl({
114142
user: testUser(auth, 'uid'),
@@ -265,6 +293,34 @@ describe('src/core/strategies/popup', () => {
265293
expect(await promise).to.eq(cred);
266294
});
267295

296+
it('completes the full flow with default resolver', async () => {
297+
const cred = new UserCredentialImpl({
298+
user,
299+
providerId: ProviderId.GOOGLE,
300+
operationType: OperationType.LINK
301+
});
302+
user.auth._popupRedirectResolver = _getInstance(resolver);
303+
idpStubs._link.returns(Promise.resolve(cred));
304+
const promise = linkWithPopup(user, provider);
305+
iframeEvent({
306+
type: AuthEventType.LINK_VIA_POPUP
307+
});
308+
expect(await promise).to.eq(cred);
309+
});
310+
311+
it('errors if resolver not provided and not on auth', async () => {
312+
const cred = new UserCredentialImpl({
313+
user,
314+
providerId: ProviderId.GOOGLE,
315+
operationType: OperationType.LINK
316+
});
317+
idpStubs._link.returns(Promise.resolve(cred));
318+
await expect(linkWithPopup(user, provider)).to.be.rejectedWith(
319+
FirebaseError,
320+
'auth/argument-error'
321+
);
322+
});
323+
268324
it('ignores events for another event id', async () => {
269325
const cred = new UserCredentialImpl({
270326
user,
@@ -422,6 +478,34 @@ describe('src/core/strategies/popup', () => {
422478
expect(await promise).to.eq(cred);
423479
});
424480

481+
it('completes the full flow with default resolver', async () => {
482+
const cred = new UserCredentialImpl({
483+
user,
484+
providerId: ProviderId.GOOGLE,
485+
operationType: OperationType.REAUTHENTICATE
486+
});
487+
user.auth._popupRedirectResolver = _getInstance(resolver);
488+
idpStubs._reauth.returns(Promise.resolve(cred));
489+
const promise = reauthenticateWithPopup(user, provider);
490+
iframeEvent({
491+
type: AuthEventType.REAUTH_VIA_POPUP
492+
});
493+
expect(await promise).to.eq(cred);
494+
});
495+
496+
it('errors if resolver not provided and not on auth', async () => {
497+
const cred = new UserCredentialImpl({
498+
user,
499+
providerId: ProviderId.GOOGLE,
500+
operationType: OperationType.REAUTHENTICATE
501+
});
502+
idpStubs._reauth.returns(Promise.resolve(cred));
503+
await expect(reauthenticateWithPopup(user, provider)).to.be.rejectedWith(
504+
FirebaseError,
505+
'auth/argument-error'
506+
);
507+
});
508+
425509
it('ignores events for another event id', async () => {
426510
const cred = new UserCredentialImpl({
427511
user,

‎packages-exp/auth-exp/src/platform_browser/strategies/popup.ts

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import {
3030
PopupRedirectResolver
3131
} from '../../model/popup_redirect';
3232
import { User } from '../../model/user';
33+
import { _withDefaultResolver } from '../popup_redirect';
3334
import { AuthPopup } from '../util/popup';
3435
import { AbstractPopupRedirectOperation } from './abstract_popup_redirect_operation';
3536

@@ -38,17 +39,18 @@ export const _AUTH_EVENT_TIMEOUT = 2020;
3839
export const _POLL_WINDOW_CLOSE_TIMEOUT = new Delay(2000, 10000);
3940

4041
export async function signInWithPopup(
41-
auth: externs.Auth,
42+
authExtern: externs.Auth,
4243
provider: externs.AuthProvider,
43-
resolverExtern: externs.PopupRedirectResolver
44+
resolverExtern?: externs.PopupRedirectResolver
4445
): Promise<externs.UserCredential> {
46+
const auth = _castAuth(authExtern);
4547
assert(provider instanceof OAuthProvider, AuthErrorCode.ARGUMENT_ERROR, {
4648
appName: auth.name
4749
});
48-
const resolver: PopupRedirectResolver = _getInstance(resolverExtern);
4950

51+
const resolver = _withDefaultResolver(auth, resolverExtern);
5052
const action = new PopupOperation(
51-
_castAuth(auth),
53+
auth,
5254
AuthEventType.SIGN_IN_VIA_POPUP,
5355
provider,
5456
resolver
@@ -59,15 +61,14 @@ export async function signInWithPopup(
5961
export async function reauthenticateWithPopup(
6062
userExtern: externs.User,
6163
provider: externs.AuthProvider,
62-
resolverExtern: externs.PopupRedirectResolver
64+
resolverExtern?: externs.PopupRedirectResolver
6365
): Promise<externs.UserCredential> {
6466
const user = userExtern as User;
6567
assert(provider instanceof OAuthProvider, AuthErrorCode.ARGUMENT_ERROR, {
6668
appName: user.auth.name
6769
});
6870

69-
const resolver: PopupRedirectResolver = _getInstance(resolverExtern);
70-
71+
const resolver = _withDefaultResolver(user.auth, resolverExtern);
7172
const action = new PopupOperation(
7273
user.auth,
7374
AuthEventType.REAUTH_VIA_POPUP,
@@ -81,14 +82,14 @@ export async function reauthenticateWithPopup(
8182
export async function linkWithPopup(
8283
userExtern: externs.User,
8384
provider: externs.AuthProvider,
84-
resolverExtern: externs.PopupRedirectResolver
85+
resolverExtern?: externs.PopupRedirectResolver
8586
): Promise<externs.UserCredential> {
8687
const user = userExtern as User;
8788
assert(provider instanceof OAuthProvider, AuthErrorCode.ARGUMENT_ERROR, {
8889
appName: user.auth.name
8990
});
9091

91-
const resolver: PopupRedirectResolver = _getInstance(resolverExtern);
92+
const resolver = _withDefaultResolver(user.auth, resolverExtern);
9293

9394
const action = new PopupOperation(
9495
user.auth,

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

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ import {
5353
reauthenticateWithRedirect,
5454
signInWithRedirect
5555
} from './redirect';
56+
import { FirebaseError } from '@firebase/util';
5657

5758
use(sinonChai);
5859
use(chaiAsPromised);
@@ -98,6 +99,27 @@ describe('src/core/strategies/redirect', () => {
9899
AuthEventType.SIGN_IN_VIA_REDIRECT
99100
);
100101
});
102+
103+
it('redirects the window with auth fallback resolver', async () => {
104+
const spy = sinon.spy(
105+
_getInstance<PopupRedirectResolver>(resolver),
106+
'_openRedirect'
107+
);
108+
await signInWithRedirect(auth, provider);
109+
expect(spy).to.have.been.calledWith(
110+
auth,
111+
provider,
112+
AuthEventType.SIGN_IN_VIA_REDIRECT
113+
);
114+
});
115+
116+
it('errors if no resolver available', async () => {
117+
auth._popupRedirectResolver = null;
118+
await expect(signInWithRedirect(auth, provider)).to.be.rejectedWith(
119+
FirebaseError,
120+
'auth/argument-error'
121+
);
122+
});
101123
});
102124

103125
context('linkWithRedirect', () => {
@@ -122,6 +144,27 @@ describe('src/core/strategies/redirect', () => {
122144
);
123145
});
124146

147+
it('redirects the window with auth fallback resolver', async () => {
148+
const spy = sinon.spy(
149+
_getInstance<PopupRedirectResolver>(resolver),
150+
'_openRedirect'
151+
);
152+
await linkWithRedirect(user, provider);
153+
expect(spy).to.have.been.calledWith(
154+
auth,
155+
provider,
156+
AuthEventType.LINK_VIA_REDIRECT
157+
);
158+
});
159+
160+
it('errors if no resolver available', async () => {
161+
auth._popupRedirectResolver = null;
162+
await expect(linkWithRedirect(user, provider)).to.be.rejectedWith(
163+
FirebaseError,
164+
'auth/argument-error'
165+
);
166+
});
167+
125168
it('persists the redirect user and current user', async () => {
126169
const redirectPersistence: Persistence = _getInstance(
127170
RedirectPersistence
@@ -180,6 +223,26 @@ describe('src/core/strategies/redirect', () => {
180223
);
181224
});
182225

226+
it('redirects the window with auth fallback resolver', async () => {
227+
const spy = sinon.spy(
228+
_getInstance<PopupRedirectResolver>(resolver),
229+
'_openRedirect'
230+
);
231+
await reauthenticateWithRedirect(user, provider);
232+
expect(spy).to.have.been.calledWith(
233+
auth,
234+
provider,
235+
AuthEventType.REAUTH_VIA_REDIRECT
236+
);
237+
});
238+
239+
it('errors if no resolver available', async () => {
240+
auth._popupRedirectResolver = null;
241+
await expect(
242+
reauthenticateWithRedirect(user, provider)
243+
).to.be.rejectedWith(FirebaseError, 'auth/argument-error');
244+
});
245+
183246
it('persists the redirect user and current user', async () => {
184247
const redirectPersistence: Persistence = _getInstance(
185248
RedirectPersistence

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

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -31,19 +31,20 @@ import {
3131
PopupRedirectResolver
3232
} from '../../model/popup_redirect';
3333
import { User, UserCredential } from '../../model/user';
34+
import { _withDefaultResolver } from '../popup_redirect';
3435
import { AbstractPopupRedirectOperation } from './abstract_popup_redirect_operation';
3536

3637
export async function signInWithRedirect(
37-
auth: externs.Auth,
38+
authExtern: externs.Auth,
3839
provider: externs.AuthProvider,
39-
resolverExtern: externs.PopupRedirectResolver
40+
resolverExtern?: externs.PopupRedirectResolver
4041
): Promise<never> {
42+
const auth = _castAuth(authExtern);
4143
assert(provider instanceof OAuthProvider, AuthErrorCode.ARGUMENT_ERROR, {
4244
appName: auth.name
4345
});
44-
const resolver: PopupRedirectResolver = _getInstance(resolverExtern);
4546

46-
return resolver._openRedirect(
47+
return _withDefaultResolver(auth, resolverExtern)._openRedirect(
4748
auth,
4849
provider,
4950
AuthEventType.SIGN_IN_VIA_REDIRECT
@@ -53,13 +54,15 @@ export async function signInWithRedirect(
5354
export async function reauthenticateWithRedirect(
5455
userExtern: externs.User,
5556
provider: externs.AuthProvider,
56-
resolverExtern: externs.PopupRedirectResolver
57+
resolverExtern?: externs.PopupRedirectResolver
5758
): Promise<never> {
5859
const user = userExtern as User;
5960
assert(provider instanceof OAuthProvider, AuthErrorCode.ARGUMENT_ERROR, {
6061
appName: user.auth.name
6162
});
62-
const resolver: PopupRedirectResolver = _getInstance(resolverExtern);
63+
64+
// Allow the resolver to error before persisting the redirect user
65+
const resolver = _withDefaultResolver(user.auth, resolverExtern);
6366

6467
const eventId = await prepareUserForRedirect(user.auth, user);
6568
return resolver._openRedirect(
@@ -73,13 +76,15 @@ export async function reauthenticateWithRedirect(
7376
export async function linkWithRedirect(
7477
userExtern: externs.User,
7578
provider: externs.AuthProvider,
76-
resolverExtern: externs.PopupRedirectResolver
79+
resolverExtern?: externs.PopupRedirectResolver
7780
): Promise<never> {
7881
const user = userExtern as User;
7982
assert(provider instanceof OAuthProvider, AuthErrorCode.ARGUMENT_ERROR, {
8083
appName: user.auth.name
8184
});
82-
const resolver: PopupRedirectResolver = _getInstance(resolverExtern);
85+
86+
// Allow the resolver to error before persisting the redirect user
87+
const resolver = _withDefaultResolver(user.auth, resolverExtern);
8388

8489
await _assertLinkedStatus(false, user, provider.providerId);
8590
const eventId = await prepareUserForRedirect(user.auth, user);
@@ -93,10 +98,10 @@ export async function linkWithRedirect(
9398

9499
export async function getRedirectResult(
95100
authExtern: externs.Auth,
96-
resolverExtern: externs.PopupRedirectResolver
101+
resolverExtern?: externs.PopupRedirectResolver
97102
): Promise<externs.UserCredential | null> {
98103
const auth = _castAuth(authExtern);
99-
const resolver: PopupRedirectResolver = _getInstance(resolverExtern);
104+
const resolver = _withDefaultResolver(auth, resolverExtern);
100105
const action = new RedirectAction(auth, resolver);
101106
const result = await action.execute();
102107

0 commit comments

Comments
 (0)