Some ideas for JWTPrincipal interface

[sst...@redhat.com]

Let's discuss the ideas for changes suggested by @hendrikebbers as presented in https://github.com/eclipse/microprofile-jwt-auth/pull/16

• removed public keyword since it is not needed in interface

Perhaps this is an old school issue of mine one would actually have to copy and paste from one place to another that is obsoleted by today's modern IDEs and their implementation method behaviors, but that is why I tend to explicitly include the public qualifier. I don't care one way or the other on this.

• added default implementation for all the getter methods that returns a specific claim

I think this is ok. If an implementation has a direct path to providing the value it can override the default method, otherwise is does simplify the burden on implementations.

• changed the return value of the method to get claim by name to a generic return value. By doing so you do not need to cast all the time.

Ok, but the generic type cannot be an extension of Serializable as that is not true for many underlying JWT libraries. The jose4j library uses a org.jose4j.jwt.NumericDate for any spec defined NumericDate types, and this is not Serializable.

• changed primitive long return values for claims to Long. All the claims are not mandatory (as far as I know) and based on this you can end in a NPE based on auto boxing here if you use primitive long

All of the explicitly stated claim methods are required for a valid MP-JWT token, so there is a possible efficiently by leaving them as native types if the underlying implementation also has this representation. 

• Changed Set return value to Stream. All the methods should return an unmodifiable collection and here Set is not the best interface since it provides methods to add entries. Sadly Java still has no interfaces for unmodifiable collections :( . But based on this maybe Stream is a good return value.

I guess I don't have a problem with this. What are other's opinions about Stream vs Set?

• Added boolean containsClaim(String claimName) convenience method. This is highly recommend if Stream is used instead of Set as return value for collections.

Accepted.

• Added a method that returns Optional for a claim.

Fine, but here also T cannot extend Serializable.

* Additional questions:

• Does it makes sense if the JWTPrincipal interface extend Serializable ?

Possibly, but there could be some issues with serializing the validated form of the validated token object as represented by the underlying JWT library. For example, say you just wrote out the raw bearer token string and then reconstructed in on deserialization. Your not going to have the information that you need to validated, and let's say you wrote that out, at some point it will be expired and deserialization will fail unless you override the expiration check. For the 1.0 API I would say no, and if usage dictates this need we can revisit.

• KeyCloak provide some more specific claim types: JsonWebToken.java Does it makes sense to add them here?

No as we just want a minimal requirement set for interoperability. It could be useful to provide a JsonWebToken utility class that exposes all of the spec defined claims via typed and names accessors given an JWTPrincipal, but I don't see a benefit to having them on the JWTPrincipal directly as most are optional and have no guarantee of a value.

Thanks for the review.

[Alasdair Nottingham]

Some thoughts below. 

On Aug 9, 2017, at 4:51 PM, sst...@redhat.com wrote:

Let's discuss the ideas for changes suggested by @hendrikebbers as presented in https://github.com/eclipse/microprofile-jwt-auth/pull/16

• removed public keyword since it is not needed in interface

Perhaps this is an old school issue of mine one would actually have to copy and paste from one place to another that is obsoleted by today's modern IDEs and their implementation method behaviors, but that is why I tend to explicitly include the public qualifier. I don't care one way or the other on this.

I’m also old school, but I’ve learned to not care.

• added default implementation for all the getter methods that returns a specific claim

I think this is ok. If an implementation has a direct path to providing the value it can override the default method, otherwise is does simplify the burden on implementations.

Agree

• changed the return value of the method to get claim by name to a generic return value. By doing so you do not need to cast all the time.

Ok, but the generic type cannot be an extension of Serializable as that is not true for many underlying JWT libraries. The jose4j library uses a org.jose4j.jwt.NumericDate for any spec defined NumericDate types, and this is not Serializable.

I agree that T extends Serializable is not right. That said I’m now worried about portability/migration of return types. If one impl chooses to return jose4j types, and another returns a Java object then the code isn’t portable. I’m also concerned that a future JWT could decide to define a claim to be a type where it had never said anything before and this would make it harder to keep current. 

• changed primitive long return values for claims to Long. All the claims are not mandatory (as far as I know) and based on this you can end in a NPE based on auto boxing here if you use primitive long

All of the explicitly stated claim methods are required for a valid MP-JWT token, so there is a possible efficiently by leaving them as native types if the underlying implementation also has this representation. 

I think leave as long, if we are concerned about it being null (even though it is required) for the default impl then we should check for null and return a default long value. 

• Changed Set return value to Stream. All the methods should return an unmodifiable collection and here Set is not the best interface since it provides methods to add entries. Sadly Java still has no interfaces for unmodifiable collections :( . But based on this maybe Stream is a good return value.

I guess I don't have a problem with this. What are other's opinions about Stream vs Set?

Any Collection allows that, but the methods are defined to be optional so I don’t care about this. One thing about Set (or a Collection) is I can do jwt.getAudience().contains(“audience”) which seems simpler than jwt.getAudience().anyMatch(aud -> “audience”.equals(aud));

• Added boolean containsClaim(String claimName) convenience method. This is highly recommend if Stream is used instead of Set as return value for collections.

Accepted.

Not sure how much I care but wondering why this is needed if you can use contains on set or anyMatch on a stream.

• Added a method that returns Optional for a claim.

Fine, but here also T cannot extend Serializable.

* Additional questions:

• Does it makes sense if the JWTPrincipal interface extend Serializable ?

Possibly, but there could be some issues with serializing the validated form of the validated token object as represented by the underlying JWT library. For example, say you just wrote out the raw bearer token string and then reconstructed in on deserialization. Your not going to have the information that you need to validated, and let's say you wrote that out, at some point it will be expired and deserialization will fail unless you override the expiration check. For the 1.0 API I would say no, and if usage dictates this need we can revisit.

I dislike Serializable, but I especially dislike it for security sensitive stuff because it makes it really easy to do dumb stuff with it. 

• KeyCloak provide some more specific claim types: JsonWebToken.java Does it makes sense to add them here?

No as we just want a minimal requirement set for interoperability. It could be useful to provide a JsonWebToken utility class that exposes all of the spec defined claims via typed and names accessors given an JWTPrincipal, but I don't see a benefit to having them on the JWTPrincipal directly as most are optional and have no guarantee of a value.

Thanks for the review.

--
You received this message because you are subscribed to the Google Groups "Eclipse MicroProfile" group.
To unsubscribe from this group and stop receiving emails from it, send an email to microprofile...@googlegroups.com.
To post to this group, send email to microp...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/microprofile/9ddd57cc-4a83-45fd-b9cf-18dba65b007f%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[sst...@redhat.com]

On Wednesday, August 9, 2017 at 4:53:43 PM UTC-7, Alasdair Nottingham wrote:

Some thoughts below. 

I agree that T extends Serializable is not right. That said I’m now worried about portability/migration of return types. If one impl chooses to return jose4j types, and another returns a Java object then the code isn’t portable. I’m also concerned that a future JWT could decide to define a claim to be a type where it had never said anything before and this would make it harder to keep current. 

The only way to address this is to define the required types for all standard claims. For non-standard claims there is nothing we can do about this, but such claims are non-portable anyway.

There are 47 claims registered with IANA under JWT:

https://www.iana.org/assignments/jwt/jwt.xhtml

[Alasdair Nottingham]

Can all the claims be mapped to a String, Number or collection of them? If so that would simplify things. If there are complex claim values though that wouldn't work. 

Alasdair Nottingham

--
You received this message because you are subscribed to the Google Groups "Eclipse MicroProfile" group.
To unsubscribe from this group and stop receiving emails from it, send an email to microprofile...@googlegroups.com.
To post to this group, send email to microp...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/microprofile/89b94fd1-0bc4-4c97-96a0-f80501a884b7%40googlegroups.com.

[David Blevins]

Adding Hendrik in case he’s not subscribed. :)

On Aug 9, 2017, at 4:53 PM, Alasdair Nottingham <alasdair....@gmail.com> wrote:

Some thoughts below. 

On Aug 9, 2017, at 4:51 PM, sst...@redhat.com wrote:

Let's discuss the ideas for changes suggested by @hendrikebbers as presented in https://github.com/eclipse/microprofile-jwt-auth/pull/16

• removed public keyword since it is not needed in interface

Perhaps this is an old school issue of mine one would actually have to copy and paste from one place to another that is obsoleted by today's modern IDEs and their implementation method behaviors, but that is why I tend to explicitly include the public qualifier. I don't care one way or the other on this.

I’m also old school, but I’ve learned to not care.

Also old-school, also don’t care :)

