[KeyVault] Challenge based authentication parallel fix, including tests

sdk/keyvault/keyvault-certificates/.eslintignore

@@ -0,0 +1,2 @@
+# Ignoring the core files since they're auto-generated. Eventually, the auto-generated code will be on par with our current eslint rules, but in the mean time we should ignore them.
+src/core

sdk/keyvault/keyvault-certificates/.prettierignore

@@ -1 +1,2 @@
-src/core
+/src/core/*
+!/src/core/challengeBasedAuthenticationPolicy.ts

sdk/keyvault/keyvault-certificates/CHANGELOG.md

@@ -6,6 +6,7 @@
 - `KVPollerLike` is now an alias of `PollerLike`.
 - `KVPollerLike` is considered deprecated. Use `PollerLike`.
 - Fixed [bug 8378](https://github.com/Azure/azure-sdk-for-js/issues/8378), which caused the challenge based authentication to re-authenticate on every new request.
+- Fixed [bug 9005](https://github.com/Azure/azure-sdk-for-js/issues/9005), which caused parallel requests to throw if one of them needed to authenticate.
 - Fixed [bug 9020](https://github.com/Azure/azure-sdk-for-js/issues/9020), which caused updateCertificateProperties to not properly send the certificate attributes to the service.
 
 ## 4.0.1 (2020-05-13)

sdk/keyvault/keyvault-certificates/package.json

@@ -108,6 +108,7 @@
     "@types/mocha": "^7.0.2",
     "@types/node": "^8.0.0",
     "@types/query-string": "6.2.0",
+    "@types/sinon": "^9.0.4",
     "@typescript-eslint/eslint-plugin": "^2.0.0",
     "@typescript-eslint/parser": "^2.0.0",
     "assert": "^1.4.1",
@@ -146,6 +147,7 @@
     "rollup-plugin-sourcemaps": "^0.4.2",
     "rollup-plugin-terser": "^5.1.1",
     "rollup-plugin-visualizer": "^3.1.1",
+    "sinon": "^9.0.2",
     "source-map-support": "^0.5.9",
     "typescript": "~3.8.3",
     "uglify-js": "^3.4.9",

sdk/keyvault/keyvault-certificates/rollup.base.config.js

@@ -121,7 +121,7 @@ export function browserConfig(test = false) {
       }),
       cjs({
         namedExports: {
-          assert: ["ok", "equal", "strictEqual"],
+          assert: ["ok", "equal", "strictEqual", "deepEqual"],
           "@opentelemetry/api": ["CanonicalCode", "SpanKind", "TraceFlags"]
         }
       })

sdk/keyvault/keyvault-certificates/src/core/challengeBasedAuthenticationPolicy.ts

@@ -2,29 +2,48 @@
 // Licensed under the MIT License. See License.txt in the project root for license information.
 
 import { TokenCredential } from "@azure/core-http";
-import { BaseRequestPolicy, RequestPolicy, RequestPolicyOptions, RequestPolicyFactory } from "@azure/core-http";
+import {
+  BaseRequestPolicy,
+  RequestPolicy,
+  RequestPolicyOptions,
+  RequestPolicyFactory
+} from "@azure/core-http";
 import { Constants } from "@azure/core-http";
 import { HttpOperationResponse } from "@azure/core-http";
-import { HttpHeaders } from "@azure/core-http";
 import { WebResource } from "@azure/core-http";
 import { AccessTokenCache, ExpiringAccessTokenCache } from "@azure/core-http";
 
+type ValidParsedWWWAuthenticateProperties =
+  // "authorization_uri" was used in the track 1 version of KeyVault.
+  // This is not a relevant property anymore, since the service is consistently answering with "authorization".
+  // | "authorization_uri"
+  | "authorization"
+  // Even though the service is moving to "scope", both "resource" and "scope" should be supported.
+  | "resource"
+  | "scope";
+
+type ParsedWWWAuthenticate = {
+  [Key in ValidParsedWWWAuthenticateProperties]?: string;
+};
+
 /**
  * Representation of the Authentication Challenge
  */
 export class AuthenticationChallenge {
-  constructor(public authorization: string, public scope: string) {
-  }
+  constructor(public authorization: string, public scope: string) {}
 
   /**
    * Checks that this AuthenticationChallenge is equal to another one given.
+   * Only compares the scope.
+   * This is exactly what C# is doing, as we can see here:
+   * https://github.com/Azure/azure-sdk-for-net/blob/70e54b878ff1d01a45266fb3674a396b4ab9c1d2/sdk/keyvault/Azure.Security.KeyVault.Shared/src/ChallengeBasedAuthenticationPolicy.cs#L143-L147
    * @param other The other AuthenticationChallenge
    */
   public equalTo(other: AuthenticationChallenge | undefined) {
-    if (!other) {
-      return false;
-    }
-    return this.authorization === other.authorization && this.scope === other.scope;
+    return other
+      ? this.scope.toLowerCase() === other.scope.toLowerCase() &&
+          this.authorization.toLowerCase() === other.authorization.toLowerCase()
+      : false;
   }
 }
 
