Fixed #26719 -- Normalized email in AbstractUser.clean(). #6788

tamhuynhit · 2016-06-16T10:15:26Z

https://code.djangoproject.com/ticket/26719

claudep · 2016-06-18T15:40:43Z

I wonder what's the more correct way to do that, the save() or the clean() method?
We might get more guaranties that normalization occurs with the save() method at the expense of calling normalization methods a bit too much. clean() might be a more fine-grained approach as we are essentially protecting against user input which should pass through clean() (via ModelForm).

claudep · 2016-06-18T15:41:51Z

django/contrib/auth/models.py

@@ -145,7 +145,6 @@ def _create_user(self, username, email, password, **extra_fields):
        if not username:
            raise ValueError('The given username must be set')
        email = self.normalize_email(email)


I guess this line should also disappear?

👍 Yes, thanks a lot. Somehow I forgot to delete that line after several double checks 😞

timgraham · 2016-06-18T16:58:10Z

Normalizing in clean() seems more expected to me so that any user code would see the normalized values. On the other hand, normalizing in the manager as is done now seems like a good idea too so that things like the createsuperuser command are protected.

timgraham · 2016-06-18T17:01:05Z

django/contrib/auth/models.py

@@ -361,6 +360,13 @@ def email_user(self, subject, message, from_email=None, **kwargs):
        """
        send_mail(subject, message, from_email, [self.email], **kwargs)

+    def save(self, *args, **kwargs):
+        if self.email:
+            self.email = UserManager.normalize_email(self.email)


Might want to account for the possibility of a custom manager with something like self.__class__._default_manager.normalize_email.

Thanks for your suggestion. It looks better to me, will fix this in the next commit

claudep · 2016-06-18T17:40:18Z

On the other hand, normalizing in the manager as is done now seems like a good idea too so that things like the createsuperuser command are protected.

Sure, in my local patch, I'm calling user.clean() in the manager.

claudep · 2016-06-19T18:53:30Z

FYI here's my current local patch: https://gist.github.com/claudep/78e3661224a9b1767b8b0ea3b8ac8e9b

tamhuynhit · 2016-06-20T05:18:47Z

@claudep 👍 Thanks for the patch. I will update the code with normalization happen in clean method. But I have some problem with testing from the discussion above.

timgraham · 2016-06-20T13:57:04Z

Please let me know if you have time to work on this in the next 8 hours. If not, I can pick it up since we're trying to get the beta release out today. Thanks!

tamhuynhit · 2016-06-20T15:32:04Z

@timgraham Thanks, I am working on it, will push the code soon

timgraham · 2016-06-20T23:48:12Z

django/contrib/auth/forms.py

@@ -111,6 +111,7 @@ def clean_password2(self):
    def save(self, commit=True):
        user = super(UserCreationForm, self).save(commit=False)
        user.set_password(self.cleaned_data["password1"])
+        user.clean()


I don't expect this would be needed since Model.clean() is called as part of ModelForm validation (the test passes with this commented out). Was it just an oversight on your part?

timgraham · 2016-06-21T00:10:28Z

I'll open a PR to your branch for these changes.

timgraham · 2016-06-21T00:41:14Z

Edits at tamhuynhit#1. I think this is good to go after that although if @claudep can take a look, that would be welcome.

tamhuynhit · 2016-06-21T02:08:14Z

PR is merged. Thanks for helping me on this @timgraham @claudep

claudep · 2016-06-21T07:54:55Z

Looks better and better! The remaining questions for me:

Does it make sense to keep having the normalize methods on the manager instead of on the AbstractUser model itself?
Should we normalize the username based on get_username(), maybe even on AbstractBaseUser.clean() level?

tamhuynhit · 2016-06-21T09:41:17Z

@claudep About the first question, I think you make sense to have that concern since the current code to call normalize_email have to go through many steps: self.__class__.objects.normalize_email. Move them to AbstractBaseUser seems good to me.
I didn't realize we have get_username before. I try to find some edge cases when USERNAME_FIELD point to any special field but not found yet. However if we want to use get_username, I think we should modify get_username with an empty default value, or force_text function will return a string 'None':

def get_username(self):
    return getattr(self, self.USERNAME_FIELD, '')

Move them up to AbstractBaseUser is a good idea. So should we make these changes happen ?

timgraham · 2016-06-21T12:03:25Z

I think I'll do a separate PR to make the changes for username. Then we can rebase this PR and make the changes for email which is more complicated with respect to backwards-compatibility if we want to move the manager method. Changing how email is normalized also isn't a release blocker for 1.10.

timgraham · 2016-06-21T13:30:08Z

Patch for username changes only: #6819

timgraham · 2016-06-21T20:32:23Z

With the other patch merged, you can either rebase your branch and force push, or open a new PR to continue the changes for normalizing email.

timgraham · 2016-06-22T18:09:43Z

docs/releases/1.10.txt

@@ -890,6 +890,11 @@ Miscellaneous
 * If you subclass ``AbstractBaseUser`` and override ``clean()``, be sure it
  calls ``super()``. :meth:`.AbstractBaseUser.normalize_username` is called in
  a new :meth:`.AbstractBaseUser.clean` method.
+* If you subclass ``AbstractUser`` and override ``clean()``, be sure it calls


We're targeting 1.11 for this change. This only needs to mention email now.

Please move this to the 1.11 release notes and also update the versionadded annotation.

Ok I've moved them to 1.11, you can check again if I didn't make it right

timgraham · 2016-06-22T18:13:56Z

You can squash commits when updating too.

timgraham · 2016-06-24T14:40:20Z

merged in 09119df, thanks!

tamhuynhit force-pushed the ticket_26719 branch from e04bfc7 to 3887b9e Compare June 16, 2016 14:42

claudep reviewed Jun 18, 2016
View reviewed changes

timgraham reviewed Jun 18, 2016
View reviewed changes

tamhuynhit changed the title ~~Fixed #26719 -- Call normalize email and username in User.save~~ Fixed #26719 -- Call normalize email and username in User.clean Jun 20, 2016

timgraham reviewed Jun 20, 2016
View reviewed changes

timgraham changed the title ~~Fixed #26719 -- Call normalize email and username in User.clean~~ Fixed #26719 -- Normalized email and username in AbstractUser.clean(). Jun 21, 2016

timgraham mentioned this pull request Jun 21, 2016

Refs #21379, #26719 -- Moved username normalization to AbstractBaseUser. #6819

Merged

timgraham changed the title ~~Fixed #26719 -- Normalized email and username in AbstractUser.clean().~~ Fixed #26719 -- Normalized username in AbstractUser.clean(). Jun 21, 2016

tamhuynhit force-pushed the ticket_26719 branch from 0416b44 to 73e8b8a Compare June 22, 2016 03:25

timgraham reviewed Jun 22, 2016
View reviewed changes

tamhuynhit force-pushed the ticket_26719 branch 2 times, most recently from caa6ab7 to ebb2487 Compare June 24, 2016 02:44

Fixed #26719 -- Call normalize email in AbstractUser.clean

528b5bb

tamhuynhit force-pushed the ticket_26719 branch from ebb2487 to 528b5bb Compare June 24, 2016 02:54

timgraham changed the title ~~Fixed #26719 -- Normalized username in AbstractUser.clean().~~ Fixed #26719 -- Normalized email in AbstractUser.clean(). Jun 24, 2016

timgraham closed this Jun 24, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #26719 -- Normalized email in AbstractUser.clean(). #6788

Fixed #26719 -- Normalized email in AbstractUser.clean(). #6788

tamhuynhit commented Jun 16, 2016 •

edited by timgraham

claudep commented Jun 18, 2016

claudep Jun 18, 2016

tamhuynhit Jun 20, 2016

timgraham commented Jun 18, 2016

timgraham Jun 18, 2016

tamhuynhit Jun 20, 2016

claudep commented Jun 18, 2016

claudep commented Jun 19, 2016

tamhuynhit commented Jun 20, 2016

timgraham commented Jun 20, 2016

tamhuynhit commented Jun 20, 2016

timgraham Jun 20, 2016

timgraham commented Jun 21, 2016

timgraham commented Jun 21, 2016

tamhuynhit commented Jun 21, 2016 •

edited

claudep commented Jun 21, 2016

tamhuynhit commented Jun 21, 2016 •

edited

timgraham commented Jun 21, 2016

timgraham commented Jun 21, 2016

timgraham commented Jun 21, 2016

timgraham Jun 22, 2016

timgraham Jun 23, 2016

tamhuynhit Jun 24, 2016

timgraham commented Jun 22, 2016

timgraham commented Jun 24, 2016

Fixed #26719 -- Normalized email in AbstractUser.clean(). #6788

Fixed #26719 -- Normalized email in AbstractUser.clean(). #6788

Conversation

tamhuynhit commented Jun 16, 2016 • edited by timgraham

claudep commented Jun 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timgraham commented Jun 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claudep commented Jun 18, 2016

claudep commented Jun 19, 2016

tamhuynhit commented Jun 20, 2016

timgraham commented Jun 20, 2016

tamhuynhit commented Jun 20, 2016

Choose a reason for hiding this comment

timgraham commented Jun 21, 2016

timgraham commented Jun 21, 2016

tamhuynhit commented Jun 21, 2016 • edited

claudep commented Jun 21, 2016

tamhuynhit commented Jun 21, 2016 • edited

timgraham commented Jun 21, 2016

timgraham commented Jun 21, 2016

timgraham commented Jun 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timgraham commented Jun 22, 2016

timgraham commented Jun 24, 2016

tamhuynhit commented Jun 16, 2016 •

edited by timgraham

tamhuynhit commented Jun 21, 2016 •

edited

tamhuynhit commented Jun 21, 2016 •

edited