Opened 7 months ago

Last modified 39 hours ago

#36225 assigned Bug

createsuperuser validation breaks if USERNAME_FIELD is distinct but user manager deliberately lacks get_by_natural_key()

Reported by: Jonas Dittrich Owned by: Abderrahmane MELEK
Component: contrib.auth Version: 5.1
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

We have a custom user model defined, that does not have a natural key and in fact all fields (except pk) may be duplicated. That is because we can have "special" users that do not have an email.

Our model looks like this (minimal user model without a natural key implementation):

from django.contrib.auth.models import PermissionsMixin
from django.db import models

class UserProfile(PermissionsMixin, models.Model):
    # null=True because certain external users don't have an address
    email = models.EmailField(max_length=255, unique=True, blank=True, null=True, verbose_name="email address")
    password = models.CharField("password", max_length=128)

    @property
    def is_anonymous(self):
        return False

    @property
    def is_authenticated(self):
        return True

    USERNAME_FIELD = "email"
    REQUIRED_FIELDS = ["password"]

Now, using ./manage.py createsuperuser is no longer possible. After entering an email address:

Email address: user@example.com
Traceback (most recent call last):
  File "./manage.py", line 22, in <module>
    main()
  File "./manage.py", line 18, in main
    execute_from_command_line(sys.argv)
  File ".../lib/python3.10/site-packages/django/core/management/__init__.py", line 442, in execute_from_command_line
    utility.execute()
  File ".../lib/python3.10/site-packages/django/core/management/__init__.py", line 436, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File ".../lib/python3.10/site-packages/django/core/management/base.py", line 413, in run_from_argv
    self.execute(*args, **cmd_options)
  File ".../lib/python3.10/site-packages/django/contrib/auth/management/commands/createsuperuser.py", line 90, in execute
    return super().execute(*args, **options)
  File ".../lib/python3.10/site-packages/django/core/management/base.py", line 459, in execute
    output = self.handle(*args, **options)
  File ".../lib/python3.10/site-packages/django/contrib/auth/management/commands/createsuperuser.py", line 132, in handle
    error_msg = self._validate_username(
  File ".../lib/python3.10/site-packages/django/contrib/auth/management/commands/createsuperuser.py", line 305, in _validate_username
    self.UserModel._default_manager.db_manager(database).get_by_natural_key(
AttributeError: 'Manager' object has no attribute 'get_by_natural_key'

Command._validate_username does check if the username field is unique by looking at the unique flag passed to the field. However, we also set null=True, so our username is not unique in the sense that a get_by_natural_key can be implemented at all.

The create superuser command should either check for a nullable or blank username field in addition to the unique flag, or directly check if the user model has the attribute get_by_natural_key defined.

Change History (11)

comment:1 by Sarah Boyce, 7 months ago

Resolution: duplicate
Status: newclosed

Hi Jonas, you should create a write a manager for your custom user model which defines create_superuser.
This is also documented here: https://docs.djangoproject.com/en/5.1/topics/auth/customizing/#writing-a-manager-for-a-custom-user-model

This is roughly a duplicate of #26412

comment:2 by Jonas Dittrich, 7 months ago

Resolution: duplicate
Status: closednew

Hello Sarah. Sadly, even after defining a user manager, the problem persists. For brevity, I removed the user manager from the initial problem description.

Here is the updated, still failing example with a user model manager. Note that I can't use BaseUserManager, because defining a natural key is impossible (also see my discussion at https://forum.djangoproject.com/t/how-to-serialize-user-profiles-without-natural-keys/34447/4).

from django.contrib.auth.models import PermissionsMixin, Group
from django.contrib.auth.password_validation import validate_password
from django.db import models


class UserProfileManager(models.Manager):
    def create_user(self, *, email, password=None):
        user = self.model(email=email)
        validate_password(password, user=user)
        user.set_password(password)
        user.save()
        return user

    def create_superuser(self, *, email, password=None):
        user = self.create_user(password=password, email=email)
        user.is_superuser = True
        user.save()
        user.groups.add(Group.objects.get(name="Manager"))
        return user


class UserProfile(PermissionsMixin, models.Model):
    # null=True because certain external users don't have an address
    email = models.EmailField(
        max_length=255, unique=True, blank=True, verbose_name="email address"
    )
    password = models.CharField("password", max_length=128)

    @property
    def is_anonymous(self):
        return False

    @property
    def is_authenticated(self):
        return True

    USERNAME_FIELD = "email"
    REQUIRED_FIELDS = ["password"]

    objects = UserProfileManager()

comment:3 by Sarah Boyce, 7 months ago

Component: Uncategorizedcontrib.auth
Summary: User profile models with nullable emails cannot use createsuperuser commandUSERNAME_FIELD must be unique in order to use createsuperuser command
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

Based off the forum thread, the confusion here I see is from the USERNAME_FIELD docs saying:

The field must be unique (e.g. have unique=True set in its definition), unless you use a custom authentication backend that can support non-unique usernames.

I think we could remove the "unless you use a custom authentication backend that can support non-unique usernames." because it implies we support this use case.
Accepting for Documentation tweaks

comment:4 by Spencer Barriball, 7 months ago

Owner: set to Spencer Barriball
Status: newassigned

comment:5 by Simon Charette, 7 months ago

Spencer, it's great that you're interested on working on this ticket but it was accepted on the basis that the documentation should be adjusted (see comment:3) not that the create_superuser command should be adjusted.

Last edited 7 months ago by Simon Charette (previous) (diff)

comment:6 by Spencer Barriball, 7 months ago

Thanks for the clarification, Simon. I initially interpreted the ticket as a code adjustment rather than just a documentation update. Since it's a documentation tweak, would you like me to update the relevant part of the USERNAME_FIELD docs and submit a PR for that instead?

On a side note, at least I got everything up and running to be able to contribute code in the future!

comment:7 by Abderrahmane MELEK, 3 weeks ago

Has patch: set
Owner: changed from Spencer Barriball to Abderrahmane MELEK

Adjusted the documentation like mentionned in comment:3 by Sarah Boyce.
PR

Last edited 3 weeks ago by Abderrahmane MELEK (previous) (diff)

comment:8 by Jacob Walls, 3 weeks ago

The plan from comment:3 to tweak the docs gives me pause, since this use case was explicitly tested and added in #24910.

From my reading of the forum post, the two sharp edges that came up for the implementer were:

  • reimplementing AbstractBaseUser from scratch to avoid having to del AbstractBaseUser.natural_key to support serialization
  • createsuperuser's username validation not being aware of null=True, distinct=True username fields

We could tweak the docs instead to explain that natural_key must be overridden on the model. The use case for nullable usernames and the attendant weirdness around having to override get_by_natural_key on the manager doesn't seem worth documenting to me: it could just be implicit from the advice to "override".

comment:9 by Jacob Walls, 3 weeks ago

Patch needs improvement: set

comment:10 by Sarah Boyce, 2 days ago

Rereading the forum post and other related ticket #35729, I agree my initial understanding of the issue was wrong

It appears that there was a custom user which inherited AbstractBaseUser with a non-unique USERNAME field (which we have a test for running createsuperuser).
This had an issue loading fixtures created with use_natural_foreign_keys=True, use_natural_primary_keys=True (see #35729).
I can see how this would be problematic. If we add a model

class UserProfile(models.Model):
    user = models.ForeignKey(CustomUserNonUniqueUsername, on_delete=models.CASCADE)

An example serialized data for UserProfile and AbstractBaseUser gives roughly:

[
  {
    "model": "auth_tests.customusernonuniqueusername",
    "fields": {"password": "md5$***", "last_login": null, "username": "joe", "email": "joe@somewhere.org", "is_staff": true, "is_superuser": true}
  },
  {
    "model": "auth_tests.customusernonuniqueusername",
    "fields": {"password": "md5$***", "last_login": null, "username": "joe", "email": "joe@somewhere.org", "is_staff": true, "is_superuser": true}
  },
  {
    "model": "auth_tests.userprofile",
    "pk": 1,
    "fields": {"user": ["joe"]}
  }
]

"joe" is not a unique identifier for the CustomUserNonUniqueUsername and updating this to be pk is still problematic as pk is omitted from the serialization due to natural_key() being defined. So getting errors loading this data is not surprising

Then, the advise was to not inherit AbstractBaseUser. The problem with this advice is there is no docs to support how to make a valid custom user without doing this and other hiccups happened. Specifically, createsuperuser assumes a get_by_natural_key implementation, which given the previous ticket, was wanted to be avoided.

Personally I am tempted to close this ticket and reopen #35729 as a bug

comment:11 by Jacob Walls, 40 hours ago

Has patch: unset
Patch needs improvement: unset
Summary: USERNAME_FIELD must be unique in order to use createsuperuser commandcreatesuperuser validation breaks if USERNAME_FIELD is distinct but user manager deliberately lacks get_by_natural_key()
Type: Cleanup/optimizationBug

Reopened #35729 as a usability issue in the serialization framework that model subclasses cannot opt out of natural key serialization.

This issue was opened after OP bit the bullet and reimplemented the User model from scratch. I agree it's not correct for createsuperuser to deduce from field.unique that you can get_by_natural_key(), since there could be a use case for omitting an implementation as doc'd:

Conversely, if (for some strange reason) you want Django to output natural keys during serialization, but not be able to load those key values, just don’t define the get_by_natural_key() method.

"strange" or not, given we've doc'd it, I think it's reasonable for createsuperuser to catch AttributeError.

Version 0, edited 40 hours ago by Jacob Walls (next)
Note: See TracTickets for help on using tickets.
Back to Top