Adding global email model #5913

ehfeng · 2017-08-17T23:50:05Z

Because emails are not distinct per User, we're using the Email to store email-specific settings (email subscription state). UserEmail will represent verification that a given user "owns" an email address.

Alternatively, UserEmail.email could just be a key for Email.id, where the id value is the email string. The value for this seems minimal (UserEmail.email is never updated in place, so the chance of UserEmail.email != UserEmail.global_email.email is low), so I've done the more naive approach.

dcramer · 2017-08-17T23:52:39Z

src/sentry/models/useremail.py


    user = FlexibleForeignKey(settings.AUTH_USER_MODEL, related_name='emails')
    email = models.EmailField(_('email address'))
+    global_email = FlexibleForeignKey('sentry.Email', related_name='useremails', null=True)


Probably don't need this since you can just index the email string and query on it

mattrobenolt

We definitely need to ship the data migration after the schema migration, otherwise you'll end up with some rows that are still NULL.

It should also be considered if we care about case sensitivity here or not. I feel like we should always lowercase the value to prevent unintended duplicates due to case differences. Matt@example.com vs matt@example.com

And, what is going to keep this up to date for new rows? I don't see any code that's adding new Email rows except for the migration.

mattrobenolt · 2017-08-17T23:57:51Z

src/sentry/south_migrations/0346_backfill_email.py

+        for useremail in RangeQuerySetWrapperWithProgressBar(queryset):
+            email, _ = Email.objects.get_or_create(email=useremail.email)
+            useremail.global_email = email
+            useremail.save()


Don't do this .save() Do the UserEmail.objects.filter(id=useremail.id).update(global_email=email) pattern instead to avoid race condition.

ehfeng · 2017-08-18T00:58:06Z

@mattrobenolt good point.

I'm looking at receivers/useremail.py and I'm assuming this is a good case for having a receiver on post_save. Especially as changes to User and UserEmail are generally user-triggered, so adding latency isn't bad?

ehfeng · 2017-08-21T18:44:53Z

@mattrobenolt I could modify it such that the host is case insensitive, but the username part of email can be case-sensitive. Obviously, Gmail, Yahoo, and Outlook make the username case-insensitive, so we could be the same, but email standard itself mandates case sensitivity.

I'm not sure if this would be a security issue—I'll defer to whatever you decide.

mattrobenolt · 2017-08-21T18:47:31Z

Yeah, internally @tkaemming brought this up. But our own behavior for normal User.email is to treat it as case-insensitive and we use this for logins. My concern is that we don't 100% handle this everywhere correctly since it requires explicit action to handle this behavior.

If we have control over this, we can just apply a functional index that would handle uniqueness as case insensitive to enforce it correctly.

dcramer · 2017-08-21T18:48:59Z

Can someone pull data on if we have case duplicated emails? if not lets move towards case insensitive.

dcramer · 2017-08-21T19:57:26Z

Let's do a few things:

Email.email is unique
Email.email is stored in LOWERCASE at all times (i dont know if this is easy to do via a custom field?)
UserEmail has a unique constraint on (user_id, email)
UserEmail.email is stored same as Email.email (always lowercase)
Migrate all existing emails in UserEmail to be lowercase

mattrobenolt · 2017-08-21T19:59:35Z

Email.email is stored in LOWERCASE at all times (i dont know if this is easy to do via a custom field?)

I was going to do this with a functional index to enforce it on the database. Something like, CREATE UNIQUE INDEX ... (lower(email)) since we run custom SQL anyways for indexes.

dcramer · 2017-08-21T20:00:31Z

@mattrobenolt is there a good reason to not store things lowercase? its a lot easier to be compatible if we do that

mattrobenolt · 2017-08-21T20:04:34Z

But yeah, we can do a field as well, I think having the index enforce it makes it more explicit on the db side.

I think my concerns are how we have to deal with User.email today. We have to always make sure we do a case insensitive filter, etc. Which may be error prone.

I don't think a field would help with this case, would it? For example, doing Email.objects.filter(email='FOO@EXAMPLE.COM') vs Email.objects.filter(email__iexact='FOO@EXAMPLE.COM') Not sure if a custom field could coerce this behavior automatically? I know the db index would.

dcramer · 2017-08-21T20:06:11Z

