-
Notifications
You must be signed in to change notification settings - Fork 945
A whole bunch of things to bring auth (compat) to parity #3970
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
cae56bc
2dce277
31690d2
e04884f
f7f3ac1
3245f62
eaf5ccc
fc4cedd
034442a
0dcc1a7
7bb3905
28c23d1
3d91877
f93bffd
f06b4e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,7 +33,8 @@ describe('api/account_management/updateProfile', () => { | |
const request = { | ||
idToken: 'my-token', | ||
email: '[email protected]', | ||
password: 'my-password' | ||
password: 'my-password', | ||
returnSecureToken: true | ||
}; | ||
|
||
let auth: TestAuth; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,6 +60,7 @@ export class AuthImpl implements Auth, _FirebaseService { | |
private idTokenSubscription = new Subscription<externs.User>(this); | ||
private redirectUser: User | null = null; | ||
private isProactiveRefreshEnabled = false; | ||
private redirectInitializerError: Error | null = null; | ||
|
||
// Any network calls will set this to true and prevent subsequent emulator | ||
// initialization | ||
|
@@ -109,17 +110,21 @@ export class AuthImpl implements Auth, _FirebaseService { | |
return; | ||
} | ||
|
||
await this.initializeCurrentUser(); | ||
await this.initializeCurrentUser(popupRedirectResolver); | ||
|
||
if (this._deleted) { | ||
return; | ||
} | ||
|
||
this._isInitialized = true; | ||
this.notifyAuthListeners(); | ||
}); | ||
|
||
return this._initializationPromise; | ||
// After initialization completes, throw any error caused by redirect flow | ||
return this._initializationPromise.then(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why then instead of await? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we await it, the error becomes part of the control flow of initialization. If there's an error with redirect during initialization, we still want auth to finish initializing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't we just wrap in a try catch then? |
||
if (this.redirectInitializerError) { | ||
throw this.redirectInitializerError; | ||
} | ||
}); | ||
} | ||
|
||
/** | ||
|
@@ -149,18 +154,40 @@ export class AuthImpl implements Auth, _FirebaseService { | |
|
||
// Update current Auth state. Either a new login or logout. | ||
await this._updateCurrentUser(user); | ||
// Notify external Auth changes of Auth change event. | ||
this.notifyAuthListeners(); | ||
} | ||
|
||
private async initializeCurrentUser(): Promise<void> { | ||
const storedUser = (await this.assertedPersistence.getCurrentUser()) as User | null; | ||
private async initializeCurrentUser( | ||
popupRedirectResolver?: externs.PopupRedirectResolver | ||
): Promise<void> { | ||
// First check to see if we have a pending redirect event. | ||
let storedUser = (await this.assertedPersistence.getCurrentUser()) as User | null; | ||
if (popupRedirectResolver) { | ||
await this.getOrInitRedirectPersistenceManager(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we have this method return the redirectUser? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no; the purpose of this method is to determine which user should be set as |
||
const redirectUserEventId = this.redirectUser?._redirectEventId; | ||
const storedUserEventId = storedUser?._redirectEventId; | ||
const result = await this.tryRedirectSignIn(popupRedirectResolver); | ||
|
||
// If the stored user (i.e. the old "currentUser") has a redirectId that | ||
// matches the redirect user, then we want to initially sign in with the | ||
// new user object from result. | ||
// TODO(samgho): More thoroughly test all of this | ||
if ( | ||
(!redirectUserEventId || redirectUserEventId === storedUserEventId) && | ||
result?.user | ||
) { | ||
storedUser = result.user as User; | ||
} | ||
} | ||
|
||
// If no user in persistence, there is no current user. Set to null. | ||
if (!storedUser) { | ||
return this.directlySetCurrentUser(storedUser); | ||
return this.directlySetCurrentUser(null); | ||
} | ||
|
||
if (!storedUser._redirectEventId) { | ||
// This isn't a redirect user, we can reload and bail | ||
// This will also catch the redirected user, if available, as that method | ||
// strips the _redirectEventId | ||
return this.reloadAndSetCurrentUserOrClear(storedUser); | ||
} | ||
|
||
|
@@ -182,6 +209,42 @@ export class AuthImpl implements Auth, _FirebaseService { | |
return this.reloadAndSetCurrentUserOrClear(storedUser); | ||
} | ||
|
||
private async tryRedirectSignIn( | ||
sam-gc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
redirectResolver: externs.PopupRedirectResolver | ||
): Promise<externs.UserCredential | null> { | ||
// The redirect user needs to be checked (and signed in if available) | ||
// during auth initialization. All of the normal sign in and link/reauth | ||
// flows call back into auth and push things onto the promise queue. We | ||
// need to await the result of the redirect sign in *inside the promise | ||
// queue*. This presents a problem: we run into deadlock. See: | ||
// ┌> [Initialization] ─────┐ | ||
// ┌> [<other queue tasks>] │ | ||
// └─ [getRedirectResult] <─┘ | ||
// where [] are tasks on the queue and arrows denote awaits | ||
// Initialization will never complete because it's waiting on something | ||
// that's waiting for initialization to complete! | ||
// | ||
// Instead, this method calls getRedirectResult() (stored in | ||
// _completeRedirectFn) with an optional parameter that instructs all of | ||
// the underlying auth operations to skip anything that mutates auth state. | ||
|
||
let result: externs.UserCredential | null = null; | ||
try { | ||
// We know this._popupRedirectResolver is set since redirectResolver | ||
// is passed in. The _completeRedirectFn expects the unwrapped extern. | ||
result = await this._popupRedirectResolver!._completeRedirectFn( | ||
this, | ||
redirectResolver, | ||
true | ||
); | ||
} catch (e) { | ||
this.redirectInitializerError = e; | ||
await this._setRedirectUser(null); | ||
} | ||
|
||
return result; | ||
} | ||
|
||
private async reloadAndSetCurrentUserOrClear(user: User): Promise<void> { | ||
try { | ||
await _reloadWithoutSaving(user); | ||
|
@@ -243,6 +306,7 @@ export class AuthImpl implements Auth, _FirebaseService { | |
{ appName: this.name } | ||
); | ||
} | ||
|
||
return this.queue(async () => { | ||
await this.directlySetCurrentUser(user as User | null); | ||
this.notifyAuthListeners(); | ||
|
@@ -335,8 +399,11 @@ export class AuthImpl implements Auth, _FirebaseService { | |
} | ||
|
||
async _redirectUserForId(id: string): Promise<User | null> { | ||
// Make sure we've cleared any pending ppersistence actions | ||
await this.queue(async () => {}); | ||
// Make sure we've cleared any pending persistence actions if we're not in | ||
// the initializer | ||
if (this._isInitialized) { | ||
await this.queue(async () => {}); | ||
} | ||
|
||
if (this._currentUser?._redirectEventId === id) { | ||
return this._currentUser; | ||
|
Uh oh!
There was an error while loading. Please reload this page.