NimbusReactiveJwtDecoder Takes Reactive Processor

[jzheaux]

No description provided.

[rwinch]

I like all the changes except for this part. I don't like that we have a public interface that has Nimbus types on it. There are a few ideas I had for this:

• Start with making the changes to use the new Nimbus APIs, but do not make a public API available for others. I like this idea a lot (at least for a start since in my mind it is not controversial)
• Move the builder and the interface into NimbusReactiveJwtDecoder since it is Nimbus specific. The class/interfaces would include Nimbus in the names since they are Nimbus specific
• Remove the interface and use something more generic like Converter<Mono<JWTClaimsSet>,SignedJWT>
• See if we can change the input and output to be something that is not Nimbus specific. For the input we could use a String. However, I'm not sure what we would do for the output. I'd like to avoid writing complex types of our own unless we are going to be using the types in other places.

I think it may be possible to use a combination of these approaches too.

[jzheaux]

I can see the value in the first option, but I figure if we can come to a consensus quickly on the contract, we might as well include it in this PR.

I'd not prefer the second as it would be ideal if the structure were roughly similar to the imperative side of things. We've already seen examples in other places where users have an imperative implementation of something and want to adapt it into their reactive applications. The way I see it, if the builder were part of NimbusReactiveJwtDecoder, the output of the builder would be the decoder itself as opposed to a contract similar to Nimbus's JWTProcessor, making this kind of reuse trickier.

I like the third option. Just to be sure, though, I think that Converter<SignedJWT, Mono<JWTClaimSet>> is correct. This is similar enough to the imperative contract that the user could easily adapt any non-blocking imperative implementation that they already have. It's also a pattern that Spring Security follows in other places, so there is some familiarity with it.

I like the fourth option; however, I tried implementing it and ran into a snag. The NimbusReactiveJwtDecoder needs to know the header in order to create an instance of Jwt. This means that it needs to parse the String before handing that String off to the processor. When I tried a contract of <String, Map<String, Object>> (token -> claim set), I ended up parsing the token twice. Alternatively, I thought I might try <String, Jwt> so that the processor can hand back the header as well, but that felt odd since we are now basically repeating the same contract as NimbusReactiveJwtDecoder.

Given all of this, I've updated the PR in a separate commit which shows what I see as a working option 3.

[GregoireW]

The second commit you did ends up pretty close to my PR #6367

However, I got an issue with the Converter<SignedJWT, Mono<JWTClaimSet>>

The code state:

@Override
public Mono<Jwt> decode(String token) throws JwtException {
	JWT jwt = parse(token);
	if (jwt instanceof SignedJWT) {
		return this.decode((SignedJWT) jwt);
	}
	throw new JwtException("Unsupported algorithm of " + jwt.getHeader().getAlgorithm());
}

Now I personally don't use EncryptedJWT (yet????) but it exists. What would be the approach for the people using it ?

Specialize another NimbusReactiveJwtDecoder ? or extends this one ?
First my thinking was to create another specialized decoder, but then I though of what can happen on my side: Multiple authorization server. It means I cannot be 100% sure of what those providers will send me. This would end up copying/pasting a lot of spring code to make this possible.

Perhaps it is better to add the EncryptedJWT use case, and so what is the better approach to this ? a constructor with one converter (signed JWT) and another with 2 converter (signed / encrypted) ? a single interface ?

[jzheaux]

Yes, this PR is definitely inspired by your initial work, @GregoireW!

I like that you are thinking about these other items, but I think we should keep this PR focused on the intended use case. It's better to solve one problem at a time. Spring Security doesn't yet support encrypted JWTs nor Authorization Server multi-tenancy, so trying to add support for it in this PR would probably jump the gun.

However, yes, I believe it would be a new constructor parameter. In the meantime, folks would probably implement ReactiveJwtDecoder and write their own reactive encryption support since neither Spring nor Nimbus has yet a reactive JWE API. They can always delegate to Spring Security's reactive JWS support once the JWT is decrypted.

[rwinch]

Thanks for the changes! I have provided comments inline

[rwinch]

After our changes I believe this name should change since there is no ReactiveJwtProcessor now.

I'm still thinking it might be valuable to move this into NimbusReactiveJwtDecoder. If we foresee it being used somewhere else, then we would want to keep it externalized though.

the output of the builder would be the decoder itself as opposed to a contract similar to Nimbus's JWTProcessor, making this kind of reuse trickier.

I don't think this is a problem. User's would still be able to inject a Converter directly.

[jzheaux]

If the builder returns a decoder and later on that builder moves out of NimbusReactiveJwtDecoder into its own class, the existing decoder constructors will need to be deprecated. I'm not so keen on their deprecation being tied to that refactor.

If we can, I'd prefer that the builder return the equivalent of a processor. (And if it comes down to naming, I have some thoughts below.)

Nevermind on this point - the decoder builder could expose a package-private method instead. Not saying that's the right choice, but it would allow those constructors to keep functioning just fine.

If we foresee it being used somewhere else ...

I think reuse is a very good point, and I struggle a bit with it because of JwtProcessors, which returns a JWTProcessor, a Nimbus-specific type. This builder has provided a lot of flexibility for NimbusJwtDecoder, making it easy to add other processing types without modifying the decoder. Will this pattern of building JWTProcessors be applied to other places than the NimbusJwtDecoder constructor? I would guess the answer to this is the answer here as well.

After our changes I believe this name should change since there is no ReactiveJwtProcessor now.

Again, this is sensible - JwtProcessors builds a JWTProcessor, JwtDecoders builds a JwtDecoder, etc.

We have an example of a converter whose parameter types don't alone describe what the converter is for: MappedJwtClaimSetConverter implements Converter<Map<String, Object>, Map<String, Object>>. Here, the name clarifies the semantics of the conversion and we don't want to call it MapMapConverter or similar.

Also, we have classes (not interfaces, AFAIK) that return Nimbus types, for example, JwtProcessors returns a JWTProcessor.

Given these, I believe it's sensible to either give a distinctive name to a certain type of converter, in order to clarify semantics -- have the interface ReactiveJwtProcessor extends Converter<SignedJWT, JWTClaimSet> (read: it's like Nimbus's JWTProcessor, but reactive) -- or to simply state what we are producing, even if the builder's name for a Converter<S,T> doesn't precisely match STConverter -- do ReactiveJwtProcessors since what is effectively produced is a reactive version of Nimbus's JWTProcessor.

Though, given that I'm saying "Nimbus's JWTProcessor" so much, it might be better to call it ReactiveNimbusJwtProcessors.

This has come about because of the absence of a reactive API for Nimbus, and I'm guessing it won't be the last time. Possibly, we could gauge Nimbus's interest in maintaining a reactive version of their API that depends on Project Reactor, though that seems unlikely. Given that, I don't think it's unreasonable to introduce types that are the reactive equivalent of Nimbus APIs and calling them such.

[jzheaux]

Had a chat offline with @rwinch and will summarize here.

The main reason for my concern is symmetry between the servlet and reactive APIs. So, since neither is released for this feature, yet, we decided to actually change the servlet version so that it also internalized the decoder builder.

[rwinch]

Thanks LGTM