From 20f09fb836e33c4f76bca99797b7e36b1bdcf959 Mon Sep 17 00:00:00 2001 From: jwngr Date: Sun, 2 Apr 2017 15:53:14 -0700 Subject: [PATCH 1/5] Proactively refresh access tokens before expiration Change-Id: Id8cc988ca5ba13a19287e203464c80171d53a64b --- src/firebase-app.ts | 30 ++++++++++++++++++ test/resources/mocks.ts | 4 ++- test/unit/auth/auth-api-request.spec.ts | 1 + test/unit/auth/auth.spec.ts | 1 + test/unit/firebase-app.spec.ts | 42 ++++++++++++++++++++++--- test/unit/firebase.spec.ts | 5 ++- test/unit/messaging/messaging.spec.ts | 2 ++ test/unit/utils.ts | 9 ++++-- test/unit/utils/api-request.spec.ts | 4 +++ 9 files changed, 89 insertions(+), 9 deletions(-) diff --git a/src/firebase-app.ts b/src/firebase-app.ts index c9014c265a..bf84285346 100644 --- a/src/firebase-app.ts +++ b/src/firebase-app.ts @@ -37,9 +37,11 @@ export type FirebaseAccessToken = { * Internals of a FirebaseApp instance. */ export class FirebaseAppInternals { + private isDeleted_ = false; private cachedToken_: FirebaseAccessToken; private cachedTokenPromise_: Promise; private tokenListeners_: Array<(token: string) => void>; + private tokenRefreshTimeout_: NodeJS.Timer; constructor(private credential_: Credential) { this.tokenListeners_ = []; @@ -87,6 +89,22 @@ export class FirebaseAppInternals { }); } + // Establish a timeout to proactively refresh the token five minutes before it expires. In + // the rare cases the token is very short-lived, refresh it immediately + let refreshTimeInSeconds = (result.expires_in - (5 * 60)); + if (refreshTimeInSeconds <= 0) { + refreshTimeInSeconds = 0; + } + + // The token refresh timeout keeps the Node.js process alive, so only create it if this + // instance has not already been deleted. + if (!this.isDeleted_) { + this.tokenRefreshTimeout_ = setTimeout(() => { + this.getToken(/* forceRefresh */ true); + clearTimeout(this.tokenRefreshTimeout_); + }, refreshTimeInSeconds * 1000); + } + return token; }) .catch((error) => { @@ -140,6 +158,16 @@ export class FirebaseAppInternals { public removeAuthTokenListener(listener: (token: string) => 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_); + } } @@ -278,6 +306,8 @@ export class FirebaseApp { this.checkDestroyed_(); this.firebaseInternals_.removeApp(this.name_); + this.INTERNAL.delete(); + return Promise.all(Object.keys(this.services_).map((serviceName) => { return this.services_[serviceName].INTERNAL.delete(); })).then(() => { diff --git a/test/resources/mocks.ts b/test/resources/mocks.ts index 922162cdb3..b973d3a7fe 100644 --- a/test/resources/mocks.ts +++ b/test/resources/mocks.ts @@ -45,7 +45,9 @@ export let appOptionsNoDatabaseUrl: FirebaseAppOptions = { }; export function app(): FirebaseApp { - return new FirebaseApp(appOptions, appName, new FirebaseNamespace().INTERNAL); + const namespaceInternals = new FirebaseNamespace().INTERNAL; + namespaceInternals.removeApp = _.noop; + return new FirebaseApp(appOptions, appName, namespaceInternals); } export function applicationDefaultApp(): FirebaseApp { diff --git a/test/unit/auth/auth-api-request.spec.ts b/test/unit/auth/auth-api-request.spec.ts index 5a1678193c..2fb1c3b84d 100644 --- a/test/unit/auth/auth-api-request.spec.ts +++ b/test/unit/auth/auth-api-request.spec.ts @@ -386,6 +386,7 @@ describe('FirebaseAuthRequestHandler', () => { afterEach(() => { _.forEach(stubs, (stub) => stub.restore()); _.forEach(mockedRequests, (mockedRequest) => mockedRequest.done()); + return mockApp.delete(); }); describe('Constructor', () => { diff --git a/test/unit/auth/auth.spec.ts b/test/unit/auth/auth.spec.ts index 44fadc58c8..74fba6d1c2 100644 --- a/test/unit/auth/auth.spec.ts +++ b/test/unit/auth/auth.spec.ts @@ -100,6 +100,7 @@ describe('Auth', () => { afterEach(() => { process.env = oldProcessEnv; + return mockApp.delete(); }); diff --git a/test/unit/firebase-app.spec.ts b/test/unit/firebase-app.spec.ts index 9cfdaaf01e..4c7cba9095 100644 --- a/test/unit/firebase-app.spec.ts +++ b/test/unit/firebase-app.spec.ts @@ -42,11 +42,9 @@ describe('FirebaseApp', () => { let firebaseNamespace: FirebaseNamespace; let firebaseNamespaceInternals: FirebaseNamespaceInternals; - before(() => utils.mockFetchAccessTokenRequests()); - - after(() => nock.cleanAll()); - beforeEach(() => { + utils.mockFetchAccessTokenRequests(); + this.clock = sinon.useFakeTimers(1000); mockApp = mocks.app(); @@ -66,6 +64,8 @@ describe('FirebaseApp', () => { _.forEach(mockedRequests, (mockedRequest) => mockedRequest.done()); mockedRequests = []; + + nock.cleanAll(); }); describe('#name', () => { @@ -275,6 +275,40 @@ describe('FirebaseApp', () => { }); }); }); + + it('proactively refreshes the token five minutes before it expires', () => { + return mockApp.INTERNAL.getToken(true).then((token1) => { + const expiryInMilliseconds = token1.expirationTime - Date.now(); + + this.clock.tick(expiryInMilliseconds - (5 * 60 * 1000) - 1000); + + return mockApp.INTERNAL.getToken().then((token2) => { + expect(token1).to.deep.equal(token2); + expect(https.request).to.have.been.calledOnce; + + this.clock.tick(1000); + + return mockApp.INTERNAL.getToken().then((token3) => { + expect(token1).to.not.deep.equal(token3); + expect(https.request).to.have.been.calledTwice; + }); + }); + }); + }); + + it('proactively refreshes the token immediately if it expires in five minutes or less', () => { + nock.cleanAll(); + utils.mockFetchAccessTokenRequests(/* token */ undefined, /* expiresIn */ 5 * 60); + + return mockApp.INTERNAL.getToken(true).then((token1) => { + this.clock.tick(1); + + return mockApp.INTERNAL.getToken().then((token2) => { + expect(token1).to.not.deep.equal(token2); + expect(https.request).to.have.been.calledTwice; + }); + }); + }); }); describe('INTERNAL.addAuthTokenListener()', () => { diff --git a/test/unit/firebase.spec.ts b/test/unit/firebase.spec.ts index 8cd198a752..ef08232fc3 100644 --- a/test/unit/firebase.spec.ts +++ b/test/unit/firebase.spec.ts @@ -26,12 +26,15 @@ describe('Firebase', () => { after(() => nock.cleanAll()); afterEach(() => { + const deletePromises = []; firebaseAdmin.apps.forEach((app) => { - app.delete(); + deletePromises.push(app.delete()); }); _.forEach(mockedRequests, (mockedRequest) => mockedRequest.done()); mockedRequests = []; + + return Promise.all(deletePromises); }); describe('#initializeApp()', () => { diff --git a/test/unit/messaging/messaging.spec.ts b/test/unit/messaging/messaging.spec.ts index 442e0b510d..ebee9bc6a6 100644 --- a/test/unit/messaging/messaging.spec.ts +++ b/test/unit/messaging/messaging.spec.ts @@ -180,6 +180,8 @@ describe('Messaging', () => { if (httpsRequestStub && httpsRequestStub.restore) { httpsRequestStub.restore(); } + + return mockApp.delete(); }); diff --git a/test/unit/utils.ts b/test/unit/utils.ts index 5a6cd3dcd8..cab0942ac0 100644 --- a/test/unit/utils.ts +++ b/test/unit/utils.ts @@ -28,14 +28,17 @@ export function createAppWithOptions(options: Object) { * is created. * @return {Object} A nock response object. */ -export function mockFetchAccessTokenRequests(token?: string): nock.Scope { +export function mockFetchAccessTokenRequests( + token: string = generateRandomAccessToken(), + expiresIn: number = 60 * 60, +): nock.Scope { return nock('https://accounts.google.com:443') .persist() .post('/o/oauth2/token') .reply(200, { - access_token: token || generateRandomAccessToken(), + access_token: token, token_type: 'Bearer', - expires_in: 3600, + expires_in: expiresIn, }, { 'cache-control': 'no-cache, no-store, max-age=0, must-revalidate', }); diff --git a/test/unit/utils/api-request.spec.ts b/test/unit/utils/api-request.spec.ts index bf93022714..76880346aa 100644 --- a/test/unit/utils/api-request.spec.ts +++ b/test/unit/utils/api-request.spec.ts @@ -309,6 +309,10 @@ describe('SignedApiRequestHandler', () => { mockApp = mocks.app(); }); + afterEach(() => { + return mockApp.delete(); + }); + describe('Constructor', () => { it('should succeed with a FirebaseApp instance', () => { expect(() => { From 28bb48f642310964e68920064e612e522d038259 Mon Sep 17 00:00:00 2001 From: jwngr Date: Sun, 2 Apr 2017 15:58:22 -0700 Subject: [PATCH 2/5] Updated logic for when to refresh token Change-Id: I3520e34d2d70deb9ae4dfca95239f61d9395cec4 --- src/firebase-app.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/firebase-app.ts b/src/firebase-app.ts index bf84285346..fe51269867 100644 --- a/src/firebase-app.ts +++ b/src/firebase-app.ts @@ -90,9 +90,9 @@ export class FirebaseAppInternals { } // Establish a timeout to proactively refresh the token five minutes before it expires. In - // the rare cases the token is very short-lived, refresh it immediately + // the rare cases the token is very short-lived, refresh it immediately. let refreshTimeInSeconds = (result.expires_in - (5 * 60)); - if (refreshTimeInSeconds <= 0) { + if (refreshTimeInSeconds <= 60) { refreshTimeInSeconds = 0; } From af42fb150497d4931727629266e5d6531cdb5dea Mon Sep 17 00:00:00 2001 From: jwngr Date: Sun, 2 Apr 2017 16:08:25 -0700 Subject: [PATCH 3/5] Added in missing expiresIn parameter description Change-Id: Idae8aaacd0b9f86980a978b8bb6a4c35695f8d6b --- test/unit/utils.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/test/unit/utils.ts b/test/unit/utils.ts index cab0942ac0..ae1fcbe26f 100644 --- a/test/unit/utils.ts +++ b/test/unit/utils.ts @@ -26,6 +26,7 @@ export function createAppWithOptions(options: Object) { * * @param {string} [token] The optional access token to return. If not specified, a random one * is created. + * @param {number} [expiresIn] The optional expires in value to use for the access token. * @return {Object} A nock response object. */ export function mockFetchAccessTokenRequests( From 499409879c0c709751a1491676774889528856f4 Mon Sep 17 00:00:00 2001 From: jwngr Date: Fri, 7 Apr 2017 15:08:40 -0700 Subject: [PATCH 4/5] Updated proactive token refresh logic Change-Id: Ie8027c789138f0a9924e52056ed85042f69c9969 --- src/firebase-app.ts | 48 ++++++++-- test/unit/firebase-app.spec.ts | 155 ++++++++++++++++++++++++++++++--- 2 files changed, 182 insertions(+), 21 deletions(-) diff --git a/src/firebase-app.ts b/src/firebase-app.ts index fe51269867..17cc593343 100644 --- a/src/firebase-app.ts +++ b/src/firebase-app.ts @@ -58,6 +58,9 @@ export class FirebaseAppInternals { if (this.cachedTokenPromise_ && !forceRefresh && !expired) { return this.cachedTokenPromise_; } 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()) @@ -89,20 +92,26 @@ export class FirebaseAppInternals { }); } - // Establish a timeout to proactively refresh the token five minutes before it expires. In - // the rare cases the token is very short-lived, refresh it immediately. + // 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)); - if (refreshTimeInSeconds <= 60) { - refreshTimeInSeconds = 0; + 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 (!this.isDeleted_) { - this.tokenRefreshTimeout_ = setTimeout(() => { - this.getToken(/* forceRefresh */ true); - clearTimeout(this.tokenRefreshTimeout_); - }, refreshTimeInSeconds * 1000); + if (numRetries && !this.isDeleted_) { + this.setTokenRefreshTimeout(refreshTimeInSeconds * 1000, numRetries); } return token; @@ -168,6 +177,27 @@ export class FirebaseAppInternals { // 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((error) => { + // 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); + } } diff --git a/test/unit/firebase-app.spec.ts b/test/unit/firebase-app.spec.ts index 4c7cba9095..b33596eb2b 100644 --- a/test/unit/firebase-app.spec.ts +++ b/test/unit/firebase-app.spec.ts @@ -24,6 +24,7 @@ chai.use(sinonChai); chai.use(chaiAsPromised); const ONE_HOUR_IN_SECONDS = 60 * 60; +const ONE_MINUTE_IN_MILLISECONDS = 60 * 1000; const deleteSpy = sinon.spy(); function mockServiceFactory(app: FirebaseApp): FirebaseServiceInterface { @@ -203,9 +204,21 @@ describe('FirebaseApp', () => { describe('INTERNAL.getToken()', () => { let httpsSpy: sinon.SinonSpy; + let getTokenSpy: sinon.SinonSpy; + let getTokenStub: sinon.SinonStub; - beforeEach(() => httpsSpy = sinon.spy(https, 'request')); - afterEach(() => httpsSpy.restore()); + beforeEach(() => { + httpsSpy = sinon.spy(https, 'request'); + }); + + afterEach(() => { + httpsSpy.restore(); + if (typeof (mockApp.INTERNAL.getToken as any).restore === 'function') { + (mockApp.INTERNAL.getToken as any).restore(); + getTokenSpy = undefined; + getTokenStub = undefined; + } + }); it('throws a custom credential implementation which returns invalid access tokens', () => { const credential = { @@ -261,7 +274,7 @@ describe('FirebaseApp', () => { this.clock.tick(1000); return mockApp.INTERNAL.getToken().then((token2) => { expect(token1).to.deep.equal(token2); - expect(https.request).to.have.been.calledOnce; + expect(httpsSpy).to.have.been.calledOnce; }); }); }); @@ -271,41 +284,159 @@ describe('FirebaseApp', () => { this.clock.tick(1000); return mockApp.INTERNAL.getToken(true).then((token2) => { expect(token1).to.not.deep.equal(token2); - expect(https.request).to.have.been.calledTwice; + expect(httpsSpy).to.have.been.calledTwice; }); }); }); it('proactively refreshes the token five minutes before it expires', () => { + // Force a token refresh. return mockApp.INTERNAL.getToken(true).then((token1) => { + // Forward the clock to five minutes and one second before expiry. const expiryInMilliseconds = token1.expirationTime - Date.now(); - - this.clock.tick(expiryInMilliseconds - (5 * 60 * 1000) - 1000); + this.clock.tick(expiryInMilliseconds - (5 * ONE_MINUTE_IN_MILLISECONDS) - 1000); return mockApp.INTERNAL.getToken().then((token2) => { + // Ensure the token has not been proactively refreshed. expect(token1).to.deep.equal(token2); - expect(https.request).to.have.been.calledOnce; + expect(httpsSpy).to.have.been.calledOnce; + // Forward the clock to exactly five minutes before expiry. this.clock.tick(1000); return mockApp.INTERNAL.getToken().then((token3) => { + // Ensure the token was proactively refreshed. + expect(token1).to.not.deep.equal(token3); + expect(httpsSpy).to.have.been.calledTwice; + }); + }); + }); + }); + + 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 = sinon.stub(mockApp.INTERNAL, 'getToken'); + getTokenStub.returns(Promise.reject(new Error('Intentionally rejected'))); + + // Forward the clock to exactly five minutes before expiry. + const expiryInMilliseconds = token1.expirationTime - Date.now(); + this.clock.tick(expiryInMilliseconds - (5 * ONE_MINUTE_IN_MILLISECONDS)); + + // Forward the clock to exactly four minutes before expiry. + this.clock.tick(60 * 1000); + + // Restore the stubbed getToken() method. + getTokenStub.restore(); + + return mockApp.INTERNAL.getToken().then((token2) => { + // Ensure the token has not been proactively refreshed. + expect(token1).to.deep.equal(token2); + expect(httpsSpy).to.have.been.calledOnce; + + // Forward the clock to exactly three minutes before expiry. + this.clock.tick(60 * 1000); + + return mockApp.INTERNAL.getToken().then((token3) => { + // Ensure the token was proactively refreshed. expect(token1).to.not.deep.equal(token3); - expect(https.request).to.have.been.calledTwice; + expect(httpsSpy).to.have.been.calledTwice; + }); + }); + }); + }); + + // TODO(jwenger): I simply cannot get this test to pass although it should. + xit('stops retrying to proactively refresh the token after five attempts', () => { + // Force a token refresh. + return mockApp.INTERNAL.getToken(true).then((token1) => { + // Stub the getToken() method to return a rejected promise. + getTokenStub = sinon.stub(mockApp.INTERNAL, 'getToken'); + getTokenStub.returns(Promise.reject(new Error('Intentionally rejected'))); + + expect(getTokenStub.callCount).to.equal(0); + + // Forward the clock to exactly five minutes before expiry. + const expiryInMilliseconds = token1.expirationTime - Date.now(); + this.clock.tick(expiryInMilliseconds - (5 * ONE_MINUTE_IN_MILLISECONDS)); + + // Ensure the token was attempted to be proactively refreshed one time. + expect(getTokenStub.callCount).to.equal(1); + + // Forward the clock to four minutes before expiry. + this.clock.tick(ONE_MINUTE_IN_MILLISECONDS); + + // Ensure the token was attempted to be proactively refreshed two times. + expect(getTokenStub.callCount).to.equal(2); + + // Forward the clock to expiry. + this.clock.tick(4 * ONE_MINUTE_IN_MILLISECONDS); + + // Ensure the token was attempted to be proactively refreshed five times. + expect(getTokenStub.callCount).to.equal(5); + }); + }); + + 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(); + this.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(httpsSpy).to.have.been.calledTwice; + + // Forward the clock to exactly five minutes before the original token's expiry. + this.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(httpsSpy).to.have.been.calledTwice; + + // Forward the clock to exactly five minutes before the refreshed token's expiry. + expiryInMilliseconds = token3.expirationTime - Date.now(); + this.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(httpsSpy).to.have.been.calledThrice; + }); }); }); }); }); - it('proactively refreshes the token immediately if it expires in five minutes or less', () => { + 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. nock.cleanAll(); - utils.mockFetchAccessTokenRequests(/* token */ undefined, /* expiresIn */ 5 * 60); + utils.mockFetchAccessTokenRequests(/* token */ undefined, /* expiresIn */ 3 * 60 + 10); + // Force a token refresh. return mockApp.INTERNAL.getToken(true).then((token1) => { - this.clock.tick(1); + getTokenSpy = sinon.spy(mockApp.INTERNAL, 'getToken'); + + // Move the clock forward to three minutes and one second before expiry. + this.clock.tick(9 * 1000); + + // Ensure getToken() was not called. + expect(getTokenSpy).to.not.have.been.called; + + // Move the clock forward to exactly three minutes before expiry. + this.clock.tick(1000); + + // Ensure getToken() was called. + expect(getTokenSpy).to.have.been.calledOnce.and.calledWith(true); return mockApp.INTERNAL.getToken().then((token2) => { + // Ensure the token was proactively refreshed. expect(token1).to.not.deep.equal(token2); - expect(https.request).to.have.been.calledTwice; }); }); }); From f3b06fe4da5481d5bee1f1287573958c92f44aef Mon Sep 17 00:00:00 2001 From: jwngr Date: Tue, 11 Apr 2017 14:50:20 -0700 Subject: [PATCH 5/5] Got all proactive token refresh tests passing Change-Id: I6b323d5f0574d841940f8d63bbd6cd4755fd35a9 --- src/firebase-app.ts | 26 ++++--- test/unit/firebase-app.spec.ts | 119 +++++++++++++++++++++++++-------- 2 files changed, 108 insertions(+), 37 deletions(-) diff --git a/src/firebase-app.ts b/src/firebase-app.ts index 17cc593343..cdc5d38cab 100644 --- a/src/firebase-app.ts +++ b/src/firebase-app.ts @@ -56,7 +56,23 @@ export class FirebaseAppInternals { public getToken(forceRefresh?: boolean): Promise { const expired = this.cachedToken_ && this.cachedToken_.expirationTime < Date.now(); if (this.cachedTokenPromise_ && !forceRefresh && !expired) { - return this.cachedTokenPromise_; + 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_); @@ -117,14 +133,6 @@ export class FirebaseAppInternals { return token; }) .catch((error) => { - // Update the cached token promise to avoid caching errors. Set it to resolve with the - // cached token if we have one; otherwise, set it to null. - if (this.cachedToken_) { - this.cachedTokenPromise_ = Promise.resolve(this.cachedToken_); - } else { - this.cachedTokenPromise_ = null; - } - let errorMessage = (typeof error === 'string') ? error : error.message; errorMessage = 'Credential implementation provided to initializeApp() via the ' + diff --git a/test/unit/firebase-app.spec.ts b/test/unit/firebase-app.spec.ts index b33596eb2b..af58126aac 100644 --- a/test/unit/firebase-app.spec.ts +++ b/test/unit/firebase-app.spec.ts @@ -204,8 +204,8 @@ describe('FirebaseApp', () => { describe('INTERNAL.getToken()', () => { let httpsSpy: sinon.SinonSpy; - let getTokenSpy: sinon.SinonSpy; - let getTokenStub: sinon.SinonStub; + let getAccessTokenSpy: sinon.SinonSpy; + let getAccessTokenStub: sinon.SinonStub; beforeEach(() => { httpsSpy = sinon.spy(https, 'request'); @@ -213,10 +213,13 @@ describe('FirebaseApp', () => { afterEach(() => { httpsSpy.restore(); - if (typeof (mockApp.INTERNAL.getToken as any).restore === 'function') { - (mockApp.INTERNAL.getToken as any).restore(); - getTokenSpy = undefined; - getTokenStub = undefined; + + if (typeof getAccessTokenSpy !== 'undefined') { + getAccessTokenSpy.restore(); + } + + if (typeof getAccessTokenStub !== 'undefined') { + getAccessTokenStub.restore(); } }); @@ -317,8 +320,8 @@ describe('FirebaseApp', () => { // Force a token refresh. return mockApp.INTERNAL.getToken(true).then((token1) => { // Stub the getToken() method to return a rejected promise. - getTokenStub = sinon.stub(mockApp.INTERNAL, 'getToken'); - getTokenStub.returns(Promise.reject(new Error('Intentionally rejected'))); + getAccessTokenStub = sinon.stub(mockApp.options.credential, 'getAccessToken'); + getAccessTokenStub.returns(Promise.reject(new Error('Intentionally rejected'))); // Forward the clock to exactly five minutes before expiry. const expiryInMilliseconds = token1.expirationTime - Date.now(); @@ -327,8 +330,9 @@ describe('FirebaseApp', () => { // Forward the clock to exactly four minutes before expiry. this.clock.tick(60 * 1000); - // Restore the stubbed getToken() method. - getTokenStub.restore(); + // Restore the stubbed getAccessToken() method. + getAccessTokenStub.restore(); + getAccessTokenStub = undefined; return mockApp.INTERNAL.getToken().then((token2) => { // Ensure the token has not been proactively refreshed. @@ -347,34 +351,93 @@ describe('FirebaseApp', () => { }); }); - // TODO(jwenger): I simply cannot get this test to pass although it should. - xit('stops retrying to proactively refresh the token after five attempts', () => { + it('stops retrying to proactively refresh the token after five attempts', () => { // Force a token refresh. - return mockApp.INTERNAL.getToken(true).then((token1) => { - // Stub the getToken() method to return a rejected promise. - getTokenStub = sinon.stub(mockApp.INTERNAL, 'getToken'); - getTokenStub.returns(Promise.reject(new Error('Intentionally rejected'))); + let originalToken; + return mockApp.INTERNAL.getToken(true).then((token) => { + originalToken = token; + + // Stub the credential's getAccessToken() method to always return a rejected promise. + getAccessTokenStub = sinon.stub(mockApp.options.credential, 'getAccessToken'); + getAccessTokenStub.returns(Promise.reject(new Error('Intentionally rejected'))); - expect(getTokenStub.callCount).to.equal(0); + // Expect the call count to initially be zero. + expect(getAccessTokenStub.callCount).to.equal(0); // Forward the clock to exactly five minutes before expiry. - const expiryInMilliseconds = token1.expirationTime - Date.now(); + const expiryInMilliseconds = token.expirationTime - Date.now(); this.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); + expect(getAccessTokenStub.callCount).to.equal(1); + + // Ensure the proactive refresh failed. + expect(token).to.deep.equal(originalToken); // Forward the clock to four minutes before expiry. this.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); + expect(getAccessTokenStub.callCount).to.equal(2); - // Forward the clock to expiry. - this.clock.tick(4 * ONE_MINUTE_IN_MILLISECONDS); + // Ensure the proactive refresh failed. + expect(token).to.deep.equal(originalToken); + + // Forward the clock to three minutes before expiry. + this.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(getAccessTokenStub.callCount).to.equal(3); + // Ensure the proactive refresh failed. + expect(token).to.deep.equal(originalToken); + + // Forward the clock to two minutes before expiry. + this.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(getAccessTokenStub.callCount).to.equal(4); + + // Ensure the proactive refresh failed. + expect(token).to.deep.equal(originalToken); + + // Forward the clock to one minute before expiry. + this.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); + expect(getAccessTokenStub.callCount).to.equal(5); + + // Ensure the proactive refresh failed. + expect(token).to.deep.equal(originalToken); + + // Forward the clock to expiry. + this.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(getAccessTokenStub.callCount).to.equal(5); + + // Ensure the token has never been refresh. + expect(token).to.deep.equal(originalToken); }); }); @@ -420,19 +483,19 @@ describe('FirebaseApp', () => { // Force a token refresh. return mockApp.INTERNAL.getToken(true).then((token1) => { - getTokenSpy = sinon.spy(mockApp.INTERNAL, 'getToken'); + getAccessTokenSpy = sinon.spy(mockApp.options.credential, 'getAccessToken'); // Move the clock forward to three minutes and one second before expiry. this.clock.tick(9 * 1000); - // Ensure getToken() was not called. - expect(getTokenSpy).to.not.have.been.called; + // Expect the call count to initially be zero. + expect(getAccessTokenSpy.callCount).to.equal(0); // Move the clock forward to exactly three minutes before expiry. this.clock.tick(1000); - // Ensure getToken() was called. - expect(getTokenSpy).to.have.been.calledOnce.and.calledWith(true); + // Expect the underlying getAccessToken() method to have been called once. + expect(getAccessTokenSpy.callCount).to.equal(1); return mockApp.INTERNAL.getToken().then((token2) => { // Ensure the token was proactively refreshed.