[Feature Request] Use System.Text.Json instead of Microsoft.IdentityModel.Json

[raV720]

Is your feature request related to a problem? Please describe.
When creating token:

var token = JwtSecurityTokenHandler.CreateJwtSecurityToken(
                issuer: issuer,
                audience: audience,
                signingCredentials: credential,
                subject: Identity.Create("pwd", claims));

there is used TokenUtilities.GetClaimValueUsingValueType() method which uses internal types (JArray, JObject from Microsoft.IdentityModel.Json.Linq). Then the System.IdentityModel.Tokens.Jwt.JsonExtensions.Serializer is called where JwtPayload is serialized. But JwtPayload contains objects with internal types (JArray, JObject) and when using custom serializer there is problem to detect how to serialize this object.

So the internal serializer is not quite internal, because its types are required to be public to properly handle library.

Describe the solution you'd like
Remove internal Microsoft.IdentityModel.Json serializer and use System.Text.Json.

Describe alternatives you've considered
Make internal serializer public - accessible from outside of assembly.

[brockallen]

Related? #1580

[brentschmaltz]

@brockallen long time no see
@raV720 we made some progress here with JsonWebToken, moving to System.Text.Json.

We would like to remove our internal Newtonsoft clone and move to System.Text.Json.
Our .45 and .461 framework targets make this hard.
We are proposing dropping .45 and .461 targets, introduce .462 to facilite moving to System.Text.Json.
The proposal includes bumping the version to 7.x, leaving 6.x for hot fixes.

Thoughts?

[kevinchalet]

👋🏻

We would like to remove our internal Newtonsoft clone and move to System.Text.Json.

A common option used by many libraries is to make the serializer configurable, so one can decide to use JSON.NET or System.Text.Json depending on their needs/preferences (JSON.NET could still be used for .NET Framework 4.5 if you wanted to keep this target).

The proposal includes bumping the version to 7.x, leaving 6.x for hot fixes.

What would that imply for Katana? Will it stay on 6.x or will it move to a newer .NET Framework TFM?

[brentschmaltz]

@kevinchalet I spoke with the asp.net team and believe Katana users will be able to use 7.x.
This has to be verified and i will do so during this work.

That is a good idea to have a model that allows users to plug in the serializer.
Hadn't thought about designing for that, i'll definitely try and make that happen.

Keeping the .net45 target is larger than just JSON.NET vs. System.Text.Json.
Our code is getting complicated with a bunch of #ifdef all over the place.
We also lose out on improvements in versions of C#.
Performance improvements around using Span etc are more complicated to implement.

I don't see Katana updating their TFM.

[kevinchalet]

@brentschmaltz thanks for your clear and detailed reply! 😃

Do you envision important API changes between 6.x and 7.x? I'm asking that because given what you said, it's likely OpenIddict will keep referencing Wilson 6.x for its .NET Standard 2.0 and .NET Framework 4.6.1 TFMs and will use Wilson 7.x only for the more recent targets. It the API changes were limited, that would be awesome 😃

Our code is getting complicated with a bunch of #ifdef all over the place.

Makes sense.

To make conditional code easier to write, read and maintain, OpenIddict defines feature constants in Directory.Builds.targets and uses them to guard features that depend on specific TFMs. Here's an example in case you'd want to use a similar approach:

<PropertyGroup
  Condition=" ('$(TargetFrameworkIdentifier)' == '.NETCoreApp'   And $([MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), '2.0')))   Or
              ('$(TargetFrameworkIdentifier)' == '.NETFramework' And $([MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), '4.7.2'))) Or
              ('$(TargetFrameworkIdentifier)' == '.NETStandard'  And $([MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), '2.1'))) ">
  <DefineConstants>$(DefineConstants);SUPPORTS_CERTIFICATE_GENERATION</DefineConstants>
</PropertyGroup>

#if SUPPORTS_CERTIFICATE_GENERATION
...
#endif

[brentschmaltz]

@kevinchalet we plan on keeping our .NET Standard 2.0 TFM, we were going to drop 461 (as it is not supported) and some promises in the crypto libraries were not true. 462, solved some of the issues. It is mainly 45 where the #ifdefs are really messy.
I like the style of the 'DefineConstants', we did some of that in a limited manner with 'DESKTOP' it is better.

Since System.Text.Json is supported on 461, it may be possible to convince the team to continue with the 461 TFM along with 462.

I don't think that anyone is really running 461, but just targeting the api set.

I will let you know what the team thinks.
Most likely only bug fixes will go in the 6.x release with new features in 7.x.

[brentschmaltz]

@kevinchalet would you be OK if we dropped 461?
The reason is that System.Text.Json 7.0 will throw compile errors for .net 461.

[kevinchalet]

@kevinchalet would you be OK if we dropped 461?

If you get rid of the explicit net461 TFM, libraries targeting net461 should be able to fall back to the netstandard2.0 TFM. But, the real question is: does the .NET Standard version of Wilson have limitations the net461 doesn't have?

The reason is that System.Text.Json 7.0 will throw compile errors for .net 461.

Have you considered using System.Text.Json 4.7.2 as the minimum referenced version for your .NET Framework/.NET Standard TFMs? It's still listed as a supported version and should offer all the APIs you need (it's the approach used in OpenIddict for full compatibility with ASP.NET Core 6/7, ASP.NET Core 2.1 on .NET Framework, "legacy" ASP.NET 4.6.1+ and desktop .NET Core/Framework apps and it works really well).

[brentschmaltz]

@kevinchalet spoke with the team and they seem open to keeping 461 around for another year and give users a heads up so there is time to migrate.

Would this work for you?

[kevinchalet]

Sounds reasonable 😃

[brentschmaltz]

@kevinchalet i'll work to get a commitment from the team.

[brentschmaltz]

@kevinchalet i negotiated with the team and we are willing to keep 461 target for at least 6 months after we drop 45.
I expect us to drop 45 sometime in June.

[kevinchalet]

Great, thanks for the information, @brentschmaltz! 😃

[brentschmaltz]

We shipped 7.x without our internal newtonsoft.