An index wouldn't coerce a filter clause, but a field could. We should see if we can do that, though someone from @getsentry/platform might need to help here.

I think doing the unique on lower() would be useful to ensure the guarantee, but we should do both.

ehfeng · 2017-08-21T20:12:19Z

We can't assume all email servers are case insensitive. If example.com's email server is case-sensitive, then someone with the address UserName@example.com would literally never be able to get our emails.

I prefer Matt's suggestion of enforcing in the index, but storing the case-sensitive value.

dcramer · 2017-08-21T20:29:59Z

I'd prefer a case insensitive text field, but given we dont have that, I'm ok with saiyng "we will not support case sensitive email servers".

ehfeng · 2017-08-21T20:34:02Z

Adding this as reference:

While the above definition for Local-part is relatively permissive, for maximum interoperability, a host that expects to receive mail SHOULD avoid defining mailboxes where the Local-part requires (or uses) the Quoted-string form or where the Local-part is case-sensitive.

Therefore, it's probably safe to assume case-insensitivity.

dcramer · 2017-08-21T21:08:50Z

Per talk internally, let's just make it a 'citext' field for postgres, and assume its case insensitive.

In MySQL we might be able to use a case insensitive collation, but im not sure if Django lets us provide that. Ensuring correctness for SQLite would be nice, but i'm not sure we have an option.

mattrobenolt

We still need to move the backfill to another deploy. The problem is there will be a race condition. We'll deploy the new model, run the backfill, then between running the backfill and new code wtih the signal being live, we'll miss data.

mattrobenolt · 2017-08-24T17:26:11Z

src/sentry/models/email.py

