renamed the authentication context, reduced the number of requests on…

… challenge, improved tests

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

@@ -17,12 +17,6 @@ export interface AddPipelineOptions {
     phase?: PipelinePhase;
 }
 
-// @public
-export interface AuthenticationContext {
-    claims?: string;
-    scopes?: string[];
-}
-
 // @public
 export function bearerTokenAuthenticationPolicy(options: BearerTokenAuthenticationPolicyOptions): PipelinePolicy;
 
@@ -34,12 +28,18 @@ export interface BearerTokenAuthenticationPolicyOptions {
     challenge?: {
         prepareRequest?(request: PipelineRequest): Promise<void>;
         getChallenge?(response: PipelineResponse): string | undefined;
-        processChallenge(challenge: string): Promise<AuthenticationContext | undefined>;
+        processChallenge(challenge: string): Promise<BearerTokenChallengeResult | undefined>;
     };
     credential: TokenCredential;
     scopes: string | string[];
 }
 
+// @public
+export interface BearerTokenChallengeResult {
+    claims?: string;
+    scopes?: string[];
+}
+
 // @public
 export function createDefaultHttpsClient(): HttpsClient;
 

sdk/core/core-https/src/index.ts

@@ -13,8 +13,7 @@ export {
   ProxySettings,
   RawHttpHeaders,
   TransferProgressEvent,
-  RequestBodyType,
-  AuthenticationContext
+  RequestBodyType
 } from "./interfaces";
 export {
   AddPolicyOptions as AddPipelineOptions,
@@ -66,6 +65,7 @@ export { formDataPolicy, formDataPolicyName } from "./policies/formDataPolicy";
 export {
   bearerTokenAuthenticationPolicy,
   BearerTokenAuthenticationPolicyOptions,
-  bearerTokenAuthenticationPolicyName
+  bearerTokenAuthenticationPolicyName,
+  BearerTokenChallengeResult
 } from "./policies/bearerTokenAuthenticationPolicy";
 export { ndJsonPolicy, ndJsonPolicyName } from "./policies/ndJsonPolicy";

sdk/core/core-https/src/interfaces.ts

@@ -274,18 +274,3 @@ export type FormDataValue = string | Blob;
  * A simple object that provides form data, as if from a browser form.
  */
 export type FormDataMap = { [key: string]: FormDataValue | FormDataValue[] };
-
-/**
- * Allows the discovery of authentication properties.
- */
-export interface AuthenticationContext {
-  /**
-   * Scopes to overwrite during the get token request.
-   */
-  scopes?: string[];
-  /**
-   * Additional claims to be included in the token.
-   * For more information on format and content: [the claims parameter specification](href="https://openid.net/specs/openid-connect-core-1_0-final.html#ClaimsParameter).
-   */
-  claims?: string;
-}

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

@@ -1,12 +1,7 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.
 
-import {
-  PipelineResponse,
-  PipelineRequest,
-  SendRequest,
-  AuthenticationContext
-} from "../interfaces";
+import { PipelineResponse, PipelineRequest, SendRequest } from "../interfaces";
 import { PipelinePolicy } from "../pipeline";
 import { TokenCredential, GetTokenOptions } from "@azure/core-auth";
 import { AccessTokenCache, ExpiringAccessTokenCache } from "../accessTokenCache";
@@ -16,6 +11,21 @@ import { AccessTokenCache, ExpiringAccessTokenCache } from "../accessTokenCache"
  */
 export const bearerTokenAuthenticationPolicyName = "bearerTokenAuthenticationPolicy";
 
+/**
+ * The processing of the challenge should return in any (or both) of these properties.
+ */
+export interface BearerTokenChallengeResult {
+  /**
+   * Scopes to overwrite during the get token request.
+   */
+  scopes?: string[];
+  /**
+   * Additional claims to be included in the token.
+   * For more information on format and content: [the claims parameter specification](href="https://openid.net/specs/openid-connect-core-1_0-final.html#ClaimsParameter).
+   */
+  claims?: string;
+}
+
 /**
  * Options to configure the bearerTokenAuthenticationPolicy
  */
@@ -48,7 +58,7 @@ export interface BearerTokenAuthenticationPolicyOptions {
     /**
      * Updates  the authentication context based on the challenge.
      */
-    processChallenge(challenge: string): Promise<AuthenticationContext | undefined>;
+    processChallenge(challenge: string): Promise<BearerTokenChallengeResult | undefined>;
   };
 }
 
