Skip to content

Allow for custom rest template #8588

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

Closed
wants to merge 1 commit into from
Closed

Allow for custom rest template #8588

wants to merge 1 commit into from

Conversation

Budlee
Copy link
Contributor

@Budlee Budlee commented May 24, 2020

No description provided.

@Budlee
Copy link
Contributor Author

Budlee commented May 24, 2020

Reference to issue #5607

@jzheaux
Copy link
Contributor

jzheaux commented May 30, 2020

@Budlee, thanks for taking some time to think this through. Making RestTemplate more configurable here would certainly be nice.

This is still an open discussion, but the options I can see are:

ReactiveJwtDecoders#fromIssuerLocation(String issuer, RestTemplate rest)

or providing a new class that implements ReactiveJwtDecoderFactory:

public class IssuerReactiveJwtDecoderFactory implements ... {
    private final RestTemplate rest;

    @Override
    public ReactiveJwtDecoder createDecoder(String issuer) {
        // ...
    }

    public void setRestTemplate(RestTemplate rest) {
        // ...
    }
}

with equivalents needed for imperative.

Either way, I think that the Utils class may still move around a bit, so I'd prefer to keep that package-private as it is. AFAICT, it's not necessary to expose it.

As far as multi-tenancy, I think that your proposal doesn't yet address the use case you raised for having a different RestTemplate per issuer. To have something that sophisticated, I think you'd still need to supply your own AuthenticationManagerResolver<String> in the constructor.

Even still, it may be valuable to expose something like a ReactiveJwtDecoderFactory<String> in JwtIssuerReactiveAuthenticationManagerResolver, but that will probably depend on how the larger conversation about RestTemplate goes.

@rwinch
Copy link
Member

rwinch commented Jun 1, 2020

Another option might be to allow the user to use RestTemplate directly.

@Budlee
Copy link
Contributor Author

Budlee commented Jun 14, 2020

@jgrandja is there any decision on what is the way forward to expose this to the RestTemplate to the user? Is the idea to commit to #8624 as the solution

@jgrandja
Copy link
Contributor

@Budlee I'm going to spend some time this week to figure out a solution. This one is a bit tricky as there are quite a few touch points in the code and a couple of special use cases.

@jgrandja
Copy link
Contributor

jgrandja commented Jun 26, 2020

@Budlee The original design intent for JwtDecoders and ReactiveJwtDecoders is that it was meant to be used as a utility class, hence the static methods. It was provided as a convenience for most use cases but not all. For more advanced use cases, eg. where you need to configure proxy settings for the underlying HTTP client, you can configure a JwtDecoder (or ReactiveJwtDecoder) and expose it as a @Bean - see example 1 and example 2.

After reviewing your PR, we do not want to expose JwtDecoderProviderConfiguration or provide a ReactiveJwtDecoders.fromIssuerLocation(String issuer, RestTemplate rest). This was never the design intent for JwtDecoders or ReactiveJwtDecoders.

I'm going to close this PR based on the above reasoning.

@jgrandja jgrandja closed this Jun 26, 2020
@jgrandja jgrandja added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 26, 2020
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: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants