smallrye-jwt-extension: Make role mapping configurable and use newer smallrye-jwt apis

[andreas-eberle]

In this PR, the following has been changed:

• Make JwtParser injectable. Currently, the JwtParser cannot be configured / intercepted. We have a use case where we need to verify the token depending on data in the token (e.g. if you would want to verify tokens from Google, Facebook, Github,... in your application without putting a Keycloak in between, you would need to read the issuer field). Unfortunately, the smallrye DefaultJwtTokenParser does not implement an interface. So I introduce an interface JwtParser and provide a default bean for it directly forwarding the calls to the smallrye DefaultJwtTokenParser.

• Make roles mapping injectable. Like the Eclipse MicroProfile RBAC documentation for groups states, it is very likely for an application to require a mapping of the groups used in a JWT to application specific roles. With the injectable JwtRolesMapper, this is now possible. Again, the default bean implementation preserves the former behavior.

I'm looking forward to your input on this. I haven't yet documented the two new interfaces in the guides yet but if your general feedback is good, I'll be happy to do so.

Edit: Add example

Usage of the JwtParser in your application could look like this

@ApplicationScoped
public class MyJwtParser implements JwtParser {

    private final JwtTokenUtils jwtTokenUtils;
    private final DefaultJWTTokenParser parser = new DefaultJWTTokenParser();

    @Inject
    public MyJwtParser(JwtTokenUtils jwtTokenUtils) {
        this.jwtTokenUtils = jwtTokenUtils;
    }

    @Override
    public JwtContext parse(String tokenString, JWTAuthContextInfo authContextInfo) throws ParseException {
        try {
            // get the token like this
            JwtClaims token = jwtTokenUtils.decodeTokenUnverified(tokenString);

            // do some custom magic, e.g. download a certificate depending on token issuer and cache it
            String publicKey = "my-special-public-key";

            // then augment the authContextInfo with it:
            JWTAuthContextInfo newAuthContextInfo = new JWTAuthContextInfo(authContextInfo);
            newAuthContextInfo.setPublicKeyContent(publicKey);

            return parser.parse(tokenString, newAuthContextInfo);

        } catch (InvalidJwtException e) {
            throw new NotAuthorizedException("Invalid token");
        }
    }
}

Use of JwtRolesMapper could look like this

@ApplicationScoped
public class MyJwtRolesMapper implements JwtRolesMapper {
    @Override
    public HashSet<String> mapGroupsAndRoles(JwtClaims claims) throws MalformedClaimException {
        HashSet<String> roles = new HashSet<>(claims.getStringListClaimValue("groups"));

        if(claims.getClaimValue("admin", Boolean.class)){
            roles.add("ADMIN");
        }

        return roles;
    }
}

Edit 2: Remove now obsoleted infos in the text.

[sberyozkin]

@andreas-eberle Thanks for this PR. I'm not sure right now about it.
A parser interface is now available in smallrye/smallrye-jwt. IMHO a good number of these changes should be done there, with CC to @darranl to avoid any side-effects in WildFly.

quarkus-oidc also manages the multi-tenancy (referring to your Google/GitHub/etc scenario) and has the tokens injected as JsonWebToken.

The other thing I'd like to propose is to split the PR into few dedicated ones, for example, may be you could start with a PR containing only those changes which clean up the smallrye-jwt code. (Note I don't think we recommend wildcard imports and I recall @stuartwdouglas advising against using the lambdas.etc during the initialization time).
Then continue with dedicated PRs addressing one use case at a time, it will be easier for me in particular to review
Thanks

[sberyozkin]

There are some interesting ideas in this PR but IMHO it needs to be split with some of those changes likely needing to go to smallrye/smallrye-jwt

[andreas-eberle]

@sberyozkin: I just added a new PR #7489 with the fixes for the warnings in the tests. I will have a look at the smallrye interfaces.

[andreas-eberle]

I removed the test changes from this PR to give a clearer picture of the actual changes.

[sberyozkin]

@andreas-eberle Thanks. Please also review these docs: https://github.com/quarkusio/quarkus/blob/master/docs/src/main/asciidoc/security-openid-connect-multitenancy.adoc

[sberyozkin]

@andreas-eberle May be also makes sense to make it a Draft given that it will be split going forward anyway; Hi @gsmet, @geoand I'm not not sure how to stop the PR build :-)

[geoand]

It should be possible to cancel builds from the GitHub Actions UI. You may need to cancel each job separately however...

[gsmet]

It's no big deal, let it run.

