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

[Feature Request] Allow multi-tenant applications to specify the AppHomeTenantId to be used for client credentials. #3121

Closed
4 of 6 tasks
jmprieur opened this issue Nov 1, 2024 · 9 comments
Assignees
Labels
enhancement New feature or request feature request
Milestone

Comments

@jmprieur
Copy link
Collaborator

jmprieur commented Nov 1, 2024

Is your feature request related to a problem? Please describe.
When a multi-tenant application (TenantId: "common" or "organizations") wants to acquire a token on behalf of itself, Microsoft.Identity.Web can't use the "TenantId" as this is not accepted by ESTS.

Today we require all developers to specify the tenant ID to use by code, or by configuration in the case of IDownstreamApi:

Describe the solution you'd like

  • Add in the MicrosoftIdentityApplicationOptions (in Microsoft.Identity.Abstractions), a new string property AppHomeTenantId, which developers of multi-tenant apps could set in order to define the home tenant of the app (the tenant for which the app can get a token to call a downstream API on behalf of itself). This will usually be the app registration tenant. For instance, add it here
  • In Microsoft.Identity.Web,
    • Add the same property to MergeOptions, and update the method that populates MergedOptions from MicrosoftIdentityApplicationOptions.
    • when the token acquisition happens "for App", and if the TenantId is "common" or "organization" and the developer has not overriden the "TenantId" in the AcquireTokenOptions, use the AppHomeTenantId value. For this we would need to change this expression here to use mergedOptions.AppHomeTenantId if it's not null, and throw otherwise, like today
  • For Auto-decrypt (SAL) use the AppHomeTenantId if the CredentialDescription.DownstreamApi.AcquireTokenOptions.Tenant is not specified. Might not be necessary as IdWeb does the token acquisition, but needs to be verified
  • For MISE modules data providers, remove the configuration property that was using this tenant and let IdWeb do (or use the new property, instead of exposing it in each module configuration).
     
    Describe alternatives you've considered
    Multiply the same tenant value in all cases of auto-decrypt and MISE modules like today
@jmprieur jmprieur added enhancement New feature or request feature request labels Nov 1, 2024
@msbw2
Copy link
Contributor

msbw2 commented Nov 5, 2024

@jmprieur

if the TenantId is "common" or "organization" and the developer has not overriden the "TenantId" in the AcquireTokenOptions, use the AppHomeTenantId value.

What does this mean? This seems conflicting (TenantId is both one of common or organizations but also null).

@jmprieur
Copy link
Collaborator Author

jmprieur commented Nov 5, 2024

You are right @msbw2, I was not very clear.
in this method, there are 3 tenants involved:

  • tenant the parameter of the method. This comes from the AcquireTokenOptions.Tenant.
  • mergedOptions.TenantId which comes from the configuration (AzureAd section, TenantId property)
  • mergedOptions.AppHomeTenantId which is the new property.

the tenant parameter always has precedence if it's not null. if it's null, we use mergedOptions.TenantId unless it contains common or organizations in which case we use mergedOptions.AppHomeTenantId, unless it's null in which case we throw the existing exception.

@msbw2
Copy link
Contributor

msbw2 commented Nov 7, 2024

You are right @msbw2, I was not very clear. in this method, there are 3 tenants involved:

  • tenant the parameter of the method. This comes from the AcquireTokenOptions.Tenant.
  • mergedOptions.TenantId which comes from the configuration (AzureAd section, TenantId property)
  • mergedOptions.AppHomeTenantId which is the new property.

the tenant parameter always has precedence if it's not null. if it's null, we use mergedOptions.TenantId unless it contains common or organizations in which case we use mergedOptions.AppHomeTenantId, unless it's null in which case we throw the existing exception.

@jmprieur Can you help me finish this table?

tenant parameter AzureAd:TenantId AzureAd:AppHomeTenantId effective tenant
set * * tenant
not set set and GUID or domain * AzureAd:TenantId
not set set and common or organization set AzureAd:AppHomeTenantId
not set set and common or organization not set current exception
not set not set 🚫 would fail earlier (but indeed AzureAd:AppHomeTenantId could be possible otherwise)
not set not set not set 🚫 would fail earlier, and needs to fail

@jmprieur
Copy link
Collaborator Author

jmprieur commented Nov 7, 2024

Sure, @msbw2. Thanks for formalizing this. I think I updated your table

@msbw2
Copy link
Contributor

msbw2 commented Nov 7, 2024

@msbw2
Copy link
Contributor

msbw2 commented Nov 7, 2024

@jmprieur #3132

@msbw2
Copy link
Contributor

msbw2 commented Nov 7, 2024

  • For Auto-decrypt (SAL) use the AppHomeTenantId if the CredentialDescription.DownstreamApi.AcquireTokenOptions.Tenant is not specified. Might not be necessary as IdWeb does the token acquisition, but needs to be verified
  • For MISE modules data providers, remove the configuration property that was using this tenant and let IdWeb do (or use the new property, instead of exposing it in each module configuration).

@jmprieur Do you have pointers to these? In SAL I only found two references to the property, but I'm not sure where AppHomeTenantId would be surfaced. In MISE none of the data providers appear to reference it or use a tenant from config whatsoever.

@jmprieur
Copy link
Collaborator Author

jmprieur commented Nov 7, 2024

I'll check @msbw2. Thank you

@jennyf19 jennyf19 added this to the 3.3.2 milestone Nov 7, 2024
@jennyf19
Copy link
Collaborator

released in 3.4.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature request
Projects
None yet
Development

No branches or pull requests

3 participants