Opened 12 years ago
Closed 12 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 , 12 years ago
Has patch: | set |
---|---|
Needs tests: | set |
Owner: | changed from | to
comment:2 by , 12 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 , 12 years ago
Component: | contrib.auth → Documentation |
---|---|
Needs documentation: | set |
Needs tests: | unset |
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
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 , 12 years ago
Resolution: | → needsinfo |
---|---|
Status: | new → closed |
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 , 12 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 , 12 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 , 12 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 , 12 years ago
Resolution: | needsinfo |
---|---|
Status: | closed → reopened |
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 , 12 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
comment:10 by , 12 years ago
Cc: | added |
---|
comment:11 by , 12 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 , 12 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:
I think we can both use the test similarly to avoid overlap.
comment:13 by , 12 years ago
Cc: | added |
---|
comment:14 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
The fix for this is on my github branch https://github.com/clelland/django/tree/ticket_19067