Introduce JwtEncoder with JWS implementation #96
Conversation
jgrandja
added
status: duplicate
|
@krajcsovszkig-ms @anoopgarlapati @jzheaux Here is the initial draft of You can run the integration test Feedback would be great. NOTE: Tests and javadoc have not been done yet. |
jgrandja
force-pushed
the
gh-81-jwt-encoder
branch
from
bd1cc6c to
854176e
Compare
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| public <T> T getKey() { |
There was a problem hiding this comment.
I'd prefer returning a Key here and let the caller cast it explicitly to what they think it is, or having separate getPrivateKey() and getSymmetricKey() methods that return null or throw an exception if the key field is not the correct type.
There was a problem hiding this comment.
This was updated to:
public <T extends Key> T getKey() {
return (T) this.key;
}To enforce a Key type
| * @author Joe Grandja | ||
| * @since 0.0.1 | ||
| */ | ||
| public final class StaticKeyGeneratingKeyManager implements KeyManager { |
There was a problem hiding this comment.
I'd call this DefaultKeyManager and document that it generates HMAC and RSA keys. Or alternatively make the generated keys configurable.
There was a problem hiding this comment.
This implementation is strictly for development/testing and I've javadoc'd it. This is good for now but it will likely go away later on when a new implementation is provided.
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| public <T> T getHeader(String name) { |
There was a problem hiding this comment.
If this is public, I'd prefer returning an Object and having the caller cast it explicitly. Perhaps have a separate private one that auto-casts to make the specific getters neater.
There was a problem hiding this comment.
I think this is cleaner than casting:
JwsAlgorithm jwsAlgorithm = joseHeader.getHeader(JoseHeaderNames.ALG)
vs.
JwsAlgorithm jwsAlgorithm = (JwsAlgorithm) joseHeader.getHeader(JoseHeaderNames.ALG)
The caller will know the type.
| JWSHeader jwsHeader = convert(headers); | ||
|
|
||
| claims = JwtClaimsSet.from(claims) | ||
| .id(UUID.randomUUID().toString()) // TODO Is this unique enough? |
There was a problem hiding this comment.
It seems to be using secureRandom() so it's as good as it gets IMO.
|
Hi @jgrandja , Do you have an approximate ETA on this? Do you expect the interfaces to change much? If not, I might start building on them in a week or two. Thanks! |
|
I'm planning on getting back to this PR this week @krajcsovszkig-ms and merging it later this week or early next week. As an FYI, this won't make it into Spring Security 5.4 (Sep 2 release) but will be the first priority for 5.5, which will be released approx. 6 months after 5.4. I'm not expecting major changes to the current API but likely there will be some minor changes. I'm very interested to see how this works out on client side so please go ahead with your implementation and provide me feedback on how we can improve on the current API. Thanks! |
jgrandja
force-pushed
the
gh-81-jwt-encoder
branch
from
854176e to
edefabd
Compare
|
@krajcsovszkig-ms I just merged this so you're good to go on your end. |
|
Great, thanks! |
Issue gh-81