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] Enable changing tenant after client construction #296

Closed
chlowell opened this issue Feb 25, 2022 · 13 comments · Fixed by #343
Closed

[Feature Request] Enable changing tenant after client construction #296

chlowell opened this issue Feb 25, 2022 · 13 comments · Fixed by #343
Labels
AzureSdk enhancement New feature or request

Comments

@chlowell
Copy link
Collaborator

Acquiring tokens as a multi-tenant application is painful because tenants are baked into clients at construction. It's therefore necessary to create a new client instance for each tenant. Please provide an API for overriding the tenant specified at construction during a single token request or enable changing it after construction. MSAL.NET has implemented the former and is evidently moving to the latter: AzureAD/microsoft-authentication-library-for-python#368 (comment).

@henrik-me
Copy link
Contributor

@bgavrilMS @siddhijain @rayluo : can you pls. help with the per request design?

@bgavrilMS
Copy link
Member

In pseudo-code, the logic in MSAL.NET is something like:

var confidentialClientApp = // create app with authority set to login.microsoft.com/common

var authResult = confidentialClientApp.AcquireTokenByMethod.WithTenantId(tenantId).Execute();

The simplified algorithm for choosing the authority on which to perform auth, given an application authority and a request tenantID is:

  • if config authority is not set, use app authority
  • if request.tenantId is configured and app.Authority is not AAD, throw an exception (only AAD supports tenants)
  • if request.tenantId is "common" or "organizations", throw an exception (the API is for forcing an actual tenantid)

The more complex case exists for AcquireTokenSilent, where we can also need to think about Account.HomeTenantId. I am not 100% sure if MSAL Go's AcquireTokenSilent(account) uses account.HomeTenantId, but if it does then if all 3 options are specified, the same as above apply, and prefer to use request.tenantId and not account.homeTenant.Id

Acceptance tests

app.Authority request.TenantId Account.HomeTenantid Expected Authority
lmo/common GUID lmo/GUID
lmo/organizations GUID lmo/GUID
lmo/consumers GUID lmo/GUID
lmo/GUID1 GUID2 lmo/GUID2
lmo/GUID common or organizations null or any ERROR
lmo/GUID1 GUID2 GUID3 lmo/GUID2
lmo/common GUID lmo/GUID

@rayluo
Copy link
Contributor

rayluo commented Oct 8, 2022

app.Authority request.TenantId Account.HomeTenantid Expected Authority
lmo/common GUID lmo/GUID
lmo/organizations GUID lmo/GUID
lmo/consumers GUID lmo/GUID

Hi @bgavrilMS , in the table above, row 3, if an app declared its authority as "lmo/consumers", wouldn't it mean the app does not want to allow AAD users to sign in? Do we still want to allow the guid adjustment in this scenario?

@bgavrilMS
Copy link
Member

Now that you mention it, yes. I don't think MSAL.NET does any better :(

@chlowell
Copy link
Collaborator Author

Thanks for catching that! I added two more error cases to #343:

  • WithTenantID(guid) when the client's configured tenant is "consumers"
  • WithTenantID("consumers") when the client's configured tenant is not "consumers"

@rayluo
Copy link
Contributor

rayluo commented Oct 11, 2022

Thanks for quick response, team!

One more conceptual question. While we say WithTenantId(tenantId), the tenantId can effectively also be a human-readable tenant name such as "contoso.onmicrosoft.com", can't it? In fact, I notice a WithTenant(...) helper function in the current Msal Go PR, its parameter is currently named "ID", but perhaps it would accept a tenant name and just work?

@chlowell
Copy link
Collaborator Author

MSAL for Go will have a few rules for dynamic tenant IDs as described in this issue (implemented here) but otherwise no opinion on their validity. So, we'll take any string and it's up to AAD whether it works. WithTenantID("contoso.onmicrosoft.com") will make the request URL https://authority/contoso.onmicrosoft.com/...

@rayluo
Copy link
Contributor

rayluo commented Oct 11, 2022

Well, in that case, it is really just a "WithTenant(tenant)" parameter, rather than "WithTenantId(tenantId)" which seemingly implies a GUID. Thanks for the clarification.

@chlowell
Copy link
Collaborator Author

That's correct. I assume it's the same for the other MSALs which call the option WithTenantID. Is semantic accuracy more important than consistency with those MSALs?

@rayluo
Copy link
Contributor

rayluo commented Oct 11, 2022

I believe the consistency is more about Conceptual Consistency: if one thing can and should be done in one MSAL, other applicable MSALs would better also have it. But the exact naming always has some variance, due to language convention and casing reasons. In particular, already-shipped parameters do not need to be changed only for consistency, and newly-introduced parameter in an MSAL has an opportunity to "name it right", if desirable.

I'm leaving an approval to your implementation PR now.

@Joon-L
Copy link

Joon-L commented Oct 14, 2022

Hello @rayluo, is there a plan to release a new package version with the WithTenantID support changes? Meanwhile if we want to acquire tokens for multiple tenants is the workaround to create client per tenant?

@bgavrilMS
Copy link
Member

Yes, create a confidential client application for each tenant / authority you have.

@Joon-L
Copy link

Joon-L commented Oct 14, 2022

Got it. Thank you for confirming. It would be ideal if we could avoid this with the new changes. When could we expect a new release version with this change?

@rayluo rayluo mentioned this issue Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AzureSdk enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants