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

Added API endpoint for token refresh #172

Closed
wants to merge 15 commits into from

Conversation

@jppellerin
Copy link

jppellerin commented Feb 3, 2016

According to issue #122, there was some interest in having an endpoint to refresh a token.

A refresh is considered to have the same token returned, but with a later expiry time.

This function takes in a decoded token, copies the attributes of this token over, and with that data, proceeds to signing a new token. The only piece of data that is replaced is the expiresIn value, which is passed in the function parameters. By extension, the iat value is also changed since a new token is created.

Jean-Philipe Pellerin added 10 commits Feb 2, 2016
…t to refresh a token. A refresh is considered to have the same token returned, but with a later expiry time.

Can take into account the header + payload of a decoded token ie : {complete: true}

More in depth testing with comparision of equality

Testing failures and async mode

Added description for refresh in README.md
@jppellerin jppellerin mentioned this pull request Feb 4, 2016
@Kiura

This comment has been minimized.

Copy link

Kiura commented Feb 4, 2016

+1

1 similar comment
@foobar8675

This comment has been minimized.

Copy link

foobar8675 commented Feb 9, 2016

+1

@jfromaniello

This comment has been minimized.

Copy link
Member

jfromaniello commented Feb 16, 2016

Very nice job, I will merge this!

@jppellerin

This comment has been minimized.

Copy link
Author

jppellerin commented Feb 16, 2016

👍 Excellent! Thank you!

@mateeyow

This comment has been minimized.

Copy link

mateeyow commented Mar 3, 2016

Quick question, when will this be merged? Any ETA?

@pmeijer

This comment has been minimized.

Copy link

pmeijer commented Mar 9, 2016

+1

@arturskeeled

This comment has been minimized.

Copy link

arturskeeled commented Mar 17, 2016

+1 Any news on when is this being implemented?

@jppellerin

This comment has been minimized.

Copy link
Author

jppellerin commented Mar 17, 2016

@mateeyow @arturskeeled I'm not sure when the next major release will be coming out, but I have just updated my forked version with the refresh endpoint : https://github.com/jppellerin/node-jsonwebtoken. You could use this for development purposes.

Disclaimer: I have not tested the merge thoroughly..

@rickli1989

This comment has been minimized.

Copy link

rickli1989 commented Apr 8, 2016

Waiting for this to get released

@rickli1989

This comment has been minimized.

Copy link

rickli1989 commented Apr 15, 2016

When is the next major release for this ?

@RWOverdijk

This comment has been minimized.

Copy link

RWOverdijk commented May 3, 2016

👍 we'd really like to implement refresh tokens.

@VMBindraban

This comment has been minimized.

Copy link

VMBindraban commented May 12, 2016

Any ETA yet?

@RWOverdijk

This comment has been minimized.

Copy link

RWOverdijk commented May 25, 2016

Bump...

@RWOverdijk

This comment has been minimized.

Copy link

RWOverdijk commented Jun 22, 2016

Last bump. I don't want to fork but I really need this.

@jppellerin

This comment has been minimized.

Copy link
Author

jppellerin commented Jun 22, 2016

@RWOverdijk : I have just updated my fork with the newest version and fixed the tests.
https://github.com/jppellerin/node-jsonwebtoken

I can keep this up to date with the newest features here until it gets merged in. If you want some assurances, I can tag you a version.

That's the best I can do.

@ghost

This comment has been minimized.

Copy link

ghost commented Jul 2, 2016

Would be really great to have this in. 💃

@@ -172,6 +172,28 @@ console.log(decoded.header);
console.log(decoded.payload)

This comment has been minimized.

