avoid adding an EmailAddress for a user when e-mail exists for other user

[clemente]

This fixes #10

[scott2b]

I am trying to understand the scenario whereby the email address on a given User object matches an existing multimail EmailAddress object for a different user. Do you know why this is happening / where it is happening in the workflow? I suspect that we want to prevent this from the outset rather than prevent creating the matching multimail EmailAddress for a user's primary.

I am also wondering if the current approach is not a bit dated at this point. The reason I initially implemented this signal to ensure the existence of an EmailAddress object matching the User object's email was because (if I remember correctly) older versions of Django required an email address on the user. I'm not sure that is still the case (I think it is not). Also, for a custom user model, there might not be an email field at all -- or it might be named differently.

So I'm left wondering 2 things: 1) Is there a deeper/more fundamental cause to the symptom of the existing EmailAddress that we should be preventing, and 2) Does it even make sense to continue to support the signal-based syncing of User.email with multimail's data?

Thoughts?

[clemente]

1. I added it to the bug description Integrity error when adding e-mail that belongs to another user #10

2. The current system is difficult to use and places a lot of logic in the users of multimail, any simplification is good (although I'm close to making the current system work for me!).

Why create an EmailAddress on user save? I would create/update it in two events only: a) create it when starting the verification process (e.g. send an e-mail), b) update it when finishing the verification process.
I think your handler creates it in a third case: c) when you see a User object with a User.email field that you didn't know yet. This is a bit intrusive in my taste: (e.g. it did #9 without asking. And as you say, it looks for „email“ even when the name could be different).

Maybe the c) part can be optional, with these options: i) if you don't need anything on save because you do it manually (as I do), you don't use it. ii) if you need it, you enable it. iii) if you need it to sync a custom field (not named „email“), you enable it by saying the name of your e-mail field

It all depends on how much time you have to rewrite it :-) I think ii) with my patch can work for my case, but i) would work too. I handle e-mail changes myself.

[scott2b]

I agree that the logic is overly complicated. I am concerned about complicating it more. The reason this is here is that pre-django 1.5, the shell would enforce an email address on the User object when creating an admin user upon syncdb. For this reason, and the general sense that an email address was more or less required in the days before custom user classes, I put this in as an approach to keep Django's internal notion of email in sync with multimail's addresses.

However, I have always disliked this whole approach. I find the use of signals for the enforcement of data integrity clunky at best, and arguably just plain wrong. And, as you have discovered, the whole approach introduces some convoluted logic that is difficult to dissect and is susceptible to special-case scenarios.

I'm not sure I completely follow your example scenario. It still seems to me that there is an issue of identity and data integrity at the application level that is outside the scope of multimail. However, I think the difficulty in understanding your case further underscores the real problem here: the use of signals to enforce data integrity.

I am thinking about how I want to handle this and will comment shortly.

[scott2b]

Edit: looking at Django's User code, I am no longer sure if this is the best approach. See further comments below.

What I think is this: The whole of the logic of the email_address_handler is clunky at best. Some of the logic seems just plain wrong. I suspect there are things in here for various historical and special case reasons.

I can't express how much I appreciate your contribution. It's really great to see somebody using this code. However, I really think I want to avoid touching this logic. I'm not sure if there are applications out there that depend on this logic as-is. I suspect your changes would not hurt anything, but I'd prefer to tread lightly here.

What I would rather do is this:

Add a new settings parameter: USE_EMAIL_ADDRESS_HANDLER. I'd like to default this to False -- which would effectively deprecate the signal handler immediately, but would allow existing deployments to upgrade and continue to use this strange logic if required. This parameter would then be used in setup_signals (line 201) as follows:

def setup_signals(user_model):
    if MM.USE_EMAIL_ADDRESS_HANDLER:
        post_save.connect(email_address_handler, sender=user_model)
    if MM.USER_DEACTIVATION_HANDLER_ON:
        post_save.connect(user_deactivation_handler, sender=user_model)

Some Django 1.4 users might want to enable this signal to handle the admin creation. Other users enable it because their current application logic depends on it -- but generally, the signal would be considered deprecated and any synchronization of multimail addresses with the User object should be handled by the application itself, so that multimail has to make no assumptions in this matter.

Does this make sense to you? Would the idea of turning off this signal entirely be helpful to you? I appreciate you contributing and pushing me to think about this some more. I think the idea of simplifying the logic and making fewer assumptions would improve the library. Let me know what you think.

