-
Notifications
You must be signed in to change notification settings - Fork 49
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
Minor JWT build improvements #246
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.
A few comments, looks good overall.
implementation/src/main/java/io/smallrye/jwt/build/JwtEncryptionBuilder.java
Outdated
Show resolved
Hide resolved
implementation/src/main/java/io/smallrye/jwt/build/impl/JwtSigningUtils.java
Outdated
Show resolved
Hide resolved
implementation/src/test/java/io/smallrye/jwt/build/JwtBuildConfigSource.java
Outdated
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.
Just a recommendation to complete the deprecated tag, but not a blocker imo.
implementation/src/main/java/io/smallrye/jwt/build/JwtEncryptionBuilder.java
Show resolved
Hide resolved
implementation/src/main/java/io/smallrye/jwt/build/JwtEncryptionBuilder.java
Show resolved
Hide resolved
implementation/src/main/java/io/smallrye/jwt/build/JwtEncryptionBuilder.java
Show resolved
Hide resolved
implementation/src/main/java/io/smallrye/jwt/build/JwtSignatureBuilder.java
Show resolved
Hide resolved
implementation/src/main/java/io/smallrye/jwt/build/JwtSignatureBuilder.java
Show resolved
Hide resolved
Hi @MikeEdgar @radcortez thanks, will deal with the comments |
@MikeEdgar @radcortez I think I've addressed all the comments, cheers |
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.
LGTM
Hi Mike thanks, I've realized I forgot to update the docs (also updated the text related to the previous changes in sync with the similar Quarkus text, @bobbyphilip - minor update around the key properties to clarify when they are effective) - please check, I can merge later this evening if there will be no comments but if some wording/grammar can be improved then it will be fine. |
@radcortez Thanks, I'll do another couple of minor updates :-) |
Merging now as it has already been approved a few times, updated the text to refer to a |
This PR:
Deprecates a number of builder API methods and introduces more compact alternatives.
For example, when we do:
Jwt.claims("somejson.json").jws().signatureKeyId("kid").sign()
,signature
insignatureKeyId
is duplicating what bothjws()
andsign
imply. Similarly for all other deprecated methods. The IDE also helps with algorithm related methods, for ex, when we dojws().algorithm(...)
we see aSignatureAlgorithm
parameter type.In fact, originally I did plan for
Jwt.claims("somejson.json").jws().keyId("kid").sign()
- the compactness of the API is a big priority, but then I somehow got offtrack when I was looking at the inner token case, where the token is signed and encrypted:Here I thought well, 2
keyId()
, 1st one sets a key if of the signing key, 2nd one - of the key encryption key.But over time I've come to the conclusion that this case is quite advanced, the mainstream case is
sign
and in this advanced case, after the formatting, it also becomes more readable.kid
s will likely be configurable too.none
for theinnerSign()
- I came to this conclusion while doing the initial MP JWT PR, it is basically useless :-) to have encrypt() with an internalnone
as it does not add anything to theencrypt
only case, i.e, it does not help with identifying who signed it. It also makes it hard to spot if the user made a typo in the config propertyAll of these deprecated methods/properties will be dropped in 3.0.0
exp
andissuer
- these are the only 2 claims that are required by MP JWT, so it will make much easier to test, every time I see a customexp
being set in the code I'm thinking, oh, we need to get it configured.