-
Notifications
You must be signed in to change notification settings - Fork 462
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
Disallow Encode(payload) with AddClaim(s) #464
Disallow Encode(payload) with AddClaim(s) #464
Conversation
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.
Ah, it ran into a merge conflict now :(
Do I understand the origin of the issue correct is that it occurs only in the builder because its Encode() method "messed up" with the payload so a complex model would work? If that's the case, what I think we should do: remove the code and move it to a custom serializer, which in turn just a decorator on the other one (maybe two decorators, one for the each underlying technology). |
I'll check the conflict when I'm at a computer. Looks like something minor. Yes, the issue is that Encode() with a parameter serializes the _jwt.Payload property. While Encode() with a parameter serializes only what we pass to it, ignoring the _jwt.Payload if we don't either add what we pass to it or adds the claims manually. |
I have reworked the Encode when also having added Claims so they are both serialized the same way as everything else. I have for now just added another Encode in JwtEncoder that just accepts two payloads. And in that case just serializes them and uses the merge functionallity in JSON.net, for System.Text.Json it was a bit more tricky but after finding out you can use the JsonNode classes it was easier. I think this is a more clean solution than iterating the payload with reflection and add all properties in the _jwt.Payload dictionary in JwtBuilder. If this is an acceptable solution we could rework the JwtEncoder.Encode method to be less copy.paste All tests pass also! :) PS. For further reading about JsonNode and Merge |
added test for stjSerializer for Encode nested property
…both claims and payload
…can use both claims and payload" This reverts commit 341da1c.
341da1c
to
8b9ac59
Compare
Awesome, thank you!! Pushed some minor formatting changes. And dropped the beta prefix, I think it'll work as-is. |
Cool, that was fast. I think we ended up with a cleaner solution with just sacrificing a minor nice-to-have functionallity |
Fixes for #456.