Extendable JWT authority mapping

[PMacho]

Expected Behavior There seems to be no standard on what JWT contains to communicate client authorities. At the moment Spring Security just maps everything within scope or scp to SCOPE_. I think, it would be nice to have the possibility to configure further mappings, to be used in conjunction with pathMatchers.

Current Behavior

As far as I could see, there is no way to extend the default in an easy way.

Context

I personally use the following:

public final class ExtendableReactiveJwtAuthenticationConverterAdapter implements Converter<Jwt, Mono<AbstractAuthenticationToken>> {

    private HashMap<String, String> authoritiesMap = new HashMap<>() {{
        put("scope", "SCOPE_");
        put("scp", "SCOPE_");
    }};

    @Override
    public Mono<AbstractAuthenticationToken> convert(Jwt jwt) {
        return Flux
                .fromIterable(authoritiesMap.entrySet())
                .filter(claimPrefixEntry -> jwt.containsClaim(claimPrefixEntry.getKey()))
                .flatMap(entry -> extractAuthorities(jwt, entry.getKey(), entry.getValue()))
                .collectList()
                .map(authorities -> new JwtAuthenticationToken(jwt, authorities));
    }

    public ExtendableReactiveJwtAuthenticationConverterAdapter addMapping(String claimName, String prefix) {
        authoritiesMap.put(claimName, prefix);
        return this;
    }

    public ExtendableReactiveJwtAuthenticationConverterAdapter setMappings(HashMap<String, String> authoritiesMap) {
        this.authoritiesMap = authoritiesMap;
        return this;
    }

    @SuppressWarnings("unchecked")
    private Flux<GrantedAuthority> extractAuthorities(Jwt jwt, String claimName, String prefix) {
        Object claim = jwt.getClaim(claimName);

        Flux<String> authorities;
        if (claim instanceof String)
            authorities = Flux.fromArray(((String) claim).split(" "));
        else if (claim instanceof Collection)
            authorities = Flux.fromIterable((Collection<String>) claim);
        else
            authorities = Flux.empty();

        return authorities.map(authority -> new SimpleGrantedAuthority(prefix + authority));
    }

}

This allows me to configure my OAUTH resource server like:

.jwt(jwtSpec -> jwtSpec.jwtAuthenticationConverter(
        new ExtendableReactiveJwtAuthenticationConverterAdapter().addMapping("groups", "GROUP_")
))

Like so, defaults are preserved and extensions are easy to configure. Plus, I think it is a bit more readable than JwtGrantedAuthoritiesConverter.

At the moment the default uses ReactiveJwtAuthenticationConverterAdapter with a hard coded dependency on JwtAuthenticationConverter which again hard coded depends on JwtGrantedAuthoritiesConverter. This chain is a little inflexible. I understand, it is due to reusing the same code for blocking and reactive implementation. However, it would (probably not just for me) be nice to have the same functionality as within ExtendableReactiveJwtAuthenticationConverterAdapter.

[jzheaux]

Thanks for the suggestion, @PMacho.

Actually, I think it's already quite simple for an application to create a composite converter:

Converter<Jwt, Flux<GrantedAuthority>> composite = jwt -> Flux
        .just(one, two)
        .map(converter -> converter.convert(jwt))
        .flatMap(Flux::fromIterable);

I think the common pain point this suggestion as well as others are trying to address is that the construction of a JwtGrantedAuthoritiesConverter could be simplified.

With your comments about mapping in mind, I wonder about adding the following to JwtGrantedAuthoritiesConverter:

public static JwtGrantedAuthoritiesConverter fromMapping(String claim, String prefix) {
    JwtGrantedAuthoritiesConverter converter = new JwtGrantedAuthoritiesConverter();
    converter.setAuthoritiesClaimName(claim);
    converter.setAuthorityPrefix(prefix);
    return converter;
}

Then, it's a bit simpler for an application to configure many converters like so:

