Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#19662 closed Bug (fixed)

Explain correct `authenticate` usage with custom user model.

Reported by: tomchristie Owned by: nobody
Component: contrib.auth Version: 1.5-beta-1
Severity: Release blocker Keywords:
Cc: marc.tamlyn@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If using a custom user model, with eg USERNAME_FIELD='email',
then it is non-obvious if the correct usage is authenticate(username=..., password=...) or authenticate(email=..., password=...)

Bit of an easy stumbling point, and it won't be obvious to the developer why login is failing, so we should really note the usage here.

This was the best phrasing I could come up with, but happy to take other suggestions.

Marking as a bug, because I'd consider this a documentation bug, until resolved.

Change History (13)

comment:1 Changed 3 years ago by tomchristie

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 3 years ago by ptone

We have to be clear not to further confuse the situation between custom users and custom backends:

https://docs.djangoproject.com/en/dev/topics/auth/customizing/#writing-an-authentication-backend

authenticate only needs credentials

It is just that for most situations, people have only dealt with auth.User and the modelbackend.

Its not clear to me that the modelbackend shouldn't further introspect the user model to get then name of the USERNAME_FIELD and look for a matching kwarg

Version 0, edited 3 years ago by ptone (next)

comment:3 Changed 3 years ago by tomchristie

We have to be clear not to further confuse the situation between custom users and custom backends ... authenticate only needs credentials

Could use something like: "Note: If you're using the default authentication backend ModelBackend, then there are two credentials required: username, and password. Note that the first named argument will always be named username, even if you're using a custom user model withUSERNAME_FIELD set to some other field."

It's a bit verbose, but better than not mentioning it.

Its not clear to me that the modelbackend shouldn't further introspect the user model to get then name of the USERNAME_FIELD and look for a matching kwarg

Could be an option, but we'd also need to consider if the same ought to then also apply to AuthenticationForm. Ie. Should the username field, instead be named email if using an email-based custom user model?

I'm not sure what the best behavior is.

comment:4 Changed 3 years ago by aaugustin

  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

We should ask Russell before releasing 1.5 final. Afterwards the API will be subject to our backwards-compatibility policy, making the change significantly more complicated.

If we choose to keep the current API, the wording proposed in comment 3 seems perfectly clear to me.

comment:5 Changed 3 years ago by russellm

I could go either way on this.

On the one hand, create_superuser() on the model manager already uses USERNAME_FIELD; on the other hand, AuthenticationForm will be a pain to convert to use USERNAME_FIELD (unless I'm missing an obvious option). So, we've got feet in both camps here.

Strictly, the authenticate() prototype is already **credentials, and it would be a fairly easy change to make it accept USERNAME_FIELD instead of username, so changing the ModelBackend to accept **credentials and then inspect for USERNAME_FIELD is probably the right thing to do.

comment:6 Changed 3 years ago by mjtamlyn

  • Cc marc.tamlyn@… added

comment:7 follow-up: Changed 3 years ago by shaneallgeier@…

I believe that authenticate() should be left as is (only expecting username). Otherwise, all DRY/pluggable authentication backends would have to first check the USERNAME_FIELD from the user model every time someone tries to authenticate. It would also require pluggable backends to check the Django version to figure out if it even needs to check for a possible USERNAME_FIELD. I know it won't be a difference in speed or anything, but it would still be annoying to write.

comment:8 Changed 3 years ago by ptone

I've implemented a patch here:

https://github.com/ptone/django/compare/ticket/19662-modelbackend

This may still need a note in the custom user docs but the note in model backend ref is probably enough. Really I think this is something many people will just expect.

I think we can leave AuthenticationForm set to username. Or at least that is a different ticket.

One thing I almost missed is that authenticate works currently with username/password as positional args, so a check is made for that.

comment:9 in reply to: ↑ 7 Changed 3 years ago by carljm

Replying to shaneallgeier@…:

I believe that authenticate() should be left as is (only expecting username). Otherwise, all DRY/pluggable authentication backends would have to first check the USERNAME_FIELD from the user model every time someone tries to authenticate. It would also require pluggable backends to check the Django version to figure out if it even needs to check for a possible USERNAME_FIELD. I know it won't be a difference in speed or anything, but it would still be annoying to write.

You mean "all DRY/pluggable authentication backends that use essentially the same authentication scheme as ModelBackend but for some reason reimplement it themselves"? How big a category of reusable authentication backends is this? The authenticate() method of authentication backends has always been documented to accept **credentials precisely so custom auth backends can accept any authentication credentials that make sense for them, not be limited to what ModelBackend does. So I don't think changing ModelBackend to use more sensible credential names with custom User classes is enforcing any changes on custom auth backends in general.

comment:10 Changed 3 years ago by mjtamlyn

I'm inclined to agree with Carl - I'd rather things had the "correct" signature and it shouldn't be massively difficult to update external backends. Any pluggable app providing auth is likely to require updating for custom user models in any case.

comment:11 Changed 3 years ago by Preston Holmes <preston@…>

  • Resolution set to fixed
  • Status changed from new to closed

In c44d748272d1d39e7cd48b625c526f532703aa29:

Fixed #19662 -- alter auth modelbackend to accept custom username fields

Thanks to Aymeric and Carl for the review.

comment:12 Changed 3 years ago by Preston Holmes <preston@…>

In 660f80c3d60083ecba26c83c0670ccca7fc56e09:

[1.5.x] Fixed #19662 -- alter auth modelbackend to accept custom username fields

Thanks to Aymeric and Carl for the review.

comment:13 Changed 3 years ago by Preston Holmes <preston@…>

In 660f80c3d60083ecba26c83c0670ccca7fc56e09:

[1.5.x] Fixed #19662 -- alter auth modelbackend to accept custom username fields

Thanks to Aymeric and Carl for the review.

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