#25232 closed New feature (fixed)
Make the ModelBackend/RemoteUser authentication backends reject inactive users
Reported by: | Ole Laursen | Owned by: | Sasha Gaevsky |
---|---|---|---|
Component: | contrib.auth | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | lau@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
I just got a bug report that inactive users could still access a site I'm maintaining. It turns out that is_active doesn't really deactivate people, it just prevents them from logging in again.
This was discussed in 2008:
https://groups.google.com/forum/#!topic/django-developers/P0b0g0sr-b8
I think the short version is that this happened by accident (login view checks is_active, so does permissions, but auth backend doesn't) but discovered late enough that Malcolm Tredinnick didn't want to break backwards compatibility.
This leaves no proper built-in way to deactivate users, a useful feature. Hence, I humbly suggest that we add a setting ala PREVENT_INACTIVE_USERS_FROM_BEING_AUTHENTICATED? It would default to None, meaning leave the current semi-broken behaviour, but you could set it to True to have the ModelBackend do a check on is_active in get_user:
https://github.com/django/django/blob/master/django/contrib/auth/backends.py#L90
Perhaps it could also be set to False to prevent the login view and permissions from checking is_active, in case anyone finds that useful.
If people like the setting, it could perhaps in the future default to True.
Attachments (1)
Change History (16)
comment:1 by , 9 years ago
comment:2 by , 9 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
Summary: | Deactivating users with is_active → Add a setting to make the ModelBackend reject inactive users |
Type: | Uncategorized → New feature |
It seems to me this could be accomplished without much difficulty by overriding the ModelBackend
. Something like (untested):
class RejectInactiveUsersBackend(ModelBackend): def get_user(self, user_id): user = super(RejectInactiveUsersBackend, self).get_user(user_id) if user and not user.is_active: return None return user
It's probably better to recommend that route instead of adding a setting.
comment:3 by , 9 years ago
Resolution: | wontfix |
---|---|
Status: | closed → new |
The manual workaround is to change a user's password, assuming SessionAuthenticationMiddleware
is installed... That said I'm not comfortable with suggesting workarounds. The current situation could easily be described as a security issue.
I don't think adding a setting is a solution because using the default value will leave sites vulnerable. I think we should fix the bug, document the backwards-incompatibility and provide a way to restore the previous behavior.
I'm going to reopen the bug in the hope to gather more feedback. If no one thinks fixing is a good idea, we can close it again.
comment:4 by , 9 years ago
I agree with Aymeric that it's just a bug if the backend behavior doesn't match the login form behavior. Both are easily overrideable.
comment:5 by , 9 years ago
Summary: | Add a setting to make the ModelBackend reject inactive users → Make the ModelBackend authentication backend reject inactive users |
---|---|
Version: | 1.8 → master |
It might also be possible to fix #24987 and remove the user.is_active
check in the test client login()
method. A draft patch is attached, but some test failures remain.
For backwards compatibility, should we provide an authentication backend that allows inactive users:
class AllowInactiveUsersModelBackend(ModelBackend): allow_inactive_users = True
(incorporating that flag into the patch).
by , 9 years ago
Attachment: | 25232.diff added |
---|
comment:6 by , 9 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:7 by , 9 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
I've started with the PR
comment:10 by , 9 years ago
Patch needs improvement: | set |
---|
I think RemoteUserBackend
should have the same behavior as ModelBackend
and respect the proposed user_can_authenticate()
method.
comment:11 by , 9 years ago
Patch needs improvement: | unset |
---|---|
Summary: | Make the ModelBackend authentication backend reject inactive users → Make the ModelBackend/RemoteUser authentication backends reject inactive users |
I updated the PR for the above comment. The second commit there should be good to go but if someone could double check the first that would be great.
It would be better to first write to the DevelopersMailingList to get a consensus on the design issues. New settings are to be avoided if possible.