Secret callback revisited #480

JacoKoster · 2018-06-03T11:07:05Z

Based on PR 413, without breaking existing functionality.

Added features

verify's secretOrToken parameter can now be a callback function, see updated README.md for details
updated tests and readme

Performance improvements

merged two decodes into one

Dependencies

Removed xtend and use Object.assign instead

See issue 406

Without the more contentious 'none'-changes

I should really add a editor.config and eslint to this project ;-)

…s as well

…er/node-jsonwebtoken into add-secret-for-callback

ziluvatar · 2018-06-06T08:30:13Z

verify.js

@@ -59,23 +59,9 @@ module.exports = function (jwtString, secretOrPublicKey, options, callback) {
    return done(new JsonWebTokenError('secret or public key must be provided'));


We should move these ^ asserts inside the getSecrect callback too, since you could have secretOrPublicKey here but it could be a function that returns value / no-value later on. I mean:

var hasSignature = parts[2].trim() !== ''; if (!hasSignature && secretOrPublicKey){ return done(new JsonWebTokenError('jwt signature is required')); } if (hasSignature && !secretOrPublicKey) { return done(new JsonWebTokenError('secret or public key must be provided')); }

I do believe, as the other PR-owner, that the jws-package already handles these cases. But will move them anyway ;-)

ziluvatar · 2018-06-06T08:37:12Z

verify.js

+      var payload;
+
+      try {
+          payload = decode(jwtString);


🤔 wasn't this the decode you wanted to clean in favor of the decode(..., { complete: true }) call you add at the beginning of the verify() function?

FYI Not sure why but in github if I see the diff omitting whitespaces it shows me you removed this, however, in the normal view or even if I click in View button for this file I can still see this decode call

You are right, should be okay now :)

ziluvatar · 2018-06-06T08:55:44Z

README.md

@@ -197,6 +198,17 @@ jwt.verify(token, cert, { algorithms: ['RS256'] }, function (err, payload) {
  // if token alg != RS256,  err == invalid signature
 });