• added default implementation for all the getter methods that returns a specific claim

I think this is ok. If an implementation has a direct path to providing the value it can override the default method, otherwise is does simplify the burden on implementations.

Agree

Good improvement.  Particularly yanking the statics.  I’d probably move those fields to string literals.

• changed the return value of the method to get claim by name to a generic return value. By doing so you do not need to cast all the time.

Ok, but the generic type cannot be an extension of Serializable as that is not true for many underlying JWT libraries. The jose4j library uses a org.jose4j.jwt.NumericDate for any spec defined NumericDate types, and this is not Serializable.

I agree that T extends Serializable is not right. That said I’m now worried about portability/migration of return types. If one impl chooses to return jose4j types, and another returns a Java object then the code isn’t portable. I’m also concerned that a future JWT could decide to define a claim to be a type where it had never said anything before and this would make it harder to keep current. 

Alasdair read my mind on this one.  I had been assuming or imagining we would restrict ourselves to basic JSon datatypes  Long or String or Boolean or perhaps JsonObject from JSON-P for complex objects.

• changed primitive long return values for claims to Long. All the claims are not mandatory (as far as I know) and based on this you can end in a NPE based on auto boxing here if you use primitive long

All of the explicitly stated claim methods are required for a valid MP-JWT token, so there is a possible efficiently by leaving them as native types if the underlying implementation also has this representation. 

