Opened 2 years ago

Closed 14 months ago

#19844 closed Cleanup/optimization (fixed)

Uniqueness validation of USERNAME_FIELD should be overridable.

Reported by: josh@… Owned by: nobody
Component: contrib.auth Version: master
Severity: Normal Keywords:
Cc: gsoc2013-checks, chris.jerdonek@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The resolution of #19822 by commit bc6746ac303ab625f2bc6fc878bd63661c784a59 introduces validation of the uniqueness of the specified USERNAME_FIELD in a way that cannot be (easily?) overridden. This is probably a good default behavior, however it breaks in any cases where you may not want unique usernames. The discussion on the original ticket clearly assumes that having non-unique usernames is an inherently bad design decision. This may very well be true, but, as I'm sure we're all aware, the exigencies of web development do not always allow us to restrict ourselves to good design decisions.

As a specific example, I am working on a site that is required to integrate with a legacy system. The requirements for the project are such that it is necessary for us to preserve the user information from the legacy system faithfully. As such, this requires us to, among other things, allow the users to log in with nothing more than their email address and password (easily done), and allow for the possibility that multiple people will use the same email address while still needing to have unique user accounts (a pain, but not horribly difficult).

The enforced design decision introduced by bc6746ac303ab625f2bc6fc878bd63661c784a59 unfortunately means that we either need to fork Django to keep that commit out of our codebase, hack around the validation by designating some field other than the email address our USERNAME_FIELD, or restrict ourselves to older code that does not have this restriction.

It seems to me that a much better solution would be to not unnecessarily restrict the flexibility introduced by swappable user models, and make this the default, but still changeable, behavior.

Change History (8)

comment:1 Changed 2 years ago by claudep

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

We could turn the validation error in a mere warning. However, it should make clear that some part of contrib.auth will not function properly (for example changepassword management command, which uses USERNAME_FIELD as a key to retrieve user instances, or ModelBackend). I'll leave to Russell about the design decision.

comment:2 Changed 2 years ago by russellm

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

So... you want to allow people to log in, and be uniquely identifiable, but without providing a unique identifier? That sounds... interesting.

I'm afraid too much of Django's internals relies on username being unique to be able to relax this condition. Django's login form uses this unique identifier on login forms (specifically because it is unique), so even if we relaxed the validation condition, you wouldn't be able to use Django's login form. You also won't be able to use the auth Model backend, or significant parts of Django's admin, or anything else that asks the simple question "Which user do you want to operate on" -- at least, not out of the box.

However, keep in mind that the username field doesn't have to be the user's name, or their login. It just needs to be a unique identifier.

From the sound of it, you might be able to get away with using the model's primary key as your USERNAME_FIELD. You will need to set up your login forms to do a search on email, rather than the username field - but you'd have to write your own login form anyway. The existing login form isn't going to work, because it assumes that the username field is unique, so there's no real change in the effort involved here. (I haven't actually tried this though, so there might be some other interesting side effects).

I'm not a fan of adding a setting or flag to allow this error to be ignored -- something is either an error or it isn't. And if you don't have a way to uniquely identify someone, you haven't got a unique object. A warning falls in a simliar bucket to me -- warnings are universally ignored by the people who need to pay attention to them; errors force you to fix the problem.

comment:3 Changed 19 months ago by russellm

  • Cc gsoc2013-checks added
  • Resolution wontfix deleted
  • Status changed from closed to new
  • Triage Stage changed from Design decision needed to Accepted
  • Type changed from Bug to Cleanup/optimization
  • Version changed from 1.5-rc-1 to master

I'm going to reopen this in the context of the GSoC project reworking validation.

Previously, we were in a position where we could only issue errors, and it was sufficiently important to raise *any* notification for this problem that an error was called for.

Part of the remit for the GSoC project is to introduce "warnings". Given that this will give us extra flexibility to raise attention about problems without preventing progress, I think a warning may be called for here rather than a hard error.

comment:4 Changed 19 months ago by ptone

I personally am not in favor of weakening this validation and agree with Russ' original comments above.

Can you explain how you plan to select the unique user if you can't uniquely specify them.

Relying on the password hash seems your only way, but there is no guarantee that that will be unique (even in combo with username)

"hack around the validation by designating some field other than the email address our USERNAME_FIELD" actually seems like not a hack, but the right approach.

Last edited 19 months ago by ptone (previous) (diff)

comment:5 Changed 19 months ago by russellm

To be clear -- this isn't *removing* the validation, it's reducing the 'level' of the notification from error to warning. Users will still be notified that there is a potential problem.

One of the reasons I softened on this was that I came across a use case myself. Consider the case where the unique thing is actually a pair of fields -- for example, a username and a subdomain. A site example.com may have 'company' accounts foo.example.com and bar.example.com. Each username is unique *on each domain*, but not on the overall user table. However, there's no ambiguity -- whenever a login form is displayed or a user lookup is performed, it's displayed in the context of a specific domain.

There are workarounds for this situation -- you make a combined "identity" field that is a concatenation of username and domain, for example -- but it's a messy workaround that shouldn't be needed.

comment:6 Changed 14 months ago by cjerdonek

I also think the limitation should be lifted.

In my use case, email address is the username field, and I would like a user account to be considered active only after the signup has been validated via email. To handle this use case, it is natural to add the account info to the user table when someone tries to sign up, and then activate that row only when the user clicks the validation link in the corresponding email. This is essentially what django-registration does in its two-phase registration workflow, with minor variations.

However, with this use case, there are edge cases where the same email address can exist unregistered in the database more than once. With the above process, the unique index should enforce the fact that each email address should be in the database only once, but when restricted to active user accounts. This can be done, for example, by having a unique index on the pair of columns username and activation key (where the activation key is set to the empty string when the user is activated).

Last edited 14 months ago by cjerdonek (previous) (diff)

comment:7 Changed 14 months ago by cjerdonek

  • Cc chris.jerdonek@… added

comment:8 Changed 14 months ago by Russell Keith-Magee <russell@…>

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

In d818e0c9b2b88276cc499974f9eee893170bf0a8:

Fixed #16905 -- Added extensible checks (nee validation) framework

This is the result of Christopher Medrela's 2013 Summer of Code project.

Thanks also to Preston Holmes, Tim Graham, Anssi Kääriäinen, Florian
Apolloner, and Alex Gaynor for review notes along the way.

Also: Fixes #8579, fixes #3055, fixes #19844.

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