Supports dSTS by ClientApplication(..., authority="https://...example.com/dstsv2/...") #772
Conversation
rayluo
changed the title
| @@ -623,6 +624,9 @@ def __init__( | |||
| # Here the self.authority will not be the same type as authority in input | |||
| if oidc_authority and authority: | |||
| raise ValueError("You can not provide both authority and oidc_authority") | |||
| if isinstance(authority, str) and urlparse(authority).path.startswith( | |||
| "/dstsv2"): # dSTS authority's path always starts with "/dstsv2" | |||
| oidc_authority = authority # So we treat it as if an oidc_authority | |||
There was a problem hiding this comment.
+1
I am wondering though how does MSAL Py handle changing the tenant at the request level? I am asking because Azure SDK does this and Azure SDK wants to remain agnostic of authority type (i.e. they want MSAL to just work with AAD, ADFS, dSTS)
There was a problem hiding this comment.
The simplest is to have request.WithTenantId(tid) do nothing in case the authority is dSTS.
Needs a unit test.
There was a problem hiding this comment.
MSAL Python does not currently support request.WithTenant(...) at all. Confirmed with a downstream partner that they are not currently using it either. It was tracked as a feature request, which we can work on.
Do we want to proceed with this PR as-is, or mark it as blocked while we pick up the WithTenant(...) feature now?
tests/test_authority.py
Outdated
| class DstsAuthorityTestCase(OidcAuthorityTestCase): | ||
| # Inherits OidcAuthority's test cases and run them with a dSTS authority | ||
| authority = ( # dSTS is single tenanted with a dummy tenant placeholder | ||
| 'https://test-instance1-dsts.dsts.core.azure-test.net/dstsv2/tid') |
There was a problem hiding this comment.
Can we also add a test case with common? I think with dsts there can be a tenant id or common used in the authority.
There was a problem hiding this comment.
I'm not really sure how common is "common" :) and user discovery in dsts. It may be worth asking Sai before.
There was a problem hiding this comment.
I'm not really sure how common is "common" :) and user discovery in dsts. It may be worth asking Sai before.
You meant OIDC discovery? This is the OIDC discovery for common and this is for the non-common. I'll get back to you when I hear back from Sai.
There was a problem hiding this comment.
Right, "common" is supported but doesn't have the same meaning and eSTS "common", since dSTS is single tenanted. So to me it doesn't make sense to inherit all of eSTS complex logic around multi-tenancy, such as replacing "common" with the id token's tid in the cache etc.
A test is fine.
There was a problem hiding this comment.
You meant OIDC discovery? This is the OIDC discovery for common and this is for the non-common. I'll get back to you when I hear back from Sai.
Confirmed with Sai that the seemingly two sets of authorities, common and un-common, are just a side effect of reusing some eSTS code. Under the hood, that common endpoint will still convert the "common" into the tenant id and mint a token for that tenant id.
So to me it doesn't make sense to inherit all of eSTS complex logic around multi-tenancy, such as replacing "common" with the id token's
tidin the cache etc.
MSAL Python's tenant agnostic approach does not attempt converting "common" into tid.
Feature request #767 calls for support for dSTS authority.
The understanding was that:
oidc_authority(i.e. no Instance Discovery, no "/v2.0" hardcoded string within its endpoint), so, it should just work if the caller would choose to put a dSTS url into theoidc_authorityparameter, even if we might not advertise so.authorityparameter, so that MSAL's downstream ecosystem can support dSTS transparently without needing to pick up theoidc_authorityparameter first.Therefore, this PR attempts an implementation that simply converts the
authority=https://foo.bar.example.com/dstsv2/placeholderintooidc_authority=https://foo.bar.example.com/dstsv2/placeholderunder the hood, and then all the oidc authority behaviors will automatically kick in.With regard to the tests:
authority=https://foo.bar.example.com/dstsv2/placeholder(therefore testing point B above)#2doesn't apply because MSAL Python does not supportWithTenant(...). But I'm open to add more test cases if desirable.This will resolve #767
P.S.: The test automation is currently failing due to other reason. They will be fixed soon outside of this PR. This situation does not prevent this PR from being reviewed.Test automation works again now.