Skip to content
This repository has been archived by the owner on Jan 18, 2024. It is now read-only.

Integrity error when adding e-mail that belongs to another user #10

Open
clemente opened this issue Dec 3, 2014 · 5 comments · May be fixed by #11
Open

Integrity error when adding e-mail that belongs to another user #10

clemente opened this issue Dec 3, 2014 · 5 comments · May be fixed by #11

Comments

@clemente
Copy link

clemente commented Dec 3, 2014

class EmailAddress(models.Model):
…
    email = models.EmailField(max_length=100, unique=True)

But then:

            try:
                addr = EmailAddress.objects.get(user=user,email=user.email)
                # Provides that an address that has been just verified
                # without use of django-multimail, is still considered
                # verified in conditions of django-multimail
                if user.is_active and not addr.verified_at:
                    addr.verified_at = now()
            except EmailAddress.DoesNotExist:
                addr = EmailAddress()
                addr.user = user
                addr.email = user.email
            addr.save(verify=False)

The search you do is for user X and e-mail Y. If there's user Z with e-mail Y, you find 0 results, then you try to add (X,Y) which fails due to uniqueness (Y already exists).
I think: before creating a new one (X,Y), you should .filter(email=user.email) to see whether that e-mail (*,Y) already exists.
Just before the code I posted there's another piece of code that needs the same change.

@scott2b
Copy link
Owner

scott2b commented Dec 3, 2014

So, the email property used to be unique_together with user. I could not recall the use case for that setup, and it seemed to me that Email addresses should be globally unique, which is why the model code stands as it is now. I am open to discussion on this, but I think the fix is to keep email addresses globally unique and to change this:

addr = EmailAddress.objects.get(user=user,email=user.email)

to this:

addr = EmailAddress.objects.get(email=user.email)

And get the user via the retrieved EmailAddress object.

clemente added a commit to clemente/django-multimail that referenced this issue Dec 17, 2014
@clemente
Copy link
Author

And get the user via the retrieved EmailAddress object.

In doing so, you could be getting a different user than intended (that is: you search user, you get addr.user, which is different). It happens in the example scenario with two users (X,Z, …) in the bug description.

I'll create a code proposal in my branch.

@clemente
Copy link
Author

How this scenario can happen:
First let's suppose I have user U1 who validated his e-mail E by clicking the link in the validation e-mail. Ok, now: I'm using python-social-auth, and when a user logs in, his basic data (name, e-mail, username) is already filled by python-social-auth, so the new User has the e-mail from Facebook. If the e-mail I see from a new Facebook user U2 happens to be E (maybe because he's the same person), django-multimail tries to create an EmailAddress (U2,E) and fails because (U1,E) already exists and E should be unique.

scott2b pushed a commit that referenced this issue May 28, 2015
This is fine for now. The logic will change a bit after changes to make user,email unique together to address issue #10. But, I think this is right, we don't want to allow unverified addresses to be set to primary.
@scott2b
Copy link
Owner

scott2b commented May 28, 2015

@clemente sorry it's been a while. I'd like to get this issue cleaned up. If I understand correctly, if we were to have it that the user and email were unique-together, instead of the email globally unique, then this code change would not be needed. Does that seem right to you?

@clemente
Copy link
Author

Hi, that's right, unique-together of email+user makes sense, whereas unique of only email doesn't.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants