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

add adfs support in Identity SDK. #11591

Merged
merged 4 commits into from
Oct 2, 2020
Merged

Conversation

g2vinay
Copy link
Member

@g2vinay g2vinay commented Oct 1, 2020

Adds support for ADFS authorities to connect to correct endpoint and bypass validation on MSAL for MSAL supported credentials.

Fixes #11323 #10226

@ghost ghost added the Azure.Identity label Oct 1, 2020
@g2vinay g2vinay changed the title add adfs support add adfs support in Identity SDK. Oct 1, 2020
@@ -143,8 +143,9 @@ export class IdentityClient extends ServiceClient implements INetworkModule {
}

try {
const urlSuffix = tenantId === "adfs" ? "oauth2/token" : "oauth2/v2.0/token";
Copy link
Contributor

Choose a reason for hiding this comment

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

We should definitely pull this out into a helper method now so that we don't have to maintain it in 4 different places.

Copy link
Member Author

Choose a reason for hiding this comment

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

moved it as a helper under util

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Looks great

Copy link
Contributor

@daviwil daviwil left a comment

Choose a reason for hiding this comment

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

Looking good so far!

@g2vinay g2vinay marked this pull request as ready for review October 2, 2020 00:28
@g2vinay g2vinay requested a review from sophiajt as a code owner October 2, 2020 00:28
@sadasant
Copy link
Contributor

sadasant commented Oct 2, 2020

Looks good, but address @daviwil's feedback please 🙏 ✨

@joshfree joshfree added Client This issue points to a problem in the data-plane of the library. feature-request This issue requires a new behavior in the product in order be resolved. blocking-release Blocks release labels Oct 2, 2020
@joshfree joshfree added this to the [2020] October milestone Oct 2, 2020
Copy link
Member

@joshfree joshfree left a comment

Choose a reason for hiding this comment

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

Could you file a tracking bug to add manual / live e2e tests for this new JS scenario?

@g2vinay g2vinay merged commit 28af1a0 into Azure:master Oct 2, 2020
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Nov 18, 2020
s360 fixes for Azure DNS (Azure#11591)

* s360 fixes for Azure DNS

* Additional property

* Property fix

* Readonly property

* CloudError fix

* Examples. cloud error

* Avocado fix

* read.md

* Prettier
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Nov 18, 2020
s360 fixes for Azure DNS (Azure#11591)

* s360 fixes for Azure DNS

* Additional property

* Property fix

* Readonly property

* CloudError fix

* Examples. cloud error

* Avocado fix

* read.md

* Prettier
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Identity blocking-release Blocks release Client This issue points to a problem in the data-plane of the library. feature-request This issue requires a new behavior in the product in order be resolved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Identity Node credentials don't work against Azure Stack.
5 participants