jazzband / django-rest-knox

Authentication Module for django rest auth
MIT License
1.17k stars 213 forks source link

Add Refresh Tokens #327

Closed dontexit closed 6 months ago

dontexit commented 10 months ago

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.

codecov[bot] commented 10 months ago

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%.

:exclamation: 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 :warning:
knox/views.py 90.56% 4 Missing and 6 partials :warning:
knox/auth.py 81.57% 4 Missing and 3 partials :warning:
knox/admin.py 0.00% 2 Missing :warning:
knox/settings.py 50.00% 1 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

johnraz commented 10 months ago

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 commented 10 months ago

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:

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

johnraz commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

giovannicimolin commented 10 months ago

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

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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.

johnraz commented 10 months ago

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.

dontexit commented 10 months ago

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 commented 6 months ago

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 😄