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

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

Merged
25 commits merged into from
Jun 30, 2021
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
b267aff
[Identity] Draft for the support for tenant Id Challenges / tenant di…
sadasant Jun 18, 2021
3f1cbbf
all the credentials now
sadasant Jun 18, 2021
b1f2e64
forgot about AzurePowerShellCredentialOptions
sadasant Jun 18, 2021
334df01
a unit test
sadasant Jun 21, 2021
b588070
Merge remote-tracking branch 'Azure/main' into identity/fix15797
sadasant Jun 21, 2021
dc1ea9e
moved allowMultiTenantAuthentication to the GetTokenOptions
sadasant Jun 22, 2021
c8adf94
added validateMultiTenantRequest on browserCommon
sadasant Jun 22, 2021
33bbaa2
Merge remote-tracking branch 'Azure/main' into identity/fix15797
sadasant Jun 24, 2021
7be7756
lint fix
sadasant Jun 24, 2021
0dacea8
asiggning the tenantId where it should go
sadasant Jun 25, 2021
a871027
Merge remote-tracking branch 'Azure/main' into identity/fix15797
sadasant Jun 25, 2021
d252fbe
better tests
sadasant Jun 25, 2021
fae2ab6
feedback from Schaab
sadasant Jun 28, 2021
a4946ab
Merge remote-tracking branch 'Azure/main' into identity/fix15797
sadasant Jun 28, 2021
e39736e
Update sdk/identity/identity/CHANGELOG.md
sadasant Jun 29, 2021
5257305
core-auth CHANGELOG update
sadasant Jun 29, 2021
b3e30cd
getAuthorityHost -> getAuthority
sadasant Jun 29, 2021
d05186d
Merge remote-tracking branch 'Azure/main' into identity/fix15797
sadasant Jun 29, 2021
cbb48dc
Merge remote-tracking branch 'Azure/main' into identity/fix15797
sadasant Jun 30, 2021
09a7b56
options.authority feedback from Will
sadasant Jun 30, 2021
b329866
validating tenants if provided to the CLI and the powershell credential
sadasant Jun 30, 2021
0983549
validateMultiTenant error message
sadasant Jun 30, 2021
f49f32b
Update sdk/identity/identity/CHANGELOG.md
sadasant Jun 30, 2021
723f825
feedback by Christopher Radek
sadasant Jun 30, 2021
e63a14b
safer than non-null assertions
sadasant Jun 30, 2021
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
1 change: 1 addition & 0 deletions sdk/core/core-auth/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## 1.3.1 (Unreleased)

- Added `tenantId` optional property to the `GetTokenOptions` interface. If `tenantId` is set, credentials will be able to use multi-tenant authentication, in the cases when it's enabled.

## 1.3.0 (2021-03-30)

