Adjust WithExtraQueryParameters APIs and cache key behavior #5536

Avery-Dunn · 2025-10-18T02:02:32Z

This PR is for KR 3310905 and issue #5361.

In short, there is a problem in our existing WithExtraQueryParameters behavior: the extra query parameters could affect tokens but were not used as part of the cache keys, so if the WithExtraQueryParameters values 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:

Add a new WithExtraQueryParameters that takes in a IDictionary<string, (string value, bool includeInCacheKey)>
- Essentially, instead of a key-value pair of strings the value is a tuple, allowing the caller to control which parameters are used in the cache key
- This new API adds the extra query parameters to the existing CacheKeyComponents field, which already does the task of adding extra parameters to the cache key
- Affects: BaseAbstractAcquireTokenParameterBuilder, AbstractApplicationBuilder
Deprecate the existing WithExtraQueryParameters APIs, and have them call the new API to set the ExtraQueryParameters fields
- Affects: AbstractAcquireTokenParameterBuilder, BaseAbstractAcquireTokenParameterBuilder, AbstractApplicationBuilder
New internal helper method in CoreHelpers to covert the old Dictionary style to the new one

This 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 deprecated WithExtraQueryParameters APIs
CacheKeyExtensionTests: A new test to show the new WithExtraQueryParameters API does not conflict with the existing WithAdditionalCacheKeyComponents API from the extensibility package
The rest of the changes to the test files were just to follow the new style and avoid deprecation warnings, no test behavior was actually changed

… deprecate existing WithExtraQueryParameters APIs

src/client/Microsoft.Identity.Client/ApiConfig/BaseAbstractAcquireTokenParameterBuilder.cs

bgavrilMS

LGTM, but the tuple in public API is not going to work

…uireTokenParameterBuilder.cs Co-authored-by: Bogdan Gavril <[email protected]>

src/client/Microsoft.Identity.Client/ApiConfig/BaseAbstractAcquireTokenParameterBuilder.cs

bgavrilMS

LGTM, minor option

src/client/Microsoft.Identity.Client/Utils/CoreHelpers.cs

neha-bhargava · 2025-10-23T16:52:14Z

tests/devapps/DesktopTestApp/PublicClientHandler.cs

            CreateOrUpdatePublicClientApp(InteractiveAuthority, ApplicationId);

            AuthenticationResult result;
+#pragma warning disable CS0618 // Type or member is obsolete


Maybe replace these with the new method and remove this line everywhere?

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.

neha-bhargava · 2025-10-23T18:07:30Z

src/client/Microsoft.Identity.Client/ApiConfig/BaseAbstractAcquireTokenParameterBuilder.cs

+        /// 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.


I would recommend adding some suggestions or examples when it will make sense to add the param to cache key and when to exclude

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.

trwalke · 2025-10-24T10:00:45Z

src/client/Microsoft.Identity.Client/ApiConfig/BaseAbstractAcquireTokenParameterBuilder.cs

+            // Add each parameter to ExtraQueryParameters and, if requested, to CacheKeyComponents
+            foreach (var kvp in extraQueryParameters)
+            {
+                CommonParameters.ExtraQueryParameters = CommonParameters.ExtraQueryParameters ?? new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);


hmm, setting a value to itself seems like an odd way to check if something is null and then initializing it. It can be misleading potentially. I would simply add a normal if check.

if (CommonParameters.ExtraQueryParameters is null) {...}

Copilot

Pull Request Overview

This PR addresses an issue where extra query parameters could affect token validity but were not included in cache keys, potentially causing invalid cached tokens to be returned when parameters changed between requests.

Introduces a new WithExtraQueryParameters API accepting IDictionary<string, (string value, bool includeInCacheKey)> for granular cache key control
Deprecates existing WithExtraQueryParameters methods that accept string or IDictionary<string, string> parameters
Updates test code and dev apps to use the new API or suppress deprecation warnings

