Remove silent auth special case for home tenant aliases #375
Conversation
This file contains 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
chlowell
changed the title
chlowell
added
bug
rayluo
reviewed
Jan 24, 2023
rayluo
approved these changes
Jan 24, 2023
|
Kudos, SonarCloud Quality Gate passed!
|
bgavrilMS
reviewed
Jan 25, 2023
| tenant := silent.TenantID | ||
| if tenant == "" { |
There was a problem hiding this comment.
😆 it's definitely that. For reference, here's AuthParams.WithTenant(), which sets the tenant for each authentication and uses the client's configured tenant when the caller doesn't specify an override.
bgavrilMS
approved these changes
Jan 25, 2023
|
It does; this change passes the prior tests and adds coverage for the affected scenario, and we've also tested it downstream in the Azure SDK and azd. |
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.
#366 made access token cache reads prefer a user's home tenant over an alias for that tenant ("common", "organizations"). However, it didn't make a corresponding change to cache writes, which use the alias for the realm. The result is an unnecessary token request during silent auth because the cache wrote a token for e.g. "common" and the client searches for one matching the home tenant.
This PR prevents that unnecessary request by making the cache tenant agnostic according to the rationale here: AzureAD/microsoft-authentication-library-for-python#341 (comment). When the client requests a token from "common", for example, it will cache that token with realm "common". I first took the other approach, having cache writes follow reads in replacing aliases with the user's home tenant ID, but found making that work might require a separate implementation of
AcquireTokenSilentfor OBO authentication. The "tenant agnostic" approach is much simpler.