@@ -45,16 +64,52 @@ export class AuthenticationChallengeCache {
  *
  * @param credential The TokenCredential implementation that can supply the challenge token.
  */
-export function challengeBasedAuthenticationPolicy(credential: TokenCredential): RequestPolicyFactory {
+export function challengeBasedAuthenticationPolicy(
+  credential: TokenCredential
+): RequestPolicyFactory {
   const tokenCache: AccessTokenCache = new ExpiringAccessTokenCache();
   const challengeCache = new AuthenticationChallengeCache();
   return {
     create: (nextPolicy: RequestPolicy, options: RequestPolicyOptions) => {
-      return new ChallengeBasedAuthenticationPolicy(nextPolicy, options, credential, tokenCache, challengeCache);
+      return new ChallengeBasedAuthenticationPolicy(
+        nextPolicy,
+        options,
+        credential,
+        tokenCache,
+        challengeCache
+      );
     }
   };
 }
 
+/**
+ * Parses an WWW-Authenticate response.
+ * This transforms a string value like:
+ * `Bearer authorization="some_authorization", resource="https://some.url"`
+ * into an object like:
+ * `{ authorization: "some_authorization", resource: "https://some.url" }`
+ * @param wwwAuthenticate string value in the WWW-Authenticate header
+ */
+export function parseWWWAuthenticate(wwwAuthenticate: string): ParsedWWWAuthenticate {
+  // First we split the string by either `, ` or ` `.
+  const parts = wwwAuthenticate.split(/,* +/);
+  // Then we only keep the strings with an equal sign after a word and before a quote.
+  // also splitting these sections by their equal sign
+  const keyValues = parts.reduce<string[][]>(
+    (parts, str) => (str.match(/\w="/) ? [...parts, str.split("=")] : parts),
+    []
+  );
+  // Then we transform these key-value pairs back into an object.
+  const parsed = keyValues.reduce<ParsedWWWAuthenticate>(
+    (result, [key, value]: string[]) => ({
+      ...result,
+      [key]: value.slice(1, -1)
+    }),
+    {}
+  );
+  return parsed;
+}
+
 /**
  *
  * Provides a RequestPolicy that can request a token from a TokenCredential
@@ -63,6 +118,9 @@ export function challengeBasedAuthenticationPolicy(credential: TokenCredential):
  *
  */
 export class ChallengeBasedAuthenticationPolicy extends BaseRequestPolicy {
+  private parseWWWAuthenticate: (
+    wwwAuthenticate: string
+  ) => ParsedWWWAuthenticate = parseWWWAuthenticate;
 
   /**
    * Creates a new ChallengeBasedAuthenticationPolicy object.
@@ -82,102 +140,102 @@ export class ChallengeBasedAuthenticationPolicy extends BaseRequestPolicy {
     super(nextPolicy, options);
   }
 
-  private parseWWWAuthenticate(www_authenticate: string): {
-    authorization: string,
-    resource: string
-  } {
-    const returnValue = {
-      authorization: "",
-      resource: ""
-    };
-    // Parses an authentication message like:
-    // ```
-    // Bearer authorization="some_authorization", resource="https://some.url"
-    // ```
-    let spaceSep = www_authenticate.split(" ");
-
-    // Split the KV comma-separated list
-    for (const spaceItem of spaceSep) {
-      const commaSep = spaceItem.split(",");
-      for (const commaItem of commaSep) {
-        // Split the key/value pairs
-        const kv = commaItem.split("=");
-        const key = kv[0].trim();
-        const removeQuotes = (x: string): string => x.trim().replace(/['"]+/g, '');
-        if (key == "authorization" || key == "authorization_uri") {
-          returnValue.authorization = removeQuotes(kv[1]);
-        } else if (key == "resource" || key == "scope") {
-          returnValue.resource = removeQuotes(kv[1]);
-        }
-      }
+  /**
+   * Gets or updates the token from the token cache into the headers of the received web resource.
+   */
+  private async loadToken(webResource: WebResource): Promise<void> {
+    let accessToken = this.tokenCache.getCachedToken();
+
+    // If there's no cached token in the cache, we try to get a new one.
+    if (accessToken === undefined) {
+      const receivedToken = await this.credential.getToken(this.challengeCache.challenge!.scope);
+      accessToken = receivedToken || undefined;
+      this.tokenCache.setCachedToken(accessToken);
+    }
+
+    if (accessToken) {
+      webResource.headers.set(
+        Constants.HeaderConstants.AUTHORIZATION,
+        `Bearer ${accessToken.token}`
+      );
     }
-    return returnValue;
   }
 
   /**
-   * Applies the Bearer token to the request through the Authorization header.
-   * @param webResource
+   * Parses the given WWW-Authenticate header, generates a new AuthenticationChallenge,
+   * then if the challenge is different from the one cached, resets the token and forces
+   * a re-authentication, otherwise continues with the existing challenge and token.
+   * @param wwwAuthenticate Value of the incoming WWW-Authenticate header.
+   * @param webResource Ongoing HTTP request.
    */
-  public async sendRequest(
+  private async regenerateChallenge(
+    wwwAuthenticate: string,
     webResource: WebResource
   ): Promise<HttpOperationResponse> {
-    if (!webResource.headers) webResource.headers = new HttpHeaders();
+    // The challenge based authentication will contain both:
+    // - An authorization URI with a token,
+    // - The resource to which that token is valid against (also called the scope).
+    const parsedWWWAuth = this.parseWWWAuthenticate(wwwAuthenticate);
+    const authorization = parsedWWWAuth.authorization!;
+    const resource = parsedWWWAuth.resource! || parsedWWWAuth.scope!;
+
+    if (!(authorization && resource)) {
+      return this._nextPolicy.sendRequest(webResource);
+    }
+
+    const challenge = new AuthenticationChallenge(authorization, resource + "/.default");
+
+    // Either if there's no cached challenge at this point (could have happen in parallel),
+    // or if the cached challenge has a different scope,
+    // we store the just received challenge and reset the cached token, to force a re-authentication.
+    if (!this.challengeCache.challenge?.equalTo(challenge)) {
+      this.challengeCache.setCachedChallenge(challenge);
+      this.tokenCache.setCachedToken(undefined);
+    }
+
+    await this.loadToken(webResource);
+    return this._nextPolicy.sendRequest(webResource);
+  }
 
-    // Ensure that we're about to use a secure connection
+  /**
+   * Applies the Bearer token to the request through the Authorization header.
+   * @param webResource Ongoing HTTP request.
+   */
+  public async sendRequest(webResource: WebResource): Promise<HttpOperationResponse> {
+    // Ensure that we're about to use a secure connection.
     if (!webResource.url.startsWith("https:")) {
       throw new Error("The resource address for authorization must use the 'https' protocol.");
     }
 
-    const originalBody = webResource.body;
+    // The next request will happen differently whether we have a challenge or not.
+    let response: HttpOperationResponse;
 
     if (this.challengeCache.challenge == undefined) {
-      // Use a blank to start the challenge
+      // If there's no challenge in cache, a blank body will start the challenge.
+      const originalBody = webResource.body;
       webResource.body = "";
+      response = await this._nextPolicy.sendRequest(webResource);
+      webResource.body = originalBody;
     } else {
-      // or use the cached token if we have one
-      await this.authenticateRequest(webResource);
+      // If we did have a challenge in memory,
+      // we attempt to load the token from the cache into the request before we try to send the request.
+      await this.loadToken(webResource);
+      response = await this._nextPolicy.sendRequest(webResource);
     }
 
-    const response = await this._nextPolicy.sendRequest(webResource);
-
-    if (response.status == 401) {
-      webResource.body = originalBody;
-
-      let www_authenticate = response.headers.get("WWW-Authenticate");
-
-      if (www_authenticate) {
-        // The challenge based authentication will contain both an authorization URI with a token,
-        // and the resource to which that token is valid against (also called the scope).
-        const { authorization, resource } = this.parseWWWAuthenticate(www_authenticate);
-        const challenge = new AuthenticationChallenge(authorization, resource + "/.default")
-
-        if (!challenge.equalTo(this.challengeCache.challenge)) {
-          this.challengeCache.setCachedChallenge(challenge);
-          this.tokenCache.setCachedToken(undefined);
-
-          await this.authenticateRequest(webResource);
-          return this._nextPolicy.sendRequest(webResource);
-        }
-        return response;
-      }
-      return response;
-    } else {
+    // If we don't receive a response with a 401 status code,
+    // then we can assume this response has nothing to do with the challenge authentication process.
+    if (response.status !== 401) {
       return response;
     }
-  }
 
-  private async authenticateRequest(webResource: WebResource): Promise<void> {
-    let accessToken = this.tokenCache.getCachedToken();
-    if (accessToken === undefined) {
-      accessToken = (await this.credential.getToken(this.challengeCache.challenge!.scope)) || undefined;
-      this.tokenCache.setCachedToken(accessToken);
+    // If the response status is 401, we only re-authenticate if the WWW-Authenticate header is present.
+    const wwwAuthenticate = response.headers.get("WWW-Authenticate");
+    if (!wwwAuthenticate) {
+      return response;
     }
 
-    if (accessToken) {
-      webResource.headers.set(
-        Constants.HeaderConstants.AUTHORIZATION,
-        `Bearer ${accessToken.token}`
-      );
-    }
+    // We re-generate the challenge and see if we have to re-authenticate.
+    return await this.regenerateChallenge(wwwAuthenticate, webResource);
   }
 }