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

This reverts commit 849472a.

Author: sadasant

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)
 

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

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<HttpOperationResponse> {
@@ -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<AccessToken | null> {
     this.authCount++;
-    return { token: "mock-token", expiresOnTimestamp: this._expiresOnTimestamp };
+    return Promise.resolve({ token: "mocktoken", expiresOnTimestamp: this._expiresOnTimestamp });
   }
 }