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

The docs have a one-liner to parse the token, but there is no Token class #2

Closed
mattjonesorg opened this issue Sep 26, 2014 · 17 comments
Milestone

Comments

@mattjonesorg
Copy link

The readme has the following line to parse the token.

Token token = Jwts.parser().setSigningKey(key).parse(jwt);

However, there is no Token class in the library. The parse method is defined to return a Jwt object.

Jwt parse(String jwt) throws MalformedJwtException, SignatureException;

The implementation returns a DefaultJwt, which does not allow for the syntactic sugar currently shown in the README.md

token.getClaims().getSubject().equals("Joe");

I'm curious how you'd like this fixed. I'll happily submit a pull request.

@lhazlewood
Copy link
Contributor

Aw you beat me to it! I'm working on that as we speak :)

@lhazlewood
Copy link
Contributor

Might have to introduce the visitor pattern so the caller can identify if the returned JWT is a JWS or not...

I was thinking of the following:

Jwt jwt = Jwts.parser().setSigningKey(key).parse(compact);

jwt.accept(new JwtVisitor() {
    public void onPlaintextJwt(Jwt jwt, String payload) {
        //plaintext String body but not a signed JWT (aka JWS)
    }
    public void onClaimsJwt(Jwt jwt, Claims claims) {
        //A Claims body but not a signed JWT (JWS)
    }
    public void onPlaintextJws(Jws jws, String payload) {
        //it was a signed JWS with a plaintext (String) body.
    }
    public void onClaimsJws(Jws jws, Claims claims) {
        //it was a signed JWS with a Claims body
    }
});

Not sure how I feel about this. I hate instanceof and lots of if-else checks in code, so I was going for something more 'automatic'. Any thoughts?

@lhazlewood
Copy link
Contributor

The alternative might be just instanceof checks;

Jwt jwt = Jwts.parser().setSigningKey(key).parse(compact);

if (jwt.getBody() instanceof Claims) {
    Claims claims = (Claims)jwt.getBody();
} else {
    String payload = (String)jwt.getBody();
}

Thoughts?

@lhazlewood
Copy link
Contributor

Another approach:

add a isPlaintext() and/or isClaims() methods to the Jwt:

Jwt jwt = Jwts.parser().setSigningKey(key).parse(compact);

if (jwt.isPlaintext()) {
    String payload = jwt.getPayload();
} else {
    Claims claims = jwt.getClaims();
}

Both methods should throw an exception if the JWT body is not of the expected return type.

@lhazlewood
Copy link
Contributor

Yet another approach:

Jwt<Claims> jwt = Jwts.parser().setSigningKey(key).parseClaims(compact);
Claims claims = jwt.getBody(); //or jwt.getBody().getIssuer(); etc

This would throw an exception if it is a plaintext jwt but succeed silently if the JWT had a claims payload.

@lhazlewood
Copy link
Contributor

And another:

Jwts.parser().setSigningKey(key).parse(compact, new JwtHandler() {
    public void onPlaintextJwt(Jwt<String> jwt) {
        //plaintext String body but not a signed JWT (aka JWS)
    }
    public void onClaimsJwt(Jwt<Claims> jwt) {
        //A Claims body but not a signed JWT (JWS)
    }
    public void onPlaintextJws(Jws<String> jws) {
        //it was a signed JWS with a plaintext (String) body.
    }
    public void onClaimsJws(Jws<Claims> jws) {
        //it was a signed JWS with a Claims body
    }
    public void onException(JwtException jwtException) {
        //couldn't parse the JWT
    }
});

With this approach, there could be a JwtHandlerAdapter that implements all the methods (and throws exceptions because that should be the default if encountering an unexpected JWT ) so you can override only the method where you're pretty sure that's the only one you're likely to encounter, e.g.:

Jwts.parser().setSigningKey(key).parse(compact, new JwtHandlerAdapter() {
    public void onClaimsJws(Jws<Claims> jws) {
        //it was a signed JWS with a Claims body
    }
});

@lhazlewood lhazlewood added this to the 0.2 milestone Sep 27, 2014
@mattjonesorg
Copy link
Author

I really like the simplicity of what is proposed in the readme:

Token token = Jwts.parser().setSigningKey(key).parse(jwt); 
token.getClaims().getSubject();