Reviewed Changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/devapps/NetFxConsoleTestApp/Program.cs	Updated to use new tuple-based API
tests/devapps/NetCoreTestApp/Program.cs	Updated to use new tuple-based API
tests/devapps/DesktopTestApp/PublicClientHandler.cs	Added deprecation warning suppressions for old API usage
tests/Microsoft.Identity.Test.Unit/TelemetryTests/OTelInstrumentationTests.cs	Updated tests to use new tuple-based API
tests/Microsoft.Identity.Test.Unit/TelemetryTests/HttpTelemetryTests.cs	Updated tests to use new tuple-based API
tests/Microsoft.Identity.Test.Unit/RequestsTests/IntegratedWindowsAuthUsernamePasswordTests.cs	Updated to use new test constant
tests/Microsoft.Identity.Test.Unit/PublicApiTests/ExtraQueryParametersTests.cs	New test class demonstrating cache key behavior
tests/Microsoft.Identity.Test.Unit/PublicApiTests/ConfidentialClientApplicationTests.cs	Updated to use new test constant
tests/Microsoft.Identity.Test.Unit/PublicApiTests/CacheKeyExtensionTests.cs	Added tests for API interaction
tests/Microsoft.Identity.Test.Unit/ParallelRequestsTests.cs	Updated to use new tuple-based API
tests/Microsoft.Identity.Test.Unit/OAuthClientTests.cs	Updated to use new tuple-based API
tests/Microsoft.Identity.Test.Unit/ApiConfigTests/AcquireTokenInteractiveBuilderTests.cs	Updated to use new tuple-based API
tests/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/FmiIntegrationTests.cs	Updated integration tests to use new API
tests/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/Agentic.cs	Updated to use new tuple-based API
tests/Microsoft.Identity.Test.Common/TestConstants.cs	Added new test constant for tuple-based parameters
src/client/Microsoft.Identity.Client/Utils/CoreHelpers.cs	Added helper method to convert old dictionary format to new tuple format
src/client/Microsoft.Identity.Client/PublicApi/*/PublicAPI.Unshipped.txt	Documents new public API methods
src/client/Microsoft.Identity.Client/Microsoft.Identity.Client.csproj	Added System.ValueTuple package reference
src/client/Microsoft.Identity.Client/AppConfig/AbstractApplicationBuilder.cs	Implemented new API and deprecated old ones
src/client/Microsoft.Identity.Client/ApiConfig/BaseAbstractAcquireTokenParameterBuilder.cs	Implemented new API and deprecated old ones
src/client/Microsoft.Identity.Client/ApiConfig/AbstractAcquireTokenParameterBuilder.cs	Deprecated old string-based API

Copilot · 2025-10-24T13:01:37Z

tests/Microsoft.Identity.Test.Unit/PublicApiTests/ExtraQueryParametersTests.cs

+
+                // Verify final cache state
+                Assert.AreEqual(4, app.AppTokenCacheInternal.Accessor.GetAllAccessTokens().Count, "Should have 4 app tokens in cache");
+                Assert.AreEqual(1, app.UserTokenCacheInternal.Accessor.GetAllAccessTokens().Count, "Should have 2 user tokens in cache");


The assertion message states 'Should have 2 user tokens in cache', but the assertion is checking for 1 token. The message should be corrected to 'Should have 1 user token in cache'.

Suggested change

Assert.AreEqual(1, app.UserTokenCacheInternal.Accessor.GetAllAccessTokens().Count, "Should have 2 user tokens in cache");

Assert.AreEqual(1, app.UserTokenCacheInternal.Accessor.GetAllAccessTokens().Count, "Should have 1 user token in cache");

Add new WithExtraQueryParameters API and behavior affects cache keys,…

215f703

… deprecate existing WithExtraQueryParameters APIs

Avery-Dunn requested a review from a team as a code owner October 18, 2025 02:02

Avery-Dunn added 4 commits October 19, 2025 16:15

Fix tests

3f853e0

Fix test

6a8aa9e

More test fixes

2921a79

Test fix

d8e728e

bgavrilMS reviewed Oct 20, 2025

View reviewed changes

src/client/Microsoft.Identity.Client/ApiConfig/BaseAbstractAcquireTokenParameterBuilder.cs Outdated Show resolved Hide resolved

bgavrilMS reviewed Oct 20, 2025

View reviewed changes

src/client/Microsoft.Identity.Client/ApiConfig/BaseAbstractAcquireTokenParameterBuilder.cs Outdated Show resolved Hide resolved

bgavrilMS requested changes Oct 20, 2025

View reviewed changes

Avery-Dunn requested a review from bgavrilMS October 22, 2025 18:27

Update src/client/Microsoft.Identity.Client/ApiConfig/BaseAbstractAcq…

d8d60ef

…uireTokenParameterBuilder.cs Co-authored-by: Bogdan Gavril <[email protected]>

bgavrilMS reviewed Oct 22, 2025

View reviewed changes

src/client/Microsoft.Identity.Client/ApiConfig/BaseAbstractAcquireTokenParameterBuilder.cs Outdated Show resolved Hide resolved

bgavrilMS approved these changes Oct 22, 2025

View reviewed changes

neha-bhargava reviewed Oct 23, 2025

View reviewed changes

src/client/Microsoft.Identity.Client/Utils/CoreHelpers.cs Outdated Show resolved Hide resolved

neha-bhargava reviewed Oct 23, 2025

View reviewed changes

Avery-Dunn and others added 2 commits October 23, 2025 15:39

PR feedback

8ab80c9

Merge branch 'main' into avdunn/extra-query-params-api-changes

5eb3f7d

trwalke reviewed Oct 24, 2025

View reviewed changes

gladjohn requested a review from Copilot October 24, 2025 13:01

Copilot AI reviewed Oct 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Adjust WithExtraQueryParameters APIs and cache key behavior #5536

Adjust WithExtraQueryParameters APIs and cache key behavior #5536

Uh oh!

Avery-Dunn commented Oct 18, 2025

Uh oh!

Uh oh!

Uh oh!

bgavrilMS left a comment

Uh oh!

Uh oh!

bgavrilMS left a comment

Uh oh!

Uh oh!

neha-bhargava Oct 23, 2025

Uh oh!

Avery-Dunn Oct 23, 2025

Uh oh!

neha-bhargava Oct 23, 2025

Uh oh!

Avery-Dunn Oct 23, 2025

Uh oh!

trwalke Oct 24, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Oct 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	Assert.AreEqual(1, app.UserTokenCacheInternal.Accessor.GetAllAccessTokens().Count, "Should have 2 user tokens in cache");
	Assert.AreEqual(1, app.UserTokenCacheInternal.Accessor.GetAllAccessTokens().Count, "Should have 1 user token in cache");

Uh oh!

Adjust WithExtraQueryParameters APIs and cache key behavior #5536

Are you sure you want to change the base?

Adjust WithExtraQueryParameters APIs and cache key behavior #5536

Uh oh!

Conversation

Avery-Dunn commented Oct 18, 2025

Uh oh!

Uh oh!

Uh oh!

bgavrilMS left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bgavrilMS left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

neha-bhargava Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Avery-Dunn Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

neha-bhargava Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Avery-Dunn Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

trwalke Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants