Feature/#445 - withPayload(Map<String, Object> payloadClaims) method#454
Feature/#445 - withPayload(Map<String, Object> payloadClaims) method#454onmax wants to merge 8 commits intoauth0:perform-releasefrom onmax:feature/#445
Conversation
jimmyjames
added
CH: Added
medium
jimmyjames
left a comment
jimmyjames
left a comment
There was a problem hiding this comment.
Thanks for the PR @onmax! In addition to the comment I left about ensuring we perform the same claim validation logic we currently have in this new method, can you remove the formatting changes to the src and test files (README is fine to leave)? Thanks!
| } | ||
|
|
||
| return this; | ||
| } |
There was a problem hiding this comment.
We should perform the same claim validations here as we do for others (key not null, values are supported types (should be able to use the isSupportedType() method).
There was a problem hiding this comment.
Yeah, I wasn't sure about that. But I can add isSupportedType(), I will check it later on
|
Yep, I will change it. Sorry, forgot I had the auto-format mode on, and didn't check my commit. My bad |
|
@onmax feel free to push additional commits to this PR to address the requested changes. I'll be away next week, so will circle back the week of November 30. |
|
Just updated the code with the requirements you asked. No auto-formatting. |
|
Hi, any news with this PR? |
jimmyjames
left a comment
jimmyjames
left a comment
There was a problem hiding this comment.
Generally looks good, but currently this does not compile and one of the tests needs updating to reflect the null handling for map claims.
If unable to follow-up with requested changes, we'll look at using this PR as a starting point and adding support for it as there does seem interest in this capability.
| * @throws IllegalArgumentException if payloadClaims is not well-formed and not having null values | ||
| * @return this same Builder instance. | ||
| */ | ||
| public Builder withPayload(Map<?, ?> payloadClaims) throws IllegalArgumentException { |
There was a problem hiding this comment.
Shouldn't this take a Map<String, Object>? As is, this does not compile (see build failure).
| payload.put(PublicClaims.KEY_ID, null); | ||
| payload.put("test2", "isSet"); | ||
|
|
||
| String signed = JWTCreator.init().withKeyId("test").withPayload(payload).sign(Algorithm.HMAC256("secret")); |
There was a problem hiding this comment.
This test would fail as implemented, as the claim validation of map entries does not allow nulls. We probably want to maintain that consistency as currently implemented for lists and maps.
|
Using this PR as a starting point, I've made #475 to get this feature in. Thanks to @onmax and others for starting this and providing feedback. I encourage anyone interested to try and/or review the changes in #475, and provide any feedback there. I'm closing this PR in favor of #475, I encourage anyone interested to try and/or review those changes and provide any feedback there. |
3 participants
Changes
Previously, you could initialize the
Headerof aJWTfrom aMapwithout any problem, but the story changed if you wanted to initialize aPayload, since you could not do it from aMap. The user had to add attributes one by one. With this new change, I added a new methodwithPayload(Map<String, Object> payloadClaims)inJWTCreator.javathat allows to initialize thePayloadfrom aMap.References
Links supporting this change such as a:
Testing
Also, this new method has its own tests in the
JWTCreatorTest.java.Checklist