-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
20201118 token expiration #478
Conversation
16fb49c
to
ed8a45c
Compare
be53861
to
e72f026
Compare
20dfe7a
to
27368aa
Compare
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.
lgtm, one usability comment:
I find it unintuitive that in the webapp token table, when I click the "Can manage tokens" slider, I still have to save that row. From my experience with Android, I'd assume it saves automatically/instantly. The background color change didn't help much in my case, as I only had two rows: the login token, dark grey becase disabled, and the current row. The current row was white to begin with, then light grey on hover, then later yellow because of unsaved changes - which I didn't notice. Two ways to improve this could be (in order of my preference): 1. introduce an indicator of unsaved changes on top of the table where the blue bar is currently (preferably in a way that doesn't move the page when it appears/disappears). 2. hide the save button of each row unless there are changes to save 3. auto-save the changes on the slider 4. popup-warning when you leave the page with unsaved changes
As those are more general concerns with how the webapp tables work, we can do this as part of this PR or later, in which case I'd like to ask you to create an issue for it :)
raise exceptions.AuthenticationFailed('Invalid token.') | ||
|
||
token.last_used = timezone.now() | ||
token.save() |
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.
Could this result in concurrency exceptions for parallel requests using the same token?
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.
This is not behavior introduced by this PR. Still, question is still worth thinking about it
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 don't think so. First, I believe that this does not happen inside a transaction; second, even if it were, one requests would simply delay the other, but not raise an exception (if I got it right).
I opened the issue regarding the usability comment (#485). |
27368aa
to
52f5b7b
Compare
This PR is based on work from another branch that is reviewed in #474. Only the last 5 commits are relevant here.