Skip to content

Commit

Permalink
Fix for claims and client capabilities JSON merge bug in net6 (#4452)
Browse files Browse the repository at this point in the history 
* fix

* build break

* more tests

* code reference

* test names

* pr comments

* pr comments

---------

Co-authored-by: Gladwin Johnson <[email protected]>
  • Loading branch information
@gladjohn @GladwinJohnson
gladjohn and GladwinJohnson authored Dec 6, 2023
1 parent 96ea5dc commit 79f951a
Show file tree
Hide file tree
Showing 5 changed files with 195 additions and 31 deletions.
9 changes: 5 additions & 4 deletions src/client/Microsoft.Identity.Client/Internal/ClaimsHelper.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,12 @@
using System.Linq;
using Microsoft.Identity.Client.Utils;
#if SUPPORTS_SYSTEM_TEXT_JSON
using System.Text;
using System.Text.Json;
using System.Text.Json.Nodes;
using JObject = System.Text.Json.Nodes.JsonObject;
using System.Buffers;
using System.Diagnostics;
#else
using Microsoft.Identity.Json;
using Microsoft.Identity.Json.Linq;
Expand Down Expand Up @@ -52,10 +55,8 @@ internal static JObject MergeClaimsIntoCapabilityJson(string claims, JObject cap
ex);
}
#if SUPPORTS_SYSTEM_TEXT_JSON
foreach (var claim in claimsJson)
{
capabilitiesJson[claim.Key] = claim.Value != null ? JsonNode.Parse(claim.Value.ToJsonString()) : null;
}

