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 complete option in jwt.verify #522

Merged
merged 5 commits into from Feb 20, 2019
Merged

Add complete option in jwt.verify #522

merged 5 commits into from Feb 20, 2019

Conversation

javespi
Copy link
Contributor

@javespi javespi commented Sep 11, 2018

Same behaviour as in decode method. Just return an object with the header, payload and signature when complete: true option is passed.

Added also a test to cover this use case.

@ziluvatar
Copy link
Contributor

The change is good to me, however, what is the use case for this change?
I saw the comment in the code, but if the token was already verified according to the options passed, what is the need for header + signature apart from the payload?

@javespi
Copy link
Contributor Author

javespi commented Sep 20, 2018

Thanks @ziluvatar for the response.

My motivation to include this feature was that inside verify method there is already a call to decode passing { complete: true } option:

decodedToken = decode(jwtString, { complete: true });

My use case requires to verify the token and also returns the full JWT (header, payload, signature) after verifying it. For now, I do 2 calls: one to verify and another one to decode if the verification goes OK. If we have this option in verify method I can avoid the second call to decode method because it's already done inside verify.

@ziluvatar
Copy link
Contributor

ziluvatar commented Sep 28, 2018

My use case requires to verify the token and also returns the full JWT (header, payload, signature) after verifying it.

That's how you want to get it done, I'm asking what the use case is for the need of returning the full JWT, what do you need to do with the header or the signature? I want to understand the root reason better.

@panva
Copy link
Contributor

panva commented Jan 31, 2019

@ziluvatar there may be any number of additional properties in the JWS protected header a developer may want to access. Exposing this option would save extra processing of something which was already parsed once.

@ziluvatar
Copy link
Contributor

I can see:

//return header if complete option is enabled. header includes claims
//such as kid and alg used to select the key within a JWKS needed to
//verify the signature

@javespi Currently that use case can be achieve by passing the getSecret(header, cb) function as "secret".

Anyway, if you remove that comment I'll merge the PR.

@javespi
Copy link
Contributor Author

javespi commented Feb 13, 2019

Thanks @ziluvatar and @panva for your feedback. I've already removed that comment.

@@ -69,6 +69,30 @@ describe('verify', function() {
});
});

describe('option: complete', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't realize about this before, can you follow similar approach as this option?
https://github.com/auth0/node-jsonwebtoken/blob/master/test/option-nonce.test.js

That is: option-complete.test.js, using testUtils.asyncCheck (to report errors correctly) and testUtils.verifyJWTHelper (to verify sync and async).

We are refactoring the tests moving towards that model so each option and claim has its own tests using common tooling.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved here 4d8f50a

Apart from that, I added another unit test for complete: false option.

README.md Outdated Show resolved Hide resolved
ziluvatar and others added 2 commits February 16, 2019 14:05
Co-Authored-By: javespi <javespalf@gmail.com>
@ziluvatar ziluvatar merged commit 8737789 into auth0:master Feb 20, 2019
@ziluvatar
Copy link
Contributor

Thanks @javespi ! 🎉 Released on v8.5.0

@javespi
Copy link
Contributor Author

javespi commented Feb 20, 2019

@ziluvatar thanks to you! Great project!

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

Successfully merging this pull request may close these issues.

None yet

3 participants