-
Notifications
You must be signed in to change notification settings - Fork 409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JsonWebKey and JsonWebKeySet using System.Text.Json for reading and writing #2208
Conversation
src/Microsoft.IdentityModel.Tokens/Json/EncodedJsonWebKeyParameterNames.cs
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Tokens/Json/JsonSerializerPrimitives.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Tokens/Json/EncodedJsonWebKeyParameterNames.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1451eef
to
df77be6
Compare
writer = new Utf8JsonWriter(memoryStream, new JsonWriterOptions { Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping }); | ||
Write(ref writer, jsonWebKeySet); | ||
writer.Flush(); | ||
return Encoding.UTF8.GetString(memoryStream.ToArray()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be using ToArray. At a minimum it can use GetBuffer() to get the MemoryStream's underlying buffer directly, rather than allocating and copying, e.g.
return Encoding.UTF8.GetString(memoryStream.GetBuffer(), 0, memoryStream.Length);
However, it would also be nice to avoid the MemoryStream and Utf8JsonWriter allocations as well. Could this instead just use JsonSerializer? It handles all the pooling for you. The task then I think would just be writing a JsonConverter for JsonWebKeySet, which should mostly be copy/paste of this code into its Write method.
public static readonly byte[] Alg = Encoding.UTF8.GetBytes("alg"); | ||
public static readonly byte[] Crv = Encoding.UTF8.GetBytes("crv"); | ||
public static readonly byte[] D = Encoding.UTF8.GetBytes("d"); | ||
public static readonly byte[] DP = Encoding.UTF8.GetBytes("dp"); | ||
public static readonly byte[] DQ = Encoding.UTF8.GetBytes("dq"); | ||
public static readonly byte[] E = Encoding.UTF8.GetBytes("e"); | ||
public static readonly byte[] K = Encoding.UTF8.GetBytes("k"); | ||
public static readonly byte[] KeyOps = Encoding.UTF8.GetBytes("key_ops"); | ||
public static readonly byte[] Keys = Encoding.UTF8.GetBytes("keys"); | ||
public static readonly byte[] Kid = Encoding.UTF8.GetBytes("kid"); | ||
public static readonly byte[] Kty = Encoding.UTF8.GetBytes("kty"); | ||
public static readonly byte[] N = Encoding.UTF8.GetBytes("n"); | ||
public static readonly byte[] Oth = Encoding.UTF8.GetBytes("oth"); | ||
public static readonly byte[] P = Encoding.UTF8.GetBytes("p"); | ||
public static readonly byte[] Q = Encoding.UTF8.GetBytes("q"); | ||
public static readonly byte[] QI = Encoding.UTF8.GetBytes("qi"); | ||
public static readonly byte[] Use = Encoding.UTF8.GetBytes("use"); | ||
public static readonly byte[] X5c = Encoding.UTF8.GetBytes("x5c"); | ||
public static readonly byte[] X5t = Encoding.UTF8.GetBytes("x5t"); | ||
public static readonly byte[] X5tS256 = Encoding.UTF8.GetBytes("x5t#S256"); | ||
public static readonly byte[] X5u = Encoding.UTF8.GetBytes("x5u"); | ||
public static readonly byte[] X = Encoding.UTF8.GetBytes("x"); | ||
public static readonly byte[] Y = Encoding.UTF8.GetBytes("y"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These can all use u8
instead and avoid doing the UTF8 encoding at run-time. In fact, the only usage appears to be as arguments to ValueTextEquals, which has span-based overloads, so these don't even need to be arrays, e.g. replace
public static readonly byte[] X = Encoding.UTF8.GetBytes("x");
with
public static ReadOnlySpan<byte> X => "x"u8;
or do away with this class entirely and just use "x"u8
at the call site.
/// </summary> | ||
internal static class EncodedJsonWebKeyParameterNames | ||
{ | ||
internal static bool _algSet; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These bools should all be volatile
|
||
public static readonly JsonEncodedText Kty = JsonEncodedText.Encode(JsonWebKeyParameterNames.Kty, JavaScriptEncoder.UnsafeRelaxedJsonEscaping); | ||
|
||
public static readonly JsonEncodedText N = JsonEncodedText.Encode(JsonWebKeyParameterNames.N, JavaScriptEncoder.UnsafeRelaxedJsonEscaping); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rhyme/reason for why some of these are lazy and some aren't? Are the prompt ones always used and the lazy ones rarely used?
} | ||
|
||
return _y; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could simplify this file with a helper like:
private static JsonEncodedText Get(ref bool initialized, ref JsonEncodedText field, string name)
{
if (!Volatile.Read(ref initialized)
{
field = JsonEncodedText.Encode(name, JavaScriptEncoder.UnsafeRelaxedJsonEscaping);
Volatile.Write(ref initialized, true);
}
return field;
}
and then your properties all become one-liners, e.g.
public static JsonEncodedText Y => Get(ref _ySet, ref _y, JsonWebKeyParameterNames.Y);
(and then you don't need to make the bool fields volatile as it's then handled by the Get helper).
} | ||
catch (JsonException ex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this catch block?
) Co-authored-by: Brent Schmaltz <[email protected]>
) Co-authored-by: Brent Schmaltz <[email protected]>
Reading and Writing JsonWebKey and JsonWebKeySet types now use System.Text.Json.Utf8JsonReaders/Writers for serialization.
JsonWebKey6x and JsonWebKeySet6x were copied from the 6x branch and Deserialization and Roundtrips were compared against what was generated using Newtonsoft.
Differences were captured and will be used to created release notes.
JsonWebKeySerializer and JsonWebKeySetSerializer use JsonSerializerPrimitives as will other types that need serialization.
JsonSerializationPrimitives will provide a robust error message with the type being serialized, the property in play and the reason for the error.
DataSets.cs was modified a considerable amount to make it easier to navigate.
JsonWebKeyTests were modified to use the ctor that takes a TheoryData object, rather than multiple parameters.
JsonSerializerPrimitivesTests was written to understand how failures would be handled, error messages and the difference between Newtonsoft and System.Text.Json.
Microsoft.IdentityModel.Tokens has two additional references to Newtonsoft, those will be removed with modifications that will update OIDC to use System.Text.Json.