+// Verify using getKey callback
+function getKey(header, callback) {


In order to use this function for JWK + openid cases we would need to add the issuer as well.

The options are:

We add it here like getKey(header, issuer, callback), and think if we want to check the issuer, if passed in options, before even calling the getSecret function.

Leave it as is in this PR. Give support for that in a different one by checking the length of getKey function (to see if the caller expects only header and callback or header, issuer and callback)

What do you think?

For the sake of progress, i would like to keep it as is. Let's make an separate issue for it and i will pick it up after this one ;-)

ziluvatar · 2018-06-06T08:59:04Z

README.md

@@ -29,6 +29,7 @@ $ npm install jsonwebtoken

 `secretOrPrivateKey` is a string, buffer, or object containing either the secret for HMAC algorithms or the PEM
 encoded private key for RSA and ECDSA. In case of a private key with passphrase an object `{ key, passphrase }` can be used (based on [crypto documentation](https://nodejs.org/api/crypto.html#crypto_sign_sign_private_key_output_format)), in this case be sure you pass the `algorithm` option.
+If `jwt.verify` is called asynchronous, `secretOrPublicKey` can be a function that should fetch the secret or public key. See below for a detailed example


This is in wrong place, right? it should be under jwt.verify section, where we should indicate secretOrPublicKey can be a function too.

ziluvatar · 2018-06-08T10:24:51Z

verify.js

-
-  if (parts.length !== 3){
-    return done(new JsonWebTokenError('jwt malformed'));
-  }


I think this should be kept here, it refers to the token format, not secret related, there is no need to potentially call the getSecret function if the jwt is a malformed

ziluvatar · 2018-06-08T10:26:54Z

verify.js

-      } catch (err) {
-          return done(err);
-      }
+      var payload = decodedToken.payload;


ziluvatar · 2018-06-08T10:28:58Z

test/verify.tests.js

+
+      it('simple callback', function (done) {
+          var keyFunc = function(header, callback) {
+              callback(undefined, key);


I just saw that even when you are passing the decodedToken the tests pass. Can you assert here you get just the content of the header from the token?

ziluvatar · 2018-06-08T10:31:54Z

verify.js

  }

-  return done(null, payload);
+  return getSecret(decodedToken, function(err, secretOrPublicKey) {


🤔 why do you pass all the decoded token now?, I think it was fine passing just the decodedToken.header, right?

Because we will be using both the header and the payload of the token now

Oh, wait, yes. that is wrong.

ziluvatar · 2018-06-08T11:41:41Z

verify.js

-
-  return done(null, payload);
+  else {
+      getSecret = function(decodedToken, secretCallback) {


even if it is not used, can you rename it back to header as well?

@JacoKoster I saw your last commit after this comment, but that commit does not address this (not sure if you realized about it)

Ah, yes, i forgot this one.

lbalmaceda · 2018-06-08T14:22:12Z

README.md

+  callback(null, key);
+}
+
+verify(token, getKey, options, function(err, decoded) {


lbalmaceda · 2018-06-08T14:23:11Z

README.md

@@ -197,6 +198,17 @@ jwt.verify(token, cert, { algorithms: ['RS256'] }, function (err, payload) {
  // if token alg != RS256,  err == invalid signature
 });

+// Verify using getKey callback
+function getKey(header, callback) {
+  // fetch secret or public key


I'd add some comment encouraging the users to cache the key with the kid so they avoid fetching it multiple times.

I will add https://github.com/auth0/node-jwks-rsa as an example to load the keys, that should make it more clear.

lbalmaceda · 2018-06-08T14:31:32Z

verify.js

+          return done(e);
+      }
+
+      if (!valid)


please be consistent on the braces usage. A few lines above this one, another one liner if clause uses them.

Noticed this as well, but because it was already on master this way. I was planning to add eslint and fixing it in one go.

lbalmaceda · 2018-06-08T14:32:02Z

verify.js

+              });
+          });
+
+          if (!match)


lbalmaceda · 2018-06-08T14:35:29Z

verify.js

  try {
-    decodedToken = jws.decode(jwtString);
+    decodedToken = decode(jwtString, { complete: true });


this {complete:true} option wasn't being passed before. How does that affect the output @ziluvatar ?

It's mostly the same, however, our decode implementation tries to parse always the JSON (if string), the jws.decode only does it when the type is JWT. With this and other change afterwards we reduce the amount of parsing to half.

lbalmaceda · 2018-06-08T14:37:21Z

verify.js

+      };
+  }
+
+  return getSecret(header, function(err, secretOrPublicKey) {


isn't enough with the kid? What's on the header that the user might want to use for key retrieval?

You may want to use other headers to find the key: https://tools.ietf.org/html/rfc7515#page-10

lbalmaceda · 2018-06-08T15:13:44Z

LGTM 👨‍🎓

ziluvatar

I will release this on Monday as a new minor version. (I don't want to publish anything on Friday :) )

jfromaniello · 2018-06-08T15:41:27Z

test/verify.tests.js

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

+  describe('secret or token as callback', function () {


This function is not properly indented with the rest of the file and doesn't follow the newly introduced .editorconfig

I'd like to make this a thing with #486

I agree with @jfromaniello on this one. Since the bad indentation is introduced in this PR there's no need to wait for a different one to fix it.

ziluvatar · 2018-06-11T15:49:10Z

🎉 Released on v8.3.0!! Awesome work @JacoKoster 👏

JacoKoster added 2 commits June 3, 2018 12:55

Introduction of the secret callback

96cb28b

Without the more contentious 'none'-changes

Removed some spaces...

798b033

I should really add a editor.config and eslint to this project ;-)

JacoKoster changed the title ~~Add secret for callback~~ Secret callback revisited Jun 3, 2018

JacoKoster added 3 commits June 3, 2018 13:35

Removed xtend as a dependency, as the native Object.Assign can do thi…

8d96f89

…s as well

Removed xtend as a dependency, as the native Object.Assign can do thi…

ddb1736

…s as well

Merge branch 'add-secret-for-callback' of https://github.com/JacoKost…

7d60544

…er/node-jsonwebtoken into add-secret-for-callback

ziluvatar reviewed Jun 6, 2018

View reviewed changes

Resolve feedback from review

6199e60

ziluvatar reviewed Jun 8, 2018

View reviewed changes

Added extra test and fixed the associated bug

7a39153

JacoKoster mentioned this pull request Jun 8, 2018

Make it easier to support openid with JWK #484

Closed

ziluvatar reviewed Jun 8, 2018

View reviewed changes

JacoKoster added 2 commits June 8, 2018 13:47

The return of the header

fdfb2ef

Forgot to change this one as well... Sorry bout that

e67f5d3

JacoKoster mentioned this pull request Jun 8, 2018

Build pipeline #486

Closed

lbalmaceda reviewed Jun 8, 2018

View reviewed changes

Updated the readme and made the if-statements consistent

2e27e84

ziluvatar approved these changes Jun 8, 2018

View reviewed changes

jfromaniello reviewed Jun 8, 2018

View reviewed changes

Space; The final frontier

f83432f

ziluvatar merged commit d01cc7b into auth0:master Jun 11, 2018

This was referenced Jun 11, 2018

Secret callback #413

Closed

Allow secretOrPrivateKey in verify to be function(kid) #406

Closed

Remove callback API #285

Closed

Deprecate callback API #246

Closed

MitMaro mentioned this pull request Jun 14, 2018

Complete ESLint conversion and cleanup #490

Merged

JacoKoster mentioned this pull request Jun 26, 2018

Support for JWK #43

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Secret callback revisited #480

Secret callback revisited #480

JacoKoster commented Jun 3, 2018 •

edited

ziluvatar Jun 6, 2018

JacoKoster Jun 8, 2018

ziluvatar Jun 6, 2018

ziluvatar Jun 6, 2018

JacoKoster Jun 8, 2018

ziluvatar Jun 6, 2018

JacoKoster Jun 8, 2018

ziluvatar Jun 6, 2018

ziluvatar Jun 8, 2018

ziluvatar Jun 8, 2018

ziluvatar Jun 8, 2018

ziluvatar Jun 8, 2018 •

edited

JacoKoster Jun 8, 2018

JacoKoster Jun 8, 2018

ziluvatar Jun 8, 2018

ziluvatar Jun 8, 2018

JacoKoster Jun 8, 2018

lbalmaceda Jun 8, 2018

lbalmaceda Jun 8, 2018

JacoKoster Jun 8, 2018

lbalmaceda Jun 8, 2018

JacoKoster Jun 8, 2018 •

edited

lbalmaceda Jun 8, 2018

lbalmaceda Jun 8, 2018

ziluvatar Jun 8, 2018

lbalmaceda Jun 8, 2018

ziluvatar Jun 8, 2018

lbalmaceda commented Jun 8, 2018

ziluvatar left a comment

jfromaniello Jun 8, 2018

JacoKoster Jun 8, 2018 •

edited

lbalmaceda Jun 8, 2018

ziluvatar commented Jun 11, 2018

		@@ -59,23 +59,9 @@ module.exports = function (jwtString, secretOrPublicKey, options, callback) {
		return done(new JsonWebTokenError('secret or public key must be provided'));

Secret callback revisited #480

Secret callback revisited #480

Conversation

JacoKoster commented Jun 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziluvatar Jun 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacoKoster Jun 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda commented Jun 8, 2018

ziluvatar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacoKoster Jun 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziluvatar commented Jun 11, 2018

JacoKoster commented Jun 3, 2018 •

edited

ziluvatar Jun 8, 2018 •

edited

JacoKoster Jun 8, 2018 •

edited

JacoKoster Jun 8, 2018 •

edited