Skip to content

Do not use recursive getToken() loop #3260

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Do not use recursive getToken() loop #3260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d1216e3
Do not use recursive getToken() loop
schmidt-sebastian Jun 23, 2020
6284a23
Merge branch 'master' into mrschmidt/gettokenloop
schmidt-sebastian Aug 11, 2020
71e52aa
Create eleven-rocks-hunt.md
schmidt-sebastian Aug 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/eleven-rocks-hunt.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@firebase/firestore": patch
---

Removed an authentication fallback that may have caused excessive usage of Firebase Auth's `getToken` API.
49 changes: 22 additions & 27 deletions packages/firestore/src/api/credentials.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,10 @@ export class FirebaseCredentialsProvider implements CredentialsProvider {
private receivedInitialUser: boolean = false;

/**
* Counter used to detect if the token changed while a getToken request was
* outstanding.
* The last token received either via the token listener or the getToken()
* API.
*/
private tokenCounter = 0;
private lastToken: OAuthToken | null = null;

/** The listener registered with setChangeListener(). */
private changeListener: CredentialChangeListener | null = null;
Expand All @@ -160,17 +160,15 @@ export class FirebaseCredentialsProvider implements CredentialsProvider {
private auth: FirebaseAuthInternal | null;

constructor(authProvider: Provider<FirebaseAuthInternalName>) {
this.tokenListener = () => {
this.tokenCounter++;
this.tokenListener = token => {
this.currentUser = this.getUser();
this.lastToken = token ? new OAuthToken(token, this.currentUser) : null;
this.receivedInitialUser = true;
if (this.changeListener) {
this.changeListener(this.currentUser);
}
};

this.tokenCounter = 0;

this.auth = authProvider.getImmediate({ optional: true });

if (this.auth) {
Expand Down Expand Up @@ -199,38 +197,35 @@ export class FirebaseCredentialsProvider implements CredentialsProvider {
'getToken cannot be called after listener removed.'
);

// Take note of the current value of the tokenCounter so that this method
// can fail (with an ABORTED error) if there is a token change while the
// request is outstanding.
const initialTokenCounter = this.tokenCounter;
const forceRefresh = this.forceRefresh;
this.forceRefresh = false;

if (!this.auth) {
return Promise.resolve(null);
}

this.lastToken = null;
return this.auth.getToken(forceRefresh).then(tokenData => {
// Cancel the request since the token changed while the request was
// outstanding so the response is potentially for a previous user (which
// user, we can't be sure).
if (this.tokenCounter !== initialTokenCounter) {
if (this.lastToken) {
// If we received a new token while this callback was pending (via
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: what is the reason to prefer lastToken to tokenData here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like you to verify this - but I think this matches the previous behavior. If we receive a token update via our callback listener, we ignore the token that is returned from the getToken call.

FWIW, while I think this is the cleaner solution to our token handling, I think the risk of it probably means we should not merge it. After all, we now have evidence that the problem we are trying to solve also exists in at least one other place (and maybe only that place).

Maybe @wilhuff wants a distraction from his current work and take a peak as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason this is complicated is that we need to subscribe to auth to get notified when the identity changes, so that we can trigger user changes in the local store.

The reason we ignored the token in that callback was because when remote store wants to restart streams it it's going to trigger a call to getToken. This restart might happen significantly later, because there may not have been active streams when the identity changed.

The central question we need to answer is whether or not the token listener will call us before the current access token expires. If it does, it seems like we could just always use the token supplied by the token listener, no? If we did that this could be much simpler.

// `this.tokenListener`), use the new token instead of the one received
// via `getToken()`.
logDebug(
'FirebaseCredentialsProvider',
'getToken aborted due to token change.'
'Re-using token from token listener.'
);
} else if (tokenData) {
hardAssert(
typeof tokenData.accessToken === 'string',
'Invalid tokenData returned from getToken():' + tokenData
);
this.lastToken = new OAuthToken(
tokenData.accessToken,
this.currentUser
);
return this.getToken();
} else {
if (tokenData) {
hardAssert(
typeof tokenData.accessToken === 'string',
'Invalid tokenData returned from getToken():' + tokenData
);
return new OAuthToken(tokenData.accessToken, this.currentUser);
} else {
return null;
}
}

return this.lastToken;
});
}

Expand Down