Skip to content
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

fix: Decoupled proactive token refresh from FirebaseApp #1194

Merged
merged 4 commits into from
Mar 18, 2021
Merged

fix: Decoupled proactive token refresh from FirebaseApp #1194

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5b7a89a
fix: Decoupled proactive token refresh from FirebaseApp
hiranya911 Mar 11, 2021
8674df3
fix: Defined constants for duration values
hiranya911 Mar 12, 2021
e7c3588
fix: Logging errors encountered while scheduling a refresh
hiranya911 Mar 12, 2021
705801a
fix: Renamed some variables for clarity
hiranya911 Mar 18, 2021
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
46 changes: 46 additions & 0 deletions src/database/database-internal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,13 @@ import { getSdkVersion } from '../utils/index';

import Database = database.Database;

const TOKEN_REFRESH_THRESHOLD_MILLIS = 5 * 60 * 1000;

export class DatabaseService {

private readonly appInternal: FirebaseApp;
private tokenRefresh: boolean;
hiranya911 marked this conversation as resolved.
Show resolved Hide resolved
private tokenRefreshTimeout: NodeJS.Timeout;

private databases: {
[dbUrl: string]: Database;
Expand All @@ -50,6 +54,12 @@ export class DatabaseService {
* @internal
*/
public delete(): Promise<void> {
if (this.tokenRefresh) {
this.appInternal.INTERNAL.removeAuthTokenListener(this.onTokenChange);
clearTimeout(this.tokenRefreshTimeout);
Copy link
Member

Choose a reason for hiding this comment

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

Does it matter the what order we disarm here? Could this cause a race condition?

Copy link
Contributor Author

@hiranya911 hiranya911 Mar 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I felt like we should unregister the listener before clearing out the timer. If we clear the timer first, theoretically it's possible for the listener to fire again and re-initialize the timer. Given both these APIs are synchronous, I don't think it's a real possibility (given how Node.js schedules code), but this seems safer.

this.tokenRefresh = false;
}

const promises = [];
for (const dbUrl of Object.keys(this.databases)) {
const db: DatabaseImpl = ((this.databases[dbUrl] as any) as DatabaseImpl);
Expand Down Expand Up @@ -96,9 +106,45 @@ export class DatabaseService {

this.databases[dbUrl] = db;
}

if (!this.tokenRefresh) {
this.tokenRefresh = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional suggestion:

You can make this a little simpler if you combine tokenRefresh and tokenRefreshTimeout. You could initialize tokenRefreshTimeout to undefined initially and then set the schedule right here, which seems more intuitive to me. With the current setup, I have to look at onTokenChange to figure out the scheduling.

I also wonder if we should use setInterval. It seems a little more appropriate, but might make things more complicated in some places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code is just intended to register the token change listener. There's no guarantee that the listener will fire immediately. So theoretically it is possible for tokenRefresh to be true, while tokenRefreshTimeout is not yet set. What we really need is a way to track whether we have registered the listener or not (so we can avoid duplicate registrations, and also unregister it at delete). So I'm renaming tokenRefresh to tokenListenerRegistered. That should clear up any confusions, and also address the above comment regarding naming.

As for setInterval(), given that we can receive tokens with arbitrary expiration times from auth (we've seen tokens with expiry times like 30min and 45min), an interval scheduler is not very useful. So we use the token listener to watch for token changes, and schedule refresh events according to the expiry time we get.

this.appInternal.INTERNAL.addAuthTokenListener(this.onTokenChange);
}

return db;
}

// eslint-disable-next-line @typescript-eslint/no-unused-vars
private onTokenChange(_: string): void {
Copy link
Member

Choose a reason for hiding this comment

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

IIRC there was no other token refresh listener. If the right action is to immediately ask for a new token from app.INTERNAL should we just remove the parameter?

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 think an even better approach would be to change the signature of the listener to accept a FirebaseAccessToken which contains both token and expiry time. That way we wouldn't have to call getToken() in the listener at all (we currently do it to get hold of the expiry time).

However, this API currently mirrors an API in the JS SDK:

https://github.com/firebase/firebase-js-sdk/blob/ec6ccc7d319732588ceacae0824d6c7551afb026/packages/app-types/private.d.ts#L76

The RTDB implementation expects our App and App.INTERNAL APIs to be compatible with the JS SDK. Therefore we'll need to check with the JS SDK team before making any changes to the listener signature.

this.appInternal.INTERNAL.getToken()
.then((token) => {
const delayMillis = token.expirationTime - TOKEN_REFRESH_THRESHOLD_MILLIS - Date.now();
// If the new token is set to expire soon (unlikely), do nothing. Somebody will eventually
// notice and refresh the token, at which point this callback will fire again.
if (delayMillis > 0) {
this.scheduleTokenRefresh(delayMillis);
}
})
// eslint-disable-next-line @typescript-eslint/no-unused-vars
.catch((_) => {
// Unlikely error, since a new token was just fetched before the callback fired.
hiranya911 marked this conversation as resolved.
Show resolved Hide resolved
// Just here to prevent the promise chain from crashing.
});
}

private scheduleTokenRefresh(delayMillis: number): void {
clearTimeout(this.tokenRefreshTimeout);
this.tokenRefreshTimeout = setTimeout(() => {
this.appInternal.INTERNAL.getToken(true)
hiranya911 marked this conversation as resolved.
Show resolved Hide resolved
.catch(() => {
// Ignore the error since this might just be an intermittent failure. If we really cannot
// refresh the token, an error will be logged once the existing token expires and we try
// to fetch a fresh one.
});
}, delayMillis);
}

private ensureUrl(url?: string): string {
if (typeof url !== 'undefined') {
return url;
Expand Down
210 changes: 65 additions & 145 deletions src/firebase-app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
*/

import { AppOptions, app } from './firebase-namespace-api';
import { credential, GoogleOAuthAccessToken } from './credential/index';
import { credential } from './credential/index';
import { getApplicationDefault } from './credential/credential-internal';
import * as validator from './utils/validator';
import { deepCopy } from './utils/deep-copy';
Expand All @@ -39,6 +39,8 @@ import { RemoteConfig } from './remote-config/remote-config';
import Credential = credential.Credential;
import Database = database.Database;

const TOKEN_REFRESH_THRESHOLD_MILLIS = 5 * 60 * 1000;

/**
* Type representing a callback which is called every time an app lifecycle event occurs.
*/
Expand All @@ -57,129 +59,80 @@ export interface FirebaseAccessToken {
* Internals of a FirebaseApp instance.
*/
export class FirebaseAppInternals {
private isDeleted_ = false;
private cachedToken_: FirebaseAccessToken;
private cachedTokenPromise_: Promise<FirebaseAccessToken> | null;
private tokenListeners_: Array<(token: string) => void>;
private tokenRefreshTimeout_: NodeJS.Timer;

constructor(private credential_: Credential) {
this.tokenListeners_ = [];
}

/**
* Gets an auth token for the associated app.
*
* @param {boolean} forceRefresh Whether or not to force a token refresh.
* @return {Promise<FirebaseAccessToken>} A Promise that will be fulfilled with the current or
* new token.
*/
public getToken(forceRefresh?: boolean): Promise<FirebaseAccessToken> {
const expired = this.cachedToken_ && this.cachedToken_.expirationTime < Date.now();
if (this.cachedTokenPromise_ && !forceRefresh && !expired) {
return this.cachedTokenPromise_
.catch((error) => {
// Update the cached token promise to avoid caching errors. Set it to resolve with the
// cached token if we have one (and return that promise since the token has still not
// expired).
if (this.cachedToken_) {
this.cachedTokenPromise_ = Promise.resolve(this.cachedToken_);
return this.cachedTokenPromise_;
}

// Otherwise, set the cached token promise to null so that it will force a refresh next
// time getToken() is called.
this.cachedTokenPromise_ = null;

// And re-throw the caught error.
throw error;
});
} else {
// Clear the outstanding token refresh timeout. This is a noop if the timeout is undefined.
clearTimeout(this.tokenRefreshTimeout_);

// this.credential_ may be an external class; resolving it in a promise helps us
// protect against exceptions and upgrades the result to a promise in all cases.
this.cachedTokenPromise_ = Promise.resolve(this.credential_.getAccessToken())
.then((result: GoogleOAuthAccessToken) => {
// Since the developer can provide the credential implementation, we want to weakly verify
// the return type until the type is properly exported.
if (!validator.isNonNullObject(result) ||
typeof result.expires_in !== 'number' ||
typeof result.access_token !== 'string') {
throw new FirebaseAppError(
AppErrorCodes.INVALID_CREDENTIAL,
`Invalid access token generated: "${JSON.stringify(result)}". Valid access ` +
'tokens must be an object with the "expires_in" (number) and "access_token" ' +
'(string) properties.',
);
}

const token: FirebaseAccessToken = {
accessToken: result.access_token,
expirationTime: Date.now() + (result.expires_in * 1000),
};

const hasAccessTokenChanged = (this.cachedToken_ && this.cachedToken_.accessToken !== token.accessToken);
const hasExpirationChanged = (this.cachedToken_ && this.cachedToken_.expirationTime !== token.expirationTime);
if (!this.cachedToken_ || hasAccessTokenChanged || hasExpirationChanged) {
this.cachedToken_ = token;
this.tokenListeners_.forEach((listener) => {
listener(token.accessToken);
});
}

// Establish a timeout to proactively refresh the token every minute starting at five
// minutes before it expires. Once a token refresh succeeds, no further retries are
// needed; if it fails, retry every minute until the token expires (resulting in a total
// of four retries: at 4, 3, 2, and 1 minutes).
let refreshTimeInSeconds = (result.expires_in - (5 * 60));
let numRetries = 4;

// In the rare cases the token is short-lived (that is, it expires in less than five
// minutes from when it was fetched), establish the timeout to refresh it after the
// current minute ends and update the number of retries that should be attempted before
// the token expires.
if (refreshTimeInSeconds <= 0) {
refreshTimeInSeconds = result.expires_in % 60;
numRetries = Math.floor(result.expires_in / 60) - 1;
}

// The token refresh timeout keeps the Node.js process alive, so only create it if this
// instance has not already been deleted.
if (numRetries && !this.isDeleted_) {
this.setTokenRefreshTimeout(refreshTimeInSeconds * 1000, numRetries);
}

return token;
})
.catch((error) => {
let errorMessage = (typeof error === 'string') ? error : error.message;

errorMessage = 'Credential implementation provided to initializeApp() via the ' +
'"credential" property failed to fetch a valid Google OAuth2 access token with the ' +
`following error: "${errorMessage}".`;

if (errorMessage.indexOf('invalid_grant') !== -1) {
errorMessage += ' There are two likely causes: (1) your server time is not properly ' +
'synced or (2) your certificate key file has been revoked. To solve (1), re-sync the ' +
'time on your server. To solve (2), make sure the key ID for your key file is still ' +
'present at https://console.firebase.google.com/iam-admin/serviceaccounts/project. If ' +
'not, generate a new key file at ' +
'https://console.firebase.google.com/project/_/settings/serviceaccounts/adminsdk.';
}

throw new FirebaseAppError(AppErrorCodes.INVALID_CREDENTIAL, errorMessage);
});

return this.cachedTokenPromise_;
public getToken(forceRefresh = false): Promise<FirebaseAccessToken> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the RTDB component just set forceRefresh to true if the expiration time has been hit? That way, we only need to deal with TOKEN_REFRESH_THRESHOLD_MILLIS in one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These 2 constants are used for slightly different purposes:

  1. In firebase-app it is used to determine how early we should treat a valid token as expired.
  2. In database-internal it is used to determine when to schedule the next refresh event.

These don't have to be the same value. For instance, we can have FirebaseApp invalidate a token 5 minutes before it expires, but have RTDB trigger the token refresh 10 minutes before the current token expires.

To make the above distinction clear, I'm renaming the constant in firebase-app to TOKEN_EXPIRY_THRESHOLD_MILLIS. This constant doesn't have anything to do with refreshing tokens. It's simply when we treat the token as expired.

if (forceRefresh || this.shouldRefresh()) {
return this.refreshToken();
}

return Promise.resolve(this.cachedToken_);
}

private refreshToken(): Promise<FirebaseAccessToken> {
return Promise.resolve(this.credential_.getAccessToken())
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused. isn't getAccesssToken() already returning a promise? Why do we need to resolve the promise instead of just chaining?

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 thought so too :)

But when I made the change, it actually caused a test failure here:

firebase-admin-node/test/unit/firebase-app.spec.ts

Lines 677 to 691 in 994fd43

it('throws a custom credential implementation which returns invalid access tokens', () => {
const credential = {
getAccessToken: () => 5,
};
const app = utils.createAppWithOptions({
credential: credential as any,
});
return app.INTERNAL.getToken().then(() => {
throw new Error('Unexpected success');
}, (err) => {
expect(err.toString()).to.include('Invalid access token generated');
});
});

This is because the credential in this case returns a value which cannot be chained. I'm not sure how much we care about this sort of edge cases, but for now I've opted to keep it as is.

.then((result) => {
// Since the developer can provide the credential implementation, we want to weakly verify
// the return type until the type is properly exported.
if (!validator.isNonNullObject(result) ||
typeof result.expires_in !== 'number' ||
typeof result.access_token !== 'string') {
throw new FirebaseAppError(
AppErrorCodes.INVALID_CREDENTIAL,
`Invalid access token generated: "${JSON.stringify(result)}". Valid access ` +
'tokens must be an object with the "expires_in" (number) and "access_token" ' +
'(string) properties.',
);
}

const token = {
accessToken: result.access_token,
expirationTime: Date.now() + (result.expires_in * 1000),
};
if (!this.cachedToken_
|| this.cachedToken_.accessToken !== token.accessToken
|| this.cachedToken_.expirationTime !== token.expirationTime) {
this.cachedToken_ = token;
this.tokenListeners_.forEach((listener) => {
listener(token.accessToken);
});
}

return token;
})
.catch((error) => {
let errorMessage = (typeof error === 'string') ? error : error.message;

errorMessage = 'Credential implementation provided to initializeApp() via the ' +
'"credential" property failed to fetch a valid Google OAuth2 access token with the ' +
`following error: "${errorMessage}".`;

if (errorMessage.indexOf('invalid_grant') !== -1) {
errorMessage += ' There are two likely causes: (1) your server time is not properly ' +
'synced or (2) your certificate key file has been revoked. To solve (1), re-sync the ' +
'time on your server. To solve (2), make sure the key ID for your key file is still ' +
'present at https://console.firebase.google.com/iam-admin/serviceaccounts/project. If ' +
'not, generate a new key file at ' +
'https://console.firebase.google.com/project/_/settings/serviceaccounts/adminsdk.';
}

throw new FirebaseAppError(AppErrorCodes.INVALID_CREDENTIAL, errorMessage);
});
}

private shouldRefresh(): boolean {
return !this.cachedToken_ || (this.cachedToken_.expirationTime - Date.now()) <= TOKEN_REFRESH_THRESHOLD_MILLIS;
}

/**
* Adds a listener that is called each time a token changes.
*
* @param {function(string)} listener The listener that will be called with each new token.
* @param listener The listener that will be called with each new token.
*/
public addAuthTokenListener(listener: (token: string) => void): void {
this.tokenListeners_.push(listener);
Expand All @@ -191,42 +144,11 @@ export class FirebaseAppInternals {
/**
* Removes a token listener.
*
* @param {function(string)} listener The listener to remove.
* @param listener The listener to remove.
*/
public removeAuthTokenListener(listener: (token: string) => void): void {
this.tokenListeners_ = this.tokenListeners_.filter((other) => other !== listener);
}

/**
* Deletes the FirebaseAppInternals instance.
*/
public delete(): void {
this.isDeleted_ = true;

// Clear the token refresh timeout so it doesn't keep the Node.js process alive.
clearTimeout(this.tokenRefreshTimeout_);
}

/**
* Establishes timeout to refresh the Google OAuth2 access token used by the SDK.
*
* @param {number} delayInMilliseconds The delay to use for the timeout.
* @param {number} numRetries The number of times to retry fetching a new token if the prior fetch
* failed.
*/
private setTokenRefreshTimeout(delayInMilliseconds: number, numRetries: number): void {
this.tokenRefreshTimeout_ = setTimeout(() => {
this.getToken(/* forceRefresh */ true)
.catch(() => {
// Ignore the error since this might just be an intermittent failure. If we really cannot
// refresh the token, an error will be logged once the existing token expires and we try
// to fetch a fresh one.
if (numRetries > 0) {
this.setTokenRefreshTimeout(60 * 1000, numRetries - 1);
}
});
}, delayInMilliseconds);
}
}

/**
Expand Down Expand Up @@ -419,8 +341,6 @@ export class FirebaseApp implements app.App {
this.checkDestroyed_();
this.firebaseInternals_.removeApp(this.name_);

this.INTERNAL.delete();

return Promise.all(Object.keys(this.services_).map((serviceName) => {
const service = this.services_[serviceName];
if (isStateful(service)) {
Expand Down
Loading