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] Removed allowMultiTenantAuthentication #17915

Merged
merged 4 commits into from
Sep 29, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions sdk/identity/identity/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@

### Breaking Changes

#### Breaking Changes from 2.0.0-beta.4

- Removed the `allowMultiTenantAuthentication` option from all of the credentials. Multi-tenant authentication is now enabled by default. On Node.js, it can be disabled with the `AZURE_IDENTITY_DISABLE_MULTITENANTAUTH` environment variable.


### Bugs Fixed

### Other Changes
Expand Down
1 change: 1 addition & 0 deletions sdk/identity/identity/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
"./dist-esm/src/credentials/applicationCredential.js": "./dist-esm/src/credentials/applicationCredential.browser.js",
"./dist-esm/src/credentials/onBehalfOfCredential.js": "./dist-esm/src/credentials/onBehalfOfCredential.browser.js",
"./dist-esm/src/util/authHostEnv.js": "./dist-esm/src/util/authHostEnv.browser.js",
"./dist-esm/src/util/validateMultiTenant.js": "./dist-esm/src/util/validateMultiTenant.browser.js",
"./dist-esm/src/tokenCache/TokenCachePersistence.js": "./dist-esm/src/tokenCache/TokenCachePersistence.browser.js",
"./dist-esm/src/plugins/consumer.js": "./dist-esm/src/plugins/consumer.browser.js",
"./dist-esm/test/httpRequests.js": "./dist-esm/test/httpRequests.browser.js"
Expand Down
1 change: 0 additions & 1 deletion sdk/identity/identity/review/identity.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,6 @@ export { TokenCredential }

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

