Skip to content

OAuth: factory methods in JwtDecoders does not allow changing timeout #9904

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
IsSkyfalls opened this issue Jun 12, 2021 · 9 comments
Closed
Assignees
Labels
status: waiting-for-feedback We need additional information before we can continue type: enhancement A general enhancement

Comments

@IsSkyfalls
Copy link

Expected Behavior

Factory methods in JwtDecoders should accept timeout values to pass into RemoteJWKSet.

Current Behavior

Well, it doesn't allow this.

Problem:

JwtDecoders has 2 factory methods, fromIssuerLocation and withProviderConfiguration. Which both initialize RemoteJWKSetwith only the jwkSetURLparameter. Because the resourceRetriever parameter is not set, it initializes with the default timeouts which are 500ms for both http_read and http_connect. Only 1 second is a bit short and just fail on my slow school wifi because SSL handshake took 2 seconds on its own.

Solution:

I could just stop using JwtDecoders and create JwtDecoder myself. This is like 15 lines of boilerplate code. But I think this should be customizable in JwtDecoders. The thing is, based on the current design(factory methods), I really couldn't find a proper place to pass in RemoteJWKSet or its parameters. Maybe a builder pattern is more appropriate in this situation?

@IsSkyfalls IsSkyfalls added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Jun 12, 2021
@aheritier
Copy link

Just faced the same issue. For some reason I reach the timeout at home and I cannot update the timeout for my local environment without rewriting what JwtDecoders.fromOidcIssuerLocation does very well

@sjohnr sjohnr assigned jgrandja and jzheaux and unassigned jgrandja Jun 14, 2021
@jzheaux
Copy link
Contributor

jzheaux commented Jun 14, 2021

Hi, @IsSkyfalls, thanks for the suggestion. Setting timeouts is a reasonable thing to want to do.

That said, it wouldn't make sense to pass in a RemoteJWKSet to JwtDecoders since it's not designed to expose any Nimbus primitives.

Instead, you can use NimbusJwtDecoder's builders to pass a RestOperations. Or if you want to work with the Nimbus API directly, you can use the jwtProcessorCustomizer method on the builder.

If that doesn't help enough, will you please share the boilerplate that you are having trouble simplifying?

@jzheaux jzheaux added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 14, 2021
@IsSkyfalls
Copy link
Author

Thanks for the help! NimbusJwtDecoder's builders are basically what I have suggested. Guess that's an RTFM for me.
Since the solution is found, I will mark this as closed.

@aheritier
Copy link

I am personally not convinced. Yes NimbusJwtDecoder's builder are fine but JwtDecoders are providing high level abstract configurations which are useful and they rely on non public code like JwtDecoderProviderConfigurationUtils thus instead of using all this generic code which is working pretty well we have to write a specific one just to be able to configure a timeout, it's a bit sad (but obviously not the end of the world).

@jzheaux
Copy link
Contributor

jzheaux commented Jun 16, 2021

Much of the code in JwtDecoderProviderConfigurationUtils is there to perform various discovery strategies that are useful for a framework but needless for an application.

For example, if you know what your JWK Set URI and supported algorithms are, the code goes from this:

@Bean 
JwtDecoder jwtDecoder() {
    return JwtDecoders.withIssuerLocation(issuerUri);
}

to this:

@Bean 
JwtDecoder jwtDecoder() {
    NimbusJwtDecoder jwtDecoder = NimbusJwtDecoders.withJwkSetUri(jwkSetUri)
            .algorithms((algs) -> algs.addAll(...)).build();
    jwtDecoder.setJwtValidator(JwtValidators.createDefaultWithIssuer(issuerUri));
    return jwtDecoder;
}

So, I'm not seeing why this is sad. @aheritier can you elaborate on what is tricky about your use case?

@aheritier
Copy link

@jzheaux The code change is effectively not difficult to do and we are effectively talking about 1 lines vs 3 lines of code.
The code in JwtDecoderProviderConfigurationUtils helps to keep the oauth integration generic with the discovery mechanism. While the 3 lines are perfectly doing the job, they are hardcoding more my application configuration with my specific authentication server.
For exemple, for jwkSetUri, I have to hardcode that it is issuerUri + .well-known/jwks.json (not sure if it is standard or specific to Auth0).
For algorithms I don't have to define them because these are the default one but I imagine that another resource server could require it.
My point is just that the code in JwtDecoderProviderConfigurationUtils and related is very useful and powerful and I am not sure that keeping them private is very valuable (I imagine it is for the team a way to reduce to the minimum the public API to maintain which I understand)

@jzheaux
Copy link
Contributor

jzheaux commented Jun 17, 2021

I wonder if this is a good point at which to introduce NimbusJwtDecoder.withIssuerLocation. It would return a builder like NimbusJwtDecoder.withJwkSetUri, except that it would additionally perform the discovery operations.

@aheritier
Copy link

it might be great @jzheaux but let's honest if we are only 2 to report the issue, it's not necessarily important.

@monowai
Copy link

monowai commented Feb 6, 2022

it might be great @jzheaux but let's honest if we are only 2 to report the issue, it's not necessarily important.

I came across this discussion while migrating from Keycloak to Auth0. I guess once you have resolved your security config issues, you don't revisit them very often.

Was surprised to find hard coded constants and lack of support to inject standard spring managed beans to create the desired behaviour. Even if fromOidcIssuerLocation accepted RestOperations or an externally configurable ResourceRetriever as an argument would be useful compared to using hard coded default implementations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-feedback We need additional information before we can continue type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants