Giter VIP home page Giter VIP logo

Comments (17)

lhazlewood avatar lhazlewood commented on May 21, 2024

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

from jjwt.

lhazlewood avatar lhazlewood commented on May 21, 2024

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?

from jjwt.

lhazlewood avatar lhazlewood commented on May 21, 2024

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?

from jjwt.

lhazlewood avatar lhazlewood commented on May 21, 2024

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.

from jjwt.

lhazlewood avatar lhazlewood commented on May 21, 2024

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.

from jjwt.

lhazlewood avatar lhazlewood commented on May 21, 2024

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
    }
});

from jjwt.

mattjonesorg avatar mattjonesorg commented on May 21, 2024

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.

from jjwt.

mattjonesorg avatar mattjonesorg commented on May 21, 2024

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.

from jjwt.

lhazlewood avatar lhazlewood commented on May 21, 2024

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!

from jjwt.

lhazlewood avatar lhazlewood commented on May 21, 2024

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!

from jjwt.

lhazlewood avatar lhazlewood commented on May 21, 2024

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!

from jjwt.

mattjonesorg avatar mattjonesorg commented on May 21, 2024

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? :)

from jjwt.

lhazlewood avatar lhazlewood commented on May 21, 2024

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

from jjwt.

lhazlewood avatar lhazlewood commented on May 21, 2024

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

from jjwt.

mattjonesorg avatar mattjonesorg commented on May 21, 2024

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.

from jjwt.

lhazlewood avatar lhazlewood commented on May 21, 2024

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

from jjwt.

lhazlewood avatar lhazlewood commented on May 21, 2024

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!

from jjwt.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.