I wonder what the most common use cases for this library would be. For my use case, I know that I'm expecting claims to be present. And looking at the four use cases (Signed vs. Not Signed && Claims vs. Plaintext), the fact that setSigningKey() is called indicates to me that the caller is expecting a signed JW*. Parse should throw an exception if the signing key is set (as it currently does). Even with the JwtHandlerAdapter, the caller is only implementing what they expect. So, why not something like this:

Claims claims = Jwts.parser().setSigningKey(key).parse(jwt).asClaims();
String body =  Jwts.parser().setSigningKey(key).parse(jwt).asString();

I think we could get away with this because the caller knows what they are expecting. Perhaps there could be an additional overload for .parse() which takes the JwtHandlerAdapter() for the use case where the user of the library doesn't know what they are getting, but I think that would be the exception rather than the rule. asClaims() and asString() would obviously throw an exception if they were not of the right type.

@mattjonesorg
Copy link
Author

It could be made even simpler:

Jwt<Claims> claims = Jwts.parser().setSigningKey(key).parse<Claims>(jwt);
Jwt<String> text =  Jwts.parser().setSigningKey(key).parse<String>(jwt);

With an additional parse method that takes the JwtHandlerAdapter() for the other use case.

@lhazlewood
Copy link
Contributor

I think our minds are aligned on this one :) I'm about to commit some changes to a branch so you can see what they look like. They're nearly identical to what you proposed!

@lhazlewood
Copy link
Contributor

Here's the current pull request: https://github.com/jwtk/jjwt/pull/5/files

I added some convenience type-specific methods to handle the 4 combinations (JWT vs JWS, String body vs Claims body). They delegate to a JwtHandler based parse method (which can also be used by users which may need to support accepting multiple types of JWTs).

I liked the type coercion .parse<Claims>(jwt) approach, but saw the suggestion after my current implementation was written. The current impl has the additional side benefit of guaranteeing those types at runtime whereas the coercion technique may not.

The return types from the convenience methods do indeed use generics and now allow the desired simplicity, for example:

String subject = Jwts.parser().setSigningKey(key).parseClaimsJws(jws).getBody().getSubject();

Let me know what you think!

@lhazlewood
Copy link
Contributor

P.S. as a side note, you'll probably like (as I do) the new convenience methods I added to the builder to set claims directly without needing to obtain or construct the intermediate Claims instance yourself first. Made my code look nicer at least!

@mattjonesorg
Copy link
Author

Nice. As I read it, you updated the pull request to implement the last three methods! I think in your last comment, you meant to use the parseClaimsJws() or parseClaimsJwt() method instead of the naked parse() method.

String subject = Jwts.parser().setSigningKey(key).parseClaimsJws(jwt).getBody().getSubject()

I think this very elegantly handles both scenarios when you know what to expect and when you don't know what to expect. I especially appreciate that it pushes most of the code that would have been repeated by clients down into the library.

Ready to release 0.2? :)

@lhazlewood
Copy link
Contributor

Yep, just about! Need to update the release docs, then doing it right after that :-D

@lhazlewood
Copy link
Contributor

Oh, and I have to write tests to ensure code coverage for regression purposes. I'm a stickler about that stuff.

@mattjonesorg
Copy link
Author

As a user of the library, I really appreciate the dedication to testing and comments that you've demonstrated. And I do appreciate anything that makes the client code more concise, such as the improved builder! Fluent APIs combined with autocompletion in modern IDEs are a lot like HATEOS in a REST API -- both make it evident to the developer what can be done with the API.

Thanks for asking for my input, I've really enjoyed this collaboration.

@lhazlewood
Copy link
Contributor

Me too - thanks for collaborating! That's what open source is all about. Glad to have the help.

@lhazlewood
Copy link
Contributor

Closing per the 0.2 release. Please allow a couple of hours for the artifact to propagate to Maven Central.

Thanks again for the great feedback!

lhazlewood pushed a commit that referenced this issue Oct 12, 2015
Issue-52: Refactoring and adding unit tests to cover the compression …
lhazlewood added a commit that referenced this issue Aug 7, 2018
# This is the 1st commit message:

[maven-release-plugin] prepare for next development iteration

# This is the commit message #2:

rebased from master before merge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants