Spring Security Improve handling of non-String principal claim values

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

Comment From: 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?

Comment From: omlip

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

Comment From: jgrandja

Thanks @omlip !

Comment From: 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 ?

Comment From: jgrandja

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

Comment From: omlip

Ok will update the PR accordingly.

Comment From: 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

Comment From: jgrandja

Sounds good @omlip