Opened 7 years ago

Last modified 18 months ago

#28645 new Bug

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

Reported by: Guilherme Junqueira Owned by: shangdahao
Component: contrib.auth Version: 3.1
Severity: Normal Keywords: 2.1
Cc: Christoph Schwarzenberg Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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

Attachments (1)

28645-jc.diff (5.5 KB ) - added by Tim Graham 6 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 by Tim Graham, 7 years ago

Summary: The inactive user error never is raised in login formAuthenticationForm's inactive user error isn't raised when using ModelBackend
Triage Stage: UnreviewedAccepted

comment:2 by shangdahao, 6 years ago

Owner: changed from nobody to shangdahao
Status: newassigned

comment:3 by shangdahao, 6 years ago

Has patch: set

comment:4 by Tim Graham, 6 years ago

Patch needs improvement: set

comment:5 by shangdahao, 6 years ago

Patch needs improvement: unset

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

Resolution: fixed
Status: assignedclosed

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.

comment:7 by Tim Graham <timograham@…>, 6 years ago

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

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

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

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

In af33fb25:

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

Reverted 359370a8b8ca0efe99b1d4630b291ec060b69225 (refs #28645).

This is a security fix.

comment:10 by Tim Graham <timograham@…>, 6 years ago

In c37bb286:

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

Reverted 359370a8b8ca0efe99b1d4630b291ec060b69225 (refs #28645).

This is a security fix.

comment:11 by Tim Graham <timograham@…>, 6 years ago

In 57b95fed:

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

Reverted 359370a8b8ca0efe99b1d4630b291ec060b69225 (refs #28645).

This is a security fix.

comment:12 by Tim Graham, 6 years ago

Has patch: unset
Keywords: 2.1 added
Resolution: fixed
Status: closednew

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.

by Tim Graham, 6 years ago

Attachment: 28645-jc.diff added

comment:13 by Christoph Schwarzenberg, 6 years ago

Cc: Christoph Schwarzenberg added

comment:14 by Christoph Schwarzenberg, 6 years ago

Maybe it is enough to check the supplied password.

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

comment:15 by Tim Graham, 6 years ago

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.

comment:16 by Christoph Schwarzenberg, 5 years ago

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

comment:17 by Dimitriovski, 4 years ago

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.

comment:18 by Mariusz Felisiak, 4 years ago

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

comment:19 by alv2017, 3 years ago

Version: 1.113.1

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

Version 1, edited 3 years ago by alv2017 (previous) (next) (diff)

comment:20 by Mohamed El-Kalioby, 18 months ago

Shouldn’t this be closed?

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