+    """
+    __core__ = True
+
+    email = CIEmailField(_('email address'))


Don't we need unique=True here?

That's true. Added.

ehfeng · 2017-08-25T18:54:27Z

Removed backfill

ehfeng · 2017-08-25T21:51:58Z

@mattrobenolt We need to create the citext extension within the test database. Can you help with this?

ghost · 2017-09-07T16:53:50Z

	2 Warnings
⚠️	Changes require @getsentry/security sign-off
⚠️	PR includes migrations

Security concerns found

src/sentry/models/email.py
src/sentry/receivers/email.py

Migration Checklist

new columns need to be nullable (unless table is new)
migration with any new index needs to be done concurrently
data migrations should not be done inside a transaction
before merging, check to make sure there aren't conflicting migration ids

Generated by 🚫 danger

dcramer · 2017-09-07T18:10:20Z

src/sentry/models/email.py

can we add a date_added field here

dcramer · 2017-09-07T18:10:55Z

this is actually good to go at this point right @ehfeng? (i didnt check tests)

mattrobenolt · 2017-09-07T18:20:58Z

I gotta take over and figure out what's causing citext to fail on some of the tests when I get a few spare minutes to poke at it.

dcramer · 2017-09-07T18:26:11Z

src/sentry/utils/pytest/sentry.py

this code should probably happen after fix_south is called.

It does. Unless you mean immediately after. But either way, I'm gonna see what makes things happy here. This was me just guessing.

I mean immediately after, but not sure if it will actually cause issues by keeping it below. Theres sort of a "things should be in an ok state at this point", and I'd just suggest bundling the "database should be ready" parts.

dcramer · 2017-09-07T21:06:40Z

i'll take this over and figure out why tests are broken real quick

dcramer · 2017-09-07T21:13:17Z

pushed a fix for citext extension

ehfeng · 2017-09-07T22:23:22Z

@dcramer not sure why it's not working, but it looks like things passed: https://travis-ci.org/getsentry/sentry/builds/273059785
Merge?

ehfeng · 2017-09-07T23:43:21Z

It looks like master is broken right now. I'm going to wait until it's green, rebase, and see what happens. The current failing test seems to have appeared in 09883f6

mattrobenolt · 2017-09-07T23:47:36Z

You'll need to regenerate migration for new id since it's out of date from current master.

ehfeng · 2017-09-08T00:25:47Z

Regenerated.

Adding Email model Adding post_save triggers on UserEmail to fill in Email Adding on_delete signal adding CIText to testing setup

mattrobenolt · 2017-09-08T18:23:27Z

src/sentry/models/email.py

+    __core__ = True
+
+    email = CIEmailField(_('email address'), unique=True)
+    date_added = models.DateTimeField(default=timezone.now)


If we don't need an index on this now, we'll probably need to add one later if we try to do anything with this table regarding cleanup.

I'm going to punt on this because I'm unsure when we'd need to do such a cleanup.

mattrobenolt · 2017-09-08T18:28:13Z

src/sentry/receivers/email.py

+
+
+def delete_email(instance, **kwargs):
+    if UserEmail.objects.filter(email=instance.email).exists():


I want to note here, that this might be slightly inaccurate due to this not being a case insensitive lookup. You should do email__iexact. Because Email is case insensitive but UserEmail is, you'll get false positives.

Good point: #6070

dcramer reviewed Aug 17, 2017

View reviewed changes

mattrobenolt suggested changes Aug 18, 2017

View reviewed changes

ehfeng force-pushed the email-model branch from a81658a to eb71e1d Compare August 23, 2017 19:44

mattrobenolt suggested changes Aug 24, 2017

View reviewed changes

ehfeng force-pushed the email-model branch from 502ed60 to ade42ef Compare August 25, 2017 20:21

ehfeng force-pushed the email-model branch from c43e78d to 96d5ef0 Compare September 7, 2017 17:18

dcramer reviewed Sep 7, 2017

View reviewed changes

dcramer approved these changes Sep 7, 2017

View reviewed changes

ehfeng force-pushed the email-model branch 2 times, most recently from be8d53c to 5120b0a Compare September 7, 2017 23:21

mattrobenolt added the Impact: Migration label Sep 7, 2017

ehfeng force-pushed the email-model branch from 094730a to f6da479 Compare September 8, 2017 00:02

Adding global email model

a4a31a3

Adding Email model Adding post_save triggers on UserEmail to fill in Email Adding on_delete signal adding CIText to testing setup

ehfeng force-pushed the email-model branch from f6da479 to a4a31a3 Compare September 8, 2017 00:53

mattrobenolt approved these changes Sep 8, 2017

View reviewed changes

ehfeng merged commit 54cb964 into master Sep 8, 2017

ehfeng deleted the email-model branch September 8, 2017 18:23

mattrobenolt reviewed Sep 8, 2017

View reviewed changes

ehfeng restored the email-model branch September 8, 2017 20:08

ehfeng deleted the email-model branch September 8, 2017 20:09

ehfeng mentioned this pull request Sep 8, 2017

backfill/Email model #6073

Merged

github-actions bot locked and limited conversation to collaborators Dec 22, 2020



		def delete_email(instance, **kwargs):
		if UserEmail.objects.filter(email=instance.email).exists():

Uh oh!

Adding global email model #5913

Adding global email model #5913

Uh oh!

Conversation

ehfeng commented Aug 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattrobenolt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ehfeng commented Aug 18, 2017

Uh oh!

ehfeng commented Aug 21, 2017

Uh oh!

mattrobenolt commented Aug 21, 2017

Uh oh!

dcramer commented Aug 21, 2017

Uh oh!

dcramer commented Aug 21, 2017

Uh oh!

mattrobenolt commented Aug 21, 2017

Uh oh!

dcramer commented Aug 21, 2017

Uh oh!

mattrobenolt commented Aug 21, 2017

Uh oh!

dcramer commented Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ehfeng commented Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcramer commented Aug 21, 2017

Uh oh!

ehfeng commented Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcramer commented Aug 21, 2017

Uh oh!

mattrobenolt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ehfeng commented Aug 25, 2017

Uh oh!

ehfeng commented Aug 25, 2017

Uh oh!

ghost commented Sep 7, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security concerns found

Migration Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcramer commented Sep 7, 2017

Uh oh!

mattrobenolt commented Sep 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcramer commented Sep 7, 2017

Uh oh!

dcramer commented Sep 7, 2017

Uh oh!

ehfeng commented Sep 7, 2017

Uh oh!

ehfeng commented Sep 7, 2017

dcramer commented Aug 21, 2017 •

edited

Loading

ehfeng commented Aug 21, 2017 •

edited

Loading

ehfeng commented Aug 21, 2017 •

edited

Loading

ghost commented Sep 7, 2017 •

edited by ghost

Loading