JwtGrantedAuthoritiesConverter one = fromMapping("groups", "GROUP_");
JwtGrantedAuthoritiesConverter two = fromMapping("scope", "SCOPE_");
Converter<Jwt, Flux<GrantedAuthority>> composite = jwt -> Flux
        .just(one, two)
// ...

Am I catching the spirit of your suggestion or is there something I've missed?

[PMacho]

I'm sorry for the delay. I'll try to answer tomorrow, latest.

[PMacho]

Hi @jzheaux ,

thanks for your answer. I agree, it is not complicated to composite converters. However, from a user perspective, I think it is more comfortable and flexible (and in a way better), if I can just configure what I need. (B.t.w. JwtSpec only digests Converter<Jwt, ? extends Mono<? extends AbstractAuthenticationToken>>, thus the composite converter still needs further mapping.)

Since you were asking, my suggestion. I would follow the lines pathMatchers(...) works. I.e.:

.jwt() // could instantiate ExtendableReactiveJwtAuthenticationConverterAdapter
.addMapping("groups", "GROUP_") // calls addMapping("groups", "GROUP_") on extendableReactiveJwtAuthenticationConverterAdapter
.addMapping("grp", "GROUP_") // calls addMapping("groups", "GROUP_") on extendableReactiveJwtAuthenticationConverterAdapter
// maybe:
.and()

This (in my eyes) is the most consistent way, having in mind, how SecurityWebFilterChain is configured usually. Plus, it allows to configure WebFlux and MVC exactly the same way.

Note: The comments are just one way to implement this. I too like your fromMapping().

Note: No matter what you decide, I think JwtGrantedAuthoritiesConverter should be replaced or reworked (e.g. like I did). At the moment the default handles an array of objects, but the only way to configure it, is to replace the array by a single value. I don't actually know if this was on intention, but for me it looks a little awkward and it make the implementation hard to read.

For the record, a (untested) sync version of ExtendableReactiveJwtAuthenticationConverterAdapter:

public final class ExtendableJwtAuthenticationConverterAdapter implements Converter<Jwt, AbstractAuthenticationToken> {

    private HashMap<String, String> authoritiesMap = new HashMap<>() {{
        put("scope", "SCOPE_");
        put("scp", "SCOPE_");
    }};

    @Override
    public AbstractAuthenticationToken convert(Jwt jwt) {
        return new JwtAuthenticationToken(
                jwt,
                authoritiesMap
                        .entrySet()
                        .stream()
                        .filter(claimPrefixEntry -> jwt.containsClaim(claimPrefixEntry.getKey()))
                        .flatMap(entry -> extractAuthorities(jwt, entry.getKey(), entry.getValue()))
                        .collect(Collectors.toList()));
    }

    public ExtendableJwtAuthenticationConverterAdapter addMapping(String claimName, String prefix) {
        authoritiesMap.put(claimName, prefix);
        return this;
    }

    public ExtendableJwtAuthenticationConverterAdapter setMappings(HashMap<String, String> authoritiesMap) {
        this.authoritiesMap = authoritiesMap;
        return this;
    }

    private Stream<GrantedAuthority> extractAuthorities(Jwt jwt, String claimName, String prefix) {
        Object claim = jwt.getClaim(claimName);

        Stream<String> authorities;
        if (claim instanceof String)
            authorities = Arrays.stream(((String) claim).split(" "));
        else if (claim instanceof Collection)
            authorities = ((Collection<String>) claim).stream();
        else
            authorities = Stream.empty();

        return authorities.map(authority -> new SimpleGrantedAuthority(prefix + authority));
    }

}

... the doubled logic can/should of course be merged.

[jzheaux]

Lots of great feedback here, @PMacho, thank you.

While I appreciate the suggestion to enhance the DSL in the pattern of pathMatchers, the oauth2ResourceServer DSL was specifically designed to be component-based, meaning that applications construct components and either wire them to the DSL or expose them as @Beans.

That doesn't mean that something like that won't change down the road, but for now I'd prefer to leave oauth2ResourceServer as just taking components.

What that means is coming up with a simpler way to construct a Converter<Jwt, Mono<AbstractAuthenticationToken>>.

