Opened 12 years ago

Closed 12 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)

validate_create_user.diff (399 bytes ) - added by Preston Holmes 12 years ago.
prevent_empty_username.diff (2.6 KB ) - added by honza 12 years ago.
validate_user_save.diff (2.8 KB ) - added by nerdap 12 years ago.
Does validation in the User.save() method, and raises a validation error if the username is empty.
patch_for_17046.diff (2.8 KB ) - added by kwadrat 12 years ago.
patch_for_17046_another.diff (2.0 KB ) - added by kwadrat 12 years ago.

Download all attachments as: .zip

Change History (14)

by Preston Holmes, 12 years ago

Attachment: validate_create_user.diff added

comment:1 by Preston Holmes, 12 years ago

Has patch: set
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted
Version: 1.3SVN

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

by honza, 12 years ago

Attachment: prevent_empty_username.diff added

comment:2 by honza, 12 years ago

Needs tests: unset
Patch needs improvement: unset
Triage Stage: AcceptedReady 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 Julien Phalip, 12 years ago

Triage Stage: Ready for checkinAccepted

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 nerdap, 12 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 kwadrat, 12 years ago

Attachment: patch_for_17046.diff added

comment:4 by Jakub Wiśniowski, 12 years ago

Cc: restless.being@… 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 kwadrat, 12 years ago

comment:5 by Piotr Banaszkiewicz, 12 years ago

Reviewed the latest patch patch_for_17046_another.diff, it works as intended.

comment:6 by Piotr Banaszkiewicz, 12 years ago

Triage Stage: AcceptedReady for checkin

comment:7 by HM, 12 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().

in reply to:  7 comment:8 by Simon Charette, 12 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.

comment:9 by Jannis Leidel, 12 years ago

Resolution: fixed
Status: newclosed

In [17628]:

Fixed #17046 -- Added a check if the username passed to User.objects.create_user is empty or not. Thanks, kwadrat.

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