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 Refresh Tokens #327

Closed
wants to merge 16 commits into from
Closed

Add Refresh Tokens #327

wants to merge 16 commits into from

Conversation

dontexit
Copy link

  • Added refresh tokens that can be enabled or disabled from knox settings along with new settings for it.
  • Documentation for the changes added.
  • All the tests pass whether u enable refresh tokens or not.
  • Added new tests specific to refresh tokens which pass too.
  • All the code changes basically inherits from the existing code style / structure except there is some mess introduced by token tracking / rotations.

I just did all this because i wanted refresh tokens without reaching for Oauth, read their overview page and tried to implement what they have surrounding refresh tokens. I'm sure there's a lot that can be improved here.

Copy link

codecov bot commented Jan 24, 2024

Codecov Report

Attention: 32 lines in your changes are missing coverage. Please review.

Comparison is base (271179a) 91.70% compared to head (790a6b2) 89.05%.

❗ Current head 790a6b2 differs from pull request most recent head b9c809b. Consider uploading reports for the commit b9c809b to get more accurate results

Files Patch % Lines
knox/models.py 81.03% 11 Missing ⚠️
knox/views.py 90.56% 4 Missing and 6 partials ⚠️
knox/auth.py 81.57% 4 Missing and 3 partials ⚠️
knox/admin.py 0.00% 2 Missing ⚠️
knox/settings.py 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #327      +/-   ##
===========================================
- Coverage    91.70%   89.05%   -2.66%     
===========================================
  Files            9        9              
  Lines          229      411     +182     
  Branches        35       75      +40     
===========================================
+ Hits           210      366     +156     
- Misses          16       33      +17     
- Partials         3       12       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@johnraz
Copy link
Collaborator

johnraz commented Jan 25, 2024

Hey!
Thanks for the PR.
We already have an auto-refresh feature in knox that is basically a refresh token feature but without the hassle of calling a dedicated endpoint.
What is the use case you have that would justify adding an actual refresh token endpoint?
I am not against this per se but unless we have a clear argument in favor, I would refrain from adding this to the code base.

@dontexit
Copy link
Author

dontexit commented Jan 25, 2024

Hi.
Thanks for taking a look.

The current model of refreshing a token falls short if one didn't want a long-lived auth token or any token, for that matter.
I basically ended up here because my auth token kept expiring too much and I had to keep logging in but at the same time I did't want neither my auth token to expire too quick or for it to live too long.

Adding a new view for refresh token means:
Worst case:

  • The same refresh token remains valid for a set maximum time period
    (e.g., the default is 30 days, but auto-renewal, similar to auth tokens, is possible).
    This token can be used to get a new token and refresh_token pair, where again the new
    refresh token can have a long lifespan while the auth token has a short one.

Best case:

  • Both auth token and refresh_token live for 24hr (when set as the default) in the base case.
    Alternatively, auth token renewal can be enabled if we wanted them to stay alive for a longer
    period with increased usage. When the auth token, eventually expires, again we just use the
    refresh token to get new token pair.

I hope that answers the question but do let me know if it doesn't and you have extra concerns.

@johnraz
Copy link
Collaborator

johnraz commented Jan 27, 2024

I still don’t understand what is the issue.

The token expiry is basically a session timeout equivalent in Knox.

Calling any endpoint will make sure to auto refresh your token the same way calling a dedicated refresh token endpoint would.

If you want a short lived token but still avoid to have to relogin, just make sure to generate activity from the client side.

I don’t get the “no token” case you mention.

The case you describe is still too abstract for me to make sense out of it.

Also keep in mind that knox is meant to be a simple token solution mostly targeted at a machine to machine use case. The target is not to implement oauth all over again. People use knox for client to server communication while I think they should use plain cookie based session for that.

@johnraz
Copy link
Collaborator

johnraz commented Jan 27, 2024

As a reminder: https://auth0.com/learn/refresh-tokens

We don’t store information in the token so it never becomes stale hence doesn’t need to be replaced when it refreshes.

As for the security aspect, if you need that extra layer, you should probably turn to a proper oauth2 lib.

Again I think knox should remain simple, we already have a limited amount of maintainers.

Others might disagree.

@i701
Copy link

i701 commented Jan 27, 2024

This functionality is already there with the AUTO_REFRESH=True option.

@giovannicimolin
Copy link
Contributor

@dontexit First of all, thank you for your contribution! 😁

I agree with @johnraz's, given our limited maintaining capabilities, I'd rather not introduce extra mechanisms that add complexity to it for now. This is meant to be a simple library that is a step up to DRF's TokenAuthentication.

The use case you mentioned + the code changes you introduced are very similar to some OAuth2 authorization flows, so it may be indeed better for your project in the long run if you switch to an OAuth2 library.

Should we add a section to the docs where we do a when to use this library vs OAuth2?

