Skip to content

update_last_login on TokenObtainPair #238

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

Merged
merged 9 commits into from
Sep 7, 2020

Conversation

mjlabe
Copy link
Contributor

@mjlabe mjlabe commented Apr 14, 2020

Fix for last login update #132

Alternative solution to Fix user last_login field update #136 that only updates when the user gets a new refresh token (i.e. login). This should result in less database hits.

@Andrew-Chen-Wang
Copy link
Member

Database access and updating is only one-half the problem. People abusing the views is the other half of the problem. Please stop trying to add last_login. ref #234 You’ll inadvertantly slow your server down by exposing a security vulnerability. You can make a PR for signals instead, but it seems like another PR has already done it. Not sure when or if it’ll get merged.

@Andrew-Chen-Wang
Copy link
Member

If you really want this, make a new viewset and throttle it with DRF at the very least. If we throttle the viewset ourselves, we make the package less flexible.

@Andrew-Chen-Wang
Copy link
Member

Andrew-Chen-Wang commented Aug 14, 2020

@mjlabe I just talked with David. We'd like to make this repo a little more flexible in terms of features. If you don't mind, can you add a setting to the SIMPLE_JWT settings for last_login? Have the default still be on False. Thanks!

And then you'll need to add a test case to make sure the last_login actually happens.

@mjlabe
Copy link
Contributor Author

mjlabe commented Aug 14, 2020

Will do. I'll also add your warnings of potential vulnerabilities to the docs.

@Andrew-Chen-Wang Andrew-Chen-Wang added this to the 5.0.0 milestone Aug 14, 2020
@mjlabe
Copy link
Contributor Author

mjlabe commented Sep 2, 2020

@Andrew-Chen-Wang @davesque

PR has been updated with requested changes. Please let me know if you feel anything should be changed!

Copy link
Member

@Andrew-Chen-Wang Andrew-Chen-Wang left a comment

Choose a reason for hiding this comment

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

@mjlabe Just saw the new commit. Taking a look at it now; to me it's great! Gonna review it one last time in an hour, but I'll be merging it soon. Thanks for the PR!

Copy link
Member

@Andrew-Chen-Wang Andrew-Chen-Wang left a comment

Choose a reason for hiding this comment

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

Use timezone.now() rather than TZ naive datetime. I can commit these suggestions and make sure they good to go.

@Andrew-Chen-Wang Andrew-Chen-Wang merged commit 41f4daf into jazzband:master Sep 7, 2020
@Andrew-Chen-Wang
Copy link
Member

Thanks again @mjlabe!

@mjlabe mjlabe deleted the update_last_login branch May 27, 2022 20:21
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.

None yet

3 participants