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

add verify overload #162

Closed
wants to merge 1 commit into from
Closed

add verify overload #162

wants to merge 1 commit into from

Conversation

joprice
Copy link

@joprice joprice commented Apr 9, 2017

This allows an already-decoded JWT to be used without decoding twice. This is useful, for instance, when the kid is used to look up the public key.

@lbalmaceda
Copy link
Contributor

Hi @joprice
Thanks for the PR. First of all, the only operations you're saving processing time in is the JSON parsing of the header/payload, and maybe the String.split() and Base64.decode() which are not as heavy as the signature verification that you're still doing on this new method so I don't think it's a win. Of course it always depends on the algorithm and size of your token, which we suggest you keep short.
Adding to that, the Algorithm and JWTVerifier classes are meant to be reusable, so you should be able to configure a single instance and use it widely (as long as your requirements doesn't change on runtime). If your concern is to access the kid or even the already verified DecodedJWT, why not just keep the value in a variable? Provide more context on how you're using the library so we can understand.

Cheers,

@joprice
Copy link
Author

joprice commented Apr 10, 2017

The use case is looking up the public key dynamically based on kid.

@lbalmaceda
Copy link
Contributor

lbalmaceda commented Apr 11, 2017

@joprice I think it's confusing to have another verify method that receives and returns DecodedJWT. I would keep the existing signature as it is, and let the one that you're suggesting return void instead and just throw if something goes wrong inside.

Something like this:

public DecodedJWT verify(String token) throws JWTVerificationException {
          DecodedJWT jwt = JWTDecoder.decode(token);
          verify(jwt);
          return jwt;
}

public void verify(DecodedJWT jwt) throws JWTVerificationException {
          //perform the actual verification
}

Please add the corresponding javadoc+tests.
Cheers,

@lbalmaceda
Copy link
Contributor

Before the above, I think you should look at this PR #149 as it adds support for KeyProviders, which seems to be what you need for the key look up. Will be available soon in the next release.

@joprice
Copy link
Author

joprice commented Apr 11, 2017

I don't see an example of looking up a key based on an id. The provider interface doesn't take any input.

@hzalaz
Copy link
Member

hzalaz commented Apr 11, 2017

@joprice will close this one since we rather improve on what @lbalmaceda mentioned for allowing using rotating keys.

@hzalaz hzalaz closed this Apr 11, 2017
@hzalaz
Copy link
Member

hzalaz commented Apr 11, 2017

We'll try to provide an example in the README and here too

@joprice
Copy link
Author

joprice commented Apr 11, 2017

Sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants