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.

Sign up for GitHub

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

Jump to bottom

Fix the CAE bug in js core lib #22324

Merged
merged 16 commits into from
Jul 8, 2022
Merged

Fix the CAE bug in js core lib #22324

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
6cf9c13
Set up CI with Azure Pipelines
MaryGao Jan 24, 2022
c82a5dd
Merge branch 'main' of https://github.com/MaryGao/azure-sdk-for-js
MaryGao Jun 21, 2022
6d67189
UPdate changes
MaryGao Jun 21, 2022
85b4f60
Bugfix
MaryGao Jun 22, 2022
dff1663
Delete useless file
MaryGao Jul 4, 2022
4bc68c2
Update the test cases
MaryGao Jul 4, 2022
a05c61f
Format code
MaryGao Jul 4, 2022
c2087ab
Update lint error
MaryGao Jul 4, 2022
0fdfc78
bump a newer version
MaryGao Jul 5, 2022
93ecd56
Remove useless renaming
MaryGao Jul 6, 2022
832c5d9
Bump patch version
MaryGao Jul 6, 2022
93fd378
Update changelog
MaryGao Jul 6, 2022
fcbe942
Update changelog
MaryGao Jul 6, 2022
35f36e7
Update a test case
MaryGao Jul 6, 2022
15b4d31
Update ci failure
MaryGao Jul 6, 2022
1f89098
Update format
MaryGao Jul 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions azure-pipelines.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Node.js
# Build a general Node.js project with npm.
# Add steps that analyze code, save build artifacts, deploy, and more:
# https://docs.microsoft.com/azure/devops/pipelines/languages/javascript
MaryGao marked this conversation as resolved.
Show resolved Hide resolved

trigger:
- main

pool:
vmImage: ubuntu-latest

steps:
- task: NodeTool@0
inputs:
versionSpec: '10.x'
displayName: 'Install Node.js'

- script: |
npm install
npm run build
displayName: 'npm install and build'
1 change: 1 addition & 0 deletions sdk/core/core-auth/review/core-auth.api.md
View file
Open in desktop
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
View file
Open in desktop
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 •
edited
Loading

Choose a reason for hiding this comment

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

Updated

}

/**
Expand Down
4 changes: 3 additions & 1 deletion sdk/core/core-rest-pipeline/src/policies/bearerTokenAuthenticationPolicy.ts
View file
Open in desktop
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
7 changes: 4 additions & 3 deletions sdk/core/core-rest-pipeline/src/util/tokenCycler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,9 +201,10 @@ 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 || !!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.

@jeremymeng, I'm wondering if we should have an option that can set mustRefresh to true instead of adding more logic around specific contents of the token in the cycler. This way the individual callbacks can make the decision about forcing refresh or not. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

It is usually clearer if you do Boolean(tokenOptions.claims)

Copy link
Member

Choose a reason for hiding this comment

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

@MaryGao can you please add some tests to validate this behavior? I would like to see specifically a test that validates that the token doesn't get refreshed when there is no need to refresh but we still have claims

Copy link
Member Author

@MaryGao MaryGao Jul 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@joheredi Currently I just assume that we must refresh the token if claim details provides. Do you mean we need to support your cases?

Update below existing test cases:

    // Case1: Will refresh token once as the first time token is empty 
    await pipeline.sendRequest(testHttpsClient, pipelineRequest);
    clock.tick(5000);
    // Case2: 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);
    // Case3: Token is still valid and no need to refresh it
    await pipeline.sendRequest(testHttpsClient, pipelineRequest);


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

Expand Down