AzureAD / azure-activedirectory-library-for-dotnet

ADAL authentication libraries for .net
http://aka.ms/aaddev
MIT License
358 stars 214 forks source link

ADAL 5.4.2 Client package is dropping /adfs/ literal from the passed authority URI #1693

Closed sumantshiv closed 4 years ago

sumantshiv commented 4 years ago

MSAL is the recommended auth library for use with the Microsoft identity platform

No new features will be implemented on ADAL. The team's efforts are on improving MSAL, the next-gen auth library. MSAL's wiki contains a migration guide from ADAL.

Only regressions, high severity issues and security issues will be fixed on ADAL. Other issues are likely to have already been fixed in MSAL.

If you think that your issue falls into the above categories, please fill in the form below.

Which Version of ADAL are you using ? Note that to get help, you need to run the latest preview or non-preview version For MSAL, please log issues to https://github.com/AzureAD/microsoft-authentication-library-for-dotnet

ADAL 5.2.4

Which platform has the issue? net452

What authentication flow has the issue?

Other? - please describe;

Is this a new or existing app?

Azure Key vault - The app is in production, and I have upgraded to a new version of ADAL from 4.19.X/4.5.1 to 5.4.2

Azure Key vault VSTS Repo

var your = (code) => here;

Expected behavior ADAL Client library should continue to goto the specified Authority URL rather than dropping literals from it.

Actual behavior Azure Key Vault is looking to update the ADAL client library nuget primarily from 3.19.X/4.5.1 to 5.4.2. We need this updated version to support our upcoming features. However, while testing we hit an issue with respect to the Get Token API. In case the callback Authority has the literal /adfs/, the ADAL client library for some reason is dropping it. Hence, in case the Authority URL is: https://AuthEndpointBase/adfs/TenantId, the ADAL client is dropping this literal and actually sending the request to https://AuthEndpointBase/TenantId.

Possible Solution

Additional context/ Logs / Screenshots Add any other context about the problem here, such as logs and screebshots. Logging is described at https://github.com/AzureAD/azure-activedirectory-library-for-dotnet/wiki/Logging-in-ADAL.Net. Don't post logs containing PII on GitHub!

jmprieur commented 4 years ago

From investigation from @sumantshiv commit https://github.com/AzureAD/azure-activedirectory-library-for-dotnet/commit/c0f5b4edf166861db1f56d319877a212921ae055 has updated the logic around parsing the tenantId from the AuthorityURL. The ADAL library always formats the endpoint url as https://host/tenantID format.

If the authority URL is like: https://localhost/adfs/tenantID.

@sumantshiv has cases where they need the server endpoint format as: https://localhost/adfs or https://localhost/graph.

The changed file is: https://github.com/AzureAD/azure-activedirectory-library-for-dotnet/blob/c0f5b4edf166861db1f56d319877a212921ae055/adal/src/Microsoft.IdentityModel.Clients.ActiveDirectory/Internal/Instance/Authenticator.cs

Tenant ID parsing is at Line number 108

cc: @trwalke @henrik-me @jennyf19 @bgavrilMS

sumantshiv commented 4 years ago

Updated the typo in Version. It should be 5.2.4.

bgavrilMS commented 4 years ago

I'll take a look at this.

keystroke commented 4 years ago

@jmprieur / @bgavrilMS this is the same thing discussed in #1653, not sure if you want to consolidate. The preference from keyvault team is that all the identity clients are updated to accommodate the spurious authority endpoint.

jennyf19 commented 4 years ago

@sumantshiv @keystroke I have a private build you can try out if you're interested. just send me an email -> jeferrie@microsoft.com

jennyf19 commented 4 years ago

Included in 5.2.7 release