Make the ModelBackend/RemoteUser authentication backends reject inactive users

[Ole Laursen]

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.

[Tim Graham]

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.

[Tim Graham]

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.

[Aymeric Augustin]

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.

[Carl Meyer]

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.

[Tim Graham]

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

[Sasha Gaevsky]

I've started with the ​PR

[Tim Graham]

Left comments for improvement.

[Tim Graham]

​Updated PR

[Tim Graham]

I think RemoteUserBackend should have the same behavior as ModelBackend and respect the proposed user_can_authenticate() method.

[Tim Graham]

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.

[Tim Graham <timograham@…>]

In e0a3d937:

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

[Tim Graham <timograham@…>]

In e69091b:

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

[Tim Graham <timograham@…>]

In 738a65a5:

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

Backport of e69091b34a34697fe7eac38763dd372b305e1ab4 from master

[GitHub <noreply@…>]

In 282d58e:

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

Thanks Jay Turner for the suggestion.