Skip to content
This repository has been archived by the owner on May 26, 2020. It is now read-only.

Fix is_active error message #391

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iliasgal
Copy link

Fix for issue #303

If user is disabled (is_active=False), the authenticate function returns None, so the user variable in:

user = authenticate(**credentials)

is always None.

Therefore, the if statement below is always False and the commands are never executed.

if not user.is_active:
    msg = _('User account is disabled.')
    raise serializers.ValidationError(msg)

My suggestion is to move the check for
if not user.is_active:
under the else statement.

We get the user with a query based on the given username. We also add an exception if ObjectDoesNotExist.

If user exists, then we check if it is active or not.
If the ObjectDoesNotExist is thrown, then it means that the login credentials were not correct.

 try:
    user = User.objects.get(**{self.username_field: attrs.get(self.username_field)})
    if not user.is_active:
        msg = _('User account is disabled.')
        raise serializers.ValidationError(msg)
    except ObjectDoesNotExist:
        msg = _('Unable to log in with provided credentials.')
        raise serializers.ValidationError(msg)

@manan
Copy link

manan commented Nov 25, 2017

Waiting for the merge :)

@manan
Copy link

manan commented Nov 25, 2017

And just FYI, this commit will not have expected behaviour in the case where someone sends a username of an inactive user and an INCORRECT password.

According to the commit, the response will be 'User account is disabled.' but it should be 'Unable to log in with provided credentials.' as the password in itself is incorrect. The way I see it is, the only time 'User account is disabled.' should be returned is when the credentials are correct but the user is inactive.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants