Extract OidcTokenValidator to an OAuth2TokenValidator #6298
Conversation
| return clientRegistration -> getJwtDecoder(); | ||
| } | ||
|
|
||
| private static JwtDecoder getJwtDecoder() { |
There was a problem hiding this comment.
Does it make sense to return a mock so that testing is easier?
There was a problem hiding this comment.
I could return a mock(JwtDecoder.class) but mocking the return value of decode() will produce the same amount of code so doesn't really make it easier. However, it might make more sense to do it this way so will apply the change.
| } | ||
|
|
||
| private static void throwInvalidIdTokenException() { |
There was a problem hiding this comment.
Why are we making this change in this set of commits? Also this appears to break passivity. Is there a reason for this?
There was a problem hiding this comment.
I'm not sure I understand? The contract for OAuth2TokenValidator is OAuth2TokenValidatorResult validate(Jwt idToken) so it should return a OAuth2TokenValidatorResult containing the Collection<OAuth2Error>.
The original implementation would throw an OAuth2AuthenticationException on validation failure but now it returns a OAuth2TokenValidatorResult and the JwtDecoder will throw a JwtException. However, both OidcAuthorizationCodeAuthenticationProvider and OidcAuthorizationCodeReactiveAuthenticationManager will catch the JwtException and throw OAuth2AuthenticationException to preserve passitivity. Am I missing something?
There was a problem hiding this comment.
Sorry I didn't notice that OidcTokenValidator was package scope. My concern was that users might be using the API directly which would break them. This is not a concern since it is package scope, so please ignore.
| @Test | ||
| public void validateIdTokenWhenIssuerNullThenHasErrors() { | ||
| this.claims.remove(IdTokenClaimNames.ISS); | ||
| assertThat(this.validateIdToken()).isNotEmpty(); |
There was a problem hiding this comment.
Previously the assertions were verifying the correct error was present. Please update these tests to assert that the correct error is in the results too.
NOTE: This comment applies to any assertThat(this.validateIdToken()).isNotEmpty();
There was a problem hiding this comment.
I updated to assert on OAuth2Error as per request.
|
Thanks @jgrandja! The code looks good to me now. Can you update labels, milestones, rebase, squash and merge these changes? |
|
@jgrandja The changes look good. Can you please go ahead and rebase, squash and merge these changes? |
jgrandja
added
type: enhancement
|
Merged via 9c0d78d |
Fixes #5930
Note: This is rebased off #5751