capabilitiesJson = JsonHelper.Merge(capabilitiesJson, claimsJson);
#else
capabilitiesJson.Merge(claimsJson, new JsonMergeSettings
{
Expand Down
132 changes: 132 additions & 0 deletions src/client/Microsoft.Identity.Client/Utils/JsonHelper.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using System.Text.Json.Nodes;
using JObject = System.Text.Json.Nodes.JsonObject;
using JToken = System.Text.Json.Nodes.JsonNode;
using System.Buffers;
#else
using Microsoft.Identity.Json;
using Microsoft.Identity.Json.Linq;
Expand Down Expand Up @@ -155,6 +156,136 @@ internal static long ExtractParsedIntOrZero(JObject json, string key)
internal static bool TryGetValue(JsonObject json, string propertyName, out JsonNode value) => json.TryGetPropertyValue(propertyName, out value);

internal static T GetValue<T>(JsonNode json) => json != null ? json.GetValue<T>() : default;

/// <summary>
/// Merges two JSON objects into a single JSON object.
/// </summary>
/// <param name="originalJson">The original JSON object to merge.</param>
/// <param name="newContent">The additional JSON object to merge.</param>
/// <returns>A JObject representing the merged JSON.</returns>
/// <remarks>
/// This method parses the original and new JSON objects, merges their elements, and returns
/// a JObject representing the merged JSON.
/// Original Code Reference: https://github.com/dotnet/runtime/issues/31433
/// </remarks>
internal static JObject Merge(JObject originalJson, JObject newContent)
{
// Output buffer to store the merged JSON
var outputBuffer = new ArrayBufferWriter<byte>();

// Parse the original and new JSON content
using (JsonDocument jDoc1 = JsonDocument.Parse(originalJson.ToJsonString()))
using (JsonDocument jDoc2 = JsonDocument.Parse(newContent.ToJsonString()))
using (var jsonWriter = new Utf8JsonWriter(outputBuffer, new JsonWriterOptions { Indented = true }))
{
// Merge the JSON elements
MergeJsonElements(jsonWriter, jDoc1.RootElement, jDoc2.RootElement);
}

// Convert the merged JSON to a UTF-8 encoded string
string mergedJsonString = Encoding.UTF8.GetString(outputBuffer.WrittenSpan);

// Parse the merged JSON string to a JObject
return ParseIntoJsonObject(mergedJsonString);
}

// Merges two JSON elements based on their value kind
private static void MergeJsonElements(Utf8JsonWriter jsonWriter, JsonElement root1, JsonElement root2)
{
switch (root1.ValueKind)
{
case JsonValueKind.Object:
MergeObjects(jsonWriter, root1, root2);
break;
case JsonValueKind.Array:
MergeArrays(jsonWriter, root1, root2);
break;
default:
// If not an object or array, directly write the value to the output
root1.WriteTo(jsonWriter);
break;
}
}

// Merges two JSON objects
private static void MergeObjects(Utf8JsonWriter jsonWriter, JsonElement root1, JsonElement root2)
{
// Start writing the merged object
jsonWriter.WriteStartObject();

// Create a HashSet to track processed property names
HashSet<string> processedProperties = new HashSet<string>();

// Iterate through properties of the first JSON object
foreach (JsonProperty property in root1.EnumerateObject())
{
string propertyName = property.Name;

JsonValueKind newValueKind;

// Check if the second JSON object has a property with the same name
if (root2.TryGetProperty(propertyName, out JsonElement newValue) && (newValueKind = newValue.ValueKind) != JsonValueKind.Null)
{
// Write the property name
jsonWriter.WritePropertyName(propertyName);
processedProperties.Add(propertyName);

JsonElement originalValue = property.Value;
JsonValueKind originalValueKind = originalValue.ValueKind;

// Recursively merge objects or arrays, otherwise, write the new value
if ((newValueKind == JsonValueKind.Object && originalValueKind == JsonValueKind.Object) ||
(newValueKind == JsonValueKind.Array && originalValueKind == JsonValueKind.Array))
{
MergeJsonElements(jsonWriter, originalValue, newValue);
}
else
{
newValue.WriteTo(jsonWriter);
}
}
else
{
// If the second object does not have the property, write the original property
property.WriteTo(jsonWriter);
}
}

// Iterate through properties unique to the second JSON object
foreach (JsonProperty property in root2.EnumerateObject())
{
if (!processedProperties.Contains(property.Name))
{
// Write properties unique to the second object
property.WriteTo(jsonWriter);
}
}

// End writing the merged object
jsonWriter.WriteEndObject();
}

// Merges two JSON arrays
private static void MergeArrays(Utf8JsonWriter jsonWriter, JsonElement root1, JsonElement root2)
{
// Start writing the merged array
jsonWriter.WriteStartArray();

// Merge elements of the first array
for (int i = 0; i < root1.GetArrayLength(); i++)
{
root1[i].WriteTo(jsonWriter);
}

// Merge elements of the second array
for (int i = 0; i < root2.GetArrayLength(); i++)
{
root2[i].WriteTo(jsonWriter);
}

// End writing the merged array
jsonWriter.WriteEndArray();
}
#else
internal static string JsonObjectToString(JObject jsonObject) => jsonObject.ToString(Formatting.None);

Expand All @@ -167,4 +298,5 @@ internal static long ExtractParsedIntOrZero(JObject json, string key)
internal static T GetValue<T>(JToken json) => json.Value<T>();
#endif
}

}
9 changes: 9 additions & 0 deletions tests/Microsoft.Identity.Test.Common/TestConstants.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,15 @@ public static HashSet<string> s_scope
public const string ClientCapabilitiesJson = @"{""access_token"":{""xms_cc"":{""values"":[""cp1"",""cp2""]}}}";
// this a JSON merge from Claims and ClientCapabilitiesJson
public const string ClientCapabilitiesAndClaimsJson = @"{""access_token"":{""xms_cc"":{""values"":[""cp1"",""cp2""]}},""userinfo"":{""given_name"":{""essential"":true},""nickname"":null,""email"":{""essential"":true},""email_verified"":{""essential"":true},""picture"":null,""http://example.info/claims/groups"":null},""id_token"":{""auth_time"":{""essential"":true},""acr"":{""values"":[""urn:mace:incommon:iap:silver""]}}}";
public const string ClaimsWithAccessToken = @"{""access_token"":{""nbf"":{""essential"":true, ""value"":""1701477303""}}}";
public const string ClientCapabilitiesAndClaimsJsonWithAccessToken = @"{""access_token"":{""xms_cc"":{""values"":[""cp1"",""cp2""]},""nbf"":{""essential"":true,""value"":""1701477303""}}}";
public const string EmptyClaimsJson = @"{}";
public const string ClaimsWithAdditionalClaim = @"{""access_token"":{""nbf"":{""essential"":true, ""value"":""1701477303""},""additional_claim"":{""key"":""value""}}}";
public const string MergedJsonWithAdditionalClaim = @"{""access_token"":{""xms_cc"":{""values"":[""cp1"",""cp2""]},""nbf"":{""essential"":true,""value"":""1701477303""},""additional_claim"":{""key"":""value""}}}";
public const string ClaimWithAdditionalKey = @"{""access_token"":{""nbf"":{""essential"":true,""value"":""1701477303""},""additional_claim"":{""key"":""value""},""new_claim"":{""new_key"":""new_value""}},""some_other_key"":{""nbf"":{""essential"":true,""value"":""1701477303""},""additional_claim"":{""key"":""value""},""new_claim"":{""new_key"":""new_value""}}}";
public const string MergedJsonWithAdditionalKey = @"{""access_token"":{""xms_cc"":{""values"":[""cp1"",""cp2""]},""nbf"":{""essential"":true,""value"":""1701477303""},""additional_claim"":{""key"":""value""},""new_claim"":{""new_key"":""new_value""}},""some_other_key"":{""nbf"":{""essential"":true,""value"":""1701477303""},""additional_claim"":{""key"":""value""},""new_claim"":{""new_key"":""new_value""}}}";
public const string ClaimWithAdditionalKeyAndAccessKey = @"{""some_other_key"":{""nbf"":{""essential"":true,""value"":""1701477303""},""additional_claim"":{""key"":""value""},""new_claim"":{""new_key"":""new_value""}},""access_token"":{""nbf"":{""essential"":true,""value"":""1701477303""},""additional_claim"":{""key"":""value""},""new_claim"":{""new_key"":""new_value""}}}";
public const string MergedJsonClaimWithAdditionalKeyAndAccessKey = @"{""access_token"":{""xms_cc"":{""values"":[""cp1"",""cp2""]},""nbf"":{""essential"":true,""value"":""1701477303""},""additional_claim"":{""key"":""value""},""new_claim"":{""new_key"":""new_value""}},""some_other_key"":{""nbf"":{""essential"":true,""value"":""1701477303""},""additional_claim"":{""key"":""value""},""new_claim"":{""new_key"":""new_value""}}}";

public const string DisplayableId = "[email protected]";
public const string RedirectUri = "urn:ietf:wg:oauth:2.0:oob";
Expand Down
43 changes: 43 additions & 0 deletions tests/Microsoft.Identity.Test.Common/TestData.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,5 +38,48 @@ public static IEnumerable<object[]> GetAuthorityWithExpectedTenantId()
yield return new AuthorityWithExpectedTenantId { Authority = new Uri(TestConstants.AadAuthorityWithTestTenantId), ExpectedTenantId = TestConstants.AadTenantId }.ToObjectArray();
yield return new AuthorityWithExpectedTenantId { Authority = new Uri(TestConstants.AuthorityWindowsNet), ExpectedTenantId = TestConstants.Utid }.ToObjectArray();
}

/// <summary>
/// Provides test data for scenarios involving the merging of claims and client capabilities.
/// </summary>
/// <remarks>
/// Test cases include various combinations of claims, client capabilities, and the expected merged JSON result.
/// </remarks>
/// <returns>Enumerable of Object Arrays with Index 0 = Claims, Index 1 = Client Capabilities, Index 2 = Expected Merged JSON</returns>
public static IEnumerable<object[]> GetClaimsAndCapabilities()
{
// Test case with non-empty claims, non-empty capabilities, and the expected merged JSON
yield return new object[] { TestConstants.Claims, TestConstants.ClientCapabilities, TestConstants.ClientCapabilitiesAndClaimsJson };

// Test case with claims containing an access token, non-empty capabilities, and the expected merged JSON
yield return new object[] { TestConstants.ClaimsWithAccessToken, TestConstants.ClientCapabilities, TestConstants.ClientCapabilitiesAndClaimsJsonWithAccessToken };

// Test case with empty claims, non-empty capabilities, and the expected merged JSON being the capabilities alone
yield return new object[] { TestConstants.EmptyClaimsJson, TestConstants.ClientCapabilities, TestConstants.ClientCapabilitiesJson };

// Test case with claims containing an additional claim, non-empty capabilities, and the expected merged JSON
yield return new object[] { TestConstants.ClaimsWithAdditionalClaim, TestConstants.ClientCapabilities, TestConstants.MergedJsonWithAdditionalClaim };

// Test case with claims containing an additional key, non-empty capabilities, and the expected merged JSON
yield return new object[] { TestConstants.ClaimWithAdditionalKey, TestConstants.ClientCapabilities, TestConstants.MergedJsonWithAdditionalKey };

// Test case with claims containing an additional key, empty capabilities, and the expected merged JSON being the claims alone
yield return new object[] { TestConstants.ClaimWithAdditionalKey, new string[0], TestConstants.ClaimWithAdditionalKey };

// Test case with non-empty claims, empty capabilities, and the expected merged JSON being the claims alone
yield return new object[] { TestConstants.Claims, new string[0], TestConstants.Claims };

// Test case with null claims, non-empty capabilities, and the expected merged JSON being the capabilities alone
yield return new object[] { null, TestConstants.ClientCapabilities, TestConstants.ClientCapabilitiesJson };

// Test case with non-empty claims, null capabilities, and the expected merged JSON being the claims alone
yield return new object[] { TestConstants.Claims, null, TestConstants.Claims };

// Test case with claims containing an access token, null capabilities, and the expected merged JSON
yield return new object[] { TestConstants.ClaimsWithAccessToken, null, TestConstants.ClaimsWithAccessToken };

// Test case with claims containing an additional key and access key (different order), non-empty capabilities, and the expected merged JSON
yield return new object[] { TestConstants.ClaimWithAdditionalKeyAndAccessKey, TestConstants.ClientCapabilities, TestConstants.MergedJsonClaimWithAdditionalKeyAndAccessKey };
}
}
}
33 changes: 6 additions & 27 deletions tests/Microsoft.Identity.Test.Unit/CoreTests/OAuth2Tests/ClaimsTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
using Microsoft.Identity.Test.Common.Core.Mocks;
using Microsoft.Identity.Test.Common.Mocks;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Microsoft.Identity.Test.Common;