Your suggested component class combines both authentication conversion and granted authority conversion, but I think that such makes the class less composable than it could be. Consider the circumstance where I've got some custom granted authority conversion that I need to do with one claim, but I want to use your claim-authority mapping for other claims. How would I configure that?

Because JwtGrantedAuthoritiesConverter converts only one claim to one collection of granted authorities, configuring that scenario is equally simple:

Converter<Jwt, Flux<GrantedAuthority>> compositeAuthoritiesConverter() {
    JwtGrantedAuthoritiesConverter scopes = fromMapping("scope", "SCOPE_");
    JwtGrantedAuthoritiesConverter groups = fromMapping("group", "GROUP_");
    Converter<Jwt, Collection<GrantedAuthority>> custom = jwt -> { ... custom code ... }
    return jwt -> Flux.just(scopes, groups, custom)
        .map(converter -> converter.convert(jwt))
        .flatMap(Flux::fromIterable);
}

If you change your component to be a Converter<Jwt, Flux<GrantedAuthority>> to address this, you get something that takes about the same amount of code:

Converter<Jwt, Flux<GrantedAuthority>> compositeAuthoritiesConverter() {
    ExtendableJwtGrantedAuthoritiesConverterAdapter authoritiesConverter = new ...
    authoritiesConverter.addMapping("scope", "SCOPE_");
    authoritiesConverter.addMapping("group", "GROUP_");
    Converter<Jwt, Collection<GrantedAuthority>> custom = jwt -> { ... custom code ... }
    return jwt -> Flux.fromIterable(custom.convert(jwt))
        .concatWith(authoritiesConverter.convert(jwt));
}

Which is why I'd advocate adding the functionality to the simpler component -- more use cases will reap the benefits.

I'm still open to discussing this more if you feel like I haven't addressed one of your points. Do you see adding JwtGrantedAuthoritiesConverter#fromMapping as being helpful for your use case, and would you be interested in submitting a PR to add it?

[PMacho]

Hi @jzheaux ,

I totally agree about the splitting of converting Jwt to GrantedAuthority and from GrantedAuthority to AbstractAuthenticationToken. I should have made this more clear. My implementation was just a draft to show the idea, merging both mappings for simplicity.

What that means is coming up with a simpler way to construct a Converter<Jwt, Mono\<AbstractAuthenticationToken>>.

Yes. So would you agree to the following? Assume, I register beans of type, Converter<Jwt, GrantedAuthority>, Converter<Jwt, Collection<GrantedAuthority>> and Converter<Jwt, Stream<GrantedAuthority>> for MVC, and Converter<Jwt, Publisher<GrantedAuthority>> for WebFlux application. In ServerHttpSecurity, the Converter<Jwt, ...> beans can be accessed from the context or directly added in the DSL and combined in say JwtAuthenticationConverterAdapter for MVC and ReactiveJwtAuthenticationConverterAdapter for WebFlux applications. Plus, there should be a simple constructor like the fromMapping for the classes above.

To allow for defaults, the (Reactive)JwtAuthenticationConverterAdapter could directly construct the converter for scp and scope. Still there should be a way to decide if I want to extend or overwrite the defaults.

Well, I think, this is all possible, not sure however, how elegant the solution will be without extending the DSL (or more specific JwtSpec). My time is quite limited at present, so I can't guarantee on how fast I can do all that. However, if you agree with the ideas above, I could start with a draft implementation and maybe discuss it with you a bit more personal, before the final polish.

Best

[Haarolean]

I've encountered multiple issues with JwtGrantedAuthoritiesConverter as well. The things I needed:

• To access internal WELL_KNOWN_AUTHORITIES_CLAIM_NAMES aiming to extend that list
• To access convert method and do some operations before the conversion itself (like, discarding some authorities)

The best I could get away with is creating a delegate wrapper and doing my things after the conversion, which is not desirable.

@jzheaux Hi, can we reiterate this? I could, perhaps, raise a PR with some limited scope of work, which will allow us some basic extension of the aforementioned class.