feat: log warning if token is being created for inactive user#873
feat: log warning if token is being created for inactive user#873vgrozdanic merged 2 commits intomasterfrom
Conversation
rest_framework_simplejwt/tokens.py
Outdated
| after authenticating the user's credentials. | ||
| """ | ||
|
|
||
| if not user.is_active: |
There was a problem hiding this comment.
| if not user.is_active: | |
| if hasattr(user, "is_active") and not user.is_active: |
There was a problem hiding this comment.
| if not user.is_active: | |
| if not getattr(user, "is_active", True): |
how about this code
There was a problem hiding this comment.
Yep, it was naive of me to assume is_active is always present in any user model...
@2ykwang thank you for the suggestion, i agree that it is shorter, but (in my personal opinion) it makes it harder for future maintainer to understand what this code is doing. The code that Andrew proposed, to me personally, is easier to read :)
|
|
||
| format_lazy: Callable = lazy(format_lazy, str) | ||
|
|
||
| logger = logging.getLogger("rest_framework_simplejwt") |
There was a problem hiding this comment.
hm, if we use this logger, won't all the logs come from rest_framework_simplejwt.utils?
Idk how other libraries do it; maybe they just do logging.getLogger(__name__) per file?
There was a problem hiding this comment.
I think the name will always be "rest_framework_simplejwt" - i have seen this pattern in Django in multiple places, that's why i have chosen it :)
There was a problem hiding this comment.
I see. Django does this on a per-"module" (or like package?) naming scheme. Since this package is smaller, wouldn't it make more sense to do this on a per-file basis with __name__?
There was a problem hiding this comment.
I have no strong opinions, but i am not sure if we do it per-file basis, how would settings look like, if someone wants to filter out this logs. I am not very familiar with logging module, but i can take a deeper look next week into this.
My main concern is how would someone for example filer out all but critical logs from this package. I think using only __name__ will only include the file/module name, and leave out rest_framework_simplejwt
There must be a way, i am just not sure what is the way to do it
There was a problem hiding this comment.
My main concern is how would someone for example filer out all but critical logs from this package. I think using only name will only include the file/module name, and leave out rest_framework_simplejwt
That's a great point. I think we can merge this then
3 participants
As per discussion, this PR adds a warning if a developer tries to create the token for the non-active user.
Part of #779