[andreas-eberle]

I opend a Zulip chat to better discuss what to do for this PR.

[andreas-eberle]

I update the code to the newer smallrye-jwt apis. Unfortunately, I currently have to use the SNAPSHOT version of it since the changes of smallrye/smallrye-jwt#192 are required to be able to implement the tests.

[sberyozkin]

@andreas-eberle Right, first it needs to be released.
However I'm still not following why we are doing these changes. What will become better ? Again lets review case by case

[sberyozkin]

What the existing smallrye.jwt properties such as groups path for example can't do that this interface will help with ?

[andreas-eberle]

At the moment I see three use cases:

1. the current implementation cannot handle JWTs like I showed in the example at the top. Sometimes tokens do not contain a groups field but contain role information in the form of other fields. In that example in the form of a boolean "admin" claim. With the JwtRolesMapper, I can write custom logic for my custom behavior to map the roles and then using the standard @RolesAllowed annotation.
2. It might well be that I do not want to use the group names as they are used in the JWT, which I might have to work with but have no control over. Sometimes the group names in the JWT are just not what they mean in my application. In that case, I can use the JwtRolesMapper to map the groups to roles that make sense in my context and thus make the code more readable.
3. I have a case where I have to handle tokens from different issuers, all using different groups. With the JwtRolesMapper, I can map their groups depending on the issuer of the token and unify it to something usable in my application. Again, I don't have power over those issuers, which is why I have to make it work in my application.

[sberyozkin]

Same question, why is this interface needed ?

[sberyozkin]

@andreas-eberle If there is a real case where the roles info is spread across multiple claims and when the existing smallrye properties can't really help with the auto extraction of these roles, then lets consider this interface (that should live then in smallrye-jwt I guess) but having it just in case would be redundant. If you have some specific requirement/mapping case in mind then lets review, thanks

[andreas-eberle]

The interface is there to allow for easy injection of own implementations of this roles mapping. Please also see my comment to your previous remark. I'm happy to move this interface to smallrye-swt. Should I prepare a PR?

[sberyozkin]

Why are these changes necessary ? I'd like to keep an option open to have the factory used instead of the parser so that people can plugin custom factories.

[andreas-eberle]

I don't see why this should not be possible any more. People can now easily inject the JwtParser implementation they like. And with the changes I proposed in smallrye/smallrye-jwt#194, it would also easily be possible to inject a different JWTCallerPrincipalFactory into the DefaultJwtParser.

[sberyozkin]

@andreas-eberle I agree about a roles mapper as you have a valid case, thanks for the clarification. However there is no need IMHO for a default mapper returning groups as it is already taken care of in smallrye-jwt OOB. The other thing is that there should be no ambiguity when smallrye-jwt prepares the roles via its groups.path property and then the mapper overwrites them or vice versa. They should be merged.

I know the users have asked about the custom roles mapping before, in fact may be in due time that can be supplied via MP JWT. So my proposal is to introduce this handy mapper interface in smallrye-jwt, without any default implementations and I suppose have it injected into the default principal implementation, and then make it also operational in Quarkus once smallrye-jwt-2.1.0 is out. We can even start from MP-JWT but it might make sense to experiment with it smallrye-jwt first and then submit, as was done for the builder API.