I think leave as long, if we are concerned about it being null (even though it is required) for the default impl then we should check for null and return a default long value. 

I’m fine as well for long.  Is there a pointer to how we were imagining leveraging ‘audience'?

• Changed Set return value to Stream. All the methods should return an unmodifiable collection and here Set is not the best interface since it provides methods to add entries. Sadly Java still has no interfaces for unmodifiable collections :( . But based on this maybe Stream is a good return value.

I guess I don't have a problem with this. What are other's opinions about Stream vs Set?

Any Collection allows that, but the methods are defined to be optional so I don’t care about this. One thing about Set (or a Collection) is I can do jwt.getAudience().contains(“audience”) which seems simpler than jwt.getAudience().anyMatch(aud -> “audience”.equals(aud));

Agree, I also have a slight preference for Set as you can easily get a stream from it, but the other way is more painful.  Good suggestion, but given the choice between the two slightly awkward things I’d go for Set with methods that throw exceptions vs Stream and losing some simple and useful collection methods.

My logic is that in either case we’re “taking away methods”.  I’d prefer to not take away the good ones so that we can take way the bad ones.

• Added boolean containsClaim(String claimName) convenience method. This is highly recommend if Stream is used instead of Set as return value for collections.

Accepted.

Not sure how much I care but wondering why this is needed if you can use contains on set or anyMatch on a stream.

I’m also fine either way.

• Added a method that returns Optional for a claim.

Fine, but here also T cannot extend Serializable.

Love this addition — Optional is wonderful here.

* Additional questions:

• Does it makes sense if the JWTPrincipal interface extend Serializable ?

Possibly, but there could be some issues with serializing the validated form of the validated token object as represented by the underlying JWT library. For example, say you just wrote out the raw bearer token string and then reconstructed in on deserialization. Your not going to have the information that you need to validated, and let's say you wrote that out, at some point it will be expired and deserialization will fail unless you override the expiration check. For the 1.0 API I would say no, and if usage dictates this need we can revisit.

I dislike Serializable, but I especially dislike it for security sensitive stuff because it makes it really easy to do dumb stuff with it. 

I wonder if the “rawToken” method is enough to obtain something that can be serialized.

-David

[sst...@redhat.com]

We could just define an enum to specify the name, description, and required types for all supported fields, do away with the current JWTPrincipal.* string constants, and only use a handful of types:

public enum JWTClaimType {
    ISSUER("iss", "Issuer", String.class),
    SUBJECT("sub", "Subject", String.class),
    AUDIENCE("aud", "Audience", String[].class),
    EXP_TIME("exp", "Expiration Time", Long.class),
    NOT_BEFORE("nbf", "Not Before", Long.class),
    ISSUED_AT_TIME("iat", "Issued At Time", Long.class),
    JTI("jti", "JWT ID", String.class),
    FULL_NAME("name", "Full name", String.class),
...
    SUB_JWK("sub_jwk", "Public key used to check the signature of an ID Token", JsonObject.class),

    private String name;
    private String description;
    private Class<?> type;
    private JWTClaimType(String name, String description, Class<?> type) {
        this.name = name;
        this.description = description;
        this.type = type;
    }

    public java.lang.String getName() {
        return name;
    }
    public java.lang.String getDescription() {
        return description;
    }
    public Class<?> getType() {
        return type;
    }
    
    public static JWTClaimType getByName(String name) {
        ...
    }
}

On Wednesday, August 9, 2017 at 8:40:51 PM UTC-7, David Blevins wrote:

Adding Hendrik in case he’s not subscribed. :)

I agree that T extends Serializable is not right. That said I’m now worried about portability/migration of return types. If one impl chooses to return jose4j types, and another returns a Java object then the code isn’t portable. I’m also concerned that a future JWT could decide to define a claim to be a type where it had never said anything before and this would make it harder to keep current. 

[Hendrik Ebbers]

• changed the return value of the method to get claim by name to a generic return value. By doing so you do not need to cast all the time.

Ok, but the generic type cannot be an extension of Serializable as that is not true for many underlying JWT libraries. The jose4j library uses a org.jose4j.jwt.NumericDate for any spec defined NumericDate types, and this is not Serializable.

I agree that T extends Serializable is not right. That said I’m now worried about portability/migration of return types. If one impl chooses to return jose4j types, and another returns a Java object then the code isn’t portable. I’m also concerned that a future JWT could decide to define a claim to be a type where it had never said anything before and this would make it harder to keep current. 

Alasdair read my mind on this one.  I had been assuming or imagining we would restrict ourselves to basic JSon datatypes  Long or String or Boolean or perhaps JsonObject from JSON-P for complex objects.

Until now I thought that this will return JSON objects / data. I it depends on the implementation / container what will be returned here this will end in problems when changing the container imply for an implementation. In that case you would mix standard spec and implementation detail. Not using Serializable here is absolutely fine but I think that not defining the return types (only raw types or JSON) would be wrong.

Another question: Wouldn’t it make sense to do this discussion in github?

I will create a new PR with the changes that you liked later today so that they can be easily merged :)

Cheers,

Hendrik

[Hendrik Ebbers]

Ok, see that it is linked and that the content of the mail is here. That's perfect :)

[Hendrik Ebbers]

This would be perfect if only this claims would be allowed. But you can define any private claim. By doing so you can not return an enum.

[Hendrik Ebbers]

is there any agreement that I need to sign for contribution?

[sst...@redhat.com]

There are no inter-op guarantees for private claims that are not defined in the JWTClaimType enum, so usage of those would be outside of the MP-JWT spec. 

I don't see a need to have a JWTPrincipal implementation behave in some strict mode where an exception would be returned if someone requested a claim that was not amongst the current set of JWTClaimType values. All the TCK would enforce is that any values exist match the JWTClaimType defined type.

[sst...@redhat.com]

In the most naive sense it is, but here I do agree with Alasdair that it is dangerous because this could open up a way to introduce a JWTPrincipal that has not been validated. So we have not defined what the minimal requirements are for accepting a JWT as an authenticated identity, but the TCK tests are expecting:

1. The issuer matches a server configured issuer
   
2. The JWT is RSA256 signed with a public key deployed to the server.
   
3. The JWT exp claim is within the current server side time within some clock skew.
   

I don't think we want to go beyond that for 1.0. When we start discussing service specific claims, then we should talk about how aud relates to such claims.

On Wednesday, August 9, 2017 at 8:40:51 PM UTC-7, David Blevins wrote: