MSI CAE Claims handling - claims merging issue

[gladjohn]

With SLC CAE, the claims we get back from ESTS is,

{
  "access_token": {
    "nbf": {
      "essential": true,
      "value": "1701477303"
    }
  }
}

And the merged claims and capab should be like this,

{
  "access_token": {
    "xms_cc": {
      "values": ["cp1", "cp2"]
    },
    "nbf": {
      "essential": true,
      "value": "1701477303"
    }
  }
}

In MSAL .NET, we build claims and capab with access_token and xms_cc, but with the new incoming claim, we fail to do a proper merge, instead just return the incoming claim without the capab.

All MSAL's need to check if this is being properly handled.

// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System.Collections.Generic;
using System.Linq;
using Microsoft.Identity.Client.Utils;
#if SUPPORTS_SYSTEM_TEXT_JSON
using System.Text.Json;
using System.Text.Json.Nodes;
using JObject = System.Text.Json.Nodes.JsonObject;
#else
using Microsoft.Identity.Json;
using Microsoft.Identity.Json.Linq;
#endif

namespace Microsoft.Identity.Client.Internal
{
    internal static class ClaimsHelper
    {
        private const string AccessTokenClaim = "access_token";
        private const string XmsClientCapability = "xms_cc";

        internal static string GetMergedClaimsAndClientCapabilities(
            string claims,
            IEnumerable<string> clientCapabilities)
        {
            if (clientCapabilities != null && clientCapabilities.Any())
            {
                JObject capabilitiesJson = CreateClientCapabilitiesRequestJson(clientCapabilities);
                JObject mergedClaimsAndCapabilities = MergeClaimsIntoCapabilityJson(claims, capabilitiesJson);

                return JsonHelper.JsonObjectToString(mergedClaimsAndCapabilities);
            }

            return claims;
        }

        internal static JObject MergeClaimsIntoCapabilityJson(string claims, JObject capabilitiesJson)
        {
            if (!string.IsNullOrEmpty(claims))
            {
                JObject claimsJson;
                try
                {
                    claimsJson = JsonHelper.ParseIntoJsonObject(claims);
                }
                catch (JsonException ex)
                {
                    throw new MsalClientException(
                        MsalError.InvalidJsonClaimsFormat,
                        MsalErrorMessage.InvalidJsonClaimsFormat(claims),
                        ex);
                }
#if SUPPORTS_SYSTEM_TEXT_JSON
                if (claimsJson is JsonObject jsonObject && jsonObject.ContainsKey("access_token"))
                {
                    JsonNode accessTokenClaim = jsonObject["access_token"];
                    claimsJson = accessTokenClaim as JObject;
                }

                foreach (var claim in claimsJson)
                {
                    capabilitiesJson[claim.Key] = claim.Value != null ? JsonNode.Parse(claim.Value.ToJsonString()) : null;
                }
#else
                if (claimsJson.ContainsKey("access_token"))
                {
                    JToken accessTokenClaim = claimsJson["access_token"];
                    claimsJson = accessTokenClaim as JObject;
                }

                capabilitiesJson.Merge(claimsJson, new JsonMergeSettings
                {
                    // union array values together to avoid duplicates
                    MergeArrayHandling = MergeArrayHandling.Union
                });
#endif
            }

            return capabilitiesJson;
        }

        private static JObject CreateClientCapabilitiesRequestJson(IEnumerable<string> clientCapabilities)
        {
            // "access_token": {
            //     "xms_cc": { 
            //         values: ["cp1", "cp2"]
            //     }
            //  }
            return new JObject
            {
                [AccessTokenClaim] = new JObject
                {
                    [XmsClientCapability] = new JObject
                    {
#if SUPPORTS_SYSTEM_TEXT_JSON
                        ["values"] = new JsonArray(clientCapabilities.Select(c => JsonValue.Create(c)).ToArray())
#else
                        ["values"] = new JArray(clientCapabilities)
#endif
                    }
                }
            };
        }
    }
}

[bgavrilMS]

I don't agree with the format proposed. The correct "merged" JSON is:

{
  "access_token": {
    "xms_cc": {
      "values": ["cp1", "cp2"]
    },
    "nbf": {
    "essential": true,
    "value": "1701477303"
  }
}

We can expect all MSALs, both public client and confidential client, to execute a "merge" operation. It is not practical to change MSALs to address this.

See original spec here: https://identitydivision.visualstudio.com/DevEx/_search?action=preview&text=client%20capabilities&type=code&lp=code-Project&filters=ProjectFilters%7BDevEx%7DRepositoryFilters%7BAuthLibrariesApiReview%7D&pageSize=25&result=DefaultCollection/DevEx/AuthLibrariesApiReview/GBdev//ClientCapabilities/overview.md clearly stating that a simple merge operation is needed.

Here is explicit merge logic in MSAL C++ https://identitydivision.visualstudio.com/DevEx/_search?action=preview&text=client%20capabilities&type=code&lp=code-Project&filters=ProjectFilters%7BDevEx%7DRepositoryFilters%7BAuthLibrariesApiReview%7D&pageSize=25&result=DefaultCollection/DevEx/AuthLibrariesApiReview/GBdev//MSAL%20CPP/ClaimsChallenge.md

[bgavrilMS]

Also, the proposed changes would likely introduce a regression in the other CAE flows (user auth)

[gladjohn]

I don't agree with the format proposed. The correct "merged" JSON is:

{
  "access_token": {
    "xms_cc": {
      "values": ["cp1", "cp2"]
    },
    "nbf": {
    "essential": true,
    "value": "1701477303"
  }
}

We can expect all MSALs, both public client and confidential client, to execute a "merge" operation. It is not practical to change MSALs to address this.

See original spec here: https://identitydivision.visualstudio.com/DevEx/_search?action=preview&text=client%20capabilities&type=code&lp=code-Project&filters=ProjectFilters%7BDevEx%7DRepositoryFilters%7BAuthLibrariesApiReview%7D&pageSize=25&result=DefaultCollection/DevEx/AuthLibrariesApiReview/GBdev//ClientCapabilities/overview.md clearly stating that a simple merge operation is needed.

Here is explicit merge logic in MSAL C++ https://identitydivision.visualstudio.com/DevEx/_search?action=preview&text=client%20capabilities&type=code&lp=code-Project&filters=ProjectFilters%7BDevEx%7DRepositoryFilters%7BAuthLibrariesApiReview%7D&pageSize=25&result=DefaultCollection/DevEx/AuthLibrariesApiReview/GBdev//MSAL%20CPP/ClaimsChallenge.md

reviewed the issue and modified the expected. it's more of an issue on merging. newtonsoft merge works but we may need to write our own merge logic for system.text.json