Expand Down
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 @@ -47,6 +47,7 @@ export interface GetTokenOptions {
requestOptions?: {
timeout?: number;
};
tenantId?: string;
tracingOptions?: {
spanOptions?: SpanOptions;
tracingContext?: Context;
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 {
*/
tracingContext?: Context;
};

/**
* Allows specifying a tenantId. Useful to handle challenges that provide tenant Id hints.
*/
tenantId?: string;
}

/**
Expand Down
2 changes: 2 additions & 0 deletions sdk/identity/identity/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
- Added `regionalAuthority` property to `ClientSecretCredentialOptions` and `ClientCertificateCredentialOptions`.
- If instead of a region, `AutoDiscoverRegion` is specified as the value for `regionalAuthority`, MSAL will be used to attempt to discover the region.
- A region can also be specified through the `AZURE_REGIONAL_AUTHORITY_NAME` environment variable.
- `AzureCliCredential` and `AzurePowerShellCredential` now allow specifying a `tenantId`.
- All credentials except `ManagedIdentityCredential` support enabling multi tenant authentication via the `allowMultiTenantAuthentication` option.

### Breaking Changes

Expand Down
15 changes: 14 additions & 1 deletion sdk/identity/identity/review/identity.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,24 @@ export enum AzureAuthorityHosts {

// @public
export class AzureCliCredential implements TokenCredential {
constructor(options?: AzureCliCredentialOptions);
getToken(scopes: string | string[], options?: GetTokenOptions): Promise<AccessToken>;
}

// @public
export interface AzureCliCredentialOptions extends TokenCredentialOptions {
tenantId?: string;
}

// @public
export class AzurePowerShellCredential implements TokenCredential {
constructor(options?: AzurePowerShellCredentialOptions);
getToken(scopes: string | string[], options?: GetTokenOptions): Promise<AccessToken | null>;
}

// @public
export interface AzurePowerShellCredentialOptions extends TokenCredentialOptions {
tenantId?: string;
}

// @public
Expand Down Expand Up @@ -297,6 +309,7 @@ export { TokenCredential }

// @public
export interface TokenCredentialOptions extends PipelineOptions {
allowMultiTenantAuthentication?: boolean;
authorityHost?: string;
}

Expand All @@ -316,7 +329,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>;
Copy link
Member

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?

Copy link
Contributor Author

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 :)

Copy link
Contributor Author

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.

Copy link
Member

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 :)

}

// @public
Expand Down
5 changes: 5 additions & 0 deletions sdk/identity/identity/src/client/identityClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -313,4 +313,9 @@ export interface TokenCredentialOptions extends PipelineOptions {
* The default is "https://login.microsoftonline.com".
*/
authorityHost?: string;

/**
* If set to true, allows authentication flows to change the tenantId of the request if a different tenantId is received from a challenge or through a direct getToken call.
*/
allowMultiTenantAuthentication?: boolean;
}
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -135,6 +138,12 @@ export class AuthorizationCodeCredential implements TokenCredential {
scopes: string | string[],
options?: GetTokenOptions
): Promise<AccessToken> {
const tenantId = processMultiTenantRequest(
this.tenantId,
this.allowMultiTenantAuthentication,
options
)!;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok I’ll add a comment!

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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;
Expand All @@ -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,
Expand All @@ -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,
Expand Down
45 changes: 41 additions & 4 deletions sdk/identity/identity/src/credentials/azureCliCredential.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ import { SpanStatusCode } from "@azure/core-tracing";
import { credentialLogger, formatSuccess, formatError } from "../util/logging";
import * as child_process from "child_process";
import { ensureValidScope, getScopeResource } from "../util/scopeUtils";
import { AzureCliCredentialOptions } from "./azureCliCredentialOptions";
import { processMultiTenantRequest } from "../util/validateMultiTenant";
import { checkTenantId } from "../util/checkTenantId";

/**
* Mockable reference to the CLI credential cliCredentialFunctions
Expand All @@ -35,13 +38,26 @@ export const cliCredentialInternals = {
* @internal
*/
async getAzureCliAccessToken(
resource: string
resource: string,
tenantId?: string
): Promise<{ stdout: string; stderr: string; error: Error | null }> {
let tenantSection: string[] = [];
if (tenantId) {
tenantSection = ["--tenant", tenantId];
sadasant marked this conversation as resolved.
Show resolved Hide resolved
}
return new Promise((resolve, reject) => {
try {
child_process.execFile(
"az",
["account", "get-access-token", "--output", "json", "--resource", resource],
[
"account",
"get-access-token",
"--output",
"json",
"--resource",
...tenantSection,
resource
],
{ cwd: cliCredentialInternals.getSafeWorkingDir() },
(error, stdout, stderr) => {
resolve({ stdout: stdout, stderr: stderr, error });
Expand All @@ -65,6 +81,19 @@ const logger = credentialLogger("AzureCliCredential");
* in via the 'az' tool using the command "az login" from the commandline.
*/
export class AzureCliCredential implements TokenCredential {
private tenantId?: string;
private allowMultiTenantAuthentication?: boolean;

/**
* Creates an instance of the {@link AzureCliCredential}.
*
* @param options - Options, to optionally allow multi-tenant requests.
*/
constructor(options?: AzureCliCredentialOptions) {
this.tenantId = options?.tenantId;
this.allowMultiTenantAuthentication = options?.allowMultiTenantAuthentication;
}

/**
* Authenticates with Azure Active Directory and returns an access token if successful.
* If authentication fails, a {@link CredentialUnavailableError} will be thrown with the details of the failure.
Expand All @@ -77,9 +106,17 @@ export class AzureCliCredential implements TokenCredential {
scopes: string | string[],
options?: GetTokenOptions
): Promise<AccessToken> {
const tenantId = processMultiTenantRequest(
this.tenantId,
this.allowMultiTenantAuthentication,
options
);
if (tenantId) {
checkTenantId(logger, tenantId);
}

const scope = typeof scopes === "string" ? scopes : scopes[0];
logger.getToken.info(`Using the scope ${scope}`);

ensureValidScope(scope, logger);
const resource = getScopeResource(scope);

Expand All @@ -88,7 +125,7 @@ export class AzureCliCredential implements TokenCredential {
const { span } = createSpan("AzureCliCredential-getToken", options);

try {
const obj = await cliCredentialInternals.getAzureCliAccessToken(resource);
const obj = await cliCredentialInternals.getAzureCliAccessToken(resource, tenantId);
if (obj.stderr) {
const isLoginError = obj.stderr.match("(.*)az login(.*)");
const isNotInstallError =
Expand Down
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
Expand Up @@ -8,6 +8,9 @@ import { credentialLogger, formatSuccess, formatError } from "../util/logging";
import { trace } from "../util/tracing";
import { ensureValidScope, getScopeResource } from "../util/scopeUtils";
import { processUtils } from "../util/processUtils";
import { AzurePowerShellCredentialOptions } from "./azurePowerShellCredentialOptions";
import { processMultiTenantRequest } from "../util/validateMultiTenant";
import { checkTenantId } from "../util/checkTenantId";

const logger = credentialLogger("AzurePowerShellCredential");

Expand Down Expand Up @@ -92,12 +95,26 @@ if (isWindows) {
* `Connect-AzAccount` from the command line.
*/
export class AzurePowerShellCredential implements TokenCredential {
private tenantId?: string;
private allowMultiTenantAuthentication?: boolean;

/**
* Creates an instance of the {@link AzurePowershellCredential}.
*
* @param options - Options, to optionally allow multi-tenant requests.
*/
constructor(options?: AzurePowerShellCredentialOptions) {
this.tenantId = options?.tenantId;
this.allowMultiTenantAuthentication = options?.allowMultiTenantAuthentication;
}

/**
* Gets the access token from Azure PowerShell
* @param resource - The resource to use when getting the token
*/
private async getAzurePowerShellAccessToken(
resource: string
resource: string,
tenantId?: string
): Promise<{ Token: string; ExpiresOn: string }> {
// Clone the stack to avoid mutating it while iterating
for (const powerShellCommand of [...commandStack]) {
Expand All @@ -109,6 +126,11 @@ export class AzurePowerShellCredential implements TokenCredential {
continue;
}

let tenantSection = "";
if (tenantId) {
tenantSection = `-TenantId "${tenantId}"`;
sadasant marked this conversation as resolved.
Show resolved Hide resolved
}

const results = await runCommands([
[
powerShellCommand,
Expand All @@ -118,7 +140,7 @@ export class AzurePowerShellCredential implements TokenCredential {
[
powerShellCommand,
"-Command",
`Get-AzAccessToken -ResourceUrl "${resource}" | ConvertTo-Json`
`Get-AzAccessToken ${tenantSection} -ResourceUrl "${resource}" | ConvertTo-Json`
]
]);

Expand All @@ -145,15 +167,22 @@ export class AzurePowerShellCredential implements TokenCredential {
options: GetTokenOptions = {}
): Promise<AccessToken | null> {
return trace(`${this.constructor.name}.getToken`, options, async () => {
const scope = typeof scopes === "string" ? scopes : scopes[0];

logger.getToken.info(`Using the scope ${scope}`);
const tenantId = processMultiTenantRequest(
this.tenantId,
this.allowMultiTenantAuthentication,
options
);
if (tenantId) {
checkTenantId(logger, tenantId);
}

const scope = typeof scopes === "string" ? scopes : scopes[0];
ensureValidScope(scope, logger);
logger.getToken.info(`Using the scope ${scope}`);
const resource = getScopeResource(scope);

try {
const response = await this.getAzurePowerShellAccessToken(resource);
const response = await this.getAzurePowerShellAccessToken(resource, tenantId);
logger.getToken.info(formatSuccess(scopes));
return {
token: response.Token,
Expand Down
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;
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { IdentityClient, TokenCredentialOptions } from "../client/identityClient
import { AzureAuthorityHosts } from "../constants";
import { checkTenantId } from "../util/checkTenantId";
import { credentialLogger, formatError, formatSuccess } from "../util/logging";
import { processMultiTenantRequest } from "../util/validateMultiTenant";
import { VSCodeCredentialFinder } from "./visualStudioCodeCredentialExtension";

const CommonTenantId = "common";
Expand Down Expand Up @@ -102,6 +103,7 @@ export class VisualStudioCodeCredential implements TokenCredential {
private identityClient: IdentityClient;
private tenantId: string;
private cloudName: VSCodeCloudNames;
private allowMultiTenantAuthentication?: boolean;

/**
* Creates an instance of VisualStudioCodeCredential to use for automatically authenticating via VSCode.
Expand All @@ -127,6 +129,7 @@ export class VisualStudioCodeCredential implements TokenCredential {
} else {
this.tenantId = CommonTenantId;
}
this.allowMultiTenantAuthentication = options?.allowMultiTenantAuthentication;

checkUnsupportedTenant(this.tenantId);
}
Expand Down Expand Up @@ -168,9 +171,15 @@ export class VisualStudioCodeCredential implements TokenCredential {
*/
public async getToken(
scopes: string | string[],
_options?: GetTokenOptions
options?: GetTokenOptions
): Promise<AccessToken> {
await this.prepareOnce();
const tenantId = processMultiTenantRequest(
this.tenantId,
this.allowMultiTenantAuthentication,
options
)!;

if (findCredentials === undefined) {
throw new CredentialUnavailableError(
"No implementation of VisualStudioCodeCredential is available (do you need to install and use the `@azure/identity-vscode` extension package?)"
Expand Down Expand Up @@ -206,7 +215,7 @@ export class VisualStudioCodeCredential implements TokenCredential {

if (refreshToken) {
const tokenResponse = await this.identityClient.refreshAccessToken(
this.tenantId,
tenantId,
AzureAccountClientId,
scopeString,
refreshToken,
Expand Down
Loading