Skip to content

Add complete option in jwt.verify - #522

Merged
ziluvatar merged 5 commits into
auth0:masterfrom
javespi:addCompleteOptionInVerify
Feb 20, 2019
Merged

Add complete option in jwt.verify#522
ziluvatar merged 5 commits into
auth0:masterfrom
javespi:addCompleteOptionInVerify

Conversation

@javespi

@javespi javespi commented Sep 11, 2018

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

javespi commented Sep 20, 2018

Copy link
Copy Markdown
Contributor Author

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

ziluvatar commented Sep 28, 2018

Copy link
Copy Markdown
Contributor

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

panva commented Jan 31, 2019

Copy link
Copy Markdown
Member

@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
Copy Markdown
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

javespi commented Feb 13, 2019

Copy link
Copy Markdown
Contributor Author

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

Comment thread test/verify.tests.js Outdated
});
});

describe('option: complete', function() {

Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread README.md Outdated
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
Copy Markdown
Contributor

Thanks @javespi ! 🎉 Released on v8.5.0

@javespi

javespi commented Feb 20, 2019

Copy link
Copy Markdown
Contributor Author

@ziluvatar thanks to you! Great project!

@javespi
javespi deleted the addCompleteOptionInVerify branch October 5, 2019 17:06
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.

3 participants