namespace Microsoft.Identity.Test.Unit.CoreTests.OAuth2Tests
{
Expand Down Expand Up @@ -241,34 +242,12 @@ public async Task Claims_Fail_WhenClaimsIsNotJson_Async()
Assert.AreEqual(MsalError.InvalidJsonClaimsFormat, ex.ErrorCode);
}

[TestMethod]
public void ClaimsMerge_Test()
{
var mergedJson = ClaimsHelper.GetMergedClaimsAndClientCapabilities(
TestConstants.Claims,
TestConstants.ClientCapabilities);

Assert.AreEqual(TestConstants.ClientCapabilitiesAndClaimsJson, mergedJson);
}

[TestMethod]
public void ClaimsMerge_NoCapabilities_Test()
[DataTestMethod]
[DynamicData(nameof(TestData.GetClaimsAndCapabilities), typeof(TestData), DynamicDataSourceType.Method)]
public void ClaimsMerge_Test(string claims, string[] capabilities, string expectedMergedJson)
{
var mergedJson = ClaimsHelper.GetMergedClaimsAndClientCapabilities(
TestConstants.Claims,
new string[0]);

Assert.AreEqual(TestConstants.Claims, mergedJson);
}

[TestMethod]
public void ClaimsMerge_NoClaims_Test()
{
var mergedJson = ClaimsHelper.GetMergedClaimsAndClientCapabilities(
null,
TestConstants.ClientCapabilities);

Assert.AreEqual(TestConstants.ClientCapabilitiesJson, mergedJson);
var mergedJson = ClaimsHelper.GetMergedClaimsAndClientCapabilities(claims, capabilities);
Assert.AreEqual(expectedMergedJson, mergedJson);
}
}
}

0 comments on commit 79f951a

Please sign in to comment.