fixes #156 -- provide token refresh endpoint #158

sphrak · 2019-01-14T15:02:40Z

Hi,
First draft of whats discussed in #156, I ended up going for a simpler approach than integrating drf throttling due to the points laid out by @johnraz. However as suggested by #157 we should prolly take a look at that aswell since at least I would expect drf throttling to also apply to our views. (different issue tho).

As discussed these endpoints are optional, and they are not exposed by default unless TOKEN_TTL and AUTO_REFRESH are set. Where AUTO_REFRESH is False by default, thus off.

Let me know what you think

knox/views.py

sphrak · 2019-01-14T15:55:43Z

knox/urls.py

+        name='knox_refresh_all'
+    ),
+]
+if knox_settings.TOKEN_TTL and knox_settings.AUTO_REFRESH:


Also check if DISABLE_REFRESH_ENDPOINT is set to true?

I guess my comments above answers this question and my opinion on this.

hm I feel there is no real gain of this flexible url setup, if I miss something let me know. We make our lifes as devs harder in terms of testing etc but do we decrease security or anything if we have this url "avaiable" despite not allowing refreshes? The view could simply return a 401.

@belugame I agree, lets do that instead. Lets return 403 as per:

This status is similar to 401, but in this case, re-authenticating will make no difference. The access is permanently forbidden and tied to the application logic, such as insufficient rights to a resource.

Agreed with the above + 403 👍

johnraz

Again, massive work here 👍
I do have a few comments regarding the design but nothing major I believe.
Also I skipped reviewing the tests for now as I wanted to focus on the design first.

docs/urls.md

johnraz · 2019-01-14T19:14:54Z

docs/views.md

+Optional view for refreshing the expiration date of a single token.
+It accepts an empty `POST` request against `/refresh` to increase the `expiry` field of the
+token by the amount set in `TOKEN_TTL`.
+This endpoint is only exposed if `AUTO_REFRESH` and `TOKEN_TTL` are enabled. 


I would argue that the endpoint would still be useful if one want's to disable AUTO_REFRESH but still want to be able to manually refresh a token.

I see the point of not enabling the endpoint if TOKEN_TTL is None as no token would ever expire.
So I would rephrase this a bit and make it clear that we clearly specify that it's only disabled when TOKEN_TTL is None.

Also see my comment below about this feature.

johnraz · 2019-01-14T19:20:04Z

docs/views.md

+token by the amount set in `TOKEN_TTL`.
+This endpoint is only exposed if `AUTO_REFRESH` and `TOKEN_TTL` are enabled. 
+
+The functionality can be disabled entirely by setting `DISABLE_REFRESH_ENDPOINT=True` in `REST_KNOX` settings.


We could get rid of the logic explained above by just having a ENABLE_REFRESH_ENDPOINT setting.
It would be easier to explain and would also make the code simpler.

The explanation would boil down to:

If you want the refresh endpoint, just opt-in by setting ENABLE_REFRESH_ENDPOINT to True.

Simple, straight, obvious, no magic.

johnraz · 2019-01-14T19:20:59Z

knox/urls.py

+        name='knox_refresh_all'
+    ),
+]
+if knox_settings.TOKEN_TTL and knox_settings.AUTO_REFRESH:


I guess my comments above answers this question and my opinion on this.

johnraz · 2019-01-14T19:28:43Z

knox/views.py

