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

[Bug] AbstractAcquireTokenParameterBuilder<T>.WithTenantId() should be supported for CIAM authorities #4191

Closed
jmprieur opened this issue Jun 28, 2023 · 6 comments · Fixed by AzureAD/microsoft-identity-web#2314 or #4208

Comments

@jmprieur
Copy link
Contributor

When doing an OBO, if no tenant is specified, it should be possible to set the tenant to the user tenant (from the tid claim). This will come as a GUID, whereas the authority, in the case of CIAM, is a domain name.

This is blocking Microsoft.Identity.Web OBO samples for CIAM,.

Logs and network traces

Microsoft.Identity.Client.dll!Microsoft.Identity.Client.AbstractAcquireTokenParameterBuilder<Microsoft.Identity.Client.AcquireTokenOnBehalfOfParameterBuilder>.WithTenantId(string tenantId) Line 272 C#
Microsoft.Identity.Web.TokenAcquisition.dll!Microsoft.Identity.Web.TokenAcquisition.GetAuthenticationResultForWebApiToCallDownstreamApiAsync(Microsoft.Identity.Client.IConfidentialClientApplication application, string tenantId, System.Collections.Generic.IEnumerable scopes, Microsoft.Identity.Web.TokenAcquisitionOptions tokenAcquisitionOptions, Microsoft.Identity.Web.MergedOptions mergedOptions) Line 727 C#
Microsoft.Identity.Web.TokenAcquisition.dll!Microsoft.Identity.Web.TokenAcquisition.GetAuthenticationResultForUserAsync(System.Collections.Generic.IEnumerable scopes, string authenticationScheme, string tenantId, string userFlow, System.Security.Claims.ClaimsPrincipal user, Microsoft.Identity.Web.TokenAcquisitionOptions tokenAcquisitionOptions) Line 243 C#
Microsoft.Identity.Web.TokenAcquisition.dll!Microsoft.Identity.Web.TokenAcquisition.GetAccessTokenForUserAsync(System.Collections.Generic.IEnumerable scopes, string authenticationScheme, string tenantId, string userFlow, System.Security.Claims.ClaimsPrincipal user, Microsoft.Identity.Web.TokenAcquisitionOptions tokenAcquisitionOptions) Line 500 C#
Microsoft.Identity.Web.TokenAcquisition.dll!Microsoft.Identity.Web.ITokenAcquisition.GetAccessTokenForUserAsync(System.Collections.Generic.IEnumerable scopes, string tenantId, string userFlow, System.Security.Claims.ClaimsPrincipal user, Microsoft.Identity.Web.TokenAcquisitionOptions tokenAcquisitionOptions) Line 42 C#
TodoListService.dll!TodoListService.Controllers.TodoListController.GetAsync() Line 57 C#

Which version of MSAL.NET are you using?
4.54.1

What authentication flow has the issue?
* [ x] On-Behalf-Of

Other?
With a CIAM authority

Is this a new or existing app?

     ClaimsPrincipal? user = _tokenAcquisitionHost.GetUserFromRequest();
      var userTenant = string.Empty;
      if (user != null)
      {
          userTenant = user.GetTenantId();
          builder.WithCcsRoutingHint(user.GetObjectId(), userTenant);
      }
      if (!string.IsNullOrEmpty(tenantId))
      {
          builder.WithTenantId(tenantId);
      }
      else
      {
          if (!string.IsNullOrEmpty(userTenant))
          {
              builder.WithTenantId(userTenant);
          }
      }

Actual behavior
Exception:
Microsoft.Identity.Client.MsalClientException: 'WithTenantId can only be used when an AAD authority is specified at the application level.'

Expected behavior
It should be possible to override the tenant with a CIAM authority.
It's not up to MSAL.NET to decide if the IdP will reject it or not (it won't in that case)

Possible solution
AuthorityInfo.IsTenantOverrideSupported shoud be set to true for CIAM authority

Additional context / logs / screenshots / links to code

@jmprieur
Copy link
Contributor Author

@bgavrilMS : FYI

@bgavrilMS
Copy link
Member

bgavrilMS commented Jun 28, 2023

  1. But can the user really be in a different tenant? Are guests supported in CIAM?

  2. What are the rules of tenant id substitution? There are 3 types of authority supported as per @trwalke 's integration tests:

Do we want all 3 authorities to be switched to:

https://MSIDLABCIAM2.ciamlogin.com/012345-6789-ABCD-01234567890 ?

Later edit: yes. Trust that the STS will do the right thing.

@jmprieur
Copy link
Contributor Author

jmprieur commented Jul 5, 2023

The problem is when we define the authority, we define it with the domain name, but then when we redeem the code or do an OBO we have the tenantId. We have no way of knowing that the tenant ID maps to the domain name.
OBO in particular always does a .WithTenant(tid in the token) unless the tenant is overridden (your idea, @bgavrilMS)
So this always fail.

@bgavrilMS
Copy link
Member

Yes, for AAD this makes sense and we had several bugs on this - this is why I suggested the fix in MSAL. But for CIAM does changing the tenant make sense or should it be a NO-OP?

@rayluo
Copy link
Contributor

rayluo commented Jul 10, 2023

  1. But can the user really be in a different tenant? Are guests supported in CIAM?

  2. What are the rules of tenant id substitution? There are 3 types of authority supported as per @trwalke 's integration tests:

    • https://MSIDLABCIAM2.ciamlogin.com/

    • https://MSIDLABCIAM2.ciamlogin.com/MSIDLABCIAM2.onmicrosoft.com

    • https://MSIDLABCIAM2.ciamlogin.com/f7416cc8-8ea1-4e5c-b230-0c978f81dfc6

Do we want all 3 authorities to be switched to:

https://MSIDLABCIAM2.ciamlogin.com/012345-6789-ABCD-01234567890 ?

@bgavrilMS , what are the conclusions to the two questions above?

In particular, what is the authority to be used in question 2? Could we end up sending an contradicting https://CONTOSO.ciamlogin.com/guid-of-FABRICAM?

@bgavrilMS
Copy link
Member

Yes, we'll let MSAL construct the authority in the way that the app developer wants it, without any constraints. And we rely on the CIAM STS to error out.

@jmprieur has been testing this out and might have found a bug in CIAM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment