Skip to content
This repository has been archived by the owner on May 26, 2020. It is now read-only.

Long running refresh tokens #123

Closed
wants to merge 5 commits into from

Conversation

ticosax
Copy link
Contributor

@ticosax ticosax commented May 26, 2015

related to #117 and #94

But change the way we obtain a new JWT. Compare to #94, now the implementation try to be more compliant with https://auth0.com/docs/refresh-token

The delegation endpoint is a POST, with the following body.

{
  "client_id":       "YOUR_CLIENT_ID",
   "grant_type":      "urn:ietf:params:oauth:grant-type:jwt-bearer",
   "refresh_token":   "YOUR_REFRESH_TOKEN",
   "api_type":        "app"
}

instead of being a POST + empty body + the refresh token passed in Authorization header.

/cc @fxdgear

TODO before merging:

  • provide a squashed migration

fxdgear and others added 5 commits May 26, 2015 14:12
This allows for a client to request refresh tokens. These refresh tokens do not expire.
They can be revoked (deleted). When a JWT has expired, it's possible to send a request
with the refresh token in the header, and get back a new JWT. This allows for the client
to not have to store username/passwords. So, if the client gets a responce about an expired token
the client can automatically make a call (behind the scenes) to delegate a new JWT using
the stored refresh token. Thus keeping the 'session' active.

moving everything to it's own sub dir, so that the refresh token functionality can be optionally installed.
and choose constraint for user and app together
The delegation endpoint
is a `POST`, with the following body.

```json
{
  "client_id":       "YOUR_CLIENT_ID",
  "grant_type":      "urn:ietf:params:oauth:grant-type:jwt-bearer",
  "refresh_token":   "your_refresh_token",
  "api_type":        "app"
}
```
@@ -0,0 +1,23 @@
# -*- coding: utf-8 -*-
from __future__ import unicode_literals
Copy link

Choose a reason for hiding this comment

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

Would probably be best to squash these migrations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I would like to do it once @jpadilla considers it ready for merging. Just in case some amendments are still necessary on the Model.
I will add it in the description as a reminder.

Copy link

Choose a reason for hiding this comment

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

👍

@jpadilla
Copy link
Owner

This looks really good, guys. api_type and grant_type aren't used anywhere really. Wondering if that would actually introduce more confusion, thoughts?

Should we keep around our current refresh feature or should this simply replace that and be the preferred way for token refresh?

As with #123 it'd be great if we could chat and figure out if we can come up with a demo for this feature.

@fxdgear
Copy link

fxdgear commented May 27, 2015

@jpadilla so I think that both have a usecase.

In the sense of a browser, the refresh token that exists currently is a better option.
In the sense of a native app this long running refresh token is a better option.

The idea is that a browser shouldn't be storing the refresh tokens (imo). And similarly a native app should NOT be storing a users password.

@alvinchow86
Copy link
Contributor

I think the current refresh feature would still be useful for certain applications but we should make sure to distinguish the two in the documentation

One thing that's bugging me is, should we consider an option to set expiration time on these tokens? (e.g. 14 days or something). One application I was thinking this refresh token would solve (and the existing one does not) is emulating browser sessions and keeping a user logged in (even if they haven't checked in in a while). But storing an permanent refresh token isn't that appealing.

@fxdgear
Copy link

fxdgear commented May 29, 2015

I would recommend NOT using this for browser sessions. This would require the browser to store long running refresh token. Which imo is a bad idea.

This is more useful in the case of an iPhone app or a native app or some sort of CLI interface.

@alvinchow86
Copy link
Contributor

Yes that's true, XSS would be an issue. This would be indeed be very useful for mobile/native apps though.

What do you think about an optional expiration time though?

@ticosax
Copy link
Contributor Author

ticosax commented Jun 1, 2015

@alvinchow86 Expiration can be implemented by deleting the RefreshToken with a celery-beat cronjob or any equivalent task scheduling system.

@ticosax
Copy link
Contributor Author

ticosax commented Jun 1, 2015

@jpadilla api_type and grant_type are not used, I agree. I blindly followed the spec https://auth0.com/docs/refresh-token .
To be honest I do not know what is better to do here, either we:

  • use those fields, but how ?
  • remove those fields.
  • keep it like that.

@alarrosa14
Copy link

Will this be merged soon?

@hoIIer
Copy link
Contributor

hoIIer commented Jul 8, 2015

hey btw I made a demo for the blacklist token feature, maybe it could be used as the general demo for this app if one doesn't exist already? lmk if anyone wants to access it (uses ember-cli on frontend) https://jwtdemo-frontend.herokuapp.com/login

@jpadilla
Copy link
Owner

jpadilla commented Dec 9, 2015

I honestly think that this would make a great third party package on its own. It's more ambitious than what this package was meant to be. If @ticosax or anyone wants to pick this up go right ahead. Also living apart will help with maintenance. Thanks to everyone who worked hard on this.

@jpadilla jpadilla closed this Dec 9, 2015
@ticosax
Copy link
Contributor Author

ticosax commented Dec 10, 2015

Fair enough.
I will post here the link to the new project once it is done, for the record.

@ticosax ticosax deleted the long_running_refresh_tokens branch January 28, 2016 14:42
@jpadilla
Copy link
Owner

@ticosax awesome!

@fxdgear
Copy link

fxdgear commented Jan 28, 2016

👍
On Thu, Jan 28, 2016 at 7:11 AM José Padilla notifications@github.com
wrote:

@ticosax https://github.com/ticosax awesome!


Reply to this email directly or view it on GitHub
#123 (comment)
.

@Darkman
Copy link

Darkman commented Jun 26, 2016

This project is pretty much the de facto standard when using JWTs with the Django rest framework. @jpadilla Can I ask why this was decided to be split into another project when refresh tokens seem to be a major part of using JWTs? Especially when most articles and stackoverflow answers recommend using them? I feel that this split will cause the new package to gain far less exposure, which is so critical with security applications.

Does anyone know if DRF is thinking about natively supporting JWTs?

@jammed343
Copy link

I agree with @Darkman

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

Successfully merging this pull request may close these issues.

None yet

8 participants