Copy link
@Tchikita35
@@ -172,6 +172,28 @@ console.log(decoded.header);
console.log(decoded.payload)
```

### jwt.refresh(token, expiresIn, secretOrPrivateKey [, callback])

Will refresh the given token. The token is __expected__ to be *decoded* and *valid*. No checks will be performed on the token. The function will copy the values of the token, give it a new expiry time based on the given `expiresIn` parameter and will return a new signed token using the `sign` function and given secretOrPrivateKey.

This comment has been minimized.

Copy link
@Tchikita35

Tchikita35 Jul 9, 2016

⭐⭐⭐⭐⭐

@mitchellporter

This comment has been minimized.

Copy link

mitchellporter commented Jul 24, 2016

Is there a reason this hasn't been merged yet?

@morgondag

This comment has been minimized.

Copy link

morgondag commented Jul 27, 2016

forking is the place.

@Isaac6702

This comment has been minimized.

Copy link

Isaac6702 commented Jul 28, 2016

Now you can use this?

@jfromaniello

This comment has been minimized.

Copy link
Member

jfromaniello commented Jul 29, 2016

I am still considering this feature, I think as it is it doesn't add too much value over sign and it creates another api to maintain and another source for confusion and vulnerabilities. I refresh should also validate the token it would be more like a verify->sign.

@jppellerin

This comment has been minimized.

Copy link
Author

jppellerin commented Jul 29, 2016

@jfromaniello Thanks for the feedback. The goal (for what we are using it for) is to extract all fields of a token and return it with a new exp. The only way (as far as I know) is to create a new one.

I didn't include verification since the token needs to be decoded for this to work. Therefore, it assumes the user has manipulated this - thus it would be safe to assume that it has been validated.

I agree that if one uses this API the wrong way and refreshes a non-valid JWT, then a valid one will be returned, which has cause for concern.

The value this endpoint has is to save the logic of copying over the fields of the existing token and re-signing it. I can agree with you that it is not something extremely difficult to do, but is something that is useful and that many people are using (or so it seems by the interest of this pull request). I would assume that this is going to be more and more common as people are moving towards serverless architectures/completely stateless.

All that being said: I think that verification would be a good idea. I also think it is a useful endpoint (I may be biased since I'm using this).

```js
// ...
var originalDecoded = jwt.decode(token, {complete: true});
var refreshed = jwt.refresh(originalDecoded, 3600, secret);

This comment has been minimized.

Copy link
@AlexanVal

AlexanVal Mar 4, 2017

I think it should be var refreshed = jwt.refresh(**token**, 3600, secret);

This comment has been minimized.

Copy link
@fiddur

fiddur Jul 14, 2017

Contributor

I don't see the point of this. You have to decode and validate it, and you will have the payload, why not just call sign on that payload instead of having a refresh function that will decode it again?

@jppellerin

This comment has been minimized.

Copy link
Author

jppellerin commented Apr 27, 2017

@jfromaniello Maybe this pull request should be rejected if it's not going to be merged? It has been over a year now.

@fiftysoft

This comment has been minimized.

Copy link

fiftysoft commented May 22, 2017

@jppellerin can you give me an example to use it ?

@natealcedo

This comment has been minimized.

Copy link

natealcedo commented May 23, 2017

Whats the update on this pull request? It's been over a year. I agree with @jppellerin that it should either get merged if not rejected. @jppellerin Have you considered making a fork of this and pushing to npm?

@jppellerin

This comment has been minimized.

Copy link
Author

jppellerin commented May 23, 2017

@fiftysoft : An example is in the readme docs of the pull request.

@ndaljr : I have considered making a fork, but the problem is keeping up with the rest of the library. I don't want to have the code duplicated and potentially falling behind on updates. If this was to become it's own module, it would have to only extend it. Which, I might put some thought into this.

@oshalygin

This comment has been minimized.

Copy link

oshalygin commented Jun 30, 2017

Any status on this?

@fiddur

This comment has been minimized.

Copy link
Contributor

fiddur commented Jul 14, 2017

Fixed by documentation: #371

@fiddur fiddur closed this Jul 14, 2017
@VMBindraban

This comment has been minimized.

Copy link

VMBindraban commented Jul 14, 2017

Well this feels like a big slap in the face for the people who were waiting for it.

@fiddur

This comment has been minimized.

Copy link
Contributor

fiddur commented Jul 14, 2017

@VMBindraban: Since you still had to decode and validate it before using the refresh function, that function added very little value over just deleting old exp/iat and using sign instead.

Keeping the API and the code clean and small without unnecessary complexity is more important than a convenience method that is hardly even convenient.

@BrOrlandi

This comment has been minimized.

Copy link

BrOrlandi commented Jul 26, 2017

I have found a problem in the refresh function that's causing it to generate the same token.
In the file refresh.js line 102
the token.iat is trying to read a property that doesn't exist because token is a string.
Even using payload.iat, the code allows passing the iat in payload, which causes the jwt.sign() generates the same token. The solution I found is to remove the iat from payload. Anyone had this problem?

@Amri91 Amri91 mentioned this pull request Dec 29, 2017
Merged
@derrickmehaffy derrickmehaffy mentioned this pull request Aug 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.