Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consistent Injection combinations of Claims #222

Closed
radcortez opened this issue Apr 20, 2020 · 6 comments · Fixed by #226
Closed

Consistent Injection combinations of Claims #222

radcortez opened this issue Apr 20, 2020 · 6 comments · Fixed by #226
Assignees
Milestone

Comments

@radcortez
Copy link
Member

There are multiple ways to inject values from a JWT. From the latest MP JWT Spec, the following injection combinations must be supported (using Long as an example):

@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;

There is also valid for other types like Boolean, String and corresponding JsonValue types. In #218, we realized that SmallRye JWT is not covering all cases and should be more consistent in how injection is done for the multiple combinations. The current implementation is able to handle the following injection combinations:

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

The main reason for injection to fail is a ClassCastException because values in the claimSet are usually stored as a JsonValue (except if it is a known claim and the type is set in the Claims enum).

So we need to determine the right type at injection point and then provide a conversion if required so the types match and injection work.

@radcortez radcortez changed the title Consistent Injection combinations. Consistent Injection combinations of Claims Apr 20, 2020
@sberyozkin
Copy link
Contributor

@radcortez What is the problem with having the claims converted to JsonValue/Array/Object and then provide the conversion when needed ? This conversion from JsonValue/etc would have to be done for the custom claims like Address anyway, as opposed to the users guessing how it is represented internally before writing their address converter

@radcortez
Copy link
Member Author

No problem at all. Right now the missing piece is conversion from the type stored in the claimSet (usually JsonValue) to another supported type when injection uses a ClaimValue wrapper.

For instance, you can see conversions being done here:
https://github.com/smallrye/smallrye-jwt/blob/b8adfd2e5e91d95419e4c9f0c6b10ee71ea27d3a/implementation/src/main/java/io/smallrye/jwt/auth/cdi/RawClaimTypeProducer.java

And that is why injection of Long or long work.

But there is no equivalent conversion code here:
https://github.com/smallrye/smallrye-jwt/blob/4f33aba47c7d177976ae5573e933ed2128426eea/implementation/src/main/java/io/smallrye/jwt/auth/cdi/ClaimValueWrapper.java

So when you try to inject a ClaimValue<Long> what you actually get is a ClaimValue<JsonNumber> and then the ClassCastException.

What we need is to determine the right type of ClaimValue and then apply the corresponding conversion.

@sberyozkin
Copy link
Contributor

@radcortez can we keep the injection type available in the constructor and do the conversion from JsonValue/etc in ClaimValueWrapper.getValue ?

@radcortez
Copy link
Member Author

Here is the code that I've written to fix the issue previously:
https://github.com/radcortez/smallrye-jwt/blob/primitives-full-fix/implementation/src/main/java/io/smallrye/jwt/auth/cdi/ClaimValueWrapper.java

I believe I've done that. Just an additional method to unwrap the type in the constructor. And then JsonUtils.convert to pass the type to convert in getValue.

@sberyozkin
Copy link
Contributor

sberyozkin commented Apr 21, 2020

@radcortez Cool, told you it would be easier for me to grok the changes if we go the smaller PR way :-).
So it all looks fine, I believe we can discuss/optimize the way when/if the conversion of the claim set to JsonValue/etc is done in a separate issue. Your changes specific to this issue look fine, please follow up with a PR when you get a chance, thanks

radcortez added a commit to radcortez/smallrye-jwt that referenced this issue Apr 22, 2020
@radcortez radcortez self-assigned this Apr 22, 2020
@radcortez
Copy link
Member Author

Done in #226.

radcortez added a commit to radcortez/smallrye-jwt that referenced this issue Apr 23, 2020
radcortez added a commit to radcortez/smallrye-jwt that referenced this issue Apr 24, 2020
@sberyozkin sberyozkin added this to the smallrye-jwt-2.1.2 release milestone Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants