-
Notifications
You must be signed in to change notification settings - Fork 927
Defer check whether auth is available #4845
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 4 commits
99b79f6
1ff8800
c15ae9f
41b5dc5
51718f1
55590e6
3624380
0faa3d9
958bcdf
bc06883
be1fa3c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,8 +23,10 @@ import { Provider } from '@firebase/component'; | |
|
||
import { User } from '../auth/user'; | ||
import { hardAssert, debugAssert } from '../util/assert'; | ||
import { AsyncQueue } from '../util/async_queue'; | ||
import { Code, FirestoreError } from '../util/error'; | ||
import { logDebug } from '../util/log'; | ||
import { Deferred } from '../util/promise'; | ||
|
||
// TODO(mikelehen): This should be split into multiple files and probably | ||
// moved to an auth/ folder to match other platforms. | ||
|
@@ -78,7 +80,7 @@ export class OAuthToken implements Token { | |
* token and may need to invalidate other state if the current user has also | ||
* changed. | ||
*/ | ||
export type CredentialChangeListener = (user: User) => void; | ||
export type CredentialChangeListener = (user: User) => Promise<void>; | ||
|
||
/** | ||
* Provides methods for getting the uid and token for the current user and | ||
|
@@ -98,8 +100,13 @@ export interface CredentialsProvider { | |
* Specifies a listener to be notified of credential changes | ||
* (sign-in / sign-out, token changes). It is immediately called once with the | ||
* initial user. | ||
* | ||
* The change listener is invoked on the provided AsyncQueue. | ||
*/ | ||
setChangeListener(changeListener: CredentialChangeListener): void; | ||
setChangeListener( | ||
asyncQueue: AsyncQueue, | ||
changeListener: CredentialChangeListener | ||
): void; | ||
|
||
/** Removes the previously-set change listener. */ | ||
removeChangeListener(): void; | ||
|
@@ -120,14 +127,17 @@ export class EmptyCredentialsProvider implements CredentialsProvider { | |
|
||
invalidateToken(): void {} | ||
|
||
setChangeListener(changeListener: CredentialChangeListener): void { | ||
setChangeListener( | ||
asyncQueue: AsyncQueue, | ||
changeListener: CredentialChangeListener | ||
): void { | ||
debugAssert( | ||
!this.changeListener, | ||
'Can only call setChangeListener() once.' | ||
); | ||
this.changeListener = changeListener; | ||
// Fire with initial user. | ||
changeListener(User.UNAUTHENTICATED); | ||
asyncQueue.enqueueRetryable(() => changeListener(User.FIRST_PARTY)); | ||
} | ||
|
||
removeChangeListener(): void { | ||
|
@@ -155,14 +165,17 @@ export class EmulatorCredentialsProvider implements CredentialsProvider { | |
|
||
invalidateToken(): void {} | ||
|
||
setChangeListener(changeListener: CredentialChangeListener): void { | ||
setChangeListener( | ||
asyncQueue: AsyncQueue, | ||
changeListener: CredentialChangeListener | ||
): void { | ||
debugAssert( | ||
!this.changeListener, | ||
'Can only call setChangeListener() once.' | ||
); | ||
this.changeListener = changeListener; | ||
// Fire with initial user. | ||
changeListener(this.token.user); | ||
asyncQueue.enqueueRetryable(() => changeListener(this.token.user)); | ||
} | ||
|
||
removeChangeListener(): void { | ||
|
@@ -175,11 +188,20 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { | |
* The auth token listener registered with FirebaseApp, retained here so we | ||
* can unregister it. | ||
*/ | ||
private tokenListener: ((token: string | null) => void) | null = null; | ||
private tokenListener: () => void; | ||
|
||
/** Tracks the current User. */ | ||
private currentUser: User = User.UNAUTHENTICATED; | ||
private receivedInitialUser: boolean = false; | ||
|
||
/** | ||
* Promise that allows blocking on the next `tokenChange` event. The Promise | ||
* is reassigned in `awaitTokenAndRaiseInitialEvent()` to allow blocking on | ||
* an a lazily loaded Auth instance. In this case, `this.receivedUser` | ||
* resolves once when the SDK first detects that there is no synchronous | ||
* Auth initialization, and then gets re-created and resolves again once Auth | ||
* is initialized. | ||
*/ | ||
private receivedUser = new Deferred(); | ||
|
||
/** | ||
* Counter used to detect if the token changed while a getToken request was | ||
|
@@ -188,44 +210,57 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { | |
private tokenCounter = 0; | ||
|
||
/** The listener registered with setChangeListener(). */ | ||
private changeListener: CredentialChangeListener | null = null; | ||
private changeListener: CredentialChangeListener = () => Promise.resolve(); | ||
|
||
private invokeChangeListener = false; | ||
|
||
private forceRefresh = false; | ||
|
||
private auth: FirebaseAuthInternal | null; | ||
private auth: FirebaseAuthInternal | null = null; | ||
|
||
private asyncQueue: AsyncQueue | null = null; | ||
|
||
constructor(authProvider: Provider<FirebaseAuthInternalName>) { | ||
this.tokenListener = () => { | ||
this.tokenCounter++; | ||
this.receivedUser.resolve(); | ||
this.currentUser = this.getUser(); | ||
this.receivedInitialUser = true; | ||
if (this.changeListener) { | ||
this.changeListener(this.currentUser); | ||
if (this.invokeChangeListener) { | ||
this.asyncQueue!.enqueueRetryable(() => | ||
this.changeListener(this.currentUser) | ||
); | ||
} | ||
}; | ||
|
||
this.tokenCounter = 0; | ||
|
||
this.auth = authProvider.getImmediate({ optional: true }); | ||
const registerAuth = (auth: FirebaseAuthInternal): void => { | ||
logDebug('FirebaseCredentialsProvider', 'Auth detected'); | ||
this.auth = auth; | ||
this.awaitTokenAndRaiseInitialEvent(); | ||
this.auth.addAuthTokenListener(this.tokenListener); | ||
}; | ||
|
||
if (this.auth) { | ||
this.auth.addAuthTokenListener(this.tokenListener!); | ||
} else { | ||
// if auth is not available, invoke tokenListener once with null token | ||
this.tokenListener(null); | ||
authProvider.get().then( | ||
auth => { | ||
this.auth = auth; | ||
if (this.tokenListener) { | ||
// tokenListener can be removed by removeChangeListener() | ||
this.auth.addAuthTokenListener(this.tokenListener); | ||
} | ||
}, | ||
() => { | ||
/* this.authProvider.get() never rejects */ | ||
authProvider.onInit(auth => registerAuth(auth)); | ||
|
||
// Our users can initialize Auth right after Firestore, so we give it | ||
// a chance to register itself with the component framework before we | ||
// determine whether to start up in unauthenticated mode. | ||
setTimeout(() => { | ||
if (!this.auth) { | ||
const auth = authProvider.getImmediate({ optional: true }); | ||
if (auth) { | ||
registerAuth(auth); | ||
} else if (this.tokenListener) { | ||
// If auth is still not available, invoke tokenListener once with null | ||
// token | ||
logDebug('FirebaseCredentialsProvider', 'Auth not yet detected'); | ||
this.tokenListener(); | ||
} | ||
); | ||
} | ||
} | ||
}, 0); | ||
|
||
this.awaitTokenAndRaiseInitialEvent(); | ||
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. Is My understanding is it is not, so
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. This seems to work in code though. Firestore will not receive a null user when auth is not initialized, but nothing seems to break. 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. I changed the logic for |
||
} | ||
|
||
getToken(): Promise<Token | null> { | ||
|
@@ -273,25 +308,20 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { | |
this.forceRefresh = true; | ||
} | ||
|
||
setChangeListener(changeListener: CredentialChangeListener): void { | ||
debugAssert( | ||
!this.changeListener, | ||
'Can only call setChangeListener() once.' | ||
); | ||
setChangeListener( | ||
asyncQueue: AsyncQueue, | ||
changeListener: CredentialChangeListener | ||
): void { | ||
debugAssert(!this.asyncQueue, 'Can only call setChangeListener() once.'); | ||
this.asyncQueue = asyncQueue; | ||
this.changeListener = changeListener; | ||
|
||
// Fire the initial event | ||
if (this.receivedInitialUser) { | ||
changeListener(this.currentUser); | ||
} | ||
} | ||
|
||
removeChangeListener(): void { | ||
if (this.auth) { | ||
this.auth.removeAuthTokenListener(this.tokenListener!); | ||
} | ||
this.tokenListener = null; | ||
this.changeListener = null; | ||
this.changeListener = () => Promise.resolve(); | ||
} | ||
|
||
// Auth.getUid() can return null even with a user logged in. It is because | ||
|
@@ -306,6 +336,33 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { | |
); | ||
return new User(currentUid); | ||
} | ||
|
||
/** | ||
* Blocks the AsyncQueue until the next user is available. This is invoked | ||
* on SDK start to wait for the first user token (or `null` if Auth is not yet | ||
* loaded). If Auth is loaded after Firestore, | ||
* `awaitTokenAndRaiseInitialEvent()` is also used to block Firestore until | ||
* Auth is fully initialized. | ||
* | ||
* This function also invokes the change listener synchronously once a token | ||
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. Do you mean asynchronously, since it happens in the asyncQueue? 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. I meant "synchronously with the code that resolves the token". I have reworded it. |
||
* is available. | ||
*/ | ||
private awaitTokenAndRaiseInitialEvent(): void { | ||
this.invokeChangeListener = false; // Prevent double-firing of the listener | ||
if (this.asyncQueue) { | ||
// Create a new deferred Promise that gets resolved when we receive the | ||
// next token. Ensure that all previous Promises also get resolved. | ||
const awaitToken = new Deferred<void>(); | ||
void awaitToken.promise.then(() => awaitToken.resolve()); | ||
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.
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. This was meant to resolve the existing this.receivedUser Promise if another one was created later on. I changed how the listener gets called if auth is not used, and am no longer using the Promise twice. This allowed me to remove this logic. |
||
this.receivedUser = awaitToken; | ||
|
||
this.asyncQueue.enqueueRetryable(async () => { | ||
await awaitToken.promise; | ||
await this.changeListener(this.currentUser); | ||
this.invokeChangeListener = true; | ||
}); | ||
} | ||
} | ||
} | ||
|
||
// Manual type definition for the subset of Gapi we use. | ||
|
@@ -368,9 +425,12 @@ export class FirstPartyCredentialsProvider implements CredentialsProvider { | |
); | ||
} | ||
|
||
setChangeListener(changeListener: CredentialChangeListener): void { | ||
setChangeListener( | ||
asyncQueue: AsyncQueue, | ||
changeListener: CredentialChangeListener | ||
): void { | ||
// Fire with initial uid. | ||
changeListener(User.FIRST_PARTY); | ||
asyncQueue.enqueueRetryable(() => changeListener(User.FIRST_PARTY)); | ||
} | ||
|
||
removeChangeListener(): void {} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment needs update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed