From 5b7a89aa54a18b03668a26fb731cfe1b72d353c5 Mon Sep 17 00:00:00 2001 From: Hiranya Jayathilaka Date: Thu, 11 Mar 2021 13:25:32 -0800 Subject: [PATCH 1/4] fix: Decoupled proactive token refresh from FirebaseApp --- src/database/database-internal.ts | 45 ++++++ src/firebase-app.ts | 208 +++++++++------------------- test/unit/database/database.spec.ts | 174 +++++++++++++++++++++++ test/unit/firebase-app.spec.ts | 201 --------------------------- 4 files changed, 282 insertions(+), 346 deletions(-) diff --git a/src/database/database-internal.ts b/src/database/database-internal.ts index b77f536b97..c74e18c220 100644 --- a/src/database/database-internal.ts +++ b/src/database/database-internal.ts @@ -31,6 +31,8 @@ import Database = database.Database; export class DatabaseService { private readonly appInternal: FirebaseApp; + private tokenRefresh: boolean; + private tokenRefreshTimeout: NodeJS.Timeout; private databases: { [dbUrl: string]: Database; @@ -50,6 +52,12 @@ export class DatabaseService { * @internal */ public delete(): Promise { + if (this.tokenRefresh) { + this.appInternal.INTERNAL.removeAuthTokenListener(this.onTokenChange); + clearTimeout(this.tokenRefreshTimeout); + this.tokenRefresh = false; + } + const promises = []; for (const dbUrl of Object.keys(this.databases)) { const db: DatabaseImpl = ((this.databases[dbUrl] as any) as DatabaseImpl); @@ -96,9 +104,46 @@ export class DatabaseService { this.databases[dbUrl] = db; } + + if (!this.tokenRefresh) { + this.tokenRefresh = true; + this.appInternal.INTERNAL.addAuthTokenListener(this.onTokenChange); + } + return db; } + // eslint-disable-next-line @typescript-eslint/no-unused-vars + private onTokenChange(_: string): void { + this.appInternal.INTERNAL.getToken() + .then((token) => { + clearTimeout(this.tokenRefreshTimeout); + const delayMillis = token.expirationTime - 5 * 60 * 1000 - Date.now(); + // If the new token is set to expire in next 5 minutes (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. + // Just here to prevent the promise chain from crashing. + }); + } + + private scheduleTokenRefresh(delayMillis: number): void { + this.tokenRefreshTimeout = setTimeout(() => { + this.appInternal.INTERNAL.getToken(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. + }); + }, delayMillis); + } + private ensureUrl(url?: string): string { if (typeof url !== 'undefined') { return url; diff --git a/src/firebase-app.ts b/src/firebase-app.ts index fb8ad8b0f5..5d1bf34b15 100644 --- a/src/firebase-app.ts +++ b/src/firebase-app.ts @@ -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'; @@ -57,129 +57,80 @@ export interface FirebaseAccessToken { * Internals of a FirebaseApp instance. */ export class FirebaseAppInternals { - private isDeleted_ = false; private cachedToken_: FirebaseAccessToken; - private cachedTokenPromise_: Promise | 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} A Promise that will be fulfilled with the current or - * new token. - */ - public getToken(forceRefresh?: boolean): Promise { - 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 { + if (forceRefresh || this.shouldRefresh()) { + return this.refreshToken(); } + + return Promise.resolve(this.cachedToken_); + } + + private refreshToken(): Promise { + return Promise.resolve(this.credential_.getAccessToken()) + .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()) <= 5 * 60 * 1000; } /** * 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); @@ -191,42 +142,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); - } } /** @@ -419,8 +339,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)) { diff --git a/test/unit/database/database.spec.ts b/test/unit/database/database.spec.ts index 3979719ab6..7f7626321b 100644 --- a/test/unit/database/database.spec.ts +++ b/test/unit/database/database.spec.ts @@ -25,6 +25,7 @@ import * as mocks from '../../resources/mocks'; import { FirebaseApp } from '../../../src/firebase-app'; import { DatabaseService } from '../../../src/database/database-internal'; import { database } from '../../../src/database/index'; +import { ServiceAccountCredential } from '../../../src/credential/credential-internal'; import * as utils from '../utils'; import { HttpClient, HttpRequestConfig } from '../../../src/utils/api-request'; @@ -118,6 +119,179 @@ describe('Database', () => { }); }); + describe('Token refresh', () => { + const MINUTE_IN_MILLIS = 60 * 1000; + + let clock: sinon.SinonFakeTimers; + let getTokenStub: sinon.SinonStub; + + beforeEach(() => { + getTokenStub = stubCredentials(); + clock = sinon.useFakeTimers(1000); + }); + + afterEach(() => { + getTokenStub.restore(); + clock.restore(); + }); + + function stubCredentials(options?: { + accessToken?: string; + expiresIn?: number; + err?: any; + }): sinon.SinonStub { + if (options?.err) { + return sinon.stub(ServiceAccountCredential.prototype, 'getAccessToken') + .rejects(options.err); + } + + return sinon.stub(ServiceAccountCredential.prototype, 'getAccessToken') + .resolves({ + access_token: options?.accessToken || 'mock-access-token', // eslint-disable-line @typescript-eslint/camelcase + expires_in: options?.expiresIn || 3600, // eslint-disable-line @typescript-eslint/camelcase + }); + } + + it('should refresh the token 5 minutes before expiration', () => { + database.getDatabase(mockApp.options.databaseURL); + expect(getTokenStub).to.have.not.been.called; + mockApp.INTERNAL.getToken() + .then((token) => { + expect(getTokenStub).to.have.been.calledOnce; + + const expiryInMillis = token.expirationTime - Date.now(); + clock.tick(expiryInMillis - (5 * MINUTE_IN_MILLIS) - 1000); + expect(getTokenStub).to.have.been.calledOnce; + + clock.tick(1000); + expect(getTokenStub).to.have.been.calledTwice; + }); + }); + + it('should not start multiple token refresher tasks', () => { + database.getDatabase(mockApp.options.databaseURL); + database.getDatabase('https://other-database.firebaseio.com'); + expect(getTokenStub).to.have.not.been.called; + mockApp.INTERNAL.getToken() + .then((token) => { + expect(getTokenStub).to.have.been.calledOnce; + + const expiryInMillis = token.expirationTime - Date.now(); + clock.tick(expiryInMillis - (5 * MINUTE_IN_MILLIS)); + expect(getTokenStub).to.have.been.calledTwice; + }); + }); + + it('should reschedule the token refresher when the underlying token changes', () => { + database.getDatabase(mockApp.options.databaseURL); + mockApp.INTERNAL.getToken() + .then((token1) => { + expect(getTokenStub).to.have.been.calledOnce; + + // Forward the clock to 30 minutes before expiry. + const expiryInMillis = token1.expirationTime - Date.now(); + clock.tick(expiryInMillis - (30 * MINUTE_IN_MILLIS)); + + // Force a token refresh + return mockApp.INTERNAL.getToken(true) + .then((token2) => { + expect(getTokenStub).to.have.been.calledTwice; + // Forward the clock to 5 minutes before old expiry time. + clock.tick(25 * MINUTE_IN_MILLIS); + expect(getTokenStub).to.have.been.calledTwice; + + // Forward the clock 1 second past old expiry time. + clock.tick(5 * MINUTE_IN_MILLIS + 1000); + expect(getTokenStub).to.have.been.calledTwice; + + const newExpiryTimeInMillis = token2.expirationTime - Date.now(); + clock.tick(newExpiryTimeInMillis - (5 * MINUTE_IN_MILLIS)); + expect(getTokenStub).to.have.been.calledThrice; + }); + }); + }); + + it('should not reschedule when the token is about to expire in 5 minutes', () => { + database.getDatabase(mockApp.options.databaseURL); + mockApp.INTERNAL.getToken() + .then((token1) => { + expect(getTokenStub).to.have.been.calledOnce; + + // Forward the clock to 30 minutes before expiry. + const expiryInMillis = token1.expirationTime - Date.now(); + clock.tick(expiryInMillis - (30 * MINUTE_IN_MILLIS)); + + getTokenStub.restore(); + getTokenStub = stubCredentials({ expiresIn: 5 * 60 }); + // Force a token refresh + return mockApp.INTERNAL.getToken(true); + }) + .then((token2) => { + expect(getTokenStub).to.have.been.calledTwice; + + const newExpiryTimeInMillis = token2.expirationTime - Date.now(); + clock.tick(newExpiryTimeInMillis); + expect(getTokenStub).to.have.been.calledTwice; + + getTokenStub.restore(); + getTokenStub = stubCredentials({ expiresIn: 60 * 60 }); + // Force a token refresh + return mockApp.INTERNAL.getToken(true); + }) + .then((token3) => { + expect(getTokenStub).to.have.been.calledThrice; + + const newExpiryTimeInMillis = token3.expirationTime - Date.now(); + clock.tick(newExpiryTimeInMillis - (5 * MINUTE_IN_MILLIS)); + expect(getTokenStub).to.have.callCount(4); + }); + }); + + it('should gracefully handle errors during token refresh', () => { + database.getDatabase(mockApp.options.databaseURL); + mockApp.INTERNAL.getToken() + .then((token1) => { + expect(getTokenStub).to.have.been.calledOnce; + + getTokenStub.restore(); + getTokenStub = stubCredentials({ err: new Error('Test error') }); + expect(getTokenStub).to.have.not.been.called; + + const expiryInMillis = token1.expirationTime - Date.now(); + clock.tick(expiryInMillis); + expect(getTokenStub).to.have.been.calledOnce; + + getTokenStub.restore(); + getTokenStub = stubCredentials(); + expect(getTokenStub).to.have.not.been.called; + // Force a token refresh + return mockApp.INTERNAL.getToken(true); + }) + .then((token2) => { + expect(getTokenStub).to.have.been.calledOnce; + + const newExpiryTimeInMillis = token2.expirationTime - Date.now(); + clock.tick(newExpiryTimeInMillis - (5 * MINUTE_IN_MILLIS)); + expect(getTokenStub).to.have.been.calledTwice; + }); + }); + + it('should stop the token refresher task at delete', () => { + database.getDatabase(mockApp.options.databaseURL); + mockApp.INTERNAL.getToken() + .then((token) => { + expect(getTokenStub).to.have.been.calledOnce; + return database.delete() + .then(() => { + // Forward the clock to five minutes before expiry. + const expiryInMillis = token.expirationTime - Date.now(); + clock.tick(expiryInMillis - (5 * MINUTE_IN_MILLIS)); + expect(getTokenStub).to.have.been.calledOnce; + }); + }); + }); + }); + describe('Rules', () => { const mockAccessToken: string = utils.generateRandomAccessToken(); let getTokenStub: sinon.SinonStub; diff --git a/test/unit/firebase-app.spec.ts b/test/unit/firebase-app.spec.ts index 49da6736c5..6edf73b7ef 100644 --- a/test/unit/firebase-app.spec.ts +++ b/test/unit/firebase-app.spec.ts @@ -90,9 +90,6 @@ describe('FirebaseApp', () => { }); clock = sinon.useFakeTimers(1000); - - mockApp = mocks.app(); - firebaseConfigVar = process.env[FIREBASE_CONFIG_VAR]; delete process.env[FIREBASE_CONFIG_VAR]; firebaseNamespace = new FirebaseNamespace(); @@ -767,204 +764,6 @@ describe('FirebaseApp', () => { }); }); - it('retries to proactively refresh the token if a proactive refresh attempt fails', () => { - // Force a token refresh. - return mockApp.INTERNAL.getToken(true).then((token1) => { - // Stub the getToken() method to return a rejected promise. - getTokenStub.restore(); - expect(mockApp.options.credential).to.exist; - getTokenStub = sinon.stub(mockApp.options.credential!, 'getAccessToken') - .rejects(new Error('Intentionally rejected')); - - // Forward the clock to exactly five minutes before expiry. - const expiryInMilliseconds = token1.expirationTime - Date.now(); - clock.tick(expiryInMilliseconds - (5 * ONE_MINUTE_IN_MILLISECONDS)); - - // Forward the clock to exactly four minutes before expiry. - clock.tick(60 * 1000); - - // Restore the stubbed getAccessToken() method. - getTokenStub.restore(); - getTokenStub = sinon.stub(ServiceAccountCredential.prototype, 'getAccessToken').resolves({ - access_token: 'mock-access-token', // eslint-disable-line @typescript-eslint/camelcase - expires_in: 3600, // eslint-disable-line @typescript-eslint/camelcase - }); - - return mockApp.INTERNAL.getToken().then((token2) => { - // Ensure the token has not been proactively refreshed. - expect(token1).to.deep.equal(token2); - expect(getTokenStub).to.have.not.been.called; - - // Forward the clock to exactly three minutes before expiry. - clock.tick(60 * 1000); - - return mockApp.INTERNAL.getToken().then((token3) => { - // Ensure the token was proactively refreshed. - expect(token1).to.not.deep.equal(token3); - expect(getTokenStub).to.have.been.calledOnce; - }); - }); - }); - }); - - it('stops retrying to proactively refresh the token after five attempts', () => { - // Force a token refresh. - let originalToken: FirebaseAccessToken; - return mockApp.INTERNAL.getToken(true).then((token) => { - originalToken = token; - - // Stub the credential's getAccessToken() method to always return a rejected promise. - getTokenStub.restore(); - expect(mockApp.options.credential).to.exist; - getTokenStub = sinon.stub(mockApp.options.credential!, 'getAccessToken') - .rejects(new Error('Intentionally rejected')); - - // Expect the call count to initially be zero. - expect(getTokenStub.callCount).to.equal(0); - - // Forward the clock to exactly five minutes before expiry. - const expiryInMilliseconds = token.expirationTime - Date.now(); - clock.tick(expiryInMilliseconds - (5 * ONE_MINUTE_IN_MILLISECONDS)); - - // Due to synchronous timing issues when the timer is mocked, make a call to getToken() - // without forcing a refresh to ensure there is enough time for the underlying token refresh - // timeout to fire and complete. - return mockApp.INTERNAL.getToken(); - }).then((token) => { - // Ensure the token was attempted to be proactively refreshed one time. - expect(getTokenStub.callCount).to.equal(1); - - // Ensure the proactive refresh failed. - expect(token).to.deep.equal(originalToken); - - // Forward the clock to four minutes before expiry. - clock.tick(ONE_MINUTE_IN_MILLISECONDS); - - // See note above about calling getToken(). - return mockApp.INTERNAL.getToken(); - }).then((token) => { - // Ensure the token was attempted to be proactively refreshed two times. - expect(getTokenStub.callCount).to.equal(2); - - // Ensure the proactive refresh failed. - expect(token).to.deep.equal(originalToken); - - // Forward the clock to three minutes before expiry. - clock.tick(ONE_MINUTE_IN_MILLISECONDS); - - // See note above about calling getToken(). - return mockApp.INTERNAL.getToken(); - }).then((token) => { - // Ensure the token was attempted to be proactively refreshed three times. - expect(getTokenStub.callCount).to.equal(3); - - // Ensure the proactive refresh failed. - expect(token).to.deep.equal(originalToken); - - // Forward the clock to two minutes before expiry. - clock.tick(ONE_MINUTE_IN_MILLISECONDS); - - // See note above about calling getToken(). - return mockApp.INTERNAL.getToken(); - }).then((token) => { - // Ensure the token was attempted to be proactively refreshed four times. - expect(getTokenStub.callCount).to.equal(4); - - // Ensure the proactive refresh failed. - expect(token).to.deep.equal(originalToken); - - // Forward the clock to one minute before expiry. - clock.tick(ONE_MINUTE_IN_MILLISECONDS); - - // See note above about calling getToken(). - return mockApp.INTERNAL.getToken(); - }).then((token) => { - // Ensure the token was attempted to be proactively refreshed five times. - expect(getTokenStub.callCount).to.equal(5); - - // Ensure the proactive refresh failed. - expect(token).to.deep.equal(originalToken); - - // Forward the clock to expiry. - clock.tick(ONE_MINUTE_IN_MILLISECONDS); - - // See note above about calling getToken(). - return mockApp.INTERNAL.getToken(); - }).then((token) => { - // Ensure the token was not attempted to be proactively refreshed a sixth time. - expect(getTokenStub.callCount).to.equal(5); - - // Ensure the token has never been refresh. - expect(token).to.deep.equal(originalToken); - }); - }); - - it('resets the proactive refresh timeout upon a force refresh', () => { - // Force a token refresh. - return mockApp.INTERNAL.getToken(true).then((token1) => { - // Forward the clock to five minutes and one second before expiry. - let expiryInMilliseconds = token1.expirationTime - Date.now(); - clock.tick(expiryInMilliseconds - (5 * ONE_MINUTE_IN_MILLISECONDS) - 1000); - - // Force a token refresh. - return mockApp.INTERNAL.getToken(true).then((token2) => { - // Ensure the token was force refreshed. - expect(token1).to.not.deep.equal(token2); - expect(getTokenStub).to.have.been.calledTwice; - - // Forward the clock to exactly five minutes before the original token's expiry. - clock.tick(1000); - - return mockApp.INTERNAL.getToken().then((token3) => { - // Ensure the token hasn't changed, meaning the proactive refresh was canceled. - expect(token2).to.deep.equal(token3); - expect(getTokenStub).to.have.been.calledTwice; - - // Forward the clock to exactly five minutes before the refreshed token's expiry. - expiryInMilliseconds = token3.expirationTime - Date.now(); - clock.tick(expiryInMilliseconds - (5 * ONE_MINUTE_IN_MILLISECONDS)); - - return mockApp.INTERNAL.getToken().then((token4) => { - // Ensure the token was proactively refreshed. - expect(token3).to.not.deep.equal(token4); - expect(getTokenStub).to.have.been.calledThrice; - }); - }); - }); - }); - }); - - it('proactively refreshes the token at the next full minute if it expires in five minutes or less', () => { - // Turn off default mocking of one hour access tokens and replace it with a short-lived token. - getTokenStub.restore(); - expect(mockApp.options.credential).to.exist; - getTokenStub = sinon.stub(mockApp.options.credential!, 'getAccessToken').resolves({ - access_token: utils.generateRandomAccessToken(), // eslint-disable-line @typescript-eslint/camelcase - expires_in: 3 * 60 + 10, // eslint-disable-line @typescript-eslint/camelcase - }); - // Expect the call count to initially be zero. - expect(getTokenStub.callCount).to.equal(0); - - // Force a token refresh. - return mockApp.INTERNAL.getToken(true).then((token1) => { - - // Move the clock forward to three minutes and one second before expiry. - clock.tick(9 * 1000); - expect(getTokenStub.callCount).to.equal(1); - - // Move the clock forward to exactly three minutes before expiry. - clock.tick(1000); - - // Expect the underlying getAccessToken() method to have been called once. - expect(getTokenStub.callCount).to.equal(2); - - return mockApp.INTERNAL.getToken().then((token2) => { - // Ensure the token was proactively refreshed. - expect(token1).to.not.deep.equal(token2); - }); - }); - }); - it('Includes the original error in exception', () => { getTokenStub.restore(); const mockError = new FirebaseAppError( From 8674df357167c8ae7c0a088b3deb5a83b26ca27a Mon Sep 17 00:00:00 2001 From: Hiranya Jayathilaka Date: Thu, 11 Mar 2021 16:32:48 -0800 Subject: [PATCH 2/4] fix: Defined constants for duration values --- src/database/database-internal.ts | 11 ++++++----- src/firebase-app.ts | 4 +++- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/database/database-internal.ts b/src/database/database-internal.ts index c74e18c220..8860fa8349 100644 --- a/src/database/database-internal.ts +++ b/src/database/database-internal.ts @@ -28,6 +28,8 @@ import { getSdkVersion } from '../utils/index'; import Database = database.Database; +const TOKEN_REFRESH_THRESHOLD_MILLIS = 5 * 60 * 1000; + export class DatabaseService { private readonly appInternal: FirebaseApp; @@ -117,11 +119,9 @@ export class DatabaseService { private onTokenChange(_: string): void { this.appInternal.INTERNAL.getToken() .then((token) => { - clearTimeout(this.tokenRefreshTimeout); - const delayMillis = token.expirationTime - 5 * 60 * 1000 - Date.now(); - // If the new token is set to expire in next 5 minutes (unlikely), do nothing. - // Somebody will eventually notice and refresh the token, at which point this callback - // will fire again. + 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); } @@ -134,6 +134,7 @@ export class DatabaseService { } private scheduleTokenRefresh(delayMillis: number): void { + clearTimeout(this.tokenRefreshTimeout); this.tokenRefreshTimeout = setTimeout(() => { this.appInternal.INTERNAL.getToken(true) .catch(() => { diff --git a/src/firebase-app.ts b/src/firebase-app.ts index 5d1bf34b15..c30c8ffd7b 100644 --- a/src/firebase-app.ts +++ b/src/firebase-app.ts @@ -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. */ @@ -124,7 +126,7 @@ export class FirebaseAppInternals { } private shouldRefresh(): boolean { - return !this.cachedToken_ || (this.cachedToken_.expirationTime - Date.now()) <= 5 * 60 * 1000; + return !this.cachedToken_ || (this.cachedToken_.expirationTime - Date.now()) <= TOKEN_REFRESH_THRESHOLD_MILLIS; } /** From e7c3588b2030b1277ae76a9dbf0a4475e9ab2409 Mon Sep 17 00:00:00 2001 From: Hiranya Jayathilaka Date: Fri, 12 Mar 2021 14:21:02 -0800 Subject: [PATCH 3/4] fix: Logging errors encountered while scheduling a refresh --- src/database/database-internal.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/database/database-internal.ts b/src/database/database-internal.ts index 8860fa8349..13c25deb44 100644 --- a/src/database/database-internal.ts +++ b/src/database/database-internal.ts @@ -126,17 +126,15 @@ export class DatabaseService { 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. - // Just here to prevent the promise chain from crashing. + .catch((err) => { + console.error('Unexpected error while attempting to schedule a token refresh:', err); }); } private scheduleTokenRefresh(delayMillis: number): void { clearTimeout(this.tokenRefreshTimeout); this.tokenRefreshTimeout = setTimeout(() => { - this.appInternal.INTERNAL.getToken(true) + this.appInternal.INTERNAL.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 From 705801a8c55c3ea2e8aa08891f1c162ad892a450 Mon Sep 17 00:00:00 2001 From: Hiranya Jayathilaka Date: Thu, 18 Mar 2021 14:35:44 -0700 Subject: [PATCH 4/4] fix: Renamed some variables for clarity --- src/database/database-internal.ts | 10 +++++----- src/firebase-app.ts | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/database/database-internal.ts b/src/database/database-internal.ts index 13c25deb44..dd28bfd919 100644 --- a/src/database/database-internal.ts +++ b/src/database/database-internal.ts @@ -33,7 +33,7 @@ const TOKEN_REFRESH_THRESHOLD_MILLIS = 5 * 60 * 1000; export class DatabaseService { private readonly appInternal: FirebaseApp; - private tokenRefresh: boolean; + private tokenListenerRegistered: boolean; private tokenRefreshTimeout: NodeJS.Timeout; private databases: { @@ -54,10 +54,10 @@ export class DatabaseService { * @internal */ public delete(): Promise { - if (this.tokenRefresh) { + if (this.tokenListenerRegistered) { this.appInternal.INTERNAL.removeAuthTokenListener(this.onTokenChange); clearTimeout(this.tokenRefreshTimeout); - this.tokenRefresh = false; + this.tokenListenerRegistered = false; } const promises = []; @@ -107,8 +107,8 @@ export class DatabaseService { this.databases[dbUrl] = db; } - if (!this.tokenRefresh) { - this.tokenRefresh = true; + if (!this.tokenListenerRegistered) { + this.tokenListenerRegistered = true; this.appInternal.INTERNAL.addAuthTokenListener(this.onTokenChange); } diff --git a/src/firebase-app.ts b/src/firebase-app.ts index c30c8ffd7b..88947bec4c 100644 --- a/src/firebase-app.ts +++ b/src/firebase-app.ts @@ -39,7 +39,7 @@ import { RemoteConfig } from './remote-config/remote-config'; import Credential = credential.Credential; import Database = database.Database; -const TOKEN_REFRESH_THRESHOLD_MILLIS = 5 * 60 * 1000; +const TOKEN_EXPIRY_THRESHOLD_MILLIS = 5 * 60 * 1000; /** * Type representing a callback which is called every time an app lifecycle event occurs. @@ -126,7 +126,7 @@ export class FirebaseAppInternals { } private shouldRefresh(): boolean { - return !this.cachedToken_ || (this.cachedToken_.expirationTime - Date.now()) <= TOKEN_REFRESH_THRESHOLD_MILLIS; + return !this.cachedToken_ || (this.cachedToken_.expirationTime - Date.now()) <= TOKEN_EXPIRY_THRESHOLD_MILLIS; } /**