Conversation
Migrate from 0.9.1 API to 0.12.6. Removed pom related dependencies to 0.9.1 version.
Alex-GF
left a comment
There was a problem hiding this comment.
Thank you so much for the contribution. I would like to give two suggestions before merging the PR. Please add some discussion if you have another point of view.
- Forcing to provide a key for JWT in base64 is not very "developer friendly". If the new version of the package forces the secret to have this structure, I would propose to create an interface between the developer configuring the PricingContext and the use of the
getJwtSecret()method by the library. In particular, you can create a hidden method within the PricingContext (e.g.getBase64JwtSecret()) that retrieves the value returned by the originalgetJwtSecretmethod, and encode it in base64. After that, substitute all the calls togetJwtSecretof the library by the new method. - I love the built-in function for detecting weak secrets! However, I think that throwing an exception limits the "freedom" of the developers, I mean, maybe you're just testing the library and don't want to generate a complicate secret. With this in mind (and considering of course that having a strong secret is fundamental), I would suggest to wrap the exception with a try/catch block and just print a WARNING.
I'm looking forward to your response!
First pointYou should note that when giving a password you have to take in account the key bit length requirement. Regarding the developer friendly statement I think that developers are accostumed to dealing with that format (JWT, base64url, secrets, etc...) plus there are a lot of tools (web, CLIs, etc...) to generate base64 secrets Options: openssl rand -base64 32Second pointI think that your aproach is not ideal due to the facts exposed in Signature Algorithms Keys. Furthermore, RFC 7518 Section 3.2 clearly states that A key of the same size as the hash output or larger MUST be used with this algorithm and throwing an exception is needed to fullfill the specification. I think is not wise to interfere in that behaviour and I don't see any benefit in doing your proposal. |
Migrate from 0.9.1 to 0.12.6 API. Now
Pricing4Javausesio.jsonwebtoken:jjwt-apiinstead ofio.jsonwebtoken:jjwtto compile the library.io.jsonwebtoken:jjwt-implandio.jsonwebtoken:jjwt-jacksondepedencies are only needed for the test suite thats is why they havetestscope.Removed depedencies were:
io.jsonwebtoken:jakarta.xml-bind-apiorg.json:jsonorg.glassfish.jaxb:jaxb-runtimeNow if a user wants to use
Pricing4Javaapi they will have to add the following dependencies withruntimescope:There are two changes in
PricingContextAPI:READMEof repo jjwtSome other changes involves code that is in no way used particulary in
JwtUtils.I have also bumped
pom.xmlversion to5.6.0-SNAPSHOTto further make some changes before making a realease in Maven repository.