[Identity] Support for tenant Id Challenges / tenant discovery in ClientCredentials #15837

sadasant · 2021-06-18T01:57:48Z

This PR adds tenantId to the getTokenOptions, and adds options on every Identity credential to allow multi-tenant authentication (which will be disabled by default).

Fixes #15797

…scovery in ClientCredentials

sdk/identity/identity/review/identity.api.md

sdk/identity/identity/src/util/validateMultiTenant.ts

sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts

sdk/core/core-auth/CHANGELOG.md

sdk/identity/identity/CHANGELOG.md

schaabs · 2021-06-29T16:16:50Z

sdk/identity/identity/review/identity.api.md

@@ -314,7 +327,7 @@ export interface UsernamePasswordCredentialOptions extends TokenCredentialOption
 // @public
 export class VisualStudioCodeCredential implements TokenCredential {
    constructor(options?: VisualStudioCodeCredentialOptions);
-    getToken(scopes: string | string[], _options?: GetTokenOptions): Promise<AccessToken>;
+    getToken(scopes: string | string[], options?: GetTokenOptions): Promise<AccessToken>;


Is this a breaking change? Can callers specify the argument name explicitly?

This is not a breaking change, just a name change :)

Callers can’t specify the argument names like this, they just pass the value.

thankfully we don't have magical kwargs :)

Co-authored-by: Scott Schaab <sschaab@microsoft.com>

schaabs

LGTM, thanks for the quick turnaround! Please get an approver from the JS team as well.

witemple-msft

I have a couple of questions in the comments, but LGTM from an API/JS/TS perspective.

sdk/identity/identity/src/msal/browserFlows/browserCommon.ts

sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts

sdk/identity/identity/test/internal/utils.spec.ts

sdk/identity/identity/CHANGELOG.md

xirzec

The public surface changes look good!

I have some remarks about the implementation

xirzec · 2021-06-30T17:55:19Z

sdk/identity/identity/review/identity.api.md

