[core-http] Token refresher fix (Azure#13736)

The current token refreshing mechanism in core-http was wrong. The token was being refreshed before the "refresh time buffer" was reached. I've added tests to verify the behavior (if we change the source back, the tests do fail, as expected).

With this PR, here's what we will be doing:
1. If a token is not there or has expired, we ask for a new one (of course).
2. Before a token expires, in a time window, we try to retrieve the token through a refresh promise.
3. As we get new requests to retrieve that token, we re-use the same refresh promise.
4. If the refresh promise fails, next incoming request will re-run the refresh promise only after some specific time. (This is true again, I reverted my changes to this)

Please let me know how this looks! Feedback appreciated.

Reverted: ~**[Important]**  
The update after Jeff's approval is to fix the behavior to match the original intention. Only within the refresh window, while the valid token is still returned, the refresh promise should try to refresh the token once every 30 milliseconds (configurable) until the token expires. Once the token expires, the process does a final attempt to update it, then it stops.~

Incidentally fixes this customer issue, in which the token was refreshed more times than expected:
Fixes Azure#13369

Author: sadasant

sdk/core/core-http/CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## 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)
 

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",

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
     );
   }
 

sdk/core/core-http/src/policies/bearerTokenAuthenticationPolicy.ts

@@ -87,30 +87,14 @@ 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<void> {
-    if (this.tokenRefresher.isReady()) {
-      const accessToken = await this.tokenRefresher.refresh(options);
-      this.tokenCache.setCachedToken(accessToken);
-    }
-  }
-
   private async getToken(options: GetTokenOptions): Promise<string | undefined> {
-    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);
+    // 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);
     }
-
-    return accessToken ? accessToken.token : undefined;
+    return token ? token.token : undefined;
   }
 }

sdk/core/core-http/test/policies/bearerTokenAuthenticationPolicyTests.ts

@@ -2,7 +2,7 @@
 // Licensed under the MIT license.
 
 import { assert } from "chai";
-import { fake, createSandbox } from "sinon";
+import Sinon, { 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,12 +11,15 @@ import { HttpOperationResponse } from "../../src/httpOperationResponse";
 import { HttpHeaders } from "../../src/httpHeaders";
 import { WebResource } from "../../src/webResource";
 import { BearerTokenAuthenticationPolicy } from "../../src/policies/bearerTokenAuthenticationPolicy";
-import {
-  ExpiringAccessTokenCache,
-  TokenRefreshBufferMs
-} from "../../src/credentials/accessTokenCache";
+import { ExpiringAccessTokenCache } 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<HttpOperationResponse> {
@@ -28,6 +31,15 @@ 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"];
@@ -53,32 +65,11 @@ 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("testscope", credentialToTest);
+    const policy = createBearerTokenPolicy("test-scope", credentialToTest);
     await policy.sendRequest(request);
 
     const sandbox = createSandbox();
@@ -88,6 +79,66 @@ 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;
@@ -101,8 +152,8 @@ describe("BearerTokenAuthenticationPolicy", function() {
     return new BearerTokenAuthenticationPolicy(
       mockPolicy,
       new RequestPolicyOptions(),
-      new ExpiringAccessTokenCache(),
-      new AccessTokenRefresher(credential, scopes)
+      new ExpiringAccessTokenCache(beforeTokenExpiresInMs),
+      new AccessTokenRefresher(credential, scopes, tokenRefreshIntervalInMs)
     );
   }
 });
@@ -115,11 +166,15 @@ class MockRefreshAzureCredential implements TokenCredential {
     this._expiresOnTimestamp = expiresOnTimestamp;
   }
 
-  public getToken(
+  public setExpiresOnTimestamp(expiresOnTimestamp: number): void {
+    this._expiresOnTimestamp = expiresOnTimestamp;
+  }
+
+  public async getToken(
     _scopes: string | string[],
     _options?: GetTokenOptions
   ): Promise<AccessToken | null> {
     this.authCount++;
-    return Promise.resolve({ token: "mocktoken", expiresOnTimestamp: this._expiresOnTimestamp });
+    return { token: "mock-token", expiresOnTimestamp: this._expiresOnTimestamp };
   }
 }