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.

Sign up for GitHub

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] Disabling regional authority support #18026

Merged
merged 2 commits into from
Oct 5, 2021
Merged

[Identity] Disabling regional authority support #18026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6c0ec73
[Identity] Disabling regional authority support
sadasant Oct 4, 2021
bf5b962
feedback from Scott Schaab
sadasant Oct 5, 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/identity/identity/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#### 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.
- Removed support for specific Azure regions on `ClientSecretCredential` and `ClientCertificateCredential. This feature will be added back on the next beta.

Copy link
Member

Choose a reason for hiding this comment

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

nit: rdded -> added. Also, I think this should mention that we intend to add these back into the next beta release. Finally in .NET we didn't remove support for the environment variable, but we also didn't mention explicitly that it was still supported. This was intended to give first party applications which are going to be required to use regional STS a way to use it with the Azure SDK without adding an API that third party apps might stumble upon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think of the new entry?

Copy link
Member

Choose a reason for hiding this comment

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

Looks great.


### Bugs Fixed
Expand Down
59 changes: 0 additions & 59 deletions sdk/identity/identity/review/identity.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,6 @@ export class ClientCertificateCredential implements TokenCredential {

// @public
export interface ClientCertificateCredentialOptions extends TokenCredentialOptions, CredentialPersistenceOptions {
regionalAuthority?: string;
sendCertificateChain?: boolean;
}

Expand All @@ -126,7 +125,6 @@ export class ClientSecretCredential implements TokenCredential {

// @public
export interface ClientSecretCredentialOptions extends TokenCredentialOptions, CredentialPersistenceOptions {
regionalAuthority?: string;
}

// @public
Expand Down Expand Up @@ -275,63 +273,6 @@ export interface OnBehalfOfCredentialSecretConfiguration {
userAssertionToken: string;
}

// @public
export enum RegionalAuthority {
AsiaEast = "eastasia",
AsiaSouthEast = "southeastasia",
AustraliaCentral = "australiacentral",
AustraliaCentral2 = "australiacentral2",
AustraliaEast = "australiaeast",
AustraliaSouthEast = "australiasoutheast",
AutoDiscoverRegion = "AutoDiscoverRegion",
BrazilSouth = "brazilsouth",
CanadaCentral = "canadacentral",
CanadaEast = "canadaeast",
ChinaEast = "chinaeast",
ChinaEast2 = "chinaeast2",
ChinaNorth = "chinanorth",
ChinaNorth2 = "chinanorth2",
EuropeNorth = "northeurope",
EuropeWest = "westeurope",
FranceCentral = "francecentral",
FranceSouth = "francesouth",
GermanyCentral = "germanycentral",
GermanyNorth = "germanynorth",
GermanyNorthEast = "germanynortheast",
GermanyWestCentral = "germanywestcentral",
GovernmentUSArizona = "usgovarizona",
GovernmentUSDodCentral = "usdodcentral",
GovernmentUSDodEast = "usdodeast",
GovernmentUSIowa = "usgoviowa",
GovernmentUSTexas = "usgovtexas",
GovernmentUSVirginia = "usgovvirginia",
IndiaCentral = "centralindia",
IndiaSouth = "southindia",
IndiaWest = "westindia",
JapanEast = "japaneast",
JapanWest = "japanwest",
KoreaCentral = "koreacentral",
KoreaSouth = "koreasouth",
NorwayEast = "norwayeast",
NorwayWest = "norwaywest",
SouthAfricaNorth = "southafricanorth",
SouthAfricaWest = "southafricawest",
SwitzerlandNorth = "switzerlandnorth",
SwitzerlandWest = "switzerlandwest",
UAECentral = "uaecentral",
UAENorth = "uaenorth",
UKSouth = "uksouth",
UKWest = "ukwest",
USCentral = "centralus",
USEast = "eastus",
USEast2 = "eastus2",
USNorthCentral = "northcentralus",
USSouthCentral = "southcentralus",
USWest = "westus",
USWest2 = "westus2",
USWestCentral = "westcentralus"
}

// @public
export function serializeAuthenticationRecord(record: AuthenticationRecord): string;

Expand Down
13 changes: 7 additions & 6 deletions sdk/identity/identity/src/credentials/clientCertificateCredentialOptions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,11 @@ export interface ClientCertificateCredentialOptions
* Set this option to send base64 encoded public certificate in the client assertion header as an x5c claim
*/
sendCertificateChain?: 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.
* If the property is not specified, the credential uses the global authority endpoint.
*/
regionalAuthority?: string;
// TODO: Export again once we're ready to release this feature.
// /**
// * 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.
// * If the property is not specified, the credential uses the global authority endpoint.
// */
// regionalAuthority?: string;
}
13 changes: 7 additions & 6 deletions sdk/identity/identity/src/credentials/clientSecretCredentialOptions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,11 @@ import { CredentialPersistenceOptions } from "./credentialPersistenceOptions";
export interface ClientSecretCredentialOptions
extends TokenCredentialOptions,
CredentialPersistenceOptions {
/**
* 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.
* If the property is not specified, the credential uses the global authority endpoint.
*/
regionalAuthority?: string;
// TODO: Export again once we're ready to release this feature.
// /**
// * 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.
// * If the property is not specified, the credential uses the global authority endpoint.
// */
// regionalAuthority?: string;
}
5 changes: 4 additions & 1 deletion sdk/identity/identity/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ export { AuthenticationRecord } from "./msal/types";
export { AuthenticationRequiredError } from "./msal/errors";
export { serializeAuthenticationRecord, deserializeAuthenticationRecord } from "./msal/utils";
export { TokenCredentialOptions } from "./client/identityClient";
export { RegionalAuthority } from "./regionalAuthority";

// TODO: Export again once we're ready to release this feature.
// export { RegionalAuthority } from "./regionalAuthority";

export { InteractiveCredentialOptions } from "./credentials/interactiveCredentialOptions";

export { ChainedTokenCredential } from "./credentials/chainedTokenCredential";
Expand Down
2 changes: 1 addition & 1 deletion sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ import {
publicToMsal
} from "../utils";
import { TokenCachePersistenceOptions } from "./tokenCachePersistenceOptions";
import { RegionalAuthority } from "../../regionalAuthority";
import { processMultiTenantRequest } from "../../util/validateMultiTenant";
import { RegionalAuthority } from "../../regionalAuthority";

/**
* Union of the constructor parameters that all MSAL flow types for Node.
Expand Down
8 changes: 5 additions & 3 deletions sdk/identity/identity/test/internal/node/clientCertificateCredential.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import * as path from "path";
import { AbortController } from "@azure/abort-controller";
import { env, isPlaybackMode, delay } from "@azure-tools/test-recorder";
import { ConfidentialClientApplication } from "@azure/msal-node";
import { ClientCertificateCredential, RegionalAuthority } from "../../../src";
import { ClientCertificateCredential } from "../../../src";
import { MsalTestCleanup, msalNodeTestSetup } from "../../msalTestUtils";
import { MsalNode } from "../../../src/msal/nodeFlows/nodeCommon";
import { Context } from "mocha";
Expand Down Expand Up @@ -121,13 +121,15 @@ describe("ClientCertificateCredential (internal)", function() {
assert.equal(doGetTokenSpy.callCount, 2);
});

it("supports specifying the regional authority", async function() {
// TODO: Enable again once we're ready to release this feature.
it.skip("supports specifying the regional authority", async function() {
const credential = new ClientCertificateCredential(
env.AZURE_TENANT_ID,
env.AZURE_CLIENT_ID,
certificatePath,
{
regionalAuthority: RegionalAuthority.AutoDiscoverRegion
// TODO: Uncomment once we're ready to release this feature.
// regionalAuthority: RegionalAuthority.AutoDiscoverRegion
}
);

Expand Down
8 changes: 5 additions & 3 deletions sdk/identity/identity/test/internal/node/clientSecretCredential.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { assert } from "chai";
import { AbortController } from "@azure/abort-controller";
import { env, delay } from "@azure-tools/test-recorder";
import { ConfidentialClientApplication } from "@azure/msal-node";
import { ClientSecretCredential, RegionalAuthority } from "../../../src";
import { ClientSecretCredential } from "../../../src";
import { MsalTestCleanup, msalNodeTestSetup } from "../../msalTestUtils";
import { MsalNode } from "../../../src/msal/nodeFlows/nodeCommon";
import { Context } from "mocha";
Expand Down Expand Up @@ -85,13 +85,15 @@ describe("ClientSecretCredential (internal)", function() {
assert.equal(doGetTokenSpy.callCount, 1);
});

it("supports specifying the regional authority", async function() {
// TODO: Enable again once we're ready to release this feature.
it.skip("supports specifying the regional authority", async function() {
const credential = new ClientSecretCredential(
env.AZURE_TENANT_ID,
env.AZURE_CLIENT_ID,
env.AZURE_CLIENT_SECRET,
{
regionalAuthority: RegionalAuthority.AutoDiscoverRegion
// TODO: Uncomment once we're ready to release this feature.
// regionalAuthority: RegionalAuthority.AutoDiscoverRegion
}
);

Expand Down
8 changes: 5 additions & 3 deletions sdk/identity/identity/test/public/node/clientSecretCredential.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { assert } from "chai";
import { env, delay, isRecordMode } from "@azure-tools/test-recorder";
import { AbortController } from "@azure/abort-controller";
import { MsalTestCleanup, msalNodeTestSetup, testTracing } from "../../msalTestUtils";
import { ClientSecretCredential, RegionalAuthority } from "../../../src";
import { ClientSecretCredential } from "../../../src";
import { Context } from "mocha";

describe("ClientSecretCredential", function() {
Expand Down Expand Up @@ -81,7 +81,8 @@ describe("ClientSecretCredential", function() {
})
);

it("supports specifying the regional authority", async function(this: Context) {
// TODO: Enable again once we're ready to release this feature.
it.skip("supports specifying the regional authority", async function(this: Context) {
// This test is extremely slow. Let's skip it for now.
// I've tried Sinon's clock and it doesn't affect it.
// We have internal tests that check that the parameters are properly sent to MSAL, which should be enough from the perspective of the SDK.
Expand All @@ -94,7 +95,8 @@ describe("ClientSecretCredential", function() {
env.AZURE_CLIENT_ID,
env.AZURE_CLIENT_SECRET,
{
regionalAuthority: RegionalAuthority.AutoDiscoverRegion
// TODO: Uncomment again once we're ready to release this feature.
// regionalAuthority: RegionalAuthority.AutoDiscoverRegion
}
);

Expand Down