AbstractAcquireTokenParameterBuilder<T>.WithAdfsAuthority throws with a null TenantId when the authority host Uri ends in `/adfs`

[christothes]

Azure.Identity recently changed from calling the deprecated:

AbstractAcquireTokenParameterBuilder<T>.WithAuthority(AuthorityHost.AbsoluteUri, tenantId);

to this:

UriBuilder uriBuilder = new UriBuilder(AuthorityHost)
{
    Path = tenantId
};
builder.WithTenantIdFromAuthority(uriBuilder.Uri);

When adfs is passed as the tenantId, WithTenantIdFromAuthority throws because tenantId is null.

I looked at the MSAL code and this is the reason we end up with a null TenantId

This creates an AdfsAuthority which hard codes null as the TenantId

Ideally WithTenantIdFromAuthority should handle the ADFS case and do the same thing as WithAuthority(AuthorityHost.AbsoluteUri, tenantId) does.

[bgavrilMS]

1. What is the actual value of the uri in WithTenantIdFromAuthority(uri) ? Someting like https://somehost.com/adfs/
2. How do you create the application object? Do you set WithAuthority(adfsAuthority) ? Or do you leave it as is?

If you leave it as is, the default is the AAD authority https://login.microsoftonline.com/common. Request can alter the tenant only, but not the host.
If you set the authority in the CCA/PCA object as https://somehost.com/adfs/ and then the same on the request, then I agree this is a bug and we can fix it.

[christothes]

1. What is the actual value of the uri in WithTenantIdFromAuthority(uri) ? Someting like https://somehost.com/adfs/

yes, something like that.

2. How do you create the application object? Do you set WithAuthority(adfsAuthority) ? Or do you leave it as is?

We leave it as is, until we apply the tenantId, which is normally required. Here is the specific example

If you leave it as is, the default is the AAD authority https://login.microsoftonline.com/common. Request can alter the tenant only, but not the host. If you set the authority in the CCA/PCA object as https://somehost.com/adfs/ and then the same on the request, then I agree this is a bug and we can fix it.

Yes, that is effectively what we are doing in the code linked above.

[bgavrilMS]

Ok, so you do create the app with the correct authority ?

https://github.com/Azure/azure-sdk-for-net/blob/203f6cdfd9e7dc991a24e0014bf89e693d7a2f7d/sdk/identity/Azure.Identity/src/MsalPublicClient.cs#L45

Here ^^ the authority is set to https://somehost.com/adfs/ ?

[christothes]

Ok, so you do create the app with the correct authority ?

https://github.com/Azure/azure-sdk-for-net/blob/203f6cdfd9e7dc991a24e0014bf89e693d7a2f7d/sdk/identity/Azure.Identity/src/MsalPublicClient.cs#L45

Here ^^ the authority is set to https://somehost.com/adfs/ ?

No - there it would be set only to https://somehost.com/. It is when the token is requested that we build up the tenantId using WithTenantIdFromAuthority

https://github.com/Azure/azure-sdk-for-net/blob/203f6cdfd9e7dc991a24e0014bf89e693d7a2f7d/sdk/identity/Azure.Identity/src/MsalPublicClient.cs#L360-L366

[johnypony3]

hi team, is there an update on this?

[bgavrilMS]

I think we fixed it in the lastest version of MSAL.NET ... Could you try?

[johnypony3]

I took over this project, I arrived here because the specific nuget package: Azure.Identiy was being flagged as a vulnerability.

Would this package be updated or should the project be updated to use msal.net?

We are using this with ASE in disconnected state.

[bgavrilMS]

Just use the latest Azure.Identity and directly refernce Microsoft.Identity.Client version 4.65

[johnypony3]

My apologies, I missed your reply to this.

I attempted your suggestion, and it failed the same way.

I completely forgot about this issue and opened a separate one with code here: Azure/azure-sdk-for-net#47584

    <PackageReference Include="Azure.Identity" Version="1.13.0" />
    <PackageReference Include="Azure.ResourceManager.Compute" Version="1.6.0" />
    <PackageReference Include="Azure.ResourceManager.Network" Version="1.9.0" />
    <PackageReference Include="Azure.ResourceManager.Storage" Version="1.3.0" />