+    permission_classes = (IsAuthenticated,)
+
+    def post(self, request, format=None):
+        return Response(


We should do an explicit update on the expiry here:

If we don't, in the context of my previous comments, it will not work for people with AUTO_REFRESH=False and wanting to manually refresh.

this view should bypass MIN_REFRESH_INTERVAL as it is an explicit request for a refresh and not an implicit one triggered by AUTO_REFRESH.

Ideally, we should find a way to disable the behavior of AUTO_REFRESH in the authentication class when hitting this endpoint.

@johnraz I agree with you, ill go back to the drawing board a bit to try to make this more explicity and flexible.

@johnraz I am not sure how to circumvent the auto refresh in auth without making it messy. I mean we could explicitly do it in the refresh view if AUTO_REFRESH is not set, but if we do that it will not bypass MIN_REFRESH_INTERVAL if the user happen to be using AUTO_REFRESH also.. which is kinda unexpected behaivor if you ask me..

So the auto-refresh flow today is:

hit a view with a token --> if MIN_REFRESH_INTERVAL allows it, update expiry and hit the DB with a write.

hit a view with a token --> if MIN_REFRESH_INTERVAL denies it, don't update expiry and don't write to the DB.

What we'd ideally want for the refresh view (whether or not auto-refresh is active) is:

hit the refresh view with a token --> refresh the token in every case by doing a single write to the DB

If auto-refresh is off, there is no issue.

If auto-refresh is on, hitting the refresh view will do this:

if MIN_REFRESH_INTERVAL is expired, the auto-refresh will update the expiry and write to the DB at authentication time, next the view will do it again, resulting in 2 writes for nothing.

if MIN_REFRESH_INTERVAL is not expired, only the view will write to the DB, 1 write everybody is happy.

The problem we need to solve is to avoid to have 2 consecutive useless writes to the DB...
Sadly I don't think we have access to the view or anything useful to identify the refresh view in the authenticate method (where AUTO_REFRESH happens)...

I would say, let the 2 DB writes be and add a note in the doc that if you use AUTO_REFRESH and the refresh view together, a double write could happen.

Taking the above statement back.
The view can be accessed in the authenticate method by doing:

request.parser_context['view']

This means we could simply add a property to the class like:

class RefreshView(): AUTO_REFRESH=False

And then check for that in the authenticate / authenticate_credentials method of the TokenAuthentication to disable auto-refresh "per view"

We can then advertise that as a sub-feature of the auto-refresh feature in the doc, if you want to enable/disable on a per view basis, use the property.

@johnraz, wonderful research, thank you. I have been experimenting with this approach, but it seems to be not as trivial as it initially seemed.

If we reference AUTO_REFRESH value in the authenticate/_credentials() methods it needs to be accessible on every view which kinda sucks because then we need to add it to each view or do some weird complex thing of trying to validate who or which part of the code is calling us -- which I dont think we should do since this is a library.

I dont like that so maybe we should make a KnoxAPIView class that extends from APIView with the only difference this attribute AUTO_REFRESH is added as a field attribute like auto_refresh or so.

But I am not sure if its a good approach, though I would like to be able to do away with things like if knox_settings.TOKEN_TTL and only have if token_ttl or auto_refresh directly as an attribute in the class. Maybe im overthinking, but thats the current status. I will ponder this some more.

If we reference AUTO_REFRESH value in the authenticate/_credentials() methods it needs to be accessible on every view

I think we could solve this with something like:

view = request.parser_context['view'] if getattr(view, AUTO_REFRESH, knox_settings.AUTO_REFRESH): self.renew_token(auth_token)

instead of the current:
https://github.com/James1345/django-rest-knox/blob/c1972fcddf86179680522d6af1b463b02c49b00c/knox/auth.py#L77-L78

That way the order of precedence would be:
Class attribute's value first and next settings value, which is what we want.

edit: I fixed the code sample above.

Thats a very nice solution, I like it. This PR does add some complexity to testing, I think I will have to mock some parts which tests the TokenAuthentication class directly because in that context there wont be any view. Is it okay if I mock it? no tests currently employs mocking.

edit: turns out its not needed, nvm the mocking stuff.

Cool! Glad you made it work 👍

knox/views.py

johnraz · 2019-01-14T19:49:09Z

Oh and regarding throttling, I think it's a picky subject.
I like the idea of making it easier on people to enable it but at the same time it can be easily broken.
eg: by a bad proxy configuration.
We need to make sure we think it through and document it properly, I suggest we get to the bottom of #157 and then move on and decide based on the outcome.

sphrak

@johnraz I have addressed the suggestions you made and it turned out pretty neat I think. There is still a test missing for proving that there are in fact no double db writes but I am working on it. Other than that I think its mostly ready.

Cheers

sphrak · 2019-01-17T09:23:44Z

knox/auth.py

@@ -37,6 +37,8 @@ class TokenAuthentication(BaseAuthentication):
    model = AuthToken

    def authenticate(self, request):
+
+        view = getattr(request, 'parser_context', None)


We either do this, or mock -- because otherwise tests will fail when we test the TokenAuthentication directly where there is no actual view calling it.

I'm against making exception in the library code to make the tests happy.
A non complete request passed here should raise and not be swallowed.

I don't believe you'd need mock here, changing the way we do the request should be enough.

Also shouldn't this be:

view = getattr(request, 'parser_context', None)["view"]

so without the workaround it should be:
view = request.parser_context["view"]

Instructions a tad unclear, would you like me to rewrite those tests?

I think the tests shouldn't be failing yes and I guess this is because we use request factory with too few arguments.

knox/views.py

johnraz

We are getting there, I still have a few comments though. Keep it up 👍

docs/settings.md

docs/views.md

johnraz · 2019-01-17T16:41:47Z

knox/auth.py

@@ -37,6 +37,8 @@ class TokenAuthentication(BaseAuthentication):
    model = AuthToken

    def authenticate(self, request):
+
+        view = getattr(request, 'parser_context', None)


I'm against making exception in the library code to make the tests happy.
A non complete request passed here should raise and not be swallowed.

I don't believe you'd need mock here, changing the way we do the request should be enough.

Also shouldn't this be:

view = getattr(request, 'parser_context', None)["view"]

so without the workaround it should be:
view = request.parser_context["view"]

knox/auth.py

johnraz · 2019-01-17T16:56:36Z

knox/urls.py

-    url(r'logoutall/', views.LogoutAllView.as_view(), name='knox_logoutall'),
+    url(r'logout/$', views.LogoutView.as_view(), name='knox_logout'),
+    url(r'logout/all/$', views.LogoutAllView.as_view(), name='knox_logout_all'),
+    url(r'refresh/$', views.TokenRefreshView.as_view(), name='knox_refresh')


I'm not sure about the refresh... I would like token/refresh better I think.
@belugame thoughts?

hm for me depends on the outcome of our discussion :) creating a new one for me would not be a "refresh". If we keep the same token I would prefer the verb "extend". "extend" I feel would fit nicely in with the others logout, login

