Add Key Id setter and set JWT Type after signing#138
Conversation
| SimpleModule module = new SimpleModule(); | ||
| module.addSerializer(ClaimsHolder.class, new PayloadSerializer()); | ||
| mapper.registerModule(module); | ||
| mapper.configure(MapperFeature.SORT_PROPERTIES_ALPHABETICALLY, true); |
There was a problem hiding this comment.
I guess this is required because of the JsonMatcher and it's own implementation of JSON serialization?
Maybe it was just better to use jackson there? It would have been, at least, less code.
There was a problem hiding this comment.
This was the previous solution to the JSON tests that were matching against a full json string. The issue was with java 7 and 8 were the json entries were ordered in a different way. After I added the entry/value check instead of comparing against a full json output, I think we can remove this.
By the way, this is Jackson. What do you mean by that?
There was a problem hiding this comment.
I meant the JsonMatcher could use jackson instead of having it's own implementation of {object/list/array/map}ToString
There was a problem hiding this comment.
Yes, I agree. There was a reason though of why I avoided that approach at first. I guess I wanted to assure that the output keys/names were actually the ones the API was expecting, mostly because there is inconsistence between the naming of the variables and the json keys.
| assertThat(signed, is(notNullValue())); | ||
| String[] parts = signed.split("\\."); | ||
| String headerJson = new String(Base64.decodeBase64(parts[0]), StandardCharsets.UTF_8); | ||
| assertThat(headerJson, JsonMatcher.hasEntry("kid", "56a8bd44da435300010000015f5ed")); |
There was a problem hiding this comment.
since this code is repeated a few times, maybe we could add a matcher, something like JWTMatcher.hasHeaderEntry ? Or at least a method to wrap the split, base64decode of parts[0] and the hasEntry.
| throw new IllegalArgumentException("The Algorithm cannot be null."); | ||
| } | ||
| headerClaims.put(PublicClaims.ALGORITHM, algorithm.getName()); | ||
| headerClaims.put(PublicClaims.TYPE, "JWT"); |
There was a problem hiding this comment.
just saw this, but why the PublicClaims.TYPE, PublicClaims.ALGORITHM? isn't this making the code harder to understand? it's a jwt lib, the field is typ so I would have expected something like PublicClaims.TYP
At least me, as an end-user, would not be really confident and would have to check the code to see if it's the actual value I'm looking for.
There was a problem hiding this comment.
I know the class is public (and also it's name, lol) but they are not really intended to be used by the user. There are individual setters for each of this "public claims" in the JwtCreator and JwtVerifier classes for that purpose. Maybe I should find another package to keep this class, something like utils or internal.
| assertThat(jwt, is(notNullValue())); | ||
| assertThat(jwt, is(token)); | ||
| String[] parts = jwt.split("\\."); | ||
| assertThat(parts[1], is("eyJuYW1lIjoidmFsdWUifQ")); |
There was a problem hiding this comment.
why are these tests different from the others (the header ones)? I mean, here you have the hardcoded value, and in the others you decode them.
There was a problem hiding this comment.
Because of the JSON entries ordering issue: In this case because the decoded body contains just 1 key, I don't have issues with the JSON order and the base64 encoded part is the same both for Java 7 and Java 8. In the case of the header, besides the key I'm specifying in each test, the signing process adds the typ and alg claims. So entries order can mess things a bit and return a different base64 encoded string for the same (but in different order) values.
lbalmaceda
force-pushed
the
add-kid-setter
branch
from
71034c0 to
d5ce251
Compare
3 participants
Add setter for header's 'kid' value. Also set the 'typ' to "JWT" after the signing process.