OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.44k stars 2.4k forks source link

Deserialize JsonWebKeySet using STJ #15322

Open MikeAlhayek opened 9 months ago

MikeAlhayek commented 9 months ago

There is an interesting problem with our implementation of STJ.

If we follow the TODO here, we run into a deserialization issue.

https://github.com/OrchardCMS/OrchardCore/blob/a8bdd63b6df6ae5d45444271e2c144425cc7db84/src/OrchardCore/OrchardCore.OpenId.Core/YesSql/Models/OpenIdApplication.cs#L60

@kevinchalet tried to implement a custom IJsonTypeInfoResolver like this which did not work

internal class JsonWebKeySetTypeInfoResolver : DefaultJsonTypeInfoResolver
{
    public override JsonTypeInfo GetTypeInfo(Type type, JsonSerializerOptions options)
    {
        if (type == typeof(JsonWebKeySet))
        {
            return base.GetTypeInfo(type, new JsonSerializerOptions(options)
            {
                ReferenceHandler = null
            });
        }

        return base.GetTypeInfo(type, options);
    }
}

This issue occurres if you enable the OpenId server and add an application if you don't already have one

sebastienros commented 9 months ago

Stacktrace:

System.InvalidOperationException: JsonObjectCreationHandling.Populate is incompatible with reference handling.
   at System.Text.Json.ThrowHelper.ThrowInvalidOperationException_ObjectCreationHandlingPropertyCannotAllowReferenceHandling()
   at System.Text.Json.Serialization.Metadata.JsonPropertyInfo.DetermineEffectiveObjectCreationHandlingForProperty()
   at System.Text.Json.Serialization.Metadata.JsonPropertyInfo.Configure()
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.ConfigureProperties()
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.Configure()
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.<EnsureConfigured>g__ConfigureSynchronized|172_0()
   at System.Text.Json.JsonSerializerOptions.GetTypeInfoInternal(Type type, Boolean ensureConfigured, Nullable`1 ensureNotNull, Boolean resolveIfMutable, Boolean fallBackToNearestAncestorType)
   at System.Text.Json.Serialization.Metadata.JsonPropertyInfo.Configure()
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.ConfigureProperties()
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.Configure()
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.<EnsureConfigured>g__ConfigureSynchronized|172_0()
   at System.Text.Json.JsonSerializerOptions.GetTypeInfoInternal(Type type, Boolean ensureConfigured, Nullable`1 ensureNotNull, Boolean resolveIfMutable, Boolean fallBackToNearestAncestorType)
   at System.Text.Json.JsonSerializer.GetTypeInfo(JsonSerializerOptions options, Type inputType)
   at System.Text.Json.JsonSerializer.Serialize(Utf8JsonWriter writer, Object value, Type inputType, JsonSerializerOptions options)
   at System.Text.Json.Serialization.DynamicJsonConverter.Write(Utf8JsonWriter writer, Object objectToWrite, JsonSerializerOptions options) in C:\code\orchardcore\src\OrchardCore\OrchardCore.Abstractions\Json\Serialization\DynamicJsonConverter.cs:line 103
   at System.Text.Json.Serialization.JsonConverter`1.TryWrite(Utf8JsonWriter writer, T& value, JsonSerializerOptions options, WriteStack& state)
   at System.Text.Json.Serialization.JsonConverter`1.WriteCore(Utf8JsonWriter writer, T& value, JsonSerializerOptions options, WriteStack& state)
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo`1.Serialize(Utf8JsonWriter writer, T& rootValue, Object rootValueBoxed)
   at System.Text.Json.JsonSerializer.WriteString[TValue](TValue& value, JsonTypeInfo`1 jsonTypeInfo)
   at System.Text.Json.JsonSerializer.Serialize[TValue](TValue value, JsonSerializerOptions options)
   at YesSql.Serialization.DefaultJsonContentSerializer.Serialize(Object item) in C:\code\orchardcore\src\OrchardCore\OrchardCore.Data.YesSql\DefaultJsonContentSerializer.cs:line 39
   at YesSql.Session.SaveEntityAsync(Object entity, String collection)
   at YesSql.Session.FlushAsync()
   at YesSql.Session.FlushAsync()
   at YesSql.Session.SaveChangesAsync()
   at YesSql.Session.SaveChangesAsync()
   at OrchardCore.OpenId.YesSql.Stores.OpenIdApplicationStore`1.CreateAsync(TApplication application, CancellationToken cancellationToken) in C:\code\orchardcore\src\OrchardCore\OrchardCore.OpenId.Core\YesSql\Stores\OpenIdApplicationStore.cs:line 58
   at OpenIddict.Core.OpenIddictApplicationManager`1.CreateAsync(TApplication application, String secret, CancellationToken cancellationToken)
   at OpenIddict.Core.OpenIddictApplicationManager`1.CreateAsync(OpenIddictApplicationDescriptor descriptor, CancellationToken cancellationToken)
   at OpenIddict.Core.OpenIddictApplicationManager`1.OpenIddict.Abstractions.IOpenIddictApplicationManager.CreateAsync(OpenIddictApplicationDescriptor descriptor, CancellationToken cancellationToken)
   at OrchardCore.OpenId.OpenIdApplicationExtensions.UpdateDescriptorFromSettings(IOpenIdApplicationManager _applicationManager, OpenIdApplicationSettings model, Object application) in C:\code\orchardcore\src\OrchardCore.Modules\OrchardCore.OpenId\OpenIdApplicationSettings.cs:line 245
   at OrchardCore.OpenId.Controllers.ApplicationController.Create(CreateOpenIdApplicationViewModel model, String returnUrl) in C:\code\orchardcore\src\OrchardCore.Modules\OrchardCore.OpenId\Controllers\ApplicationController.cs:line 184
sebastienros commented 9 months ago

Looks like this bug https://github.com/dotnet/runtime/issues/92877

MikeAlhayek commented 9 months ago

That makes sense. Because I thought that the approach we took should have worked.

kevinchalet commented 9 months ago

@sebastienros I don't think it's related: in that issue, you get an incorrect result but no exception is thrown, which is not the case here (also, JsonWebKeySet has both a constructor taking the raw JSON string and a parameterless constructor).

@kevinchalet tried to implement a custom IJsonTypeInfoResolver like this which did not work

Yeah, that approach doesn't work: STJ deliberately prevents you from using a derived JsonSerializerOptions when calling base.GetTypeInfo() (I don't recall the exact message, but it's very clear about the fact you're not allowed to do that).

The problem here is that OC uses a ReferenceHandler, which is not compatible with the JsonWebKeySet.Keys property that is decorated with [JsonObjectCreationHandling(JsonObjectCreationHandling.Populate)]: both options are incompatible and STJ throws an exception if you specify a non-null JsonSerializerOptions.ReferenceHandler when any property in the graph uses JsonObjectCreationHandling.Populate.

Sadly, as mentioned on Gitter, the ReferenceHandler setting is global and there's apparently no way to set one via JsonTypeInfo/JsonPropertyInfo (otherwise, it would be very as we'd just have to opt out the global ReferenceHandler, which isn't needed to (de)serialize JsonWebKeySet).

I can only see two options if we really want to use JsonWebKeySet here:

Or we could just wait for STJ to support configuring ReferenceHandler via JsonTypeInfo/JsonPropertyInfo.

Or... the simplest option: we don't do anything and keep using JsonObject instead of JsonWebKeySet 😅