-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[Identity] Support for tenant Id Challenges / tenant discovery in ClientCredentials #15837
Changes from 23 commits
b267aff
3f1cbbf
b1f2e64
334df01
b588070
dc1ea9e
c8adf94
33bbaa2
7be7756
0dacea8
a871027
d252fbe
fae2ab6
a4946ab
e39736e
5257305
b3e30cd
d05186d
cbb48dc
09a7b56
b329866
0983549
f49f32b
723f825
e63a14b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ import { SpanStatusCode } from "@azure/core-tracing"; | |
import { credentialLogger, formatSuccess, formatError } from "../util/logging"; | ||
import { getIdentityTokenEndpointSuffix } from "../util/identityTokenEndpoint"; | ||
import { checkTenantId } from "../util/checkTenantId"; | ||
import { processMultiTenantRequest } from "../util/validateMultiTenant"; | ||
|
||
const logger = credentialLogger("AuthorizationCodeCredential"); | ||
|
||
|
@@ -30,6 +31,7 @@ export class AuthorizationCodeCredential implements TokenCredential { | |
private authorizationCode: string; | ||
private redirectUri: string; | ||
private lastTokenResponse: TokenResponse | null = null; | ||
private allowMultiTenantAuthentication?: boolean; | ||
|
||
/** | ||
* Creates an instance of CodeFlowCredential with the details needed | ||
|
@@ -105,6 +107,7 @@ export class AuthorizationCodeCredential implements TokenCredential { | |
|
||
this.clientId = clientId; | ||
this.tenantId = tenantId; | ||
this.allowMultiTenantAuthentication = options?.allowMultiTenantAuthentication; | ||
sadasant marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if (typeof redirectUriOrOptions === "string") { | ||
// the clientId+clientSecret constructor | ||
|
@@ -135,6 +138,12 @@ export class AuthorizationCodeCredential implements TokenCredential { | |
scopes: string | string[], | ||
options?: GetTokenOptions | ||
): Promise<AccessToken> { | ||
const tenantId = processMultiTenantRequest( | ||
this.tenantId, | ||
this.allowMultiTenantAuthentication, | ||
options | ||
)!; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see you're using the non-null assertion. When would There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tenantId has a default for this credential, which is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I see. I can throw an error just in case There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok I’ll add a comment! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh look, I can do this! const tenantId = processMultiTenantRequest(
this.tenantId,
this.allowMultiTenantAuthentication,
options
) || this.tenantId; Safer ^_^ |
||
|
||
const { span, updatedOptions } = createSpan("AuthorizationCodeCredential-getToken", options); | ||
try { | ||
let tokenResponse: TokenResponse | null = null; | ||
|
@@ -146,7 +155,7 @@ export class AuthorizationCodeCredential implements TokenCredential { | |
// Try to use the refresh token first | ||
if (this.lastTokenResponse && this.lastTokenResponse.refreshToken) { | ||
tokenResponse = await this.identityClient.refreshAccessToken( | ||
this.tenantId, | ||
tenantId, | ||
this.clientId, | ||
scopeString, | ||
this.lastTokenResponse.refreshToken, | ||
|
@@ -157,9 +166,9 @@ export class AuthorizationCodeCredential implements TokenCredential { | |
} | ||
|
||
if (tokenResponse === null) { | ||
const urlSuffix = getIdentityTokenEndpointSuffix(this.tenantId); | ||
const urlSuffix = getIdentityTokenEndpointSuffix(tenantId); | ||
const webResource = this.identityClient.createWebResource({ | ||
url: `${this.identityClient.authorityHost}/${this.tenantId}/${urlSuffix}`, | ||
url: `${this.identityClient.authorityHost}/${tenantId}/${urlSuffix}`, | ||
method: "POST", | ||
disableJsonStringifyOnBody: true, | ||
deserializationMapper: undefined, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
// Copyright (c) Microsoft Corporation. | ||
// Licensed under the MIT license. | ||
|
||
import { TokenCredentialOptions } from "../client/identityClient"; | ||
|
||
/** | ||
* Options for the {@link AzureCliCredential} | ||
*/ | ||
export interface AzureCliCredentialOptions extends TokenCredentialOptions { | ||
/** | ||
* Allows specifying a tenant ID | ||
*/ | ||
tenantId?: string; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
// Copyright (c) Microsoft Corporation. | ||
// Licensed under the MIT license. | ||
|
||
import { TokenCredentialOptions } from "../client/identityClient"; | ||
|
||
/** | ||
* Options for the {@link AzurePowerShellCredential} | ||
*/ | ||
export interface AzurePowerShellCredentialOptions extends TokenCredentialOptions { | ||
/** | ||
* Allows specifying a tenant ID | ||
*/ | ||
tenantId?: string; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a breaking change? Can callers specify the argument name explicitly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a breaking change, just a name change :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Callers can’t specify the argument names like this, they just pass the value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thankfully we don't have magical kwargs :)