Makes sense 👍

johnraz · 2019-01-17T17:12:24Z

knox/views.py

+        if knox_settings.TOKEN_TTL is None:
+            raise ValueError(
+                'Value of \'TOKEN_TTL\' cannot be \'None\' in this context.'
+            )


This will trigger a 500, I think we should make a note about the error code in the doc.

Also I would rather make this check at application loading time so it blows when you try to run the server and not when someone tries to hit the view... I'm not sure how to achieve that easily.

I'm not going to block if it stays as it is with a note in the doc though.

The error is documented here: https://github.com/James1345/django-rest-knox/pull/158/files#diff-1819b1daaccb3d358620ade9c67e9118R66

But I will add a note saying it will cause a HTTP 500 if improperly configured. But I agree, I'd rather have it fail immediatly at runtime rather than when a user tries to refresh, but am unaware of how.

Works for me, we can investigate settings checking at runtime later on.

tests/tests.py

johnraz · 2019-01-18T15:02:59Z

@sphrak to fix the current conflict, just remove docs/changes 😉

belugame · 2019-03-27T08:13:05Z

@sphrak @johnraz Vetos against releasing current master? I want to get your work out of the door, as things seem to be stuck at the moment. We will never run out of version numbers :)

sphrak · 2019-03-27T10:22:58Z

@belugame I say go for it, release it. I unfortunatly won't have time for a couple more months to work on the stuck #156 issue right now. I will make a comment at least if anyone else want to work on it in the meantime.

johnraz · 2019-03-27T11:55:32Z

Go ahead yes 👍 😊

belugame · 2019-03-27T12:36:50Z

4.0.0 is on pypi
https://pypi.org/project/django-rest-knox/4.0.0/

sphrak commented Jan 14, 2019

View reviewed changes

knox/views.py Outdated Show resolved Hide resolved

sphrak commented Jan 14, 2019

View reviewed changes

knox/views.py Show resolved Hide resolved

sphrak commented Jan 14, 2019

View reviewed changes

johnraz requested changes Jan 14, 2019

View reviewed changes

sphrak commented Jan 17, 2019

View reviewed changes

johnraz requested changes Jan 17, 2019

View reviewed changes

fixes #156 -- provide token refresh endpoint

0c886ec

This was referenced Feb 10, 2019

release 4.0 #168

Merged

[discussion] Provide endpoint for refreshing token expiry #156

Open

sphrak closed this Dec 25, 2022

fixes #156 -- provide token refresh endpoint #158

fixes #156 -- provide token refresh endpoint #158

Conversation

sphrak commented Jan 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnraz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnraz Jan 15, 2019 • edited Loading

Choose a reason for hiding this comment

johnraz Jan 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnraz Jan 16, 2019 • edited Loading

Choose a reason for hiding this comment

sphrak Jan 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnraz commented Jan 14, 2019

sphrak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnraz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnraz commented Jan 18, 2019

belugame commented Mar 27, 2019

sphrak commented Mar 27, 2019

johnraz commented Mar 27, 2019

belugame commented Mar 27, 2019

sphrak commented Jan 14, 2019 •

edited

Loading

johnraz Jan 15, 2019 •

edited

Loading

johnraz Jan 15, 2019 •

edited

Loading

johnraz Jan 16, 2019 •

edited

Loading

sphrak Jan 17, 2019 •

edited

Loading