diff --git a/sdk/core/core-http/CHANGELOG.md b/sdk/core/core-http/CHANGELOG.md index a565706cfbeb..a234df2435f5 100644 --- a/sdk/core/core-http/CHANGELOG.md +++ b/sdk/core/core-http/CHANGELOG.md @@ -2,7 +2,6 @@ ## 1.2.4 (Unreleased) -- Updated the `bearerTokenAuthenticationPolicy` to refresh tokens only when they're about to expire and not multiple times before. This fixes the issue: [13369](https://github.com/Azure/azure-sdk-for-js/issues/13369). ## 1.2.3 (2021-02-04) diff --git a/sdk/core/core-http/karma.conf.ts b/sdk/core/core-http/karma.conf.ts index ddd105fe3992..c7eac486725b 100644 --- a/sdk/core/core-http/karma.conf.ts +++ b/sdk/core/core-http/karma.conf.ts @@ -4,7 +4,7 @@ const defaults = { process.env.CHROME_BIN = require("puppeteer").executablePath(); -module.exports = function (config: any) { +module.exports = function(config: any) { config.set({ plugins: [ "karma-mocha", diff --git a/sdk/core/core-http/src/credentials/accessTokenRefresher.ts b/sdk/core/core-http/src/credentials/accessTokenRefresher.ts index 1798664e0554..7c0c92347282 100644 --- a/sdk/core/core-http/src/credentials/accessTokenRefresher.ts +++ b/sdk/core/core-http/src/credentials/accessTokenRefresher.ts @@ -23,7 +23,7 @@ export class AccessTokenRefresher { public isReady(): boolean { // We're only ready for a new refresh if the required milliseconds have passed. return ( - !this.lastCalled || Date.now() - this.lastCalled >= this.requiredMillisecondsBeforeNewRefresh + !this.lastCalled || Date.now() - this.lastCalled > this.requiredMillisecondsBeforeNewRefresh ); } diff --git a/sdk/core/core-http/src/policies/bearerTokenAuthenticationPolicy.ts b/sdk/core/core-http/src/policies/bearerTokenAuthenticationPolicy.ts index febaa7109aab..a688c909595c 100644 --- a/sdk/core/core-http/src/policies/bearerTokenAuthenticationPolicy.ts +++ b/sdk/core/core-http/src/policies/bearerTokenAuthenticationPolicy.ts @@ -87,14 +87,30 @@ export class BearerTokenAuthenticationPolicy extends BaseRequestPolicy { return this._nextPolicy.sendRequest(webResource); } + /** + * Attempts a token update if any other time related conditionals have been reached based on the tokenRefresher class. + */ + private async updateTokenIfNeeded(options: GetTokenOptions): Promise { + if (this.tokenRefresher.isReady()) { + const accessToken = await this.tokenRefresher.refresh(options); + this.tokenCache.setCachedToken(accessToken); + } + } + private async getToken(options: GetTokenOptions): Promise { - // We reset the cached token some before it expires, - // after that point, we retry the refresh of the token only if the token refresher is ready. - let token = this.tokenCache.getCachedToken(); - if (!token && this.tokenRefresher.isReady()) { - token = await this.tokenRefresher.refresh(options); - this.tokenCache.setCachedToken(token); + let accessToken = this.tokenCache.getCachedToken(); + if (accessToken === undefined) { + // Waiting for the next refresh only if the cache is unable to retrieve the access token, + // which means that it has expired, or it has never been set. + accessToken = await this.tokenRefresher.refresh(options); + this.tokenCache.setCachedToken(accessToken); + } else { + // If we still have a cached access token, + // And any other time related conditionals have been reached based on the tokenRefresher class, + // then attempt to refresh without waiting. + this.updateTokenIfNeeded(options); } - return token ? token.token : undefined; + + return accessToken ? accessToken.token : undefined; } } diff --git a/sdk/core/core-http/test/policies/bearerTokenAuthenticationPolicyTests.ts b/sdk/core/core-http/test/policies/bearerTokenAuthenticationPolicyTests.ts index 1284cec6ccc5..fdc60a2cca0f 100644 --- a/sdk/core/core-http/test/policies/bearerTokenAuthenticationPolicyTests.ts +++ b/sdk/core/core-http/test/policies/bearerTokenAuthenticationPolicyTests.ts @@ -2,7 +2,7 @@ // Licensed under the MIT license. import { assert } from "chai"; -import Sinon, { fake, createSandbox } from "sinon"; +import { fake, createSandbox } from "sinon"; import { OperationSpec } from "../../src/operationSpec"; import { TokenCredential, GetTokenOptions, AccessToken } from "@azure/core-auth"; import { RequestPolicy, RequestPolicyOptions } from "../../src/policies/requestPolicy"; @@ -11,15 +11,12 @@ import { HttpOperationResponse } from "../../src/httpOperationResponse"; import { HttpHeaders } from "../../src/httpHeaders"; import { WebResource } from "../../src/webResource"; import { BearerTokenAuthenticationPolicy } from "../../src/policies/bearerTokenAuthenticationPolicy"; -import { ExpiringAccessTokenCache } from "../../src/credentials/accessTokenCache"; +import { + ExpiringAccessTokenCache, + TokenRefreshBufferMs +} from "../../src/credentials/accessTokenCache"; import { AccessTokenRefresher } from "../../src/credentials/accessTokenRefresher"; -// Token is refreshed one millisecond BEFORE it expires. -const beforeTokenExpiresInMs = 1000; - -// To avoid many refresh requests, we make new refresh requests only after: -const tokenRefreshIntervalInMs = 500; - describe("BearerTokenAuthenticationPolicy", function() { const mockPolicy: RequestPolicy = { sendRequest(request: WebResource): Promise { @@ -31,15 +28,6 @@ describe("BearerTokenAuthenticationPolicy", function() { } }; - let clock: Sinon.SinonFakeTimers; - - beforeEach(() => { - clock = Sinon.useFakeTimers(Date.now()); - }); - afterEach(() => { - clock.restore(); - }); - it("correctly adds an Authentication header with the Bearer token", async function() { const mockToken = "token"; const tokenScopes = ["scope1", "scope2"]; @@ -65,11 +53,32 @@ describe("BearerTokenAuthenticationPolicy", function() { ); }); + it("refreshes access tokens when they expire", async () => { + const now = Date.now(); + const refreshCred1 = new MockRefreshAzureCredential(now); + const refreshCred2 = new MockRefreshAzureCredential(now + TokenRefreshBufferMs); + const notRefreshCred1 = new MockRefreshAzureCredential(now + TokenRefreshBufferMs + 5000); + + const credentialsToTest: [MockRefreshAzureCredential, number, string][] = [ + [refreshCred1, 2, "Refresh credential 1"], + [refreshCred2, 2, "Refresh credential 2"], + [notRefreshCred1, 1, "Not refresh credential 1"] + ]; + + const request = createRequest(); + for (const [credentialToTest, expectedCalls, message] of credentialsToTest) { + const policy = createBearerTokenPolicy("testscope", credentialToTest); + await policy.sendRequest(request); + await policy.sendRequest(request); + assert.strictEqual(credentialToTest.authCount, expectedCalls, `${message} failed`); + } + }); + it("tests that AccessTokenRefresher is working", async function() { const now = Date.now(); const credentialToTest = new MockRefreshAzureCredential(now); const request = createRequest(); - const policy = createBearerTokenPolicy("test-scope", credentialToTest); + const policy = createBearerTokenPolicy("testscope", credentialToTest); await policy.sendRequest(request); const sandbox = createSandbox(); @@ -79,66 +88,6 @@ describe("BearerTokenAuthenticationPolicy", function() { assert.strictEqual(credentialToTest.authCount, 2); }); - it("refreshes the token on initial request", async () => { - const expiresOn = Date.now() + 1000 * 60; // One minute later. - const credential = new MockRefreshAzureCredential(expiresOn); - const request = createRequest(); - const policy = createBearerTokenPolicy("test-scope", credential); - - await policy.sendRequest(request); - assert.strictEqual(credential.authCount, 1); - }); - - it("refreshes the token again only once it expired", async () => { - const expireDelayMs = 5000; - let tokenExpiration = Date.now() + expireDelayMs; - const credential = new MockRefreshAzureCredential(tokenExpiration); - const request = createRequest(); - const policy = createBearerTokenPolicy("test-scope", credential); - - // The token is cached and remains cached for a bit. - await policy.sendRequest(request); - await policy.sendRequest(request); - assert.strictEqual(credential.authCount, 1); - - // The token will remain cached until tokenExpiration - testTokenRefreshBufferMs, so in (5000 - 1000) milliseconds. - - // For safe measure, we test the token is still cached a second earlier than the refresh expectation. - clock.tick(expireDelayMs - beforeTokenExpiresInMs - 1000); - await policy.sendRequest(request); - assert.strictEqual(credential.authCount, 1); - - // The new token will last 5 seconds again. - tokenExpiration = Date.now() + expireDelayMs; - credential.setExpiresOnTimestamp(tokenExpiration); - - // Now we wait until it expires: - clock.tick(1000); - await policy.sendRequest(request); - assert.strictEqual(credential.authCount, 2); - }); - - it("access token refresher should prevent refreshers to happen too fast", async () => { - const expiresOn = Date.now(); // Already expired - const credential = new MockRefreshAzureCredential(expiresOn); - const request = createRequest(); - const policy = createBearerTokenPolicy("test-scope", credential); - - await policy.sendRequest(request); - assert.strictEqual(credential.authCount, 1); - credential.setExpiresOnTimestamp(Date.now()); - - await policy.sendRequest(request); - assert.strictEqual(credential.authCount, 1); - credential.setExpiresOnTimestamp(Date.now()); - - clock.tick(tokenRefreshIntervalInMs); - - await policy.sendRequest(request); - assert.strictEqual(credential.authCount, 2); - credential.setExpiresOnTimestamp(Date.now()); - }); - function createRequest(operationSpec?: OperationSpec): WebResource { const request = new WebResource(); request.operationSpec = operationSpec; @@ -152,8 +101,8 @@ describe("BearerTokenAuthenticationPolicy", function() { return new BearerTokenAuthenticationPolicy( mockPolicy, new RequestPolicyOptions(), - new ExpiringAccessTokenCache(beforeTokenExpiresInMs), - new AccessTokenRefresher(credential, scopes, tokenRefreshIntervalInMs) + new ExpiringAccessTokenCache(), + new AccessTokenRefresher(credential, scopes) ); } }); @@ -166,15 +115,11 @@ class MockRefreshAzureCredential implements TokenCredential { this._expiresOnTimestamp = expiresOnTimestamp; } - public setExpiresOnTimestamp(expiresOnTimestamp: number): void { - this._expiresOnTimestamp = expiresOnTimestamp; - } - - public async getToken( + public getToken( _scopes: string | string[], _options?: GetTokenOptions ): Promise { this.authCount++; - return { token: "mock-token", expiresOnTimestamp: this._expiresOnTimestamp }; + return Promise.resolve({ token: "mocktoken", expiresOnTimestamp: this._expiresOnTimestamp }); } }