-
Notifications
You must be signed in to change notification settings - Fork 378
Adjust WithExtraQueryParameters APIs and cache key behavior #5536
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
base: main
Are you sure you want to change the base?
Changes from all commits
215f703
3f853e0
6a8aa9e
2921a79
d8e728e
d8d60ef
8ab80c9
5eb3f7d
730714f
8b2d4d4
09e9233
47bfd55
fa56194
da73ffe
5317594
a655e75
741b3fe
baad7a2
b8965dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -89,10 +89,49 @@ public T WithCorrelationId(Guid correlationId) | |||||||||
| /// as a string of segments of the form <c>key=value</c> separated by an ampersand character. | ||||||||||
| /// The parameter can be null.</param> | ||||||||||
| /// <returns>The builder to chain the .With methods.</returns> | ||||||||||
| [Obsolete("This method is deprecated. Use the WithExtraQueryParameters(IDictionary<string, (string value, bool includeInCacheKey)>) method instead, which provides control over which parameters are included in the cache key.", false)] | ||||||||||
| public T WithExtraQueryParameters(Dictionary<string, string> extraQueryParameters) | ||||||||||
| { | ||||||||||
| CommonParameters.ExtraQueryParameters = extraQueryParameters ?? | ||||||||||
| new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); | ||||||||||
| return WithExtraQueryParameters(CoreHelpers.ConvertToTupleParameters(extraQueryParameters)); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /// <summary> | ||||||||||
| /// Sets Extra Query Parameters for the query string in the HTTP authentication request with control over which parameters are included in the cache key | ||||||||||
| /// </summary> | ||||||||||
| /// <param name="extraQueryParameters">This parameter will be appended as is to the query string in the HTTP authentication request to the authority, and merged with those added to the application-level WithExtraQueryParameters API. | ||||||||||
| /// Each dictionary entry maps a parameter name to a tuple containing: | ||||||||||
| /// - Value: The parameter value that will be appended to the query string | ||||||||||
| /// - IncludeInCacheKey: Whether this parameter should be included when computing the token's cache key. | ||||||||||
| /// To help ensure the correct token is returned from the cache, IncludeInCacheKey should be true if the parameter affects token content or validity (e.g., resource-specific claims or parameters). | ||||||||||
| /// The parameter can be null.</param> | ||||||||||
| /// <returns>The builder to chain .With methods.</returns> | ||||||||||
| public T WithExtraQueryParameters(IDictionary<string, (string Value, bool IncludeInCacheKey)> extraQueryParameters) | ||||||||||
| { | ||||||||||
| if (extraQueryParameters == null) | ||||||||||
| { | ||||||||||
| CommonParameters.ExtraQueryParameters = null; | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should you also remove cache keys that were previously set? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the same as the current behavior: Line 92 in 7bf0a28
If a null There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would clarify the behavior of this new API. Because this one adds the cache key, the previous one didn't. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What sort of clarification do you think it needs? The comments above it explain how to get the query parameters added to the cache key (set that boolean to true). So if a customer passes in some parameters, we will add those parameters to the cache key. And if a customer passes in nothing, then we will add nothing to the cache key. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so, to remove don't you need to add again, as I said - I am not looking into the requirements doc, I am just asking if this is a possibility. If you think this is not a valid case, ignore this comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This dictionary builds per request. I don't think we need remove. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this is per-request behavior, so if a customer changes what parameters should be used in the cache key then the new request will have a new cache entry, and the cache entry for the other request will be unaffected. I just added
Requests 1 and 2 result in different tokens even though they would have the same contents, but request 3 finds the token from request 2 even though they have different contents. In short, the new API allows a caller to recreate the exact behavior that was a problem in the old API, where we aren't uniquely identifying a token based on the query parameters originally used to retrieve it. However, I don't think this is something we need to fix: the API is meant to give the caller control over how tokens are identified in the cache, and if they want to keep turning the option on and off they then presumably they some reason for it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
how do we communicate this to the caller? |
||||||||||
| return this as T; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| CommonParameters.ExtraQueryParameters = CommonParameters.ExtraQueryParameters ?? new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); | ||||||||||
|
|
||||||||||
| // Add each parameter to ExtraQueryParameters and, if requested, to CacheKeyComponents | ||||||||||
| foreach (var kvp in extraQueryParameters) | ||||||||||
| { | ||||||||||
| CommonParameters.ExtraQueryParameters[kvp.Key] = kvp.Value.Value; | ||||||||||
|
|
||||||||||
| if (kvp.Value.IncludeInCacheKey) | ||||||||||
| { | ||||||||||
| CommonParameters.CacheKeyComponents = CommonParameters.CacheKeyComponents ?? new SortedList<string, Func<CancellationToken, Task<string>>>(); | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. create the SortedList with a stable, case‑insensitive, ordinal comparer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please add a case sensitive test to validate this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is how the CacheKeyComponents field is set in other parts of the library: Line 159 in 7bf0a28
Is there a unique issue here, or should it also be fixed elsewhere? Making it more stable here would mean we each change the existing behavior elsewhere too, or make this version inconsistent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. adding a case sensitive test should validate if there are existing bugs in the code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the SortedList will use its default comparer if one is not specified. By default, this is Comparer.Default, which for string is StringComparer.Ordinal. The behavior will be same whether or not you add ordinal comparer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can validate this with test cases.
gladjohn marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about this when I was first writing it, but as best as I can tell (through Copilot) the "??=" was only introduced in newer frameworks than we support, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIK, this should work in all current TFMs we have |
||||||||||
|
|
||||||||||
| // Capture the value in a local to avoid closure issues | ||||||||||
| string valueToCache = kvp.Value.Value; | ||||||||||
|
|
||||||||||
| // Add to cache key components - uses a func that returns the value as a task | ||||||||||
| CommonParameters.CacheKeyComponents[kvp.Key] = (CancellationToken _) => Task.FromResult(valueToCache); | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can someone remove a cache key? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you mean? Like remove a single, specific entry in the token cache? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. something like, Request 1 - Request 2 - There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If someone selects to not add the extra query param to the cache I think the cache key will be different for that request and the request will go to IDP There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #5536 (comment) for more context, but I I just added
|
||||||||||
|
|
||||||||||
| return this as T; | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| Microsoft.Identity.Client.AbstractApplicationBuilder<T>.WithExtraQueryParameters(System.Collections.Generic.IDictionary<string, (string Value, bool IncludeInCacheKey)> extraQueryParameters) -> T | ||
| Microsoft.Identity.Client.BaseAbstractAcquireTokenParameterBuilder<T>.WithExtraQueryParameters(System.Collections.Generic.IDictionary<string, (string Value, bool IncludeInCacheKey)> extraQueryParameters) -> T | ||
| const Microsoft.Identity.Client.MsalError.CannotSwitchBetweenImdsVersionsForPreview = "cannot_switch_between_imds_versions_for_preview" -> string | ||
| const Microsoft.Identity.Client.MsalError.InvalidCertificate = "invalid_certificate" -> string | ||
| const Microsoft.Identity.Client.MsalError.MtlsNotSupportedForManagedIdentity = "mtls_not_supported_for_managed_identity" -> string | ||
|
|
@@ -7,4 +9,4 @@ Microsoft.Identity.Client.IMsalMtlsHttpClientFactory.GetHttpClient(System.Securi | |
| Microsoft.Identity.Client.ManagedIdentityApplication.GetManagedIdentitySourceAsync() -> System.Threading.Tasks.Task<Microsoft.Identity.Client.ManagedIdentity.ManagedIdentitySource> | ||
| Microsoft.Identity.Client.ManagedIdentity.ManagedIdentitySource.ImdsV2 = 8 -> Microsoft.Identity.Client.ManagedIdentity.ManagedIdentitySource | ||
| Microsoft.Identity.Client.ManagedIdentityApplicationBuilder.WithExtraQueryParameters(System.Collections.Generic.IDictionary<string, string> extraQueryParameters) -> Microsoft.Identity.Client.ManagedIdentityApplicationBuilder | ||
| static Microsoft.Identity.Client.ApplicationBase.ResetStateForTest() -> void | ||
| static Microsoft.Identity.Client.ApplicationBase.ResetStateForTest() -> void | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can this be undone? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure why Github is marking that as a change, that line's the same as it is in the main branch: https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/main/src/client/Microsoft.Identity.Client/PublicApi/net462/PublicAPI.Unshipped.txt Maybe it's just because my lines went to the top instead of the bottom of the file? I just let it merge automatically. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| Microsoft.Identity.Client.AbstractApplicationBuilder<T>.WithExtraQueryParameters(System.Collections.Generic.IDictionary<string, (string Value, bool IncludeInCacheKey)> extraQueryParameters) -> T | ||
| Microsoft.Identity.Client.BaseAbstractAcquireTokenParameterBuilder<T>.WithExtraQueryParameters(System.Collections.Generic.IDictionary<string, (string Value, bool IncludeInCacheKey)> extraQueryParameters) -> T | ||
| const Microsoft.Identity.Client.MsalError.CannotSwitchBetweenImdsVersionsForPreview = "cannot_switch_between_imds_versions_for_preview" -> string | ||
| const Microsoft.Identity.Client.MsalError.InvalidCertificate = "invalid_certificate" -> string | ||
| const Microsoft.Identity.Client.MsalError.MtlsNotSupportedForManagedIdentity = "mtls_not_supported_for_managed_identity" -> string | ||
|
|
@@ -7,4 +9,4 @@ Microsoft.Identity.Client.IMsalMtlsHttpClientFactory.GetHttpClient(System.Securi | |
| Microsoft.Identity.Client.ManagedIdentityApplication.GetManagedIdentitySourceAsync() -> System.Threading.Tasks.Task<Microsoft.Identity.Client.ManagedIdentity.ManagedIdentitySource> | ||
| Microsoft.Identity.Client.ManagedIdentity.ManagedIdentitySource.ImdsV2 = 8 -> Microsoft.Identity.Client.ManagedIdentity.ManagedIdentitySource | ||
| Microsoft.Identity.Client.ManagedIdentityApplicationBuilder.WithExtraQueryParameters(System.Collections.Generic.IDictionary<string, string> extraQueryParameters) -> Microsoft.Identity.Client.ManagedIdentityApplicationBuilder | ||
| static Microsoft.Identity.Client.ApplicationBase.ResetStateForTest() -> void | ||
| static Microsoft.Identity.Client.ApplicationBase.ResetStateForTest() -> void | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same comment like before There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as your other comment, that line here is exactly the same as in the main branch so I'm not sure why Github is saying it changed. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be an experimental API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bgavrilMS can weigh in on this because I'm not sure how y'all decide what to put behind that experimental flag.
In our initial discussion we also planned on putting certain extensibility APIs behind the experimental flag, but it turned out the relevant API already was using it:
microsoft-authentication-library-for-dotnet/src/client/Microsoft.Identity.Client/Extensibility/AcquireTokenForClientBuilderExtensions.cs
Line 52 in 7bf0a28