Wrong behavior if a field is empty

[jpadilla]

Wrong behavior if a field is empty

[jpadilla]

@arnuschky what would be the expected error?

[stanhu]

@jpadilla, the e-mail address in the Django User model can be null. The change in stanhu@e493480 unnecessarily enforced non-null e-mail addresses. The user lookup should still happen if the user_id exists and a JWT token is valid.

[arnuschky]

Exactly. Getting an invalid token is just wrong, I think. Either token creation fails or the token must validate.

[jpadilla]

@stanhu @arnuschky yeah that's perfect. Should we then remove completely email from that authenticate_credentials method?

[arnuschky]

I would say so, yes. In the end it's model-based decision what identified a user uniquely and what fields are used, so the only safe choice is user.id I guess.

[jpadilla]

@stanhu could you update your PR to include the change stated above?

[stanhu]

According to https://docs.djangoproject.com/en/dev/topics/auth/customizing/#auth-custom-user, the requirements for a custom user model include:

Django expects your custom User model to meet some minimum requirements.

Your model must have an integer primary key.
Your model must have a single unique field that can be used for identification purposes. This can be a username, an email address, or any other unique attribute.
Your model must provide a way to address the user in a “short” and “long” form. The most common interpretation of this would be to use the user’s given name as the “short” identifier, and the user’s full name as the “long” identifier. However, there are no constraints on what these two methods return - if you want, they can return exactly the same value.

Would it make sense to include the unique identifier (e.g. username) instead of the e-mail address?

Obviously id may be all that you need, but I do like being able to see human-readable data (as opposed to the database ID) when I decode the payload.

[jpadilla]

@stanhu you could still leave whatever other data you want on the payload, obviously id would be required in the authentication process. If we add the unique identifier authenticating would validate that user with that pk/id exists, it's active, and it still has the same unique identifier, which might be a case that might confuse people if they accidentally bump into it.

So, my general opinion would be to use authenticate with the primary key and is_active. You could still leave the email and username in the payload encoder.

Thoughts?

[stanhu]

Agreed; I like the simplicity. Pull request has been updated.

[arnuschky]

Agree as well. Everything other than id is model/usecase-dependent and should go in the payload encoder.

[arnuschky]

To test this, should I clone or made this change already its way into the pip? Sorry for asking, I haven't fully understood the python packaging process yet.

[jpadilla]

@arnuschky If you'd like to run tests against the latest code in the master branch you could clone it and then run:

python setup.py install

I'll try and release this into PyPI sometime this week. I'll update this thread and close the issue when I do.

[arnuschky]

Tested and confirmed to be working as expected. Thanks!