The override of ModelBackend's get_user() function allows inactivated users to continue accessing the site until their session expires

[markbaird]

While testing some user account lock scenarios in an application I work on, I noticed that when I deactivate a user account that has a current session that was established with a username/password login, they immediately lose all access to the website and are sent back to the login page on the next request they try to make. However, when I deactivate a user that has an active session established through an OIDC login they retain access to the website until their session times out. They will get permission errors when accessing any Django views that check for specific permissions, but views that just specify @login_required are still accessible.

I found that this library overrides Django's ModelBackend.get_user() function, defined here, with an implementation that is less secure. Ever since this commit back in 2016, Django's ModelBackend has checked that the user account is active before returning the user:

    def get_user(self, user_id):
        try:
            user = UserModel._default_manager.get(pk=user_id)
        except UserModel.DoesNotExist:
            return None
        return user if self.user_can_authenticate(user) else None

Where the user_can_authenticate() function checks if user.is_active is True.

Contrast that to the current implementation in mozilla-django-oidc:

    def get_user(self, user_id):
        """Return a user based on the id."""

        try:
            return self.UserModel.objects.get(pk=user_id)
        except self.UserModel.DoesNotExist:
            return None

Notice how it simply returns the user account if it exists, without checking if the account is currently active.

To fix this behavior in my application, I've extended OIDCAuthenticationBackend to override get_user() with an implementation that checks the is_active field again. However, I believe this is a security issue that should be fixed in the mozilla-django-oidc library itself. I believe anyone deactivating a user account in their Django app would want that user account to be locked out of the app as quickly as possible, not after some potentially long session timeout.

I started to submit a pull request to fix this, but I wonder if there may be some scenarios where you have OIDC logins automatically create accounts, where you might want the current behavior of not checking if the account is active? If that's the case, then perhaps this needs to be something controlled by a setting, like OIDC_CHECK_ACCOUNT_ACTIVE. Or perhaps the is_active field should only be checked if OIDC_CREATE_USER is False? In any case, the override of the default Django user account deactivation behavior here is unexpected, and at the very least should be explained in the documentation.

If this behavior was unintended, then I think the get_user() function override should just be deleted from the mozilla-django-oidc source, so that it goes back to the default Django behavior of checking if the user is active. Users of this library would still be able to override the function for their own purposes if needed.