Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#26957 closed Cleanup/optimization (fixed)

Inconsistency between doc and code: authenticate() when user.is_active == False

Reported by: Shen Li Owned by: Damian
Component: contrib.auth Version: 1.10
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description (last modified by Shen Li)

In the code:
https://github.com/django/django/blob/stable/1.10.x/django/contrib/auth/backends.py#L12-L32

    def authenticate(self, username=None, password=None, **kwargs):
        UserModel = get_user_model()
        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 non-existing user (#20760).
            UserModel().set_password(password)
        else:
            if user.check_password(password) and self.user_can_authenticate(user):
                return user

    def user_can_authenticate(self, user):
        """
        Reject users with is_active=False. Custom user models that don't have
        that attribute are allowed.
        """
        is_active = getattr(user, 'is_active', None)
        return is_active or is_active is None

authenticate() returns None when user.is_active == False.

However, the documentation suggests that when user.is_active == False, the authenticate() method returns the user normally.

https://docs.djangoproject.com/en/1.10/topics/auth/default/#auth-web-requests

from django.contrib.auth import authenticate, login

def my_view(request):
    username = request.POST['username']
    password = request.POST['password']
    user = authenticate(username=username, password=password)
    if user is not None:
        if user.is_active:
            login(request, user)
            # Redirect to a success page.
        else:
            # Return a 'disabled account' error message
            ...
    else:
        # Return an 'invalid login' error message.
        ...

Change History (11)

comment:1 by Shen Li, 8 years ago

Description: modified (diff)
Version: 1.91.10

comment:2 by Tim Graham, 8 years ago

Easy pickings: set
Triage Stage: UnreviewedAccepted

Yes, it would be good to adjust or clarify that example in light of #25232.

comment:3 by Austin Simmons, 8 years ago

Owner: changed from nobody to Austin Simmons
Status: newassigned

comment:4 by Austin Simmons, 8 years ago

Owner: Austin Simmons removed
Status: assignednew

comment:5 by Damian, 8 years ago

Owner: set to Damian
Status: newassigned

comment:6 by Damian, 8 years ago

I understand that the user.is_active() check is redundant after we run authenticate method in the example code. However I would like to ask for some advice, before I do anything(as a newbie I would like to make it all right). I would add some information in the box that the authenticate method returns None if the user is inactive, even if the user entered correct login detail(thus checking if user is active after that is redundant)? And then I would mention to use separately user.is_active() and user.check_password() to display separate error messages, for each case, as in the example code above ? Would that clear things up?

comment:7 by Shen Li, 8 years ago

Running user.is_active() and user.check_password() separately is a good idea, but I think it would result in redundant SQL queries, right?

comment:8 by Damian, 8 years ago

Version 1, edited 8 years ago by Damian (previous) (next) (diff)

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

Resolution: fixed
Status: assignedclosed

In c52350b:

[1.10.x] Fixed #26957 -- Corrected authenticate() docs regarding User.is_active.

Backport of c412aaca735c7cc1c766b85c1512f9ff434ce63a from master

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

In c412aac:

Fixed #26957 -- Corrected authenticate() docs regarding User.is_active.

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