AuthenticationForm's inactive user error isn't raised when using ModelBackend

[Guilherme Junqueira]

Looking in file django.contrib.auth.forms

The class AuthenticationForm and clean method.

The inactive user never is raised, this happens because after Django 1.10 all users that is not active cannot authenticate, so self.user_chache is always be None for inactive users, even if has a correct user and pass.

So the code needed to be changed to raise the correct error for a user that is not active.

My stackoverflow thread about this:
​https://stackoverflow.com/questions/46459258/how-to-inform-a-user-that-he-is-not-active-in-django-login-view/46459998#46459998

[Tim Graham]

Regression in e0a3d937309a82b8beea8f41b17d8b6298da2a86 (#25232).

[shangdahao]

​PR

[Tim Graham <timograham@…>]

In 359370a:

Fixed #28645 -- Reallowed AuthenticationForm to raise the inactive user error when using ModelBackend.

Regression in e0a3d937309a82b8beea8f41b17d8b6298da2a86.

Thanks Guilherme Junqueira for the report and Tim Graham for the review.

[Tim Graham <timograham@…>]

In 36dd0126:

[2.0.x] Fixed #28645 -- Reallowed AuthenticationForm to raise the inactive user error when using ModelBackend.

Regression in e0a3d937309a82b8beea8f41b17d8b6298da2a86.

Thanks Guilherme Junqueira for the report and Tim Graham for the review.

Backport of 359370a8b8ca0efe99b1d4630b291ec060b69225 from master

[Tim Graham <timograham@…>]

In 308f644:

[1.11.x] Fixed #28645 -- Reallowed AuthenticationForm to raise the inactive user error when using ModelBackend.

Regression in e0a3d937309a82b8beea8f41b17d8b6298da2a86.

Thanks Guilherme Junqueira for the report and Tim Graham for the review.

Backport of 359370a8b8ca0efe99b1d4630b291ec060b69225 from master

[Tim Graham <timograham@…>]

In af33fb25:

Fixed CVE-2018-6188 -- Fixed information leakage in AuthenticationForm.

Reverted 359370a8b8ca0efe99b1d4630b291ec060b69225 (refs #28645).

This is a security fix.

[Tim Graham <timograham@…>]

In c37bb286:

[2.0.x] Fixed CVE-2018-6188 -- Fixed information leakage in AuthenticationForm.

Reverted 359370a8b8ca0efe99b1d4630b291ec060b69225 (refs #28645).

This is a security fix.

[Tim Graham <timograham@…>]

In 57b95fed:

[1.11.x] Fixed CVE-2018-6188 -- Fixed information leakage in AuthenticationForm.

Reverted 359370a8b8ca0efe99b1d4630b291ec060b69225 (refs #28645).

This is a security fix.

[Tim Graham]

Reopening since the fix had to be reverted. We'll try to develop a solution for Django 2.1. Probably the solution will be too invasive to backport to the stable branches.

In a mail to the security mailing list, Jack Cushman suggested:

It's desirable for auth backends to enforce rules like “no inactive users” when supplied with otherwise-correct credentials – that’s more of a backend concern than a display concern, and forms shouldn’t be required to enforce it. But it is desirable for auth forms to show custom error messages when an auth backend rejects a user, if and only if the user supplied correct credentials. The ideal way to solve both problems would be for auth backends to return a tuple of (user or None, custom_error_code or None), but that would break backwards compatibility.

So, can we let auth backends return custom error codes with backwards compatibility?

Attached is an untested patch that hopefully does that, by adding an authenticate_with_error_code method that backends can optionally implement and forms can optionally consume. I think this is a good angle on the problem – it cleans up the can of worms with displaying custom errors, and also totally avoids dealing with attacker-submitted data after credentials fail to validate, which is key to avoiding any subtle security issues.

I'll attach the patch, but I haven't evaluated it in much detail.

[Christoph Schwarzenberg]

Maybe it is enough to check the supplied password.

I've modified the code from shangdahao accordingly:
​https://stackoverflow.com/a/49138231/9453030

[Tim Graham]

That approach may leak whether or not a username exists because of the time it takes to hash a password. For user names that exist, password hashing will run twice compared to once for user names that don't exist. See #20760 for a past example.

[Christoph Schwarzenberg]

Is there any progress regarding the issue?

An other suggestion:
In ModelBackend authentication maybe add an optional parameter, defaulting to False to allow inactive users to pass through authenticate() and catch them later in AuthenticationForm.confirm_login_allowed()
E.g.

class ModelBackend:
    def authenticate(self, request, username=None, password=None, allow_inactive=False, **kwargs):
        if username is None:
            username = kwargs.get(UserModel.USERNAME_FIELD)
        try:
            user = UserModel._default_manager.get_by_natural_key(username)
        except UserModel.DoesNotExist:
            # Run the default password hasher once to reduce the timing
            # difference between an existing and a nonexistent user (#20760).
            UserModel().set_password(password)
        else:
            if user.check_password(password) and (self.user_can_authenticate(user) or allow_inactive):
                return user

[Dimitriovski]

3 years have passed and still no fix for this?
I see it was fixed and then reverted.

Last comment from 15 months ago provides nice suggestion as well, but I guess it was somehow "missed" and left out?
I'm using v3.0 in 2020 and this is still a bug (even though there are custom work around(s)) - I'd appreciate when the Django team would fix this by default.

[Mariusz Felisiak]

Dimitriovski, such comment will not move fix forward, feel-free to prepare a patch.

[alv2017]

I'm using Django 3.1, and I spent some time on figuring the issue with ModelBackend.authenticate() method. In my application I was willing to authenticate active and inactive users, and the default ModelBackend is not designed to do this.

In Django 3.1 (backends.py) there is a class AllowAllUsersModelBackend that inherits all the functionality from the ModelBackend and permits authentication of active and inactive users.

PS: The problem seems to be solved, however it took me some time to figure the things out.

[Mohamed El-Kalioby]

Shouldn’t this be closed?