Skip to content
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

Update JsonWebToken to enable extensibility and use underlying byte stream #2535

Draft
wants to merge 12 commits into
base: dev
Choose a base branch
from

Conversation

pmaytak
Copy link
Contributor

@pmaytak pmaytak commented Mar 21, 2024

Fixes #2583.

Changes:

  • Extracts the reading from token bytes code into a separate method so that child classes can overwrite it with their own reading logic.
  • When reading the token bytes the start index and length of claim value can be saved into a dictionary, which is located in the Payload JsonClaimSet instance.
  • The properties then can use the saved indices to slice into the original token bytes, returning them.
  • Commit 5 shows a second approach. Instead of saving the value indices and looking into the original token bytes; it copies the claim bytes into a separate Memory instances which are saved into a dictionary.

@pmaytak pmaytak changed the title Update JsonWebToken for child classes. Update JsonWebToken to enable extensibility Apr 24, 2024
@pmaytak pmaytak changed the title Update JsonWebToken to enable extensibility Update JsonWebToken to enable extensibility and use underlying byte stream May 9, 2024
{
ArrayPool<byte>.Shared.Return(output, true);
}
byte[] output = new byte[outputSize];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to still pool this buffer? Maybe by making JsonClaimSet disposable (or some new type that wraps a JsonClaimSet and a buffer), and it returns the buffer back to the pool once it has been disposed of.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just simple change to make POC work. Yep, sounds like that could work.

@@ -24,15 +25,31 @@ internal class JsonClaimSet

internal object _claimsLock = new();
internal readonly Dictionary<string, object> _jsonClaims;
internal readonly Dictionary<string, (int startIndex, int length)> _jsonClaimsUtf8;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this / should this be in the ifdef too?

If so, could this use Range instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, all of this new UTF8 related code is only for .NET7+ so I'll refactor inside ifdef. Good idea, I'll see if Range works. Yea, the not ideal thing about this approach is that keeping track of indices is a bit less straightforward than just an object instance...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] In JsonWebToken use the values as bytes instead of converting to string
4 participants