Opened 11 years ago

Closed 11 years ago

#19067 closed Bug (fixed)

createsuperuser fails when custom user model has no username

Reported by: Ian Clelland Owned by: Ian Clelland
Component: Documentation Version: dev
Severity: Normal Keywords:
Cc: django@…, tom@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

There is a dependency in the createsuperuser management command on the existence of a field called 'username' in a user model.

Since #3011 was fixed, user models do not necessarily have that field. They are required to have a single identifying field, which is passed in as the first parameter to their create_superuser() method, but it is not necessarily named username.

As a result, if syncdb installs django.contrib.auth, and the user model does not have a 'username' field, then the creation of the initial superuser will fail.

I have a (one line) patch which fixes this, and will try to get tests written for it later today.

Change History (14)

comment:1 by Ian Clelland, 11 years ago

Has patch: set
Needs tests: set
Owner: changed from nobody to Ian Clelland

The fix for this is on my github branch https://github.com/clelland/django/tree/ticket_19067

comment:2 by Ian Clelland, 11 years ago

I've just re-re-read the docs, and realized that the api contract for create_user and create_superuser explicitly shows username being the first argument, and that seems to imply that it refers to the USERNAME_FIELD, and not specifically a field named username

Please feel free to mark this as invalid if that's the case, and I'll add a note to the docs instead :)

However, that interpretation does mean that you wouldn't be able to create a model like this:

class MyUser(AbstractBaseUser):
    email = models.CharField(max_length=254, unique=True)
    username = models.CharField(max_length=20, unique=True)
    USERNAME_FIELD = 'email'
    REQUIRED_FIELDS = ['username']

As the required fields would be passed into create_user as keyword arguments, and this would conflict with the named username parameter.

It's an edge case, but one that we can easily get around by not constraining the parameter names in the manager methods.

comment:3 by Stephen Burrows, 11 years ago

Component: contrib.authDocumentation
Needs documentation: set
Needs tests: unset
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

The create_superuser management command is actually just a thin wrapper around UserManager.create_superuser(), which is pretty tied into the way the AbstractUserModel (i.e. the old User model) works. For example, it assumes there are is_staff, is_superuser, and is_active fields. It also assumes that there's a username.

I don't think there's an elegant way to rewrite create_superuser to not make any assumptions and still present the same contract. If you can find a way, that's great.

Clarifying in the docs that these commands currently only work with User models that look a lot like the default model is easy and easy to approve. Rewriting create_superuser might be worth bringing up on the django-dev mailing list.

comment:4 by Preston Holmes, 11 years ago

Resolution: needsinfo
Status: newclosed

Has someone actually hit an error here or is this from interpretation of the code?

The relevant lines in the management command are:

UserModel = get_user_model()

username_field = UserModel._meta.get_field(getattr(UserModel, 'USERNAME_FIELD', 'username'))

This will get a field as specified by the class USERNAME_FIELD, or will otherwise look for a field name 'username'.

If neither of those are specified, you will get an error that username field does not exist, which could be the problem being experienced?

If you have a case of failure that you can provide more details on, please reopen.

comment:5 by Ian Clelland, 11 years ago

Here's what I actually did --

I created a user model like this:

class SocialUser(AbstractBaseUser):
    # Use a sufficiently long email field
    email = models.EmailField(verbose_name='email address', max_length=254, unique=True)

    twitter_screenname = models.CharField(max_length=20, null=True, blank=True)
    facebook_user_id = models.BigIntegerField(null=True, blank=True)
    google_plus_userid = models.CharField(max_length=50, null=True, blank=True)

    is_staff = models.BooleanField(default=False)
    is_active = models.BooleanField(default=True)
    is_superuser = models.BooleanField(default=False)

    USERNAME_FIELD = 'email'
    REQUIRED_FIELDS = []

    objects = SocialUserManager()

and then, by mistake apparently, wrote the manager like this:

class SocialUserManager(BaseUserManager):

    def create_user(self, email, password=None, **kwargs):
        user = self.model(email=email, **kwargs)
        user.set_password(password)
        user.save()
        return user

    def create_superuser(self, email, password, **kwargs):
        user = self.model(email=email, is_staff=True, is_superuser=True, **kwargs)
        user.set_password(password)
        user.save()
        return user

(note that the first parameter on the manager methods is 'email', and not 'username', as it is in the docs)

At that point I ran syncdb on a brand new database.
The createsuperuser management command correctly identifies the USERNAME_FIELD, and uses the name of that field to prompt for the new superuser's email address. However, then it calls SocialUserManager.create_superuser with all named parameters, including "username=<new email address>". That failed, of course, since the method I wrote required an 'email' parameter instead.

As I said in comment:1, absolutely feel free to close this as invalid; what I wrote was not what was in the instructions.

My point afterwards, though, was that my patch fixes this, and makes the only requirement on create_user and create_superuser that the USERNAME_FIELD be the first positional parameter, and the password the second positional parameter.

