AzureAD / azure-activedirectory-identitymodel-extensions-for-dotnet

IdentityModel extensions for .Net
MIT License
1.06k stars 401 forks source link

[Bug] Unset array fields of JsonWebKey are not omitted when serialised to JSON #2254

Open polys opened 1 year ago

polys commented 1 year ago

Which version of Microsoft.IdentityModel are you using? 7.0.0-preview3

Where is the issue?

Is this a new or an existing app?

Repro

var jwks = new JsonWebKeySet();

var key = new X509SecurityKey(cert);
var jwk = JsonWebKeyConverter.ConvertFromX509SecurityKey(key, true);
jwk.Use = "sig";

jwks.Keys.Add(jwk);
{
  "keys": [
    {
      "e": "AQAB",
      "key_ops": [],
      "kid": "7f4...",
      "kty": "RSA",
      "n": "v1L8m2wgy0ddn....",
      "oth": [],
      "use": "sig",
      "x5c": []
    }
  ]
}

Expected behavior key_ops, oth and x5c should be omitted since they were unset, not show up as empty lists

Actual behavior List fields of array/list type are returned as empty lists

Possible solution The getters seem to swap nulls with empty arrays before returning, which means [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] has no effect.

Additional context / logs / screenshots / links to code

Add any other context about the problem here, such as logs and screenshots or links to code.

brentschmaltz commented 1 year ago

@polys what are you using to Serialize? Newtonsoft? System.Text.Json?

polys commented 1 year ago

@polys what are you using to Serialize?

System.Text.Json in a modern out-of-the-box ASP.NET project on .NET 7

brentschmaltz commented 1 year ago

@polys the issue here is key_ops, oth, x5c are collections that are non-null by design. This is a change from 6x where these properties had a get/set pair and could be null.

We have a JsonWebKeySetSerializer that is used internally to skip the empty arrays. Adding JsonWebKeySet.Write() would not be a solution for derived types.

I will discuss with the team.

polys commented 1 year ago

We have a JsonWebKeySetSerializer that is used internally to skip the empty arrays.

Is JsonWebKeySetSerializer something we can use as well?

All I'm trying to do is have a simple API endpoint returning my public JWKS, similar to how the OIDC /.well-known/openid-configuration endpoint points to a jwks_uri.

I could of course serialise manually, or remove the extra fields manually, but if feels like a hack.

Adding JsonWebKeySet.Write() would not be a solution for derived types.

Is there a way you can narrow the scope or constrain the expectations somehow so this would not be an issue?

For example, saying something like Write RFC7517 only promises RFC7517-compliant serialisation, as opposed to Write that suggests complete serialisation, even for derived types.

Some options that come to mind:

I'm really quite excited and appreciative of the work you're all doing on version 7. Thank you :)

brentschmaltz commented 1 year ago

@polys we are up against a hard deadline to coordinate with asp.net 8. Opening up the JsonWebKeySetSerializer may make sense, but it will need some work for derived types.

In the past we have used controls such as 'bool strict' for compliance. Rfc7517, states parameter names are case-sensitive, the 'kty' parameter MUST exist, etc. As our 6x release was not strict, we decided to release 7 as lax.

I agree a bool is not enough, something similar to JsonSerializerOptions but scoped to JsonWebKey.

It would be best to design this with extensibility of OpenIdConnectConfiguration also as the two chain together.

brentschmaltz commented 1 year ago

We can improve this in .net 8.

mus65 commented 1 year ago

This even causes a regression in our frontend because the JS crypto API complains about it:

DOMException: The JWK "key_ops" member was inconsistent with that specified by the Web Crypto call. The JWK usage must be a superset of those requested

We worked around this by special-casing JsonWebKey in our JsonTypeInfoResolver to ignore empty list properties:

// Workaround https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/2254
if (type == typeof(JsonWebKey) && jsonProperty.PropertyType.IsGenericType && jsonProperty.PropertyType.GetGenericTypeDefinition() == typeof(IList<>))
{
  jsonProperty.ShouldSerialize = (obj, _) =>
  {
    PropertyInfo propertyInfo = (PropertyInfo)jsonProperty.AttributeProvider;
    System.Collections.IList? list = (System.Collections.IList?)propertyInfo.GetValue(obj);
    return list != null && list.Count > 0;
  };
}
brentschmaltz commented 8 months ago

@polys @mus65 I think opening up the serializers for: JsonWebToken, JsonWebKey, JsonWebKey, OpenIdConnectConfiguration with extensibility seems to be a good solution.

Marked this as an enhancement.