Skip to content
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ public T WithClaims(string claims)
/// The string needs to be properly URL-encoded and ready to send as a string of segments of the form <c>key=value</c> separated by an ampersand character.
/// </param>
/// <returns>The builder to chain .With methods.</returns>
[Obsolete("This method is deprecated. Please 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(string extraQueryParameters)
{
if (!string.IsNullOrWhiteSpace(extraQueryParameters))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,46 @@ 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. Please 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.
/// For each parameter, you can specify whether it should be included in the cache key.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/// The parameter can be null.</param>
/// <returns>The builder to chain .With methods.</returns>
public T WithExtraQueryParameters(IDictionary<string, (string value, bool includeInCacheKey)> extraQueryParameters)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming: (string ParameterValue, bool IncludeInCacheKey)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the latest commit the names start with an uppercase letter, and I improved the comments explaining each parameter.

{
if (extraQueryParameters == null)
{
CommonParameters.ExtraQueryParameters = null;
return this as T;
}

// 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


CommonParameters.ExtraQueryParameters[kvp.Key] = kvp.Value.value;

if (kvp.Value.includeInCacheKey)
{
CommonParameters.CacheKeyComponents = CommonParameters.CacheKeyComponents ?? new SortedList<string, Func<CancellationToken, Task<string>>>();

// 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);
}
}

return this as T;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -292,10 +292,10 @@ protected T WithOptions(ApplicationOptions applicationOptions)
/// 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. Please 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(IDictionary<string, string> extraQueryParameters)
{
Config.ExtraQueryParameters = extraQueryParameters;
return this as T;
return WithExtraQueryParameters(CoreHelpers.ConvertToTupleParameters(extraQueryParameters));
}

/// <summary>
Expand All @@ -305,6 +305,7 @@ public T WithExtraQueryParameters(IDictionary<string, string> extraQueryParamete
/// The string needs to be properly URL-encoded and ready to send as a string of segments of the form <c>key=value</c> separated by an ampersand character.
/// </param>
/// <returns></returns>
[Obsolete("This method is deprecated. Please 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(string extraQueryParameters)
{
if (!string.IsNullOrWhiteSpace(extraQueryParameters))
Expand All @@ -314,6 +315,41 @@ public T WithExtraQueryParameters(string extraQueryParameters)
return this as T;
}

/// <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.
/// 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 the .With methods</returns>
public T WithExtraQueryParameters(IDictionary<string, (string value, bool includeInCacheKey)> extraQueryParameters)
{
if (extraQueryParameters == null)
{
Config.ExtraQueryParameters = null;
return this as T;
}

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

Config.ExtraQueryParameters[kvp.Key] = kvp.Value.value;

if (kvp.Value.includeInCacheKey)
{
// Initialize the cache key components if needed
Config.CacheKeyComponents = Config.CacheKeyComponents ?? new SortedList<string, string>();

// Add to cache key components - uses a func that returns the value as a task
Config.CacheKeyComponents[kvp.Key] = kvp.Value.value;
}
}

return this as T;
}

/// <summary>
/// Microsoft Identity specific OIDC extension that allows resource challenges to be resolved without interaction.
/// Allows configuration of one or more client capabilities, e.g. "llt"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@

<ItemGroup>
<PackageReference Include="Microsoft.IdentityModel.Abstractions" />
<PackageReference Include="System.ValueTuple" />
</ItemGroup>

<ItemGroup Label="For public api analyzer support">
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
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
18 changes: 18 additions & 0 deletions src/client/Microsoft.Identity.Client/Utils/CoreHelpers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,24 @@ public static Dictionary<string, string> ParseKeyValueList(string input, char de
return ParseKeyValueList(input, delimiter, urlDecode, true, requestContext);
}

// Helper method intended to help deprecate some WithExtraQueryParameters APIs.
// Convert from Dictionary<string, string> to Dictionary<string, (string value, bool includeInCacheKey)>,
// with all includeInCacheKey set to false by default to maintain existing behavior of those older APIs.
internal static IDictionary<string, (string value, bool includeInCacheKey)> ConvertToTupleParameters(IDictionary<string, string> parameters)
{
if (parameters == null)
{
return null;
}

var result = new Dictionary<string, (string value, bool includeInCacheKey)>(StringComparer.OrdinalIgnoreCase);
foreach (var kvp in parameters)
{
result[kvp.Key] = (kvp.Value, false); // Include all parameters in cache key by default
}
return result;
}

internal static IReadOnlyList<string> SplitWithQuotes(string input, char delimiter)
{
if (string.IsNullOrWhiteSpace(input))
Expand Down
12 changes: 12 additions & 0 deletions tests/Microsoft.Identity.Test.Common/TestConstants.cs
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,18 @@ public static IDictionary<string, string> ExtraQueryParameters
};
}
}
public static IDictionary<string, (string, bool)> ExtraQueryParametersNoAffectOnCacheKeys
{
get
{
return new Dictionary<string, (string, bool)>(StringComparer.OrdinalIgnoreCase)
{
{ "extra", ("qp", false) },
{ "key1", ("value1%20with%20encoded%20space", false) },
{ "key2", ("value2", false) }
};
}
}

public const string MsalCCAKeyVaultUri = "https://id4skeyvault.vault.azure.net/secrets/AzureADIdentityDivisionTestAgentSecret/";

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Security.Cryptography.X509Certificates;
using System.Threading.Tasks;
Expand Down Expand Up @@ -58,7 +59,7 @@ private static async Task AgentUserIdentityGetsTokenForGraphAsync()
.WithAuthority("https://login.microsoftonline.com/", TenantId)
.WithCacheOptions(CacheOptions.EnableSharedCacheOptions)
.WithExperimentalFeatures(true)
.WithExtraQueryParameters("slice=first")
.WithExtraQueryParameters(new Dictionary<string, (string value, bool includeInCacheKey)> { { "slice", ("first", false) } })
.WithClientAssertion((AssertionRequestOptions _) => GetAppCredentialAsync(AgentIdentity))
.Build();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
using Microsoft.Identity.Test.Common.Core.Helpers;
using System;
using System.IdentityModel.Tokens.Jwt;
using System.Collections.Generic;

namespace Microsoft.Identity.Test.Integration.NetCore.HeadlessTests
{
Expand All @@ -26,6 +27,7 @@ public class FmiIntegrationTests
{
private byte[] _serializedCache;
private const string Testslice = "dc=ESTSR-PUB-WUS-LZ1-TEST"; //Updated slice for regional tests
private Dictionary<string, (string, bool)> TestsliceQueryParam = new Dictionary<string, (string value, bool includeInCacheKey)> { { "dc", ("ESTSR-PUB-WUS-LZ1-TEST", false) } };
private const string AzureRegion = "westus3";
private const string TenantId = "f645ad92-e38d-4d1a-b510-d1b09a74a8ca"; //Tenant Id for the test app

Expand All @@ -45,7 +47,7 @@ public async Task Flow1_Credential_From_Cert()
var confidentialApp = ConfidentialClientApplicationBuilder
.Create(clientId)
.WithAuthority("https://login.microsoftonline.com/", TenantId)
.WithExtraQueryParameters(Testslice) //Enables MSAL to target ESTS Test slice
.WithExtraQueryParameters(TestsliceQueryParam) //Enables MSAL to target ESTS Test slice
.WithCertificate(cert, sendX5C: true) //sendX5c enables SN+I auth which is required for FMI flows
.WithAzureRegion(AzureRegion)
.BuildConcrete();
Expand Down Expand Up @@ -90,7 +92,7 @@ public async Task Flow2_Token_From_CertTest()
var confidentialApp = ConfidentialClientApplicationBuilder
.Create(clientId)
.WithAuthority("https://login.microsoftonline.com/", TenantId)
.WithExtraQueryParameters(Testslice)
.WithExtraQueryParameters(TestsliceQueryParam)
.WithCertificate(cert, sendX5C: true)
.WithAzureRegion(AzureRegion)
.BuildConcrete();
Expand Down Expand Up @@ -130,7 +132,7 @@ public async Task Flow3_FmiCredential_From_AnotherFmiCredential()
var confidentialApp = ConfidentialClientApplicationBuilder
.Create(clientId)
.WithAuthority("https://login.microsoftonline.com/", TenantId)
.WithExtraQueryParameters(Testslice)
.WithExtraQueryParameters(TestsliceQueryParam)
.WithClientAssertion((options) => GetFmiCredentialFromRma(options, Testslice))
.WithAzureRegion(AzureRegion)
.BuildConcrete();
Expand Down Expand Up @@ -171,7 +173,7 @@ public async Task Flow4_SubRma_FIC_From_FmiCredential()
var confidentialApp = ConfidentialClientApplicationBuilder
.Create(clientId)
.WithAuthority("https://login.microsoftonline.com/", TenantId)
.WithExtraQueryParameters(Testslice)
.WithExtraQueryParameters(TestsliceQueryParam)
.WithClientAssertion((options) => GetFmiCredentialFromRma(options, Testslice))
.WithAzureRegion(AzureRegion)
.BuildConcrete();
Expand Down Expand Up @@ -211,7 +213,7 @@ public async Task Flow5_FmiToken_From_FmiCred()
var confidentialApp = ConfidentialClientApplicationBuilder
.Create(clientId)
.WithAuthority("https://login.microsoftonline.com/", TenantId)
.WithExtraQueryParameters(Testslice)
.WithExtraQueryParameters(TestsliceQueryParam)
.WithClientAssertion((options) => GetFmiCredentialFromRma(options, Testslice))
.WithAzureRegion(AzureRegion)
.BuildConcrete();
Expand Down Expand Up @@ -246,6 +248,7 @@ private static async Task<string> GetFmiCredentialFromRma(AssertionRequestOption

X509Certificate2 cert = CertificateHelper.FindCertificateByName(TestConstants.AutomationTestCertName);

#pragma warning disable CS0618 // Type or member is obsolete
//Create application
var confidentialApp = ConfidentialClientApplicationBuilder
.Create(clientId)
Expand All @@ -254,6 +257,7 @@ private static async Task<string> GetFmiCredentialFromRma(AssertionRequestOption
.WithCertificate(cert, sendX5C: true) //sendX5c enables SN+I auth which is required for FMI flows
.WithAzureRegion(AzureRegion)
.BuildConcrete();
#pragma warning restore CS0618 // Type or member is obsolete

//Acquire Token
var authResult = await confidentialApp.AcquireTokenForClient(new[] { scope })
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ public async Task TestAcquireTokenInteractiveBuilderWithPromptAndExtraQueryParam
{
await AcquireTokenInteractiveParameterBuilder.Create(_harness.Executor, TestConstants.s_scope)
.WithLoginHint(TestConstants.DisplayableId)
.WithExtraQueryParameters("domain_hint=mydomain.com")
.WithExtraQueryParameters(new Dictionary<string, (string value, bool includeInCacheKey)> { { "domain_hint", ("mydomain.com", false) } })
.ExecuteAsync()
.ConfigureAwait(false);

Expand Down
3 changes: 2 additions & 1 deletion tests/Microsoft.Identity.Test.Unit/OAuthClientTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License.

using System;
using System.Collections.Generic;
using System.Net;
using System.Net.Http;
using System.Net.Http.Headers;
Expand Down Expand Up @@ -197,7 +198,7 @@ out Arg.Any<string>())

AuthenticationResult result = await pca
.AcquireTokenInteractive(TestConstants.s_scope)
.WithExtraQueryParameters("qp1=v1")
.WithExtraQueryParameters(new Dictionary<string, (string value, bool includeInCacheKey)>{{ "qp1", ("v1", false) }})
.ExecuteAsync()
.ConfigureAwait(false);
// Assert that the endpoint sent to the device auth manager doesnt not have query params
Expand Down
6 changes: 3 additions & 3 deletions tests/Microsoft.Identity.Test.Unit/ParallelRequestsTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,10 @@ public override void TestInitialize()
[TestMethod]
public async Task ExtraQP()
{
Dictionary<string, string> extraQp = new()
Dictionary<string, (string, bool)> extraQp = new()
{
{ "key1", "1" },
{ "key2", "2" }
{ "key1", ("1", false) },
{ "key2", ("2", false) }
};

// Arrange
Expand Down
Loading
Loading