@@ -57,9 +67,9 @@ export interface BearerTokenAuthenticationPolicyOptions {
  * and if the response contained the header "WWW-Authenticate" with a non-empty value.
  */
 function defaultGetChallenge(response: PipelineResponse): string | undefined {
-  const challenges = response.headers.get("WWW-Authenticate");
-  if (response.status === 401 && challenges) {
-    return challenges;
+  const challenge = response.headers.get("WWW-Authenticate");
+  if (response.status === 401 && challenge) {
+    return challenge;
   }
   return;
 }
@@ -71,18 +81,17 @@ function defaultGetChallenge(response: PipelineResponse): string | undefined {
 export function bearerTokenAuthenticationPolicy(
   options: BearerTokenAuthenticationPolicyOptions
 ): PipelinePolicy {
-  const { credential, scopes } = options;
+  const { credential, scopes, challenge } = options;
   const tokenCache: AccessTokenCache = new ExpiringAccessTokenCache();
-  const { prepareRequest, getChallenge = defaultGetChallenge, processChallenge } =
-    options.challenge || {};
+  const { prepareRequest, getChallenge = defaultGetChallenge, processChallenge } = challenge ?? {};
 
   /**
-   * getToken will call to the underlying credential's getToken request with properties coming from the request,
+   * retrieveToken will call to the underlying credential's getToken request with properties coming from the request,
    * as well as properties coming from parsing the CAE challenge, if any.
    */
-  async function getToken(
+  async function retrieveToken(
     request: PipelineRequest,
-    context?: AuthenticationContext
+    context?: BearerTokenChallengeResult
   ): Promise<string | undefined> {
     let accessToken = tokenCache.getCachedToken();
     const getTokenOptions: GetTokenOptions = {
@@ -104,13 +113,16 @@ export function bearerTokenAuthenticationPolicy(
    * Populates the token in the request headers.
    */
   function assignToken(request: PipelineRequest, token?: string): PipelineRequest {
-    request.headers.set("Authorization", `Bearer ${token}`);
+    if (token) {
+      request.headers.set("Authorization", `Bearer ${token}`);
+    }
     return request;
   }
 
   /**
    * Uses the challenge parameters to:
    * - Prepare the outgoing request (if the `prepareRequest` method has been provided).
+   * - Send an initial request to receive the challenge if it fails.
    * - Process a challenge if the response contains it.
    * - Retrieve a token with the challenge information, then re-send the request.
    */
@@ -121,24 +133,31 @@ export function bearerTokenAuthenticationPolicy(
     if (prepareRequest) {
       await prepareRequest(request);
     }
+
     const response = await next(request);
     const challenge = getChallenge(response);
+
     if (challenge && processChallenge) {
       const context = await processChallenge(challenge);
-      const token = await getToken(request, context);
+      const token = await retrieveToken(request, context);
       return next(assignToken(request, token));
     }
     return response;
   }
 
   return {
     name: bearerTokenAuthenticationPolicyName,
+    /**
+     * If there's no cached token and we have challenge options, this policy will try to authenticate using the challenges.
+     * If there's an access token in the cache, it will avoid any challenge processing.
+     */
     async sendRequest(request: PipelineRequest, next: SendRequest): Promise<PipelineResponse> {
-      const token = await getToken(request);
-      if (token) {
-        return next(assignToken(request, token));
-      } else {
+      let accessToken = tokenCache.getCachedToken();
+      if (!accessToken && processChallenge) {
         return await challengePolicy(request, next);
+      } else {
+        const token = await retrieveToken(request);
+        return next(assignToken(request, token));
       }
     }
   };

sdk/core/core-https/test/node/bearerTokenAuthenticationPolicyChallenge.spec.ts

@@ -11,7 +11,7 @@ import {
   HttpsClient,
   PipelineResponse
 } from "../../src";
-import { AuthenticationContext } from "../../src/interfaces";
+import { BearerTokenChallengeResult } from "../../src/policies/bearerTokenAuthenticationPolicy";
 
 export interface TestChallenge {
   scope: string;
@@ -63,7 +63,7 @@ function parseCAEChallenge(challenges: string): any[] {
     );
 }
 
-async function processChallenge(challenge: string): Promise<AuthenticationContext> {
+async function processChallenge(challenge: string): Promise<BearerTokenChallengeResult> {
   const challenges: TestChallenge[] = parseCAEChallenge(challenge) || [];
 
   const parsedChallenge = challenges.find((x) => x.claims);
@@ -83,16 +83,16 @@ async function processChallenge(challenge: string): Promise<AuthenticationContex
 class MockRefreshAzureCredential implements TokenCredential {
   public authCount = 0;
   public scopesAndClaims: { scope: string | string[]; challengeClaims: string | undefined }[] = [];
-  public tokens: (AccessToken | null)[];
+  public getTokenResponses: (AccessToken | null)[];
 
-  constructor(tokens: (AccessToken | null)[]) {
-    this.tokens = tokens;
+  constructor(getTokenResponses: (AccessToken | null)[]) {
+    this.getTokenResponses = getTokenResponses;
   }
 
   public getToken(scope: string | string[], options: GetTokenOptions): Promise<AccessToken | null> {
     this.authCount++;
     this.scopesAndClaims.push({ scope, challengeClaims: options.claims });
-    return Promise.resolve(this.tokens.shift()!);
+    return Promise.resolve(this.getTokenResponses.shift()!);
   }
 }
 
@@ -124,23 +124,25 @@ describe("bearerTokenAuthenticationPolicy with challenge", function() {
     ];
 
     const expiresOn = Date.now() + 5000;
-    const tokens = [null, { token: "mock-token", expiresOnTimestamp: expiresOn }];
-    const credential = new MockRefreshAzureCredential(tokens);
+    const getTokenResponse = { token: "mock-token", expiresOnTimestamp: expiresOn };
+    const credential = new MockRefreshAzureCredential([getTokenResponse]);
 
     const pipeline = createEmptyPipeline();
-    const policy = bearerTokenAuthenticationPolicy({
+    const bearerPolicy = bearerTokenAuthenticationPolicy({
       // Intentionally left empty, as it should be replaced by the challenge.
       scopes: "",
       credential,
       challenge: {
         processChallenge
       }
     });
+    pipeline.addPolicy(bearerPolicy);
 
-    pipeline.addPolicy(policy);
+    const finalSendRequestHeaders: (string | undefined)[] = [];
 
     const testHttpsClient: HttpsClient = {
       sendRequest: async (req) => {
+        finalSendRequestHeaders.push(req.headers.get("Authorization"));
         if (responses.length) {
           const response = responses.shift()!;
           response.request = req;
@@ -152,16 +154,20 @@ describe("bearerTokenAuthenticationPolicy with challenge", function() {
 
     await pipeline.sendRequest(testHttpsClient, request);
 
-    assert.equal(credential.authCount, 2);
+    // Our goal is to test that:
+    // - Only one getToken request was sent.
+    // - That the only getToken request contained the scope and the claims of the challenge.
+    // - That the HTTP requests that were sent were:
+    //   - Once without the token, to retrieve the challenge.
+    //   - A final one with the token.
+
+    assert.equal(credential.authCount, 1);
     assert.deepEqual(credential.scopesAndClaims, [
-      {
-        scope: "",
-        challengeClaims: undefined
-      },
       {
         scope: [expected.scope],
         challengeClaims: expected.challengeClaims
       }
     ]);
+    assert.deepEqual(finalSendRequestHeaders, [undefined, `Bearer ${getTokenResponse.token}`]);
   });
 });