Right now, from my reading of the code (I haven't tested this or run into it in a real situation,) it looks like we have an undocumented restriction on user model fields:

If your custom model has a field called 'username', that field MUST be the USERNAME_FIELD, or the manager methods will not work.

I'll write up a test case for that and reopen the ticket for discussion on that point.

comment:6 by Preston Holmes, 11 years ago

OK - I can see the confusion.

The issue is that creating superusers with a 'username' has been around a while - so we can't change the signature of that method without breaking backwards compatibility.

however, a point of confusion in your conclusion, is that just because the kwarg has to be username, doesn't mean that has to be the name of a field, or that a field named username has to be the one you use as a username, you'd just rewrite the above as:

class SocialUserManager(BaseUserManager):


    def create_superuser(self, username, password, **kwargs):
        user = self.model(email=username, is_staff=True, is_superuser=True,
**kwargs)
        user.set_password(password)
        user.save()
        return user

I generally prefer the explicitness of passing named kwargs rather than positional - as there is less chance for mistakes (assuming the first position is one thing when it is something else).

I do agree this particular detail of legacy needs to be highlighted in the docs - and I'm considering a major refactor of the auth docs currently.

comment:7 by Ian Clelland, 11 years ago

The issue is that creating superusers with a 'username' has been around a while - so we can't change the signature of that method without breaking backwards compatibility.

Agreed. That was the subtle point that I didn't pick up in the docs originally. (Actually, reading them again, it's left completely ambiguous in the docs what to do if your username field *isn't* called 'username'. The point is only made, extremely subtly, in contrib/auth/tests/custom_user.py, which uses email in create_user and username in create_superuser.

This is backwards compatibility issue, and maybe we can't change the signature of the create_superuser method. In that case, though, the docs definitely need to be clearer about the exact method signature.

however, a point of confusion in your conclusion, is that just because the kwarg has to be username, doesn't mean that has to be the name of a field, or that a field named username has to be the one you use as a username, you'd just rewrite the above as:

No, I stand by my point. If your user model has a required field called username, there is currently no way to write a manager for it unless that field is declared to be the USERNAME_FIELD. If it is in REQUIRED_FIELDS, then createsuperuser.py will try to call create_superuser with two keyword arguments called username.

This breaks in at least two ways:

manage.py syncdb will fail after querying for a new admin's credentials, terminating with

TypeError: create_superuser() got multiple values for keyword argument 'username' 

If you try to run manage.py createsuperuser directly, then it fails earlier: optparse will throw an error when it tries to create a second --username argument. The final error being

optparse.OptionConflictError: option --username: conflicting option string(s): --username

comment:8 by Preston Holmes, 11 years ago

Resolution: needsinfo
Status: closedreopened

Indeed there would be collision there - I was mainly working from your example in comment 5 - which didn't have the required fields

so at a minimum - this is a documentation improvement need - and possible something that results in a warning if you define a field named 'username' , that is not your USERNAME_FIELD and have it in REQUIRED_FIELDS - raising an error or warning that username is reserved or limited in some way. My guess is that any fix that tries to work around this, would end up being "too clever" for its own good.

comment:9 by Preston Holmes, 11 years ago

Needs documentation: unset
Patch needs improvement: unset

Current patch will raise an error inside the management command - I couldn't really find a better place, would be a good use case for model validation.

Also clarified some aspects of REQUIRED_FIELDS in docs

refactored the management command a bit to not keep getting information about the custom user in as many places, and improved command feedback.

This may need an additional note that --username arg is now '--%s' % USERNAME_FIELD, but still stored as username

https://github.com/django/django/pull/431

comment:10 by Chris Streeter, 11 years ago

Cc: django@… added

comment:11 by Preston Holmes, 11 years ago

In addition to the changes so far it would make sense to clarify this section of the docs:

https://docs.djangoproject.com/en/dev/topics/auth/#django.contrib.auth.UserManager.create_user

which implies that you get to change the signature, esp the 'username' arg if you have a different set required_fields/username_fields

alternatively - we bail on the legacy of using create_superuser(username='x', password='y') and just use positional. We could update all django calling code (but not UserManager) to use positional args, and existing code that expects a regular User would not break. 3rd party/legacy code using a username kwarg would get a relatively meaningful error for custom user models that don't have/require a username field.

comment:12 by Thomas Schreiber, 11 years ago

I've came across this issue in my work on https://code.djangoproject.com/ticket/19086 that repoduces this bug, and I have a test case for CustomUser that is helpful here:

https://github.com/rizumu/django/compare/19086#L1R182

I think we can both use the test similarly to avoid overlap.

comment:13 by Thomas Schreiber, 11 years ago

Cc: tom@… added

comment:14 by Russell Keith-Magee <russell@…>, 11 years ago

Resolution: fixed
Status: reopenedclosed

In b3b3db3d954a5226f870a0b4403343c78efae8dc:

Fixed #19067 -- Clarified handling of username in createsuperuser.

Thanks to clelland for the report, and Preston Holmes for the draft patch.

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