Skip to content
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

JwkSourceReactiveJwtDecoderBuilder should be typed SignedJWT instead of JWT #6771

Closed
jgrandja opened this issue Apr 11, 2019 · 5 comments · Fixed by #6827
Closed

JwkSourceReactiveJwtDecoderBuilder should be typed SignedJWT instead of JWT #6771

jgrandja opened this issue Apr 11, 2019 · 5 comments · Fixed by #6827
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: first-timers-only An issue that can only be worked on by brand new contributors
Milestone

Comments

@jgrandja
Copy link
Contributor

jgrandja commented Apr 11, 2019

We should consider removing NimbusReactiveJwtDecoder.JwkSourceReactiveJwtDecoderBuilder since it's not used.

I also noticed that the equivalent does not exist in NimbusJwtDecoder.

If we decide to keep it than we should change the generic types from Function<JWT, Flux<JWK>> to Function<SignedJWT, Flux<JWK>>.

@jgrandja jgrandja added Refactoring in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) labels Apr 11, 2019
@jgrandja jgrandja added this to the 5.2.0.M2 milestone Apr 11, 2019
@jzheaux
Copy link
Contributor

jzheaux commented Apr 12, 2019

Agreed that it should be SignedJWT.

This implementation addresses #6367 and is intended to simplify the construction of a Converter<SignedJWT, Mono<JWTClaimSet>>. Constructing one of these is more complex than a JWTProcessor (the rough NimbusJwtDecoder equivalent) because the user can't simply supply an implementation of JWKSource when configuring it.

With traditional Nimbus, the user can customize key retreival like so:

JWKSource<SecurityContext> jwkSource = // use a Nimbus implementation or customize
JWSKeySelector<SecurityContext> jwsKeySelector =
		new JWSVerificationKeySelector<>(jwsAlgorithm, jwkSource);
DefaultJWTProcessor<SecurityContext> jwtProcessor = new DefaultJWTProcessor<>();
jwtProcessor.setJWSKeySelector(jwsKeySelector);
NimbusJwtDecoder jwtDecoder = new NimbusJwtDecoder(jwtProcessor);

However, with reactive, it's quite a bit more roundabout, using the Nimbus API in a counter-intuitive way:

Function<SignedJWT, Flux<JWK>> reactiveJwkSource = // get the keys reactively
JWKSecurityContextJWKSet jwkSource = new JWKSecurityContextJWKSet();
JWSKeySelector<JWKSecurityContext> jwsKeySelector =
		new JWSVerificationKeySelector<>(this.jwsAlgorithm, jwkSource);
DefaultJWTProcessor<JWKSecurityContext> jwtProcessor = new DefaultJWTProcessor<>();
jwtProcessor.setJWSKeySelector(jwsKeySelector);
Converter<SignedJWT, Mono<JWTClaimSet>> reactiveJwtProcessor = signedJWT -> 
        reactiveJwkSource.apply(signedJWT)
                .collectList()
                .map(jwks -> createClaimSet(jwtProcessor, signedJWT, new JWKSecurityContext(jwks)));
NimbusReactiveJwtDecoder jwtDecoder = new NimbusReactiveJwtDecoder(reactiveJwtProcessor);

I believe it would be challenging and cumbersome for the user to configure the decoder in this way. So, they can instead do:

Function<SignedJWT, Flux<JWK>> reactiveJwkSource = // get the keys reactively
NimbusReactiveJwtDecoder jwtDecoder = NimbusReactiveJwtDecoder
    .withJwkSource(reactiveJwkSource).build();

Since the primary configuration of a NimbusXXXDecoder is to indicate the key retreival strategy, this seems like a reasonable abstraction to expose.

Of course, we could add NimbusJwtDecoder#withJwkSource(JWKSource<SecurityContext> jwkSource) for symmetry, but I don't see the same need since setting a JWKSource on a blocking JWTProcessor is quite straightforward.

For some extra context, the original impetus was from some requirements outlined by @GregoireW.

If you are agreed that we should keep it and simply change the parameter type from JWT to SignedJWT, I'd propose changing the description in this ticket to indicate as much and then make it a first-timers-only ticket since it would be quite a simple change.

@jgrandja jgrandja modified the milestones: 5.2.0.M2, 5.2.0.RC1 Apr 15, 2019
@jgrandja jgrandja changed the title JwkSourceReactiveJwtDecoderBuilder not used JwkSourceReactiveJwtDecoderBuilder should be typed SignedJWT instead of JWT Apr 28, 2019
@jgrandja jgrandja added the status: first-timers-only An issue that can only be worked on by brand new contributors label Apr 28, 2019
@jgrandja
Copy link
Contributor Author

jgrandja commented Apr 28, 2019

Thanks for the explanation @jzheaux. This makes sense. We only need to change Function<JWT, Flux<JWK>> to Function<SignedJWT, Flux<JWK>>. I updated the ticket.

@alurysharad
Copy link
Contributor

Hello
Is anyone working on this ?

Thank you
Sharad

@jzheaux
Copy link
Contributor

jzheaux commented Apr 30, 2019

@alurysharad, no, are you interested in submitting a PR for it?

@alurysharad
Copy link
Contributor

Yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: first-timers-only An issue that can only be worked on by brand new contributors
Projects
None yet
3 participants