Adjust WithExtraQueryParameters APIs and cache key behavior

[Avery-Dunn]

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

[bgavrilMS]

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

[bgavrilMS]

LGTM, minor option

[neha-bhargava]

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

[Avery-Dunn]

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.

[trwalke]

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)
{...}

[Avery-Dunn]

One the one hand it does seem weird, but on the other hand I couldn't find any guidance saying not to do it like that.

However, after trying out the if statement option I don't think it made it more readable: since CommonParameters.CacheKeyComponents does the same thing a few lines down it'd be 3 if statements in one small method, some of which were wrapped up in a loop.

But I did move the null check/initialization outside of the for loop in the latest commit, since we don't need to do it multiple times.

[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

[bgavrilMS]

nit: It's not really a "core helper" and could be included in ApiConfig or AppConfig

btw, you can rewrite this in 1-liner

 return (parameters ?? Enumerable.Empty<KeyValuePair<string, string>>())
        .ToDictionary(
            kvp => kvp.Key,
            kvp => (kvp.Value, false),
            StringComparer.OrdinalIgnoreCase);