@@ -314,7 +327,7 @@ export interface UsernamePasswordCredentialOptions extends TokenCredentialOption
 // @public
 export class VisualStudioCodeCredential implements TokenCredential {
    constructor(options?: VisualStudioCodeCredentialOptions);
-    getToken(scopes: string | string[], _options?: GetTokenOptions): Promise<AccessToken>;
+    getToken(scopes: string | string[], options?: GetTokenOptions): Promise<AccessToken>;


thankfully we don't have magical kwargs :)

sdk/identity/identity/src/credentials/azureCliCredential.ts

sdk/identity/identity/src/credentials/azurePowerShellCredential.ts

sdk/identity/identity/src/util/validateMultiTenant.ts

sdk/identity/identity/CHANGELOG.md

Co-authored-by: chradek <51000525+chradek@users.noreply.github.com>

sdk/identity/identity/src/credentials/authorizationCodeCredential.ts

chradek · 2021-06-30T20:32:57Z

sdk/identity/identity/src/credentials/authorizationCodeCredential.ts

+    const tenantId = processMultiTenantRequest(
+      this.tenantId,
+      this.allowMultiTenantAuthentication,
+      options
+    )!;


I see you're using the non-null assertion. When would processMultiTenantRequest return undefined where that would be ok? We've been bitten by the non-null assertion in other packages and I wonder if it makes more sense to throw an error instead, or at least handle the undefined case.

The tenantId has a default for this credential, which is common. It’s set on the credential options. processMultiTenantRequest will only pick the getToken options’ tenant if it exists, so either it exists in the options, or is assigned by the user, or is just common. Would a comment help?

Oh I see. I can throw an error just in case

I was hoping we could do some TypeScript magic like this:

export function processMultiTenantRequest<T extends string | undefined>( tenantId: T, allowMultiTenantAuthentication?: boolean, getTokenOptions?: GetTokenOptions ): T extends string ? string : string | undefined { if ( !allowMultiTenantAuthentication && getTokenOptions?.tenantId && tenantId && getTokenOptions.tenantId !== tenantId ) { throw new Error(multiTenantErrorMessage); } if (allowMultiTenantAuthentication && getTokenOptions?.tenantId) { return getTokenOptions.tenantId; } return tenantId as any; }

That works but I had to resort to casting the final return as any.

Anyway, it looks like you're really only doing non-null assertion here, I see now your other calls handle undefined. With that said, maybe just add a comment so someone running across it will know this.tenantId is always available here.

ok I’ll add a comment!

Oh look, I can do this!

const tenantId = processMultiTenantRequest( this.tenantId, this.allowMultiTenantAuthentication, options ) || this.tenantId;

Safer ^_^

ghost · 2021-06-30T21:13:54Z

Hello @sadasant!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Add swagger for Compute Diagnostic Resource Provider (Azure#15837)

[Identity] Draft for the support for tenant Id Challenges / tenant di…

b267aff

…scovery in ClientCredentials

sadasant requested review from christothes and schaabs June 18, 2021 01:57

sadasant self-assigned this Jun 18, 2021

ghost added Azure.Core Azure.Identity labels Jun 18, 2021

This comment has been minimized.

Sign in to view

christothes reviewed Jun 18, 2021

View reviewed changes

sdk/identity/identity/review/identity.api.md Outdated Show resolved Hide resolved

sadasant added 4 commits June 18, 2021 20:39

all the credentials now

3f1cbbf

forgot about AzurePowerShellCredentialOptions

b1f2e64

a unit test

334df01

Merge remote-tracking branch 'Azure/main' into identity/fix15797

b588070

christothes reviewed Jun 22, 2021

View reviewed changes

sdk/identity/identity/review/identity.api.md Outdated Show resolved Hide resolved

sadasant added 4 commits June 22, 2021 20:10

moved allowMultiTenantAuthentication to the GetTokenOptions

dc1ea9e

added validateMultiTenantRequest on browserCommon

c8adf94

Merge remote-tracking branch 'Azure/main' into identity/fix15797

33bbaa2

lint fix

7be7756

christothes reviewed Jun 24, 2021

View reviewed changes

sdk/identity/identity/src/util/validateMultiTenant.ts Outdated Show resolved Hide resolved

sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts Show resolved Hide resolved

sadasant marked this pull request as ready for review June 25, 2021 21:35

sadasant requested review from bterlson, chradek, deyaaeldeen, jeremymeng, witemple-msft and xirzec as code owners June 25, 2021 21:35

sadasant changed the title ~~[Identity] Draft for the support for tenant Id Challenges / tenant discovery in ClientCredentials~~ [Identity] Support for tenant Id Challenges / tenant discovery in ClientCredentials Jun 25, 2021

sadasant added 3 commits June 25, 2021 21:59

asiggning the tenantId where it should go

0dacea8

Merge remote-tracking branch 'Azure/main' into identity/fix15797

a871027

better tests

d252fbe

sadasant requested a review from christothes June 28, 2021 13:51

schaabs reviewed Jun 29, 2021

View reviewed changes

sdk/core/core-auth/CHANGELOG.md Outdated Show resolved Hide resolved

schaabs reviewed Jun 29, 2021

View reviewed changes

sdk/identity/identity/CHANGELOG.md Outdated Show resolved Hide resolved

schaabs reviewed Jun 29, 2021

View reviewed changes

sadasant and others added 4 commits June 29, 2021 12:49

Update sdk/identity/identity/CHANGELOG.md

e39736e

Co-authored-by: Scott Schaab <sschaab@microsoft.com>

core-auth CHANGELOG update

5257305

getAuthorityHost -> getAuthority

b3e30cd

Merge remote-tracking branch 'Azure/main' into identity/fix15797

d05186d

schaabs approved these changes Jun 29, 2021

View reviewed changes

witemple-msft approved these changes Jun 29, 2021

View reviewed changes

sadasant added 2 commits June 30, 2021 15:02

Merge remote-tracking branch 'Azure/main' into identity/fix15797

cbb48dc

options.authority feedback from Will

09a7b56

xirzec reviewed Jun 30, 2021

View reviewed changes

sdk/identity/identity/CHANGELOG.md Outdated Show resolved Hide resolved

xirzec reviewed Jun 30, 2021

View reviewed changes

sadasant added 2 commits June 30, 2021 18:24

validating tenants if provided to the CLI and the powershell credential

b329866

validateMultiTenant error message

0983549

sadasant requested a review from xirzec June 30, 2021 18:36

chradek reviewed Jun 30, 2021

View reviewed changes

sdk/identity/identity/CHANGELOG.md Outdated Show resolved Hide resolved

Update sdk/identity/identity/CHANGELOG.md

f49f32b

Co-authored-by: chradek <51000525+chradek@users.noreply.github.com>

chradek suggested changes Jun 30, 2021

View reviewed changes

sdk/identity/identity/src/credentials/authorizationCodeCredential.ts Outdated Show resolved Hide resolved

feedback by Christopher Radek

723f825

sadasant requested a review from chradek June 30, 2021 20:27

chradek reviewed Jun 30, 2021

View reviewed changes

chradek approved these changes Jun 30, 2021

View reviewed changes

safer than non-null assertions

e63a14b

sadasant added the auto-merge label Jun 30, 2021

ghost merged commit d8407ad into Azure:main Jun 30, 2021

sadasant deleted the identity/fix15797 branch June 30, 2021 23:25

openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Sep 1, 2021

CodeGen from PR 15837 in Azure/azure-rest-api-specs

d4d24ab

Add swagger for Compute Diagnostic Resource Provider (Azure#15837)

sadasant mentioned this pull request May 10, 2022

[KeyVault] Keyvault access is broken in newer version on Managed Identity #21798

Closed

6 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Identity] Support for tenant Id Challenges / tenant discovery in ClientCredentials #15837

[Identity] Support for tenant Id Challenges / tenant discovery in ClientCredentials #15837

sadasant commented Jun 18, 2021 •

edited

Loading

This comment has been minimized.

schaabs Jun 29, 2021

sadasant Jun 29, 2021

sadasant Jun 29, 2021

xirzec Jun 30, 2021

schaabs left a comment •

edited

Loading

witemple-msft left a comment

xirzec left a comment

xirzec Jun 30, 2021

chradek Jun 30, 2021

sadasant Jun 30, 2021

sadasant Jun 30, 2021

chradek Jun 30, 2021

sadasant Jun 30, 2021

sadasant Jun 30, 2021

ghost commented Jun 30, 2021

[Identity] Support for tenant Id Challenges / tenant discovery in ClientCredentials #15837

[Identity] Support for tenant Id Challenges / tenant discovery in ClientCredentials #15837

Conversation

sadasant commented Jun 18, 2021 • edited Loading

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schaabs left a comment • edited Loading

Choose a reason for hiding this comment

witemple-msft left a comment

Choose a reason for hiding this comment

xirzec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 30, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

sadasant commented Jun 18, 2021 •

edited

Loading

schaabs left a comment •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.