Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the CAE bug in js core lib #22324

Merged
merged 16 commits into from
Jul 8, 2022
1 change: 1 addition & 0 deletions sdk/core/core-auth/review/core-auth.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export class AzureSASCredential implements SASCredential {
// @public
export interface GetTokenOptions {
abortSignal?: AbortSignalLike;
claims?: string;
requestOptions?: {
timeout?: number;
};
Expand Down
5 changes: 5 additions & 0 deletions sdk/core/core-auth/src/tokenCredential.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the claims we get from the challenge right? We are already passing it in authorizeRequestOnClaimChallenge callback but forgot to update this interface when the callback was originally added

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes exactly! This parameter is already passed in code

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Thanks for adding it! Could you please add an entry to core-auth CHANGELOG?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to also remove the "as GetTokenOptions" cast https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/core/core-client/src/authorizeRequestOnClaimChallenge.ts#L87 as it is no longer needed.

Copy link
Member Author

@MaryGao MaryGao Jul 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

}

/**
Expand Down
2 changes: 1 addition & 1 deletion sdk/core/core-rest-pipeline/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@azure/core-rest-pipeline",
"version": "1.9.1",
"version": "1.9.2",
"description": "Isomorphic client library for making HTTP requests in node.js and browser.",
"sdk-type": "client",
"main": "dist/index.js",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down
8 changes: 5 additions & 3 deletions sdk/core/core-rest-pipeline/src/util/tokenCycler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so whenever we see claims, we know we have to refresh? Can we add a test with claims and one without to make sure those booleans are covered?

Copy link
Member Author

@MaryGao MaryGao Jul 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test to cover that


if (mustRefresh) return refresh(scopes, tokenOptions);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export interface TestChallenge {
}

let cachedChallenge: string | undefined;
let cachedPreviousToken: AccessToken | null;

/**
* Converts a uint8Array to a string.
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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: [],
Expand All @@ -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,
Expand All @@ -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 with 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:
Expand All @@ -334,7 +349,8 @@ describe("bearerTokenAuthenticationPolicy with challenge", function () {
undefined,
`Bearer ${getTokenResponses[0].token}`,
`Bearer ${getTokenResponses[1].token}`,
`Bearer ${getTokenResponses[1].token}`,
`Bearer ${getTokenResponses[2].token}`,
`Bearer ${getTokenResponses[2].token}`,
]);
});

Expand Down