dontexit and others added 5 commits January 27, 2024 14:06
* refresh_token cant be auto renewed
* RefreshTokenView inherits from LoginView and AuthenticationToken
* MAX_TOKEN_LIMIT to limit the number of refresh tokens to track
* tests for refresh_token expired signal and MAX_TOKEN_HISTORY
@dontexit
Copy link
Author

First of all, thanks for all the responses.

To clarify on the motivation, it would basically be the following:

While authentication token is openly accessible, refresh token can be stored on the client encrypted.
When a refresh is necessary, we can automate the process and add client-side validation mechanisms like CAPTCHA etc.
This would help avoid most of the problems related to token exposure.

So from what I've seen, the general consensus seems to be that adding extra auth mechanism is something off the table entirely? Or is it just the case that the purposed changes seem to add too much complexity for the scope of the module given the maintenance capacity etc? From my perspective, while the the first PR was a buggy mess and pretty much just
a way for me to test the waters, the new changes remove most of the complexity, as most of the behavior is just inherited from the existing stable mechanisms. I'd also argue the changes are pretty minimal too for what it offers but I can see how it could be seen as excessive and totally understand not wanting to take this direction, if that's the case I'm happy to close my PR.

As far as just using an OAuth library goes, the whole point of me not going there is that I don't need the whole
shebang OAuth offers and neither do I want to fix something if it works. Knox works just fine for me.
I suppose its already apparent that I'm using it for client-to-server communication, and as someone who started out abusing APIs, I value regular token invalidation a lot , as it has made my life difficult too many times and seems like a harmless addition.

@johnraz
Copy link
Collaborator

johnraz commented Jan 31, 2024

I think the issue you are trying to solve is that your token is exposed… it shouldn’t be.

The token should live in a secured cookie with flags http-only and secure set to true to be protected on the client side.

Every communication should be done over SSL so it’s always encrypted.

It shouldn’t be stored in the local storage or anything that javascript has access to.

If you are doing anything different than that you are doing it wrong.

You can’t store the refresh token on the client side in any more secure way either.

I said it before but I really highly recommend you rely on the session / cookie feature of DJRF / Django for client authentication, knox doesn’t bring any value there.

Feel free to elaborate on your use case as I’m willing to help 😉

@dontexit
Copy link
Author

Thanks for the advice! I appreciate your input. Just to clarify, I already wrap my auth token in an http-only cookie, so the token itself is protected on the client side. However, I still need refresh tokens to control the lifespan of the token. The main concern here is not necessarily external threats like token sniffing etc, but rather the possibility of users abusing the API. Even with the cookie, API resources can still be directly accessed, so it doesn't make a huge difference in my case.

Moreover, using refresh tokens, which are always issued through our client-side interactions, adds an additional layer of security. To obtain a new token, users must perform certain hard-to-automate actions through the client, increasing the difficulty of unauthorized/external access. In the worst-case scenario, users can only directly access the API resource every 24 hours until they complete manual actions, significantly reducing the risk vector. The goal is make sure all access happens through the client only.

dontexit and others added 3 commits January 31, 2024 19:08
    * Fix the bug when we were trying the handle the case where an expired
    refresh token wasn't the newest member of its family.
    * Added REFRESH_TOKEN_INTERVAL to control minimum timedelta between refresh_token issue.
@johnraz
Copy link
Collaborator

johnraz commented Jan 31, 2024

I think that this is a niche use case really, why don’t you just let your token expire and the user login again?

If your aim is security, this is far more secure and requires way less work than displaying a captcha prompt instead of a login/password prompt.

I think it would be more valuable to implement a passwordless login or something like that to increase the UX.

I think I stand on my initial thought that there is little value for the library to add such a change for now.

This revision ensures that the migration utilizes the token model from the KNOX_SETTING namespace in project settings.
@dontexit
Copy link
Author

dontexit commented Feb 2, 2024

Having to log in repeatedly is pretty inconvenient. Personally, I often struggle to remember most of my passwords and dislike having to reset them every time I need to log in. I don't even remember my Gmail password. Even with Discord, I rely on the "add device" feature (QR code) from my phone, where I've been logged in forever without ever needing to log in again. I assume Discord isn't persisting the same token (like Knox does now) all this time (I've been inactive for a lot of time in-between); they're probably refreshing the tokens every time I'm active there. Regarding passwordless logins, refresh tokens are effectively just that, and using them with captchas would be an extreme example. I hate failing to explain the usefulness of refresh tokens, but it's understandable not wanting to add them, if I were you and never stumbled upon their usefulness.

@johnraz
Copy link
Collaborator

johnraz commented May 26, 2024

I am going to close this PR, thanks again for the contrib @dontexit

I do think you can still use knox and customize the behavior in your own project or maybe create a dedicated package to achieve this.

If you are facing blocking issues to extend knox in a separate package, feel free to open another PR and I'll be more than welcome to help get that merged 😄

@johnraz johnraz closed this May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants