Support Boolean and boolean as a valid injection point.

[radcortez]

This is some work to support microprofile/microprofile-jwt-auth#127.

An interesting this is that the spec includes support for Boolean, but there is not TCK test for it, so it was never implemented in SmallRye JWT.

While doing the work, I've also found another issue. Our ClaimValue support does not unwrap JsonValue to wrapper primitive types. So if you try to inject a custom ClaimValue<Long> you will get a ClassCastException, since for some reason we are wrapping the original claimSet in DefaultJWTCallerPrincipal#fixJoseTypes

This doesn't happen with ClaimValue<Long> of aud for instance, because we are not remapping that claim name so it works fine.

I think we need to change this and don't modify the original claims set. This will fix the injection for wrapper primitive types. If we need a JsonValue type of sub class of it, we need to verify the injection point and do the conversion on the fly. Actually, this could be handled by the conversion work being done in #212 .

[radcortez]

I've added a test (and it is failing on purpose) so we can see the issue.

[radcortez]

@sberyozkin any thoughts about this?

[radcortez]

@sberyozkin I'm proposing some changes to make the injection more consistent. I've tried to be conservative on the changes.

One of the main issues is our claimSet map, because we don't keep a consistent type for everything that is inside. Some objects are using the Json types and some are using the standard Java types. This is annoying because you need to test what type you have in there and provide the appropriate conversion if required. For instance, you may want to inject a JsonString but you have a String in the claimSet. I'm wondering if we should clarify this in the spec and say that everything on the claimSet needs to be a json subtype for unknown objects. At least it would keep some consistency across implementations and you would also know the main type in the claimSet.

What this PR does, is to retrieve the actual type that needs to be injected and I have implemented a few rules to provide manual conversions. I think we can improve this with some of the Conversion code I'm working on in the other project, but I wanted to keep things simple for now and not be too disruptive.

Also, the way we produce the Claims is somehow limited, with fixed producers for each type. For instance, we are unable to produce a proper JsonValue of TRUE or FALSE type, because we get an ambiguous dependency resolution in JsonValueProducer, since all the producers can produce a JsonValue but if you provide just a JsonValue producer then you need to figure out the subtype.

My recommendation would be to refactor all the injection code and do it like SmallRye Config. We can observe all of the @Claim injection points and provide a custom Bean with the logic to figure out the source and target types, like I have done in the prototype for Converters here: #212

Finally, the TCK is not covering everything. It covers most cases, but to make things fun, you can inject the same value in 8 or 9 different way. Look at Long for instance:

        @Inject
        @Claim("long")
        private long longClaim;
        @Inject
        @Claim("long")
        private Long longClaimWrapper;
        @Inject
        @Claim("long")
        private ClaimValue<Long> longClaimValue;
        @Inject
        @Claim("long")
        private Optional<Long> longOptional;
        @Inject
        @Claim("long")
        private ClaimValue<Optional<Long>> longClaimValueOptional;
        @Inject
        @Claim("long")
        private JsonNumber longJson;
        @Inject
        @Claim("long")
        private Optional<JsonNumber> longOptionalJson;
        @Inject
        @Claim("long")
        private ClaimValue<JsonNumber> longClaimValueJson;
        @Inject
        @Claim("long")
        private ClaimValue<Optional<JsonNumber>> longClaimValueOptionalJson;

I guess we overcomplicated this a little bit :)

[sberyozkin]

@radcortez Sorry for a delay, IMHO we need give the implementations a chance to keep it internally in whatever way they prefer. I believe our own set should have all custom types kept converted to JSON objects/etc as it is what is expected by default (except when it is a String, long, etc)

There is nothing wrong IMHO with the code above, but I'd say it is the users who want to inject long into ClaimValue<Optional<JsonNumber>> are overcomplicating it :-)

[sberyozkin]

@radcortez I propose that you update this PR around the PR subject only (boolean/Boolean support) - anything else can indeed be discussed in a different issue, thanks

[radcortez]

Not sure if I agree. Our CDI injection code has some flaws and I believe it would be easier to tackle this as a whole than just fixing the issues for boolean. Taking the long example, with the current master code, here is the result:

Injection Type	Result
long	OK
Long	OK
ClaimValue<Long>	Fails
Optional<Long>	Fails
ClaimValue<Optional<Long>>	Fails
JsonNumber	OK
Optional<JsonNumber>	OK
ClaimValue<JsonNumber>	OK
ClaimValue<Optional<JsonNumber>>	OK

I think we will spent considerable more effort if we handle this separately.

[sberyozkin]

@radcortez

I think we will spent considerable more effort if we handle this separately.

I'm not very concerned that some extra time will be spent to fix all properly. IMHO splitting one PR into a few ones would not involve a considerableamount of effort.

It is easier to me to review, make sure that I don't miss something, given the sensitivity around all these conversions. I'm not worried right now that for example ClaimValue<Optional<Long>> fails - may be it should keep failing to prevent the users trying to write such a code :-).

This PR touches on too many things, Boolean/boolean, String related conversion, Optional etc. Some related related to CDI, some related to the conversion rules. But it is easier to narrow down when for example Quarkus or Wildfly TCK fails after such a PR merge what may have caused it.

For example, you have mentioned ClaimValue<Long> does not work for the custom claims. But it is not a bug, the original CDI/injection code was I believe written around the assumption that custom claims can only be accessed as JSON values.

I agree though that Boolean/boolean and Long/long are the same with the respect how they should be handled. But I think the internal JSONObject/etc conversion for the custom claims which is done in place should stay because the default jwt.getClaim("customName") returns JSONObject/etc. This acts well as a common base irrespectively of whether the injection or the manual retrieval is done.

Lets focus on the Long/long and Boolean'boolean first.
Thanks

[radcortez]

I'm not worried right now that for example ClaimValue<Optional<Long>> fails - may be it should keep failing to prevent the users trying to write such a code :-).

The issue is in the spec, since we allow in the spec for multiple combinations of injection types, and the interesting thing is that the TCK don't ever cover them all. So, we do pass the TCK, but the TCK is not testing all combinations specified in the spec, which is another problem.

For example, you have mentioned ClaimValue<Long> does not work for the custom claims. But it is not a bug, the original CDI/injection code was I believe written around the assumption that custom claims can only be accessed as JSON values.

I think it is. If we are able to use ClaimValue<Long> as an injection point to a standard claim like iat, how do we explain to a user that he is not able to use ClaimValue<Long> to inject a custom claim and he needs to use ClaimValue<JsonNumber> instead? It is a poor user experience.

Anyway, I've reverted most changes and added only consistent boolean support with all our other types.

I'm opening another issue regarding the inconsistencies that we have.