Opened 13 years ago
Closed 13 years ago
#17046 closed Bug (fixed)
User.objects.create_user() allows empty usernames
Reported by: | honza | Owned by: | nobody |
---|---|---|---|
Component: | contrib.auth | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | restless.being@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
This code will work:
User.objects.create_user(username='', email='test@test.com', password='password')
It should raise some kind of exception.
I searched for this in the tracker and didn't find any references. I'm happy to work on this given some direction from the core.
Attachments (5)
Change History (14)
by , 13 years ago
Attachment: | validate_create_user.diff added |
---|
comment:1 by , 13 years ago
Has patch: | set |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
Version: | 1.3 → SVN |
by , 13 years ago
Attachment: | prevent_empty_username.diff added |
---|
comment:2 by , 13 years ago
Needs tests: | unset |
---|---|
Patch needs improvement: | unset |
Triage Stage: | Accepted → Ready for checkin |
I added a better patch. Passing in an empty string will raise an IntegrityError. Includes tests. Marking as ready for checkin.
comment:3 by , 13 years ago
Triage Stage: | Ready for checkin → Accepted |
---|
Please do not promote your own patches to RFC and let someone else do it if it passes review. From looking at the patch, it feels like the validation shouldn't be done in the manager but deeper down at the model level. Also ValidationError
would seem more appropriate than IntegrityError
.
by , 13 years ago
Attachment: | validate_user_save.diff added |
---|
Does validation in the User.save() method, and raises a validation error if the username is empty.
by , 13 years ago
Attachment: | patch_for_17046.diff added |
---|
comment:4 by , 13 years ago
Cc: | added |
---|
Kwadrat's patch looks OK and works as intended.
I wondered about using self.full_clean() in save, instead of the above. However, according to JKocherhans comment here: https://code.djangoproject.com/ticket/13100 using full_clean would break backward compatibility.
by , 13 years ago
Attachment: | patch_for_17046_another.diff added |
---|
comment:5 by , 13 years ago
Reviewed the latest patch patch_for_17046_another.diff
, it works as intended.
comment:6 by , 13 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
follow-up: 8 comment:7 by , 13 years ago
I'd say "design decision needed" on this one. It is not so uncommon to use the email-address as a "username", possibly copying it to the username-field after the create().
comment:8 by , 13 years ago
Replying to HM:
I'd say "design decision needed" on this one. It is not so uncommon to use the email-address as a "username", possibly copying it to the username-field after the create().
Isn't there a unique constraint on the username field? Doing so might raise constraint violations in a multi-threaded environment if you are trying to have two users in the copying email to username state at the same time since both share an empty string username while doing it.
I do think the manager function should validate the model - attached is a very quick patch that does this - it is not a complete patch