Extract the ID Token JwtDecoderFactory to enable user customization #6415
Conversation
jgrandja
changed the title
jgrandja
changed the title
There was a problem hiding this comment.
Thanks for the PR @raphaelDL ! Please see my comments inline.
There was a problem hiding this comment.
Let's move the static members to top of declarations.
There was a problem hiding this comment.
We don't need to use DelegatingOAuth2TokenValidator given that we're directly assigning the one returned instance.
There was a problem hiding this comment.
Let's move the static members to top of declarations.
There was a problem hiding this comment.
I don't think this test makes sense. It's pretty much the same as above test. Please remove it.
There was a problem hiding this comment.
We should Assert.notNull on clientRegistration
There was a problem hiding this comment.
We should Assert.notNull on clientRegistration
There was a problem hiding this comment.
I feel that the only tests we need here are as follows:
setJwtValidatorFactoryWhenNullThenThrowIllegalArgumentExceptioncreateDecoderWhenClientRegistrationNullThenThrowIllegalArgumentExceptioncreateDecoderWhenJwkSetUriEmptyThenThrowOAuth2AuthenticationExceptioncreateDecoderWhenClientRegistrationValidThenReturnDecodercreateDecoderWhenCustomJwtValidatorFactorySetThenApplied
Can you please add these tests? Please let me know if you need any clarification.
There was a problem hiding this comment.
Thank you very much, I've submitted the changes
There was a problem hiding this comment.
Same comments apply from OidcIdTokenDecoderFactoryTests
raphaelDL
force-pushed
the
gh-6379
branch
4 times, most recently
from
d5b48d0 to
9918402
Compare
There was a problem hiding this comment.
This is not testing that the custom JwtValidatorFactory is actually called. We need to verify that Function.apply() is actually called. For an example, see NimbusReactiveJwtDecoderTests.decodeWhenUsingSignedJwtThenReturnsClaimsGivenByClaimSetConverter. That test mock(Converter.class) and than verify(claimSetConverter).convert(any(Map.class)). You will need to do the same for Function. Make sense?
There was a problem hiding this comment.
Yes, I'm working on it
There was a problem hiding this comment.
How's it coming along. Do you need any help?
There was a problem hiding this comment.
I've submitted the changes, I don't know why they are not reflected here 🤔
There was a problem hiding this comment.
Our preferred method of testing a thrown exception is assertThatThrownBy(). Let's remove expected = IllegalArgumentException.class for all test cases and use assertThatThrownBy() instead.
There was a problem hiding this comment.
Please remove this test - createDecoderWhenCustomJwtValidatorFactorySetThenApplied is the main thing we need to test.
There was a problem hiding this comment.
Please remove this line as it will never get called
There was a problem hiding this comment.
Can we remove this and just create it in each test?
There was a problem hiding this comment.
Can we remove this and just create it in each test?
This commit ensures that the JwtDecoder is not a private field inside the Oidc authentication provider by extracting this class and giving the possibility to customize the way different providers are validated. Fixes: spring-projectsgh-6379
|
|
||
| when(customValidator.apply(any(ClientRegistration.class))) | ||
| .thenReturn(customJwtValidatorFactory.apply(registration.build())); | ||
|
|
There was a problem hiding this comment.
@jgrandja here are the changes, I think they are not reflected because the comment is in the method signature
jgrandja
added
New Feature
in: oauth2
|
Thanks @raphaelDL for getting this done before tomorrow's release. This is now in master! I added a polish commit for some minor updates to the tests and javadoc. |
3 participants
This commit ensures that the JwtDecoder is not a private field inside
the Oidc authentication provider by extracting this class and giving the
possibility to customize the way different providers are validated.
Fixes: gh-6379