Fix the CAE bug in js core lib

sdk/core/core-auth/CHANGELOG.md

@@ -1,14 +1,8 @@
 # Release History
 
-## 1.3.3 (Unreleased)
+## 1.3.3 (2022-07-06)
 
-### Features Added
-
-### Breaking Changes
-
-### Key Bugs Fixed
-
-### Fixed
+- Added `claims` optional property to the `GetTokenOptions` interface. If `claims` is set, it indicates that we are in challenge process and force to refresh the token.
 
 ## 1.3.2 (2021-07-01)
 

sdk/core/core-auth/review/core-auth.api.md

@@ -37,6 +37,7 @@ export class AzureSASCredential implements SASCredential {
 // @public
 export interface GetTokenOptions {
     abortSignal?: AbortSignalLike;
+    claims?: string;
     requestOptions?: {
         timeout?: number;
     };

sdk/core/core-auth/src/tokenCredential.ts

@@ -52,6 +52,11 @@ export interface GetTokenOptions {
    * Allows specifying a tenantId. Useful to handle challenges that provide tenant Id hints.
    */
   tenantId?: string;
+
+  /**
+   * Claim details to perform the Continuous Access Evaluation authentication flow
+   */
+  claims?: string;
 }
 
 /**

sdk/core/core-client/src/authorizeRequestOnClaimChallenge.ts

@@ -2,7 +2,6 @@
 // Licensed under the MIT license.
 
 import { AuthorizeRequestOnChallengeOptions } from "@azure/core-rest-pipeline";
-import { GetTokenOptions } from "@azure/core-auth";
 import { logger as coreClientLogger } from "./log";
 import { decodeStringToString } from "./base64";
 
@@ -85,7 +84,7 @@ export async function authorizeRequestOnClaimChallenge(
     parsedChallenge.scope ? [parsedChallenge.scope] : scopes,
     {
       claims: decodeStringToString(parsedChallenge.claims),
-    } as GetTokenOptions
+    }
   );
 
   if (!accessToken) {

sdk/core/core-rest-pipeline/CHANGELOG.md

@@ -1,14 +1,10 @@
 # Release History
 
-## 1.9.1 (Unreleased)
-
-### Features Added
-
-### Breaking Changes
+## 1.9.1 (2022-07-06)
 
 ### Bugs Fixed
 
-### Other Changes
+- Fixed a bug in claim challenge we failed to refresh our token. [#22324](https://github.com/Azure/azure-sdk-for-js/pull/22324)
 
 ## 1.9.0 (2022-06-03)
 

sdk/core/core-rest-pipeline/src/policies/bearerTokenAuthenticationPolicy.ts

@@ -6,6 +6,7 @@ import { AzureLogger } from "@azure/logger";
 import { PipelineRequest, PipelineResponse, SendRequest } from "../interfaces";
 import { PipelinePolicy } from "../pipeline";
 import { createTokenCycler } from "../util/tokenCycler";
+import { logger as coreLogger } from "../log";
 
 /**
  * The programmatic identifier of the bearerTokenAuthenticationPolicy.
@@ -136,7 +137,8 @@ function getChallenge(response: PipelineResponse): string | undefined {
 export function bearerTokenAuthenticationPolicy(
   options: BearerTokenAuthenticationPolicyOptions
 ): PipelinePolicy {
-  const { credential, scopes, challengeCallbacks, logger } = options;
+  const { credential, scopes, challengeCallbacks } = options;
+  const logger = options.logger || coreLogger;
   const callbacks = {
     authorizeRequest: challengeCallbacks?.authorizeRequest ?? defaultAuthorizeRequest,
     authorizeRequestOnChallenge: challengeCallbacks?.authorizeRequestOnChallenge,

sdk/core/core-rest-pipeline/src/util/tokenCycler.ts

@@ -201,9 +201,11 @@ export function createTokenCycler(
     //   step 1.
     //
 
-    // IF the tenantId passed in token options is different to the one we have, we need to
-    // refresh the token with the new tenantId.
-    const mustRefresh = tenantId !== tokenOptions.tenantId || cycler.mustRefresh;
+    // If the tenantId passed in token options is different to the one we have
+    // Or if we are in claim challenge and the token was rejected and a new access token need to be issued, we need to
+    // refresh the token with the new tenantId or token.
+    const mustRefresh =
+      tenantId !== tokenOptions.tenantId || Boolean(tokenOptions.claims) || cycler.mustRefresh;
 
     if (mustRefresh) return refresh(scopes, tokenOptions);
 

sdk/core/core-rest-pipeline/test/node/bearerTokenAuthenticationPolicyChallenge.spec.ts

@@ -21,6 +21,7 @@ export interface TestChallenge {
 }
 
 let cachedChallenge: string | undefined;
+let cachedPreviousToken: AccessToken | null;
 
 /**
  * Converts a uint8Array to a string.
@@ -95,6 +96,9 @@ async function authorizeRequestOnChallenge(
   if (!accessToken) {
     return false;
   }
+  if (cachedPreviousToken) {
+    cachedPreviousToken = accessToken;
+  }
 
   options.request.headers.set("Authorization", `Bearer ${accessToken.token}`);
   return true;
@@ -259,17 +263,22 @@ describe("bearerTokenAuthenticationPolicy with challenge", function () {
         request: pipelineRequest,
         status: 200,
       },
+      {
+        headers: createHttpHeaders(),
+        request: pipelineRequest,
+        status: 200,
+      },
     ];
 
     const getTokenResponses = [
       { token: "mock-token", expiresOnTimestamp: Date.now() + 5000 },
-      { token: "mock-token2", expiresOnTimestamp: Date.now() + 10000 },
+      { token: "mock-token2", expiresOnTimestamp: Date.now() + 100000 },
+      { token: "mock-token3", expiresOnTimestamp: Date.now() + 100000 },
     ];
     const credential = new MockRefreshAzureCredential([...getTokenResponses]);
 
     const pipeline = createEmptyPipeline();
     let firstRequest: boolean = true;
-    let previousToken: AccessToken | null;
     const bearerPolicy = bearerTokenAuthenticationPolicy({
       // Intentionally left empty, as it should be replaced by the challenge.
       scopes: [],
@@ -280,13 +289,13 @@ describe("bearerTokenAuthenticationPolicy with challenge", function () {
             firstRequest = false;
             // send first request without the Authorization header
           } else {
-            if (!previousToken) {
-              previousToken = await getAccessToken([], {});
-              if (!previousToken) {
+            if (!cachedPreviousToken) {
+              cachedPreviousToken = await getAccessToken([], {});
+              if (!cachedPreviousToken) {
                 throw new Error("Failed to retrieve an access token");
               }
             }
-            request.headers.set("Authorization", `Bearer ${previousToken.token}`);
+            request.headers.set("Authorization", `Bearer ${cachedPreviousToken.token}`);
           }
         },
         authorizeRequestOnChallenge,
@@ -308,8 +317,14 @@ describe("bearerTokenAuthenticationPolicy with challenge", function () {
       },
     };
 
+    // Will refresh token once as the first time token is empty
     await pipeline.sendRequest(testHttpsClient, pipelineRequest);
     clock.tick(5000);
+    // Will refresh token twice
+    // - 1st refreshing because the token is epxired
+    // - 2nd refreshing because the response with old token has 401 error and claim details so we need refresh token again
+    await pipeline.sendRequest(testHttpsClient, pipelineRequest);
+    // Token is still valid and no need to refresh it
     await pipeline.sendRequest(testHttpsClient, pipelineRequest);
 
     // Our goal is to test that:
@@ -334,6 +349,107 @@ describe("bearerTokenAuthenticationPolicy with challenge", function () {
       undefined,
       `Bearer ${getTokenResponses[0].token}`,
       `Bearer ${getTokenResponses[1].token}`,
+      `Bearer ${getTokenResponses[2].token}`,
+      `Bearer ${getTokenResponses[2].token}`,
+    ]);
+  });
+
+  it("tests that once the challenge is processed we won't refresh the token again and again", async function () {
+    const expected = [
+      {
+        scope: ["http://localhost/.default"],
+        challengeClaims: JSON.stringify({
+          access_token: { foo: "bar" },
+        }),
+      },
+    ];
+
+    const pipelineRequest = createPipelineRequest({ url: "https://example.com" });
+    const responses: PipelineResponse[] = [
+      {
+        headers: createHttpHeaders({
+          "WWW-Authenticate": `Bearer scope="${expected[0].scope[0]}", claims="${encodeString(
+            expected[0].challengeClaims
+          )}"`,
+        }),
+        request: pipelineRequest,
+        status: 401,
+      },
+      {
+        headers: createHttpHeaders(),
+        request: pipelineRequest,
+        status: 200,
+      },
+      {
+        headers: createHttpHeaders(),
+        request: pipelineRequest,
+        status: 200,
+      },
+      {
+        headers: createHttpHeaders(),
+        request: pipelineRequest,
+        status: 200,
+      },
+    ];
+
+    const getTokenResponses = [
+      { token: "mock-token-initialzation", expiresOnTimestamp: Date.now() + 180000 },
+      // ensure the token will not expire
+      { token: "mock-token-challenge", expiresOnTimestamp: Date.now() + 180000 },
+    ];
+    const credential = new MockRefreshAzureCredential([...getTokenResponses]);
+
+    const pipeline = createEmptyPipeline();
+    const bearerPolicy = bearerTokenAuthenticationPolicy({
+      scopes: [],
+      credential,
+      challengeCallbacks: {
+        authorizeRequestOnChallenge,
+      },
+    });
+    pipeline.addPolicy(bearerPolicy);
+
+    const finalSendRequestHeaders: (string | undefined)[] = [];
+
+    const testHttpsClient: HttpClient = {
+      sendRequest: async (req) => {
+        finalSendRequestHeaders.push(req.headers.get("Authorization"));
+        if (responses.length) {
+          const response = responses.shift()!;
+          response.request = req;
+          return response;
+        }
+        throw new Error("No responses found");
+      },
+    };
+
+    // Will refresh token twice
+    // - 1st refreshing to initialize the token
+    // - 2nd refreshing to handle challenge process
+    await pipeline.sendRequest(testHttpsClient, pipelineRequest);
+    assert.equal(credential.authCount, 2);
+    clock.tick(5000);
+    // Will not refresh the token because the previous one is still valid
+    await pipeline.sendRequest(testHttpsClient, pipelineRequest);
+    await pipeline.sendRequest(testHttpsClient, pipelineRequest);
+
+    // Our goal is to test that:
+    // - After a challenge is received and processed and once the token is valid, we'll use it in future calls
+    assert.equal(credential.authCount, 2);
+    assert.deepEqual(credential.scopesAndClaims, [
+      {
+        scope: [],
+        challengeClaims: undefined,
+      },
+      {
+        scope: expected[0].scope,
+        challengeClaims: expected[0].challengeClaims,
+      },
+    ]);
+    assert.deepEqual(finalSendRequestHeaders, [
+      `Bearer ${getTokenResponses[0].token}`,
+      `Bearer ${getTokenResponses[1].token}`,
+      `Bearer ${getTokenResponses[1].token}`,
       `Bearer ${getTokenResponses[1].token}`,
     ]);
   });