Improve handling of non-String principal claim values

[omlip]

Context

When configuring an Spring application as an OAuth Resource Server and we use Jwt, we can set manually the principalClaimName on JwtAuthenticationConverter object.

Example like this

fun getCustomAuthenticationConverter(authoritiesClaimName : String): Converter<Jwt, Mono<AbstractAuthenticationToken>> {
    return ReactiveJwtAuthenticationConverterAdapter(
            JwtAuthenticationConverter().apply {
                setPrincipalClaimName("anInt")
                setJwtGrantedAuthoritiesConverter(GrantedAuthoritiesExtractor(authoritiesClaimName))
            }
    )
}

Current Behavior

If we use a specific principalClaimName on a JwtAuthenticationConverter and if the value behind the claim name is not a String, a ClassCastException is thrown in the convert method.

	@Override
	public final AbstractAuthenticationToken convert(Jwt jwt) {
		Collection<GrantedAuthority> authorities = extractAuthorities(jwt);
		if (this.principalClaimName == null) {
			return new JwtAuthenticationToken(jwt, authorities);
		}
		String name = jwt.getClaim(this.principalClaimName); // this line throw a ClassCastException
		return new JwtAuthenticationToken(jwt, authorities, name);
	}

We can easily reproduce the situation by adding a test in class JwtAuthenticationConverterTests.

	@Test
	public void convertWhenPrincipalClaimNameSetWithOtherValueThanString() {
		this.jwtAuthenticationConverter.setPrincipalClaimName("user_id");
		Jwt jwt = TestJwts.jwt().claim("user_id", 100).build();
                //TODO: complete with assertions
	}

Expected Behavior

I think we should adapt the code to not lead to a ClassCastException but maybe throw a proper exception which "explain" what the problem is.

Maybe by adapting like this ?

Object claimValue = jwt.getClaim(this.principalClaimName);
Assert.isInstanceOf(String.class, claimValue, "The principal value is not a String");

String name = (String)claimValue;

or maybe with the usage of Jwt.claimAsString(...) method ?

What do you think ?
Let me know if I can help.

PS : by the way, regarding this RFC https://tools.ietf.org/html/rfc7519#section-4.1.2 about JWT token, nothing say that a claim value must be a String as it is implemented in JwtAuthenticationToken. Maybe this discussion is out of scope but indeed

Best regards,
Olivier

[jgrandja]

@omlip I agree that we should avoid a ClassCastException from being thrown.

I think adding Assert.isInstanceOf() (as suggested) is a reasonable improvement. Would you be interested in submitting a PR for this?

[omlip]

@jgrandja Sure I will, you can assign it to me

[jgrandja]

Thanks @omlip !

[omlip]

@jgrandja PR submitted

Sould not we, by the way, adjust the documentation to mention that it is possible to set manually a principalClaimName and that must be a String ?

[jgrandja]

Thanks @omlip. Agreed, please update the javadoc for setPrincipalClaimName().

[omlip]

Ok will update the PR accordingly.

[omlip]

Hi @jgrandja I was talking about Spring Security Reference documentation.
In particular, the following sentence:

The resulting Authentication#getPrincipal, by default, is a Spring Security Jwt object, and Authentication#getName maps to the JWT’s sub property, if one is present.

to be transformed in something like this :

The resulting Authentication#getPrincipal, by default, is a Spring Security Jwt object, and Authentication#getName maps to the JWT’s sub property, if one is present. If none is present, you can still set the principal claim name to any JWT's property by using setPrincipalClaimName method. The value behind the claim must be a String

but also I can update javadoc of course :-)

What do you think ?

br
Olivier

[jgrandja]

Sounds good @omlip