Add JwtEncoderAlternative #30
Conversation
I like JwtEncoderAlternative because: - It supports delegation, a powerful design pattern - It provides reasonable defaults, and because those defaults can be easily overridden with delegation, we can afford to be very opinionated, further simplfying common use cases - It doesn't introduce new domain objects, only extracts Jwt.Builder into interfaces - Its format is more compact since it's not necessary to create a JoseHeader and JwtClaimSet in order to call - Since it is simpler to have reasonable defaults, the minimal call is factory.signer().sign(), which is surprisinging succinct - A utility-level encoder(headers, claims) can still be abstracted out later - also it seems likely that Nimbus will provide something like this anyway - It allows signing and encryption to be specified in one call
| * I like this interface due to the symmetry with Spring Security's claim accessors | ||
| * | ||
| */ | ||
| public interface JwsHeaderMutator<M extends JwsHeaderMutator<M>> { |
There was a problem hiding this comment.
This interface represents the minimal set of headers necessary to specify a JWT.
I like this interface due to the symmetry it provides to Spring Security's claim accessors, like JwtClaimAccessor.
| * @param value | ||
| * @return | ||
| */ | ||
| default M criticalHeader(String name, Object value) { |
There was a problem hiding this comment.
The JWS spec indicates that when the crit header is specified, the related critical header must also be in the header. This introduces the possibility for error. Spring Security can alleviate this by asking for critical headers in a separate method.
Critical headers are still added, but they also ultimately add the crit header whose value is the set of all critical header names.
| * @param value | ||
| * @return | ||
| */ | ||
| default M header(String name, Object value) { |
There was a problem hiding this comment.
Since no other headers are required, and since those headers are quite easy to get wrong in the general case, I think it's best to leave other headers out for now.
Generally speaking, those headers are for looking up keys anyway, which is something likely better decided centrally in an encoder instead of by the caller. And even if that's not the case, this method still exists so that an application can specify them if needed.
| * @see <a target="_blank" href= | ||
| * "https://tools.ietf.org/html/rfc7519#section-4.1">Registered Claim Names</a> | ||
| */ | ||
| public interface JwtClaimMutator<M extends JwtClaimMutator<M>> { |
There was a problem hiding this comment.
I like the symmetry that this provides to JwtClaimAccessor. It's reasonable for this to be an interface since it can be implemented by builders (like Jwt.Builder) as well as by method parameter mutators like JwtMutator.
| * | ||
| * @param <B> | ||
| */ | ||
| interface JwtMutator<B extends JwtMutator<B>> { |
There was a problem hiding this comment.
It's more natural to return a builder-like object since encoding a JWT is a sophisticated operation with a hard-to-reverse result.
| // other examples in their JavaDoc. Spring Security has a custom | ||
| // Nimbus SecurityContext in the reactive code as another example. | ||
|
|
||
| private static class TenantJwkSource implements JWKSource<SecurityContext> { |
There was a problem hiding this comment.
Obviously, there are numerous ways to represent multi-tenancy, and this example is not meant to indicate that this is the way it should be done. This is simply an example of something application-specific that an application may want to pass into their Nimbus code. Nimbus lists other examples in their JavaDoc. Spring Security additionally uses a custom Nimbus SecurityContext in the reactive code, as another example.
| * For example, note that because of the application configuration, I only | ||
| * need one Spring Security component to sign and serialize a JWT. | ||
| */ | ||
| @GetMapping("/token/two/simple") |
There was a problem hiding this comment.
What's nice about this pattern is that it's minimal for the application. For example, note that because of the application configuration, I only need one Spring Security component to sign and serialize a JWT.
| NimbusJwtEncoder delegate = new NimbusJwtEncoder(); | ||
| return () -> delegate.encoder() | ||
| .jwsKey(key) // simple to specify the key since I can expose Nimbus-specific methods when returning a builder | ||
| .jwsHeaders((jws) -> jws.criticalHeader("exp", Instant.now())) // application-level opinion that might need overriding |
There was a problem hiding this comment.
Here, one can imagine a platform team creating a Spring Security component and including some critical header by default. It's nice to do it here so that the application developer that is calling the encoder doesn't need to remember to add it in.
And, in the exceptional case when there was something specified by the platform team that needs overriding, it can still be easily overridden.
| NimbusJwsEncoder delegate = new NimbusJwsEncoder(jwks); | ||
| return (headers, claims) -> { | ||
| JoseHeader.Builder defaultHeaders = JoseHeader.from(headers); | ||
| if (!headers.getHeaders().containsKey(JoseHeaderNames.CRIT)) { // can't tell if caller wants `crit` to not be in the header or wants to take the encoder's opinion |
There was a problem hiding this comment.
The problem here is that the implementation cannot tell what the caller means by having left this header out. Either it means that the encoder should decide or it means that the caller wants there to be no crit header.
This is meant to illustrate that the original PR forces the component developer down a particular path of developing multiple components that the application developer now has to remember to put together in the right way.
| * is required to get all the benefit, which I think is quite nice for the application developer. | ||
| */ | ||
| @GetMapping("/token/two") | ||
| String tokenTwo() { |
There was a problem hiding this comment.
This method is intended to demonstrate that both coding styles are possible with JwtEncoderAlternative, though I'll point out that this is less preferred due to the number of Spring Security components involved. An easier to invoke API would be more secure.
In Authorization Server, there is an additional component for customizing claims and headers. Using this customizer, it now may require four components to encode a JWT:
- The customizer to create:
- A header object, and
- A claims object. Then,
- Pass those two objects into a fourth and final object, the encoder
| /** | ||
| * Use this {@link JWK} to sign the JWT | ||
| */ | ||
| public NimbusJwtMutator jwsKey(JWK jwk) { |
There was a problem hiding this comment.
This is intended to demonstrate that this pattern simplifies exposing library-specific domain objects that we don't want to replicate in Spring Security, like Nimbus's JWK.
I think this method might be unnecessary since with jwsSecurityContext the caller could specify a JWKSecurityContext. But, I've left it in here for demonstration.
| /** | ||
| * Send this {@link SecurityContext} to Nimbus's signing infrastructure | ||
| */ | ||
| public NimbusJwtMutator jwsSecurityContext(SecurityContext context) { |
There was a problem hiding this comment.
This is intended to demonstrate the power of exposing library-specific domain objects at the method-invocation level. Since Nimbus takes a SecurityContext as a method parameter, an API that also accepts it as a method parameter will have the least impedance mismatch.
| */ | ||
| public interface JwtEncoderAlternative { | ||
|
|
||
| enum EncodingMode { SIGN, ENCRYPT, SIGN_THEN_ENCRYPT } |
There was a problem hiding this comment.
I'm not aware of any use cases where applications would want to use Spring Security to only encrypt a JWT, but this additional commit shows that JWS, JWE, and JWS+JWE can be accommodated passively.
1 participant
I like JwtEncoderAlternative because:
can be easily overridden with delegation, implementers can afford to
be very opinionated, further simplfying common use cases
Jwt.Builder into interfaces
to create a JoseHeader and JwtClaimSet in order to call
the minimal call is factory.encoder().encode(), which is
surprisinging succinct
abstracted out later - also it seems likely that Nimbus
will provide something like this anyway
in one call by allowing for two different mutator methods, one for
jwsHeadersand one for
jweHeaders-- such is not possible with the original PRsince both the JWS algorithm and JWS algorithm cannot be
specified in the same invocation.
Note that this pull request contains three sample applications that compare
JwtEncoder(the original PR) andJwtEncoderAlternative. As part of these sample applications, there is a README to describe how to interpret them. The main argument for this proposed change is here.