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 a margin to the automatic JWT refresh #33

Merged
merged 3 commits into from
Mar 21, 2015
Merged

Add a margin to the automatic JWT refresh #33

merged 3 commits into from
Mar 21, 2015

Conversation

jdhoek
Copy link
Contributor

@jdhoek jdhoek commented Mar 16, 2015

By configuring refreshMargin the JWT gets refreshed that many seconds
before the current token expires.

PR for #32.

By configuring `refreshMargin` the JWT gets refreshed that many seconds
before the current token expires.
@jdhoek
Copy link
Contributor Author

jdhoek commented Mar 16, 2015

I've had a go at implementing the feature I suggested in #32 by adding a configurable parameter that defaults to 0. It might actually make sense to default to some value (10 seconds? A minute? 5 minutes?), but I'm just getting my feet wet with JSON Web Tokens, so I might be mistaken.

@validkeys
Copy link
Contributor

+1 on this one. All of my refresh requests keep getting cancelled as the JWT is expired by the time the refresh makes the requests.

Great work @jdhoek

@jpadilla
Copy link
Contributor

I definitely thought this would have been an issue. @erichonkanen haven't you run into this?

This looks pretty good as is to me, not too sure about the actual name for the setting refreshMargin, definitely not against it. We use leeway in PyJWT.

@hoIIer
Copy link
Contributor

hoIIer commented Mar 17, 2015

@jpadilla I have not run into this issue in any of the projects Ive used this with. Two of these projects are deployed to heroku currently in an ember-cli client/django backend setup with proxy.

The only thing I can think of for me is that I have JWT_LEEWAY set to 3 seconds on the django jwt side.

That said it seems logical to have something like this.

cheers

@hoIIer
Copy link
Contributor

hoIIer commented Mar 17, 2015

@validkeys have you tried setting a leeway on the server side? that is the most likely reason you ran into this and I didn't

@jdhoek
Copy link
Contributor Author

jdhoek commented Mar 17, 2015

@jpadilla
leeway does sound better, I'll push a commit renaming this to refreshLeeway.

I'm implementing a Java API that serves up and refreshes JWT. I considered giving the timeout some leeway server-side, but that would encourage an API where the client sends expired or very nearly expired tokens to the server. That doesn't feel quite right in terms of a clean API, although for all practical purposes both approaches should work equally fine.

@jdhoek
Copy link
Contributor Author

jdhoek commented Mar 17, 2015

Thanks for the 👍 vote of confidence.

@jpadilla
I've updated the documentation in README.md as well, please have a look if all is in order there.

@validkeys
Copy link
Contributor

@erichonkanen I didn't but I just tried it out and that works well for me. Having that hint as part of the documentation would be great for JWT noobs (like me) (Best Practice would be to include a leeway in your JWT's decode method server side).

@validkeys
Copy link
Contributor

@erichonkanen @jpadilla put a quick note into the docs: /pull/34

jpadilla added a commit that referenced this pull request Mar 21, 2015
Add a margin to the automatic JWT refresh
@jpadilla jpadilla merged commit 36a312a into fenichelar:master Mar 21, 2015
@jdhoek
Copy link
Contributor Author

jdhoek commented Mar 22, 2015

Thanks!

@jdhoek jdhoek deleted the feature/refreshMargin branch March 22, 2015 10:12
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.

4 participants