From 5d9f840a04033e5eba3990027ed153278bbb651a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Rodr=C3=ADguez?= Date: Wed, 29 Sep 2021 18:41:46 -0400 Subject: [PATCH] [Identity] Removed allowMultiTenantAuthentication (#17915) * [Identity] Removed allowMultiTenantAuthentication * small changelog improvement * review file * adfs error message --- sdk/identity/identity/CHANGELOG.md | 5 ++ sdk/identity/identity/package.json | 1 + sdk/identity/identity/review/identity.api.md | 1 - .../identity/src/client/identityClient.ts | 5 -- .../src/credentials/azureCliCredential.ts | 8 +-- .../credentials/azurePowerShellCredential.ts | 8 +-- .../credentials/visualStudioCodeCredential.ts | 6 +- .../src/msal/browserFlows/browserCommon.ts | 7 +-- .../identity/src/msal/nodeFlows/nodeCommon.ts | 7 +-- .../src/util/validateMultiTenant.browser.ts | 29 ++++++++++ .../identity/src/util/validateMultiTenant.ts | 34 ++++++----- .../identity/test/internal/node/utils.spec.ts | 40 +++++++++++++ .../identity/test/internal/utils.spec.ts | 58 +++++-------------- 13 files changed, 112 insertions(+), 97 deletions(-) create mode 100644 sdk/identity/identity/src/util/validateMultiTenant.browser.ts create mode 100644 sdk/identity/identity/test/internal/node/utils.spec.ts diff --git a/sdk/identity/identity/CHANGELOG.md b/sdk/identity/identity/CHANGELOG.md index 3eef824370fa..17e208653ba3 100644 --- a/sdk/identity/identity/CHANGELOG.md +++ b/sdk/identity/identity/CHANGELOG.md @@ -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 diff --git a/sdk/identity/identity/package.json b/sdk/identity/identity/package.json index 6d126d10988a..801d59397eb2 100644 --- a/sdk/identity/identity/package.json +++ b/sdk/identity/identity/package.json @@ -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" diff --git a/sdk/identity/identity/review/identity.api.md b/sdk/identity/identity/review/identity.api.md index f0a1a02344db..0e19256e8868 100644 --- a/sdk/identity/identity/review/identity.api.md +++ b/sdk/identity/identity/review/identity.api.md @@ -346,7 +346,6 @@ export { TokenCredential } // @public export interface TokenCredentialOptions extends CommonClientOptions { - allowMultiTenantAuthentication?: boolean; authorityHost?: string; } diff --git a/sdk/identity/identity/src/client/identityClient.ts b/sdk/identity/identity/src/client/identityClient.ts index 0b757c2cac9e..244d09d04253 100644 --- a/sdk/identity/identity/src/client/identityClient.ts +++ b/sdk/identity/identity/src/client/identityClient.ts @@ -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; } diff --git a/sdk/identity/identity/src/credentials/azureCliCredential.ts b/sdk/identity/identity/src/credentials/azureCliCredential.ts index 1b45cc79e462..6485c5bf390b 100644 --- a/sdk/identity/identity/src/credentials/azureCliCredential.ts +++ b/sdk/identity/identity/src/credentials/azureCliCredential.ts @@ -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}. @@ -91,7 +90,6 @@ export class AzureCliCredential implements TokenCredential { */ constructor(options?: AzureCliCredentialOptions) { this.tenantId = options?.tenantId; - this.allowMultiTenantAuthentication = options?.allowMultiTenantAuthentication; } /** @@ -106,11 +104,7 @@ export class AzureCliCredential implements TokenCredential { scopes: string | string[], options?: GetTokenOptions ): Promise { - const tenantId = processMultiTenantRequest( - this.tenantId, - this.allowMultiTenantAuthentication, - options - ); + const tenantId = processMultiTenantRequest(this.tenantId, options); if (tenantId) { checkTenantId(logger, tenantId); } diff --git a/sdk/identity/identity/src/credentials/azurePowerShellCredential.ts b/sdk/identity/identity/src/credentials/azurePowerShellCredential.ts index c6900a0a2a83..8d61fc988fed 100644 --- a/sdk/identity/identity/src/credentials/azurePowerShellCredential.ts +++ b/sdk/identity/identity/src/credentials/azurePowerShellCredential.ts @@ -96,7 +96,6 @@ if (isWindows) { */ export class AzurePowerShellCredential implements TokenCredential { private tenantId?: string; - private allowMultiTenantAuthentication?: boolean; /** * Creates an instance of the {@link AzurePowershellCredential}. @@ -105,7 +104,6 @@ export class AzurePowerShellCredential implements TokenCredential { */ constructor(options?: AzurePowerShellCredentialOptions) { this.tenantId = options?.tenantId; - this.allowMultiTenantAuthentication = options?.allowMultiTenantAuthentication; } /** @@ -167,11 +165,7 @@ export class AzurePowerShellCredential implements TokenCredential { options: GetTokenOptions = {} ): Promise { 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); } diff --git a/sdk/identity/identity/src/credentials/visualStudioCodeCredential.ts b/sdk/identity/identity/src/credentials/visualStudioCodeCredential.ts index 19c075e47baa..574f822c2962 100644 --- a/sdk/identity/identity/src/credentials/visualStudioCodeCredential.ts +++ b/sdk/identity/identity/src/credentials/visualStudioCodeCredential.ts @@ -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. @@ -134,7 +133,6 @@ export class VisualStudioCodeCredential implements TokenCredential { } else { this.tenantId = CommonTenantId; } - this.allowMultiTenantAuthentication = options?.allowMultiTenantAuthentication; checkUnsupportedTenant(this.tenantId); } @@ -180,9 +178,7 @@ export class VisualStudioCodeCredential implements TokenCredential { ): Promise { 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( diff --git a/sdk/identity/identity/src/msal/browserFlows/browserCommon.ts b/sdk/identity/identity/src/msal/browserFlows/browserCommon.ts index 6f8a91127f04..3a362dc266af 100644 --- a/sdk/identity/identity/src/msal/browserFlows/browserCommon.ts +++ b/sdk/identity/identity/src/msal/browserFlows/browserCommon.ts @@ -23,7 +23,6 @@ import { processMultiTenantRequest } from "../../util/validateMultiTenant"; export interface MsalBrowserFlowOptions extends MsalFlowOptions { redirectUri?: string; loginStyle: BrowserLoginStyle; - allowMultiTenantAuthentication?: boolean; loginHint?: string; } @@ -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; @@ -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; @@ -146,9 +143,7 @@ export abstract class MsalBrowser extends MsalBaseUtilities implements MsalBrows scopes: string[], options: CredentialFlowGetTokenOptions = {} ): Promise { - 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); diff --git a/sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts b/sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts index 3e27f270beaf..974f6353a73b 100644 --- a/sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts +++ b/sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts @@ -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. @@ -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; @@ -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 @@ -280,9 +277,7 @@ To work with multiple accounts for the same Client ID and Tenant ID, please prov scopes: string[], options: CredentialFlowGetTokenOptions = {} ): Promise { - const tenantId = - processMultiTenantRequest(this.tenantId, this.allowMultiTenantAuthentication, options) || - this.tenantId; + const tenantId = processMultiTenantRequest(this.tenantId, options) || this.tenantId; options.authority = getAuthority(tenantId, this.authorityHost); diff --git a/sdk/identity/identity/src/util/validateMultiTenant.browser.ts b/sdk/identity/identity/src/util/validateMultiTenant.browser.ts new file mode 100644 index 000000000000..2599b31845bf --- /dev/null +++ b/sdk/identity/identity/src/util/validateMultiTenant.browser.ts @@ -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; +} diff --git a/sdk/identity/identity/src/util/validateMultiTenant.ts b/sdk/identity/identity/src/util/validateMultiTenant.ts index 7976b0011f14..cfc55015142d 100644 --- a/sdk/identity/identity/src/util/validateMultiTenant.ts +++ b/sdk/identity/identity/src/util/validateMultiTenant.ts @@ -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."; /** - * 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; } diff --git a/sdk/identity/identity/test/internal/node/utils.spec.ts b/sdk/identity/identity/test/internal/node/utils.spec.ts new file mode 100644 index 000000000000..9fead4071b7c --- /dev/null +++ b/sdk/identity/identity/test/internal/node/utils.spec.ts @@ -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); + }); + }); +}); diff --git a/sdk/identity/identity/test/internal/utils.spec.ts b/sdk/identity/identity/test/internal/utils.spec.ts index c073279c7967..fcf8b3adaa2f 100644 --- a/sdk/identity/identity/test/internal/utils.spec.ts +++ b/sdk/identity/identity/test/internal/utils.spec.ts @@ -3,32 +3,22 @@ import { assert } from "chai"; import { - multiTenantErrorMessage, + multiTenantADFSErrorMessage, processMultiTenantRequest } from "../../src/util/validateMultiTenant"; describe("Identity utilities", function() { describe("validateMultiTenantRequest", function() { - it("throws if multi-tenant authentication is disallowed, and the tenants don't match", async function() { - let error: Error | undefined; - try { - processMultiTenantRequest("credential-options-tenant-id", false, { - tenantId: "get-token-options-tenant-id" - }); - } catch (e) { - error = e; - } - assert.ok( - error, - "validateMultiTenantRequest should throw if multi-tenant is disallowed and the tenants don't match" - ); - assert.equal(error!.message, multiTenantErrorMessage); + it("returns the original tenant if getTokenOptions does not have a tenantId", async function() { + const originalTenant = "credential-options-tenant-id"; + const resultingTenant = processMultiTenantRequest(originalTenant); + assert.equal(resultingTenant, originalTenant); }); - it("throws if multi-tenant authentication is undefined, and the tenants don't match", async function() { + it("throws if a tenant is provided through getTokenoptions and the original tenant Id is 'asdf'", async function() { let error: Error | undefined; try { - processMultiTenantRequest("credential-options-tenant-id", undefined, { + processMultiTenantRequest("adfs", { tenantId: "get-token-options-tenant-id" }); } catch (e) { @@ -36,49 +26,27 @@ describe("Identity utilities", function() { } assert.ok( error, - "validateMultiTenantRequest should throw if multi-tenant is disallowed and the tenants don't match" + "validateMultiTenantRequest should throw if a tenant is provided through getTokenoptions and the original tenant Id is 'asdf'" ); - assert.equal(error!.message, multiTenantErrorMessage); + assert.equal(error!.message, multiTenantADFSErrorMessage); }); - it("If allowMultiTenantAuthentication is disallowed, it shouldn't throw if the tenant received is the same as the tenant we already had, that same tenant should be returned", async function() { - assert.equal( - processMultiTenantRequest("same-tenant", false, { - tenantId: "same-tenant" - }), - "same-tenant" - ); + it("it shouldn't throw if the tenant received is the same as the tenant we already had", async function() { assert.equal( - processMultiTenantRequest("same-tenant", undefined, { + processMultiTenantRequest("same-tenant", { tenantId: "same-tenant" }), "same-tenant" ); }); - it("If we had a tenant and the options have another tenant, we pick the tenant from the options", async function() { + it("should pick the tenant from the options", async function() { assert.equal( - processMultiTenantRequest("credential-options-tenant-id", true, { + processMultiTenantRequest("credential-options-tenant-id", { tenantId: "get-token-options-tenant-id" }), "get-token-options-tenant-id" ); }); - - it("If we had a tenant and there is no tenant in the options, we pick the tenant we already had", async function() { - assert.equal( - processMultiTenantRequest("credential-options-tenant-id", true, {}), - "credential-options-tenant-id" - ); - }); - - it("If the tenant received is the same as the tenant we already had, that same tenant should be returned", async function() { - assert.equal( - processMultiTenantRequest("same-tenant", true, { - tenantId: "same-tenant" - }), - "same-tenant" - ); - }); }); });