Adjusting token cache to work with ADFS2019 #77
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
abhidnya13
approved these changes
Jul 22, 2019
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR will eventually resolve #54 .
1. Research
Right now (when this PR is first created), it contains only untested and uncompleted proof-of-concept code, solely based on the following understandings of same feature in MSAL .Net implementation, about what potential implementation changes would be needed to support ADFS2019.
In MSAL .Net, since tenant_id in ADFS scenario would be null, so the HomeAccountId would just use
oid(which is subject from IdToken in this context), such as:https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/pull/834/files#diff-f9f5327b7941596b4cb9f5f97f0c1d25R45
Such behavior is different than Unified Cache schema convention of using
uid.utidto build HomeAccountId. There was also an open conversation for this in the internal ApiReview PR, line 40.Here in this MSAL Python PR, the home_account_id would look like "subject1234.adfs" because the literal "adfs" part is conventionally the "tenant" placeholder for an ADFS authorityBased on investigation documented in subsequent internal ApiReview comments, we also follow the MSAL .Net approach to computehttps://fs.mdidlab8.com/adfsalready, so that "adfs" keyword is now used as a tenant id placeholder too.home_account_idby ID token's "sub" claim only.ID Token from ADFS 2019 will contain "upn", but not a "preferred_username".
https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/pull/834/files#diff-b589e656da83aee7a0a5b4eb25a75a26R67
https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/pull/834/files#diff-493cb9b85dfb569d469984a55883699cR53
Per convention, an ADFS authority uses "adfs" as its tenant name.
https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/pull/834/files#diff-7c9455d24a0c6c4e5ddcd5bcf8069710R63
Because ADFS2019's webfinger url does NOT contain "adfs" as its root path, MSAL .Net also changes its webfinger relevant logic, here and there. MSAL Python did not use webfinger so this is not relevant.
ADFS does not have a concept of a tenant ID. This prevents ADFS from supporting multiple tenants
https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/pull/834/files#diff-f167606f212022b725fc558995efababR26
client_info won't be available from ADFS 2019 response
https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/pull/834/files#diff-ab96d4792df914c78c0b6f386e82f384R192
MSAL .Net writes a log when/if the "subject" would be absent from ADFS 2019 id token. In MSAL Python we just make sure we won't blow up with KeyError exception.
In MSAL .Net, "The preferred_username value cannot be null or empty in order to comply with the ADAL/MSAL Unified cache schema. It will be set to "preferred_username not in idtoken". In MSAL Python, the raw id token is flexible so we do not need to change anything about the id token itself, we just make sure the account would be populated by ADFS id token's
upn.In MSAL .Net, some adjustment on Authority Migration seems necessary. In MSAL Python, the happy path would yield a token hit, so Authority Migration does not need to kick in.
In MSAL .Net, cache lookup logic needs to handle such tenant-less situation, and probably some of these 800 line changes were also relevant. In MSAL Python, we use "adfs" as a placeholder for tenant_id, and then the existing cache logic seems to work just fine. We added a new unit test case in this PR too.
The rest 2/3 of the .Net PRs are tests.
2. Self-Check
After implementation, we review the checklist above, leave explanation when implementation detail is significantly different (but the end result would be the same), and then mark those items as checked.
3. Known Limitations
Confidential Client with Certificate scenario is not tested this time. Test environment is not available.
Device Flow would work but due to ADFS 2019 does not really accept
scopeparameter (it acceptsresource), the scopes passed in MSAL Python will actually be ignored by ADFS 2019. So the tokens returned would presumably contain some default authorizations only.PS: Thanks @abhidnya13 for lots of hard work especially in testing!