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

Cannot override WebClient for NimbusReactiveJwtDecoder in ReactiveOidcIdTokenDecoderFactory #14178

Closed
ZIRAKrezovic opened this issue Nov 21, 2023 · 8 comments · Fixed by #14357
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement

Comments

@ZIRAKrezovic
Copy link

Expected Behavior

As the title says ... the NimbusReactiveJwtDecoder is created with default web client initialized statically. There is no way to change it. It should be possible to change the WebClient passed to NimbusReactiveJwtDecoder.

Current Behavior

Compared to other parts of the reactive stack, it is currently impossible to customize WebClient passed to NimbusReactiveJwtDecoder

Context

I have a Keycloak instance behind a self-signed SSL certificate. I have configured webclient instance to get it using WebClientSsl builder. But I can't pass that WebClient to NimbusReactiveJwtDecoder.

@ZIRAKrezovic ZIRAKrezovic added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Nov 21, 2023
@jzheaux
Copy link
Contributor

jzheaux commented Dec 1, 2023

I believe it is already possible:

@Bean 
ReactiveJwtDecoder jwtDecoder() {
    return NimbusReactiveJwtDecoder.withIssuerLocation("https://example.org/issuer")
        .webClient(web).build();
}

or

@Bean 
ReactiveJwtDecoder jwtDecoder() {
    return NimbusReactiveJwtDecoder.withJwkSetUri("https://example.org/jwks")
        .webClient(web).build();
}

Are you having trouble with either of these approaches?

@jzheaux jzheaux self-assigned this Dec 1, 2023
@jzheaux jzheaux added status: waiting-for-feedback We need additional information before we can continue in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) and removed status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Dec 1, 2023
@krezovic
Copy link
Contributor

krezovic commented Dec 1, 2023

Hi @jzheaux, the problem is not wit NimbusReactiveJwtDecoder itself, but rather one that is created within ReactiveOidcIdTokenDecoderFactory.

Examine the line: https://github.com/spring-projects/spring-security/blob/main/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/ReactiveOidcIdTokenDecoderFactory.java#L166

There is no way to pass WebClient to that specific instance, which is not created by myself and cannot be easily overriden due to private methods all around ...

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Dec 1, 2023
@jzheaux
Copy link
Contributor

jzheaux commented Dec 11, 2023

I see, sorry that I missed that.

I think it would be reasonable to add a method to ReactiveOidcTokenDecoderFactory for specifying the WebClient. instance. Something like:

public void setWebClientFactory(Function<ClientRegistration, WebClient> webClientFactory) {
    // ...
}

Are you able to provide a PR to add that to ReactiveOidcIdTokenDecoderFactory and OidcIdTokenDecoderFactory?

@jzheaux jzheaux added status: ideal-for-contribution An issue that we actively are looking for someone to help us with type: enhancement A general enhancement and removed status: feedback-provided Feedback has been provided labels Dec 11, 2023
@ZIRAKrezovic
Copy link
Author

Hi @jzheaux, I may be able to do it during the oncoming holidays.

@jgrandja
Copy link
Contributor

@jzheaux

Are you able to provide a PR to add that to ReactiveOidcIdTokenDecoderFactory and OidcIdTokenDecoderFactory?

I don't think we should enhance OidcIdTokenDecoderFactory. Please see this comment for further details.

Furthermore, given that we will eventually provide support for the new RestClient, this would result in deprecating usages of RestOperations.

jzheaux added a commit that referenced this issue Jan 19, 2024
jzheaux added a commit that referenced this issue Jan 22, 2024
jzheaux added a commit that referenced this issue Jan 22, 2024
@jzheaux jzheaux removed the status: ideal-for-contribution An issue that we actively are looking for someone to help us with label Feb 1, 2024
@jzheaux
Copy link
Contributor

jzheaux commented Feb 1, 2024

Closed in favor of #14357

@jzheaux
Copy link
Contributor

jzheaux commented Apr 11, 2024

Based on this comment as well as the simplicity of this construction, I think the following arrangement is preferred:

@Component
public class MyJwtDecoderFactory implements JwtDecoderFactory<ClientRegistration> {
    @Cacheable
    public JwtDecoder createDecoder(ClientRegistration registration) {
        String issuerUri = client.getProviderDetails().getIssuerUri();
        NimbusJwtDecoder decoder = NimbusJwtDecoder.withIssuerLocation(issuerUri).restOperations(this.rest).build();
        decoder.setJwtValidator(new DelegatingOAuth2TokenValidator<>(
                JwtValidators.createDefault(), new OidcIdTokenValidator(client)));
        decoder.setClaimTypeConverter(
                new ClaimTypeConverter(OidcIdTokenDecoderFactory.createDefaultClaimTypeConverters()));
        return decoder;
    }
}

Also, recent 6.3 changes will simplify this to:

@Component
public class MyJwtDecoderFactory implements JwtDecoderFactory<ClientRegistration> {
    @Cacheable
    public JwtDecoder createDecoder(ClientRegistration registration) {
        String issuerUri = client.getProviderDetails().getIssuerUri();
        NimbusJwtDecoder decoder = NimbusJwtDecoder.withIssuerLocation(issuerUri).restOperations(this.rest).build();
        decoder.setJwtValidator(JwtValidators.createDefaultWithValidators(new OidcIdTokenValidator(client)));
        decoder.setClaimTypeConverter(OidcIdTokenDecoderFactory.createDefaultClaimTypeConverter());
        return decoder;
    }
}

As such, I think we should revert the setWebClient and setRestOperations changes for now. We can always add them back in later on should this prove to be an issue.

@neoXfire
Copy link

neoXfire commented Jun 1, 2024

Hi @jzheaux,

For info, i am not a OIDC expert so please double check what I am saying just after 😃

Customizing the WebClient instance must be needed for something as simple as customizing the proxy policy to adapt to end user technical environment like with the issue #8882

The ReactiveOidcIdTokenDecoderFactory provided by default in Spring Security offers a lot of funcitonnalities like handling multipe jws algorithms (Signature, Mac, Secret...) and default claim types mappings for example.
Reimplementing a JwtDecoderFactory from scratch could be as simple as the examples you provided in your previous comment. But I think it is only applicable on situations where you are totally aware of the technical environment where your application is going to be deployed. In other cases, it means saying goodbye to some of the functionnalities provided by spring-security in the default implementation. My guess is that those are the functionnalities which make the library working smoothly with a lot of OIDC solution providers by default.

I was very happy with #14357 merge. It is sad that was reverted because in my case the only solution I see right now is copy pasting the ReactiveOidcIdTokenDecoderFactory tweaking the class as you initially proposed. Do you think it can be added back ?

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) type: enhancement A general enhancement
Projects
None yet
6 participants