[scott2b]

Well, now I am looking at the Django code for the User model, and I notice this: email is marked blank=True, but the following is also indicated: REQUIRED_FIELDS = ['email']

I'm not sure what the implications of this are for everything I've stated above. I guess I need to dig into this a bit more. Thoughts/recommendations are welcome.

[scott2b]

Ok, I've dug into this a bit. Based on what I am seeing in the docs here: https://docs.djangoproject.com/en/1.7/topics/auth/customizing/#django.contrib.auth.models.CustomUser.REQUIRED_FIELDS, and on some quick checks against different versions of Django -- it seems to me that REQUIRED_FIELDS is entirely misnamed. This setting, which is set to ['email'] in the Django User object, really only affects the createsuperuser command, and only affects which fields are requested for this command, not what fields are actually required. As far as I can tell, for Django 1.5+, the email address is not required on the Django User object. Thus, I will stand by the idea of deprecating the signal creation of an EmailAddress from the User email property.

Again, let me know your thoughts.

[clemente]

Sorry about the late reply.
email_address_handler does 2 things: auto-create EmailAddress, and check the „primary“-ness.

• I don't really need the „primary“ things yet because I'm still using 1 e-mail per user (I use it only for the validation part). But when I use it, I'll also want to control myself which e-mail is the primary one.
• And the autocreation part: I don't need but I currently depend on it (my tests fail when I disable the signal because they expect 1 EA and they find 0). But I can create an EmailAddress myself. Or better, I could expect some users not to have an EmailAddress.
  If you make it optional, which of the two should I choose? I'll prefer the 2nd one, because not all the users are created by me; some are automatically created by python-social-auth (e.g. via Facebook login).

Therefore I don't need the signal. But other users may prefer the automatical handlings of those two points, so if that's the case, making it optional is better (only: you can only use the automatical handler if your model User has a field called email, which is the default. If you don't use the default, you'll have to handle EmailAddress manually too).

[scott2b]

Ok, I had forgotten that the user creation process depends on that signal to create the EmailAddress object when a user is originally created. Furthermore, the demo code relies on Django's builtin UserCreationForm which necessitates this signal-based approach. I dislike the use of signals for this kind of logic. On the other hand, removing the signal puts a burden on the developer that I was hoping to avoid, since it would no longer be possible to use that form.

I think the solution might be to provide a form class that would handle validation in a way that it creates the EmailAddress object for you. Ideally this form would work with whatever user model class you are using and would expose the properties of that model in the form as well.

If we did this, providing a class called, say, MultimailUserCreationForm, which creates both the User object and the EmailAddress object in save, then I think there would be no need for the signal handler.

[clemente]

If you add MultimailUserCreationForm, you can ask the developer to inherit from that class (instead of being forced to use it), so that any form to edit user data can get this new behaviour. Your class will look at the old e-mail and the new e-mail, and then decide whether to send confirmation e-mail or not, etc. It will be able to customize that behaviour by using a CustomMultimailUserCreationForm form. That seems good. But it wouldn't work for admin etc. Actually signals aren't so bad as long as they are simple and work.

As a django-multimail user, I don't like having to know the implementation details like whether an EmailAddress object will be created automatically or when or whether that behaviour depends on uniqueness of e-mail. In my project, we relied too much on those details and that wasn't good; I'll try to avoid it.

Returning to the topic of this discussion: I think that a second EmailAddress shouldn't be added. If (U1,E1) exists, then you cannot add (U2,E1) because e-mail needs to be unique. But does that mean that U2 cannot verify his e-mail because U1 is still waiting for verification? That seems bad for U2, because U1 (which may not even own E1) is blocking it.

But if you accepted to have (U1,E1) and (U2,E1), then security problems appear. If you send to E1 an e-mail „U1, please verify your e-mail“ and then when adding (U2,E1) you send to E1 an e-mail „U2, please verify your e-mail“, who will really be able to verify it? U2 or U1? Or both or none? Or will they be able to steal the verified status from one another?

[clemente]

I'll add: why is the field "e-mail" unique? I think the uniqueness should be in the combination (user,email). It makes sense to allow (U1,E1) and (U2,E1) at the same time: both users are trying to validate the same e-mail. Maybe one of them succeeds, maybe they don't. But the direct way to represent this state is to store both lines in DB.