Expand Down
5 changes: 0 additions & 5 deletions sdk/identity/identity/src/client/identityClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -307,9 +307,4 @@ export interface TokenCredentialOptions extends CommonClientOptions {
* 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 @@ -82,7 +82,6 @@ const logger = credentialLogger("AzureCliCredential");
*/
export class AzureCliCredential implements TokenCredential {
private tenantId?: string;
private allowMultiTenantAuthentication?: boolean;

/**
* Creates an instance of the {@link AzureCliCredential}.
Expand All @@ -91,7 +90,6 @@ export class AzureCliCredential implements TokenCredential {
*/
constructor(options?: AzureCliCredentialOptions) {
this.tenantId = options?.tenantId;
this.allowMultiTenantAuthentication = options?.allowMultiTenantAuthentication;
}

/**
Expand All @@ -106,11 +104,7 @@ export class AzureCliCredential implements TokenCredential {
scopes: string | string[],
options?: GetTokenOptions
): Promise<AccessToken> {
const tenantId = processMultiTenantRequest(
this.tenantId,
this.allowMultiTenantAuthentication,
options
);
const tenantId = processMultiTenantRequest(this.tenantId, options);
if (tenantId) {
checkTenantId(logger, tenantId);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,6 @@ if (isWindows) {
*/
export class AzurePowerShellCredential implements TokenCredential {
private tenantId?: string;
private allowMultiTenantAuthentication?: boolean;

/**
* Creates an instance of the {@link AzurePowershellCredential}.
Expand All @@ -105,7 +104,6 @@ export class AzurePowerShellCredential implements TokenCredential {
*/
constructor(options?: AzurePowerShellCredentialOptions) {
this.tenantId = options?.tenantId;
this.allowMultiTenantAuthentication = options?.allowMultiTenantAuthentication;
}

/**
Expand Down Expand Up @@ -167,11 +165,7 @@ export class AzurePowerShellCredential implements TokenCredential {
options: GetTokenOptions = {}
): Promise<AccessToken> {
return trace(`${this.constructor.name}.getToken`, options, async () => {
const tenantId = processMultiTenantRequest(
this.tenantId,
this.allowMultiTenantAuthentication,
options
);
const tenantId = processMultiTenantRequest(this.tenantId, options);
if (tenantId) {
checkTenantId(logger, tenantId);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ 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 Down Expand Up @@ -134,7 +133,6 @@ export class VisualStudioCodeCredential implements TokenCredential {
} else {
this.tenantId = CommonTenantId;
}
this.allowMultiTenantAuthentication = options?.allowMultiTenantAuthentication;

checkUnsupportedTenant(this.tenantId);
}
Expand Down Expand Up @@ -180,9 +178,7 @@ export class VisualStudioCodeCredential implements TokenCredential {
): Promise<AccessToken> {
await this.prepareOnce();

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

if (findCredentials === undefined) {
throw new CredentialUnavailableError(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import { processMultiTenantRequest } from "../../util/validateMultiTenant";
export interface MsalBrowserFlowOptions extends MsalFlowOptions {
redirectUri?: string;
loginStyle: BrowserLoginStyle;
allowMultiTenantAuthentication?: boolean;
loginHint?: string;
}

Expand Down Expand Up @@ -71,7 +70,6 @@ export abstract class MsalBrowser extends MsalBaseUtilities implements MsalBrows
protected loginStyle: BrowserLoginStyle;
protected clientId: string;
protected tenantId: string;
protected allowMultiTenantAuthentication?: boolean;
protected authorityHost?: string;
protected account: AuthenticationRecord | undefined;
protected msalConfig: msalBrowser.Configuration;
Expand All @@ -87,7 +85,6 @@ export abstract class MsalBrowser extends MsalBaseUtilities implements MsalBrows
}
this.clientId = options.clientId;
this.tenantId = resolveTenantId(this.logger, options.tenantId, options.clientId);
this.allowMultiTenantAuthentication = options?.allowMultiTenantAuthentication;
this.authorityHost = options.authorityHost;
this.msalConfig = defaultBrowserMsalConfig(options);
this.disableAutomaticAuthentication = options.disableAutomaticAuthentication;
Expand Down Expand Up @@ -146,9 +143,7 @@ export abstract class MsalBrowser extends MsalBaseUtilities implements MsalBrows
scopes: string[],
options: CredentialFlowGetTokenOptions = {}
): Promise<AccessToken> {
const tenantId =
processMultiTenantRequest(this.tenantId, this.allowMultiTenantAuthentication, options) ||
this.tenantId;
const tenantId = processMultiTenantRequest(this.tenantId, options) || this.tenantId;

if (!options.authority) {
options.authority = getAuthority(tenantId, this.authorityHost);
Expand Down
7 changes: 1 addition & 6 deletions sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import { processMultiTenantRequest } from "../../util/validateMultiTenant";
export interface MsalNodeOptions extends MsalFlowOptions {
tokenCachePersistenceOptions?: TokenCachePersistenceOptions;
tokenCredentialOptions: TokenCredentialOptions;
allowMultiTenantAuthentication?: boolean;
/**
* Specifies a regional authority. Please refer to the {@link RegionalAuthority} type for the accepted values.
* If {@link RegionalAuthority.AutoDiscoverRegion} is specified, we will try to discover the regional authority endpoint.
Expand Down Expand Up @@ -75,7 +74,6 @@ export abstract class MsalNode extends MsalBaseUtilities implements MsalFlow {
protected msalConfig: msalNode.Configuration;
protected clientId: string;
protected tenantId: string;
protected allowMultiTenantAuthentication?: boolean;
protected authorityHost?: string;
protected identityClient?: IdentityClient;
protected requiresConfidential: boolean = false;
Expand All @@ -86,7 +84,6 @@ export abstract class MsalNode extends MsalBaseUtilities implements MsalFlow {
super(options);
this.msalConfig = this.defaultNodeMsalConfig(options);
this.tenantId = resolveTenantId(options.logger, options.tenantId, options.clientId);
this.allowMultiTenantAuthentication = options?.allowMultiTenantAuthentication;
this.clientId = this.msalConfig.auth.clientId;

// If persistence has been configured
Expand Down Expand Up @@ -280,9 +277,7 @@ To work with multiple accounts for the same Client ID and Tenant ID, please prov
scopes: string[],
options: CredentialFlowGetTokenOptions = {}
): Promise<AccessToken> {
const tenantId =
processMultiTenantRequest(this.tenantId, this.allowMultiTenantAuthentication, options) ||
this.tenantId;
const tenantId = processMultiTenantRequest(this.tenantId, options) || this.tenantId;

options.authority = getAuthority(tenantId, this.authorityHost);

Expand Down
29 changes: 29 additions & 0 deletions sdk/identity/identity/src/util/validateMultiTenant.browser.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

import { GetTokenOptions } from "@azure/core-auth";

/**
* @internal
*/
export const multiTenantADFSErrorMessage =
"A new tenant Id can't be assigned through the GetTokenOptions when a credential has been originally configured to use the tenant `adfs`.";

/**
* Of getToken contains a tenantId, this functions allows picking this tenantId as the appropriate for authentication,
* unless multitenant authentication has been disabled through the AZURE_IDENTITY_DISABLE_MULTITENANTAUTH (on Node.js),
* or unless the original tenant Id is `adfs`.
* @internal
*/
export function processMultiTenantRequest(
tenantId?: string,
getTokenOptions?: GetTokenOptions
): string | undefined {
if (!getTokenOptions?.tenantId) {
return tenantId;
}
if (tenantId === "adfs") {
throw new Error(multiTenantADFSErrorMessage);
}
return getTokenOptions?.tenantId;
}
34 changes: 19 additions & 15 deletions sdk/identity/identity/src/util/validateMultiTenant.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,29 +6,33 @@ import { GetTokenOptions } from "@azure/core-auth";
/**
* @internal
*/
export const multiTenantErrorMessage =
"A getToken request was attempted with a tenant different than the tenant configured at the initialization of the credential, but multi-tenant authentication was not enabled in this credential instance.";
export const multiTenantDisabledErrorMessage =
"A getToken request was attempted with a tenant different than the tenant configured at the initialization of the credential, but multi-tenant authentication has been disabled by the environment variable AZURE_IDENTITY_DISABLE_MULTITENANTAUTH.";
sadasant marked this conversation as resolved.
Show resolved Hide resolved

/**
* Verifies whether locally assigned tenants are equal to tenants received through getToken.
* Returns the appropriate tenant.
* @internal
*/
export const multiTenantADFSErrorMessage =
"A new tenant Id can't be assigned through the GetTokenOptions when a credential has been originally configured to use the tenant `adfs`.";

/**
* Of getToken contains a tenantId, this functions allows picking this tenantId as the appropriate for authentication,
* unless multitenant authentication has been disabled through the AZURE_IDENTITY_DISABLE_MULTITENANTAUTH (on Node.js),
* or unless the original tenant Id is `adfs`.
* @internal
*/
export function processMultiTenantRequest(
tenantId?: string,
allowMultiTenantAuthentication?: boolean,
getTokenOptions?: GetTokenOptions
): string | undefined {
if (
!allowMultiTenantAuthentication &&
getTokenOptions?.tenantId &&
tenantId &&
getTokenOptions.tenantId !== tenantId
) {
throw new Error(multiTenantErrorMessage);
if (!getTokenOptions?.tenantId) {
return tenantId;
}
if (process.env.AZURE_IDENTITY_DISABLE_MULTITENANTAUTH) {
throw new Error(multiTenantDisabledErrorMessage);
}
if (allowMultiTenantAuthentication && getTokenOptions?.tenantId) {
return getTokenOptions.tenantId;
if (tenantId === "adfs") {
throw new Error(multiTenantADFSErrorMessage);
}
return tenantId;
return getTokenOptions?.tenantId;
}
40 changes: 40 additions & 0 deletions sdk/identity/identity/test/internal/node/utils.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

import { assert } from "chai";
import {
multiTenantDisabledErrorMessage,
processMultiTenantRequest
} from "../../../src/util/validateMultiTenant";

describe("Identity utilities (Node.js only)", function() {
describe("validateMultiTenantRequest (Node.js only)", function() {
afterEach(() => {
delete process.env.AZURE_IDENTITY_DISABLE_MULTITENANTAUTH;
});

it("returns the original tenant and doesn't throw if getTokenOptions does not have a tenantId, even if AZURE_IDENTITY_DISABLE_MULTITENANTAUTH is defined", async function() {
process.env.AZURE_IDENTITY_DISABLE_MULTITENANTAUTH = "true";
const originalTenant = "credential-options-tenant-id";
const resultingTenant = processMultiTenantRequest(originalTenant);
assert.equal(resultingTenant, originalTenant);
});

it("throws if multi-tenant authentication is disabled via AZURE_IDENTITY_DISABLE_MULTITENANTAUTH", async function() {
let error: Error | undefined;
process.env.AZURE_IDENTITY_DISABLE_MULTITENANTAUTH = "true";
try {
processMultiTenantRequest("credential-options-tenant-id", {
tenantId: "get-token-options-tenant-id"
});
} catch (e) {
error = e;
}
assert.ok(
error,
"validateMultiTenantRequest should throw if multi-tenant authentication is disabled"
);
assert.equal(error!.message, multiTenantDisabledErrorMessage);
});
});
});
Loading