-
-
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
Count non expiring tokens when determining if the limit is reached #318
base: develop
Are you sure you want to change the base?
Conversation
Fixes jazzband#280 Thanks to @pablomm for the code in the ticket
@morty @pablomm Thanks for the contribution! That's a nice catch! I have one concern with this change though. Once we start considering non-expiring tokens, it is possible that users end up getting locked out of their accounts after issuing a certain number of tokens without logging out properly. We either need this behind settings flag or a way for new log-ins to remove the oldest unused token. |
HI @giovannicimolin and thanks to @morty for turning the fix into a pull request. In my case, I use these non-expiring tokens for users that uses my platform through an API. They have access to a control panel (which they access with another authentication system) where they can create, view and delete their api tokens (knox tokens which may be non-expiring). So logging out does not make sense in my use case. The problem you comment, wouldn't it also happen if someone creates too many tokens within the expiration time? |
To get to this situation two settings ( My client is using this with a very small number of end users where they could handle support requests to delete tokens if required. I could see this wouldn't be possible where there are a very large number of end users. If |
@morty @giovannicimolin I believe the change made here makes full sense, this is clearly a bug that needs to be fixed. I would add a note to the documentation (and probably a comment in the code) that clearly explains the consequence of having dangling non-expiring tokens around. Using non-expiring tokens should be also discouraged as it's a bad practice anyway. |
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.
@morty Sorry for the tremendous delay on this. This contribution is still valuable, so I'll move forward with merging it and releasing the changes.
Thanks a lot for your effort on this!
👍
Looks good, and works as expected.
Fixes #280
Thanks to @pablomm for the code in the ticket