Adjust WithExtraQueryParameters APIs and cache key behavior #5536
Conversation
… deprecate existing WithExtraQueryParameters APIs
src/client/Microsoft.Identity.Client/ApiConfig/BaseAbstractAcquireTokenParameterBuilder.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ApiConfig/BaseAbstractAcquireTokenParameterBuilder.cs
Outdated
Show resolved
Hide resolved
…uireTokenParameterBuilder.cs Co-authored-by: Bogdan Gavril <bogavril@microsoft.com>
| /// For each parameter, you can specify whether it should be included in the cache key. | ||
| /// The parameter can be null.</param> | ||
| /// <returns>The builder to chain .With methods.</returns> | ||
| public T WithExtraQueryParameters(IDictionary<string, (string value, bool includeInCacheKey)> extraQueryParameters) |
There was a problem hiding this comment.
Naming: (string ParameterValue, bool IncludeInCacheKey)
There was a problem hiding this comment.
In the latest commit the names start with an uppercase letter, and I improved the comments explaining each parameter.
| CreateOrUpdatePublicClientApp(InteractiveAuthority, ApplicationId); | ||
|
|
||
| AuthenticationResult result; | ||
| #pragma warning disable CS0618 // Type or member is obsolete |
There was a problem hiding this comment.
Maybe replace these with the new method and remove this line everywhere?
There was a problem hiding this comment.
I think this is the only file where I suppressed the warnings instead of making the actual change. Unlike all the other places, using the new API here would've meant multiple method signature changes, then changes to everything that referenced those methods, then changes to fields in different classes...
It was already started to be a big PR, and those suppressions seemed to be used in a lot of tests, so I figured I'd take a shortcut on this one test app.
| /// 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. | ||
| /// For each parameter, you can specify whether it should be included in the cache key. |
There was a problem hiding this comment.
I would recommend adding some suggestions or examples when it will make sense to add the param to cache key and when to exclude
There was a problem hiding this comment.
In the latest commit I adjusted the comments here and at the app-level API: explained the different parameters in more detail, and gave suggestions on when to set the cache key value to true.
3 participants
This PR is for KR 3310905 and issue #5361.
In short, there is a problem in our existing
WithExtraQueryParametersbehavior: the extra query parameters could affect tokens but were not used as part of the cache keys, so if theWithExtraQueryParametersvalues changed between requests we could return cached tokens that were not valid for the new request. However, simply adding the extra query parameters to the cache key would not work: it would break existing cache lookup behavior, and not all parameters should be cached.This PR tries to solve that problem with the following changes:
WithExtraQueryParametersthat takes in aIDictionary<string, (string value, bool includeInCacheKey)>CacheKeyComponentsfield, which already does the task of adding extra parameters to the cache keyBaseAbstractAcquireTokenParameterBuilder,AbstractApplicationBuilderWithExtraQueryParametersAPIs, and have them call the new API to set theExtraQueryParametersfieldsAbstractAcquireTokenParameterBuilder,BaseAbstractAcquireTokenParameterBuilder,AbstractApplicationBuilderCoreHelpersto covert the old Dictionary style to the new oneThis PR also adds some new tests to cover the new behavior, and makes some small changes to existing tests:
ExtraQueryParametersTests: A new test class with tests covering the new cache key behavior, as well as demonstrating the behavior of the deprecatedWithExtraQueryParametersAPIsCacheKeyExtensionTests: A new test to show the newWithExtraQueryParametersAPI does not conflict with the existingWithAdditionalCacheKeyComponentsAPI from the extensibility package