-
-
Notifications
You must be signed in to change notification settings - Fork 211
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
fixes #156 -- provide token refresh endpoint #158
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,3 +79,30 @@ def post(self, request, format=None): | |
user_logged_out.send(sender=request.user.__class__, | ||
request=request, user=request.user) | ||
return Response(None, status=status.HTTP_204_NO_CONTENT) | ||
|
||
|
||
class TokenRefreshView(APIView): | ||
|
||
authentication_classes = (TokenAuthentication,) | ||
permission_classes = (IsAuthenticated,) | ||
auto_refresh = False | ||
|
||
def post(self, request, format=None): | ||
|
||
if not knox_settings.ENABLE_REFRESH_ENDPOINT: | ||
return Response( | ||
status=status.HTTP_403_FORBIDDEN | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe adding a message here would be nice:
Not sure about the data returned's format though... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure I agree -- I dont think configuration specific messages for the library should be exposed to the users here, no value to enduser imo. Are you sure? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do agree that exposing config related stuff is not a good practice. |
||
) | ||
|
||
if knox_settings.TOKEN_TTL is None: | ||
raise ValueError( | ||
'Value of \'TOKEN_TTL\' cannot be \'None\' in this context.' | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Works for me, we can investigate settings checking at runtime later on. |
||
|
||
request.auth.expiry = timezone.now() + knox_settings.TOKEN_TTL | ||
request.auth.save() | ||
|
||
return Response( | ||
sphrak marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{'expiry': request.auth.expiry}, | ||
status=status.HTTP_200_OK | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the
refresh
... I would liketoken/refresh
better I think.@belugame thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense 👍