Please, lets just deal with this factory <-> parser relationship later (for the record, I don't agree :-), but we can continue the conversation in the open smallrye-jwt issue).

IMHO it makes sense to close this PR and return with a mapper related one first. It will be easier to deal with the factory/parser discussions as part of the dedicated issue/PR/etc. Do you agree ?

[andreas-eberle]

@sberyozkin: The idea of the default mapper is to have an injectable bean handling the normal code execution. This can then be exchanged by the user by providing his own implementation of the RolesMapper interface. Unfortunately, there is (afaik) no way to have optional dependencies and (afaik) this is the way such situations are handled.

In this implementation, the roles provided by smallrye-jwt are used in the exact same way as they have been used before. There is no overriding. In the default implementation, the code works like before. And if the user wants to map the roles with custom code (i.e. by implementing the RolesMapper interface), he is free to do with the groups what he wants. If he wants to keep the mapped roles, he can do this by adding them to the resulting list. However, if he doesn't, which might well be the case, he can decide to only include his mapped roles.

Sure, we can move the RolesMapper to smallrye-jwt. However, we will still need a default implementation, due to the missing optional dependency injection mentioned above. Also, I don't see why the default implementation is a problem. Can you elaborate what's troubling you about it?

Can you elaborate on the smallrye-jwt issue why you don't agree on the factory/injection issue, so I can try to understand your viewpoint better?

I'm not sure yet if this PR should be closed. I will have to see what implementations we will come up with / can agree on. For now, I would keep it open.

[andreas-eberle]

@sberyozkin: I was thinking about the RolesMapper interface again. I don't think it should be moved to smallrye-jwt. My thoughts are:

• The smallrye-jwt library is concerned with parsing and validating a JWT token. This is currently done and the resulting JsonWebToken object can be retrieved from the library. This token then is a representation of the actual token (string).

• Mapping the groups of the jwt to application specific roles used in the @RolesAllowed annotation is however not one of the concerns of the jwt parsing smallrye-jwt library. It is a concern of the connection of smallrye-jwt with quarkus.

• In my proposed changes, this also becomes clear by the fact that the mapped roles are only set to the SecurityIdentity and not the JsonWebToken.

• Users might want to inject the original JsonWebToken object as the representation of the actually provided jwt into their code. With the original groups and not the mapped roles. If we add the roles mapping to smallrye-jwt, the JsonWebToken might not contain the original groups any more (depending on the users implementation of the roles mapping). Even if we add the original groups on top of the mapped groups, the JsonWebToken object will no longer be a representation of the actual jwt string.

Therefore, in the interest of separation of concerns as well as the flexibility to get the original JsonWebToken injected while also mapping the groups of the jwt to application specific roles, I think the RolesMapper interface is correctly located in the quarkus extension.

What do you think?

[sberyozkin]

@andreas-eberle, sorry for a delay,

The smallrye-jwt library is concerned with parsing and validating a JWT token.

smallrye-jwt is bigger than a thin wrapper around Jose4J :-). it already offers many properties for adapting the tokens to the MP-JWT/Principal expectations. Now it also ships a builder API which we've in the process of contributing to MP JWT.

Mapping the groups of the jwt to application specific roles used in the @RolesAllowed annotation is however not one of the concerns of the jwt parsing smallrye-jwt library. It is a concern of the connection of smallrye-jwt with quarkus.

I don't agree with this assertion. Ideally all smallrye-jwt integrators could avoid coming up with the container specific mechanics on how to do it... Of course, when really needed we can do things in Quarkus, etc. But this case does not seem to be such a case.

In my proposed changes, this also becomes clear by the fact that the mapped roles are only set to the SecurityIdentity and not the JsonWebToken

Are you saying that you'd like to target the case where for example a groups claim has role1 while the application service code is only aware of role2 ? Your code in the description is dealing with the case where the missing groups claim is dealt with by checking a custom boolean admin claim.
The existing smallrye-jwt properties can not handle these cases, but at the very least the interface should be in smallrye-jwt

Users might want to inject the original JsonWebToken object as the representation of the actually provided jwt into their code...

But note JsonWebToken is a Principal which is used by SecurityIdentity. The token representation transformations are not done secretly, they are done on the user's demand to ensure the 3rd party tokens can be accessed alongside MP JWT compliant tokens in a uniform way. JsonWebToken.getRawToken is there if the original representation is needed to propagate, log, etc.

I'm fine with not wiring this interface at the smallrye-jwt level, but IMHO it should be shipped there to improve the portability expectations around the use of smallrye-jwt. Once it is there we can then wire it in in Quarkus

Thanks

[gsmet]

@sberyozkin I let you drive that puppy home.

[gsmet]

Please don't merge as is, there's a SNAPSHOT version.

[sberyozkin]

@gsmet I'm keeping the PR open to have a discussion at this stage as IMHO it is not possible to get it in directly right now, the mapper interface should be in smallrye-jwt

[andreas-eberle]

@sberyozkin: I just had a look at this again. How would you like to integrate the RolesMapper into smallrye-jwt? Currently the quarkus extension uses the JsonWebToken.getGroups() method to get the groups claim and sets it as the roles for the QuarkusSecurityIdentity (see code). Should we add a new getter getRoles() to the JsonWebToken class which then contains the mapped roles (by default it would use the getGroups() method)? This way the quarkus extension could also use the getRoles() method to set the roles instead of the getGroups().

What do you think?

[sberyozkin]

Hi @andreas-eberle
I've opened smallrye/smallrye-jwt#285, thanks for your idea, it will be better to have the mapper in smallrye-jwt and improve along the way how the default parser sets the roles (ex, we should likely stop overriding groups if the custom roles are set, etc)