Opened 5 years ago

Closed 5 years ago

#17046 closed Bug (fixed)

User.objects.create_user() allows empty usernames

Reported by: honza Owned by: nobody
Component: contrib.auth Version: master
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 5 years ago.
prevent_empty_username.diff (2.6 KB) - added by honza 5 years ago.
validate_user_save.diff (2.8 KB) - added by nerdap 5 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 5 years ago.
patch_for_17046_another.diff (2.0 KB) - added by kwadrat 5 years ago.

Download all attachments as: .zip

Change History (14)

Changed 5 years ago by Preston Holmes

Attachment: validate_create_user.diff added

comment:1 Changed 5 years ago by Preston Holmes

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

Changed 5 years ago by honza

Attachment: prevent_empty_username.diff added

comment:2 Changed 5 years ago by honza

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 Changed 5 years ago by Julien Phalip

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.

Changed 5 years ago by nerdap

Attachment: validate_user_save.diff added

Does validation in the User.save() method, and raises a validation error if the username is empty.

Changed 5 years ago by kwadrat

Attachment: patch_for_17046.diff added

comment:4 Changed 5 years ago by Jakub Wiśniowski

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.

Changed 5 years ago by kwadrat

comment:5 Changed 5 years ago by Piotr Banaszkiewicz

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

comment:6 Changed 5 years ago by Piotr Banaszkiewicz

Triage Stage: AcceptedReady for checkin

comment:7 Changed 5 years ago by 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().

comment:8 in reply to:  7 Changed 5 years ago by Simon Charette

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 Changed 5 years ago by Jannis Leidel

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