Opened 9 years ago

Closed 9 years ago

Last modified 2 years ago

#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)

25232.diff (2.4 KB ) - added by Tim Graham 9 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 by Tim Graham, 9 years ago

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.

comment:2 by Tim Graham, 9 years ago

Resolution: wontfix
Status: newclosed
Summary: Deactivating users with is_activeAdd a setting to make the ModelBackend reject inactive users
Type: UncategorizedNew 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 Aymeric Augustin, 9 years ago

Resolution: wontfix
Status: closednew

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 Carl Meyer, 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 Tim Graham, 9 years ago

Summary: Add a setting to make the ModelBackend reject inactive usersMake the ModelBackend authentication backend reject inactive users
Version: 1.8master

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 Tim Graham, 9 years ago

Attachment: 25232.diff added

comment:6 by Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted

comment:7 by Sasha Gaevsky, 9 years ago

Has patch: set
Owner: changed from nobody to Sasha Gaevsky
Status: newassigned

I've started with the PR

comment:8 by Tim Graham, 9 years ago

Patch needs improvement: set

Left comments for improvement.

comment:9 by Tim Graham, 9 years ago

Patch needs improvement: unset

comment:10 by Tim Graham, 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 Tim Graham, 9 years ago

Patch needs improvement: unset
Summary: Make the ModelBackend authentication backend reject inactive usersMake 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.

comment:12 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: assignedclosed

In e0a3d937:

Fixed #25232 -- Made ModelBackend/RemoteUserBackend reject inactive users.

comment:13 by Tim Graham <timograham@…>, 8 years ago

In e69091b:

Refs #25232 -- Documented AllowAll*Backend in "new features" section of 1.10 release notes.

comment:14 by Tim Graham <timograham@…>, 8 years ago

In 738a65a5:

[1.10.x] Refs #25232 -- Documented AllowAll*Backend in "new features" section of 1.10 release notes.

Backport of e69091b34a34697fe7eac38763dd372b305e1ab4 from master

comment:15 by GitHub <noreply@…>, 2 years ago

In 282d58e:

Refs #25232 -- Simplified ModelBackend.user_can_authenticate().

Thanks Jay Turner for the suggestion.

Note: See TracTickets for help on using tickets.
Back to Top