From 663d7ebe5d01298ea637a435363f10c6ec892b2d Mon Sep 17 00:00:00 2001 From: Daniel Rodriguez Date: Wed, 3 Mar 2021 14:17:22 +0000 Subject: [PATCH] renamed the authentication context, reduced the number of requests on challenge, improved tests --- sdk/core/core-https/review/core-https.api.md | 14 ++--- sdk/core/core-https/src/index.ts | 6 +- sdk/core/core-https/src/interfaces.ts | 15 ----- .../bearerTokenAuthenticationPolicy.ts | 63 ++++++++++++------- ...TokenAuthenticationPolicyChallenge.spec.ts | 36 ++++++----- 5 files changed, 72 insertions(+), 62 deletions(-) diff --git a/sdk/core/core-https/review/core-https.api.md b/sdk/core/core-https/review/core-https.api.md index 553318e27c24..a76dd3c562c6 100644 --- a/sdk/core/core-https/review/core-https.api.md +++ b/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; getChallenge?(response: PipelineResponse): string | undefined; - processChallenge(challenge: string): Promise; + processChallenge(challenge: string): Promise; }; credential: TokenCredential; scopes: string | string[]; } +// @public +export interface BearerTokenChallengeResult { + claims?: string; + scopes?: string[]; +} + // @public export function createDefaultHttpsClient(): HttpsClient; diff --git a/sdk/core/core-https/src/index.ts b/sdk/core/core-https/src/index.ts index cb50724323e5..f7563fa73332 100644 --- a/sdk/core/core-https/src/index.ts +++ b/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"; diff --git a/sdk/core/core-https/src/interfaces.ts b/sdk/core/core-https/src/interfaces.ts index 6ce5a29372e8..8fbb7b2af9d5 100644 --- a/sdk/core/core-https/src/interfaces.ts +++ b/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; -} diff --git a/sdk/core/core-https/src/policies/bearerTokenAuthenticationPolicy.ts b/sdk/core/core-https/src/policies/bearerTokenAuthenticationPolicy.ts index 3dc7e05610c8..3a14b686eff3 100644 --- a/sdk/core/core-https/src/policies/bearerTokenAuthenticationPolicy.ts +++ b/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; + processChallenge(challenge: string): Promise; }; } @@ -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 { 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,11 +133,13 @@ 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; @@ -133,12 +147,17 @@ export function bearerTokenAuthenticationPolicy( 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 { - 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)); } } }; diff --git a/sdk/core/core-https/test/node/bearerTokenAuthenticationPolicyChallenge.spec.ts b/sdk/core/core-https/test/node/bearerTokenAuthenticationPolicyChallenge.spec.ts index 5d7f0ce4a41a..b7174b68460d 100644 --- a/sdk/core/core-https/test/node/bearerTokenAuthenticationPolicyChallenge.spec.ts +++ b/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 { +async function processChallenge(challenge: string): Promise { const challenges: TestChallenge[] = parseCAEChallenge(challenge) || []; const parsedChallenge = challenges.find((x) => x.claims); @@ -83,16 +83,16 @@ async function processChallenge(challenge: string): Promise { this.authCount++; this.scopesAndClaims.push({ scope, challengeClaims: options.claims }); - return Promise.resolve(this.tokens.shift()!); + return Promise.resolve(this.getTokenResponses.shift()!); } } @@ -124,11 +124,11 @@ 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, @@ -136,11 +136,13 @@ describe("bearerTokenAuthenticationPolicy with challenge", function() { 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}`]); }); });