fix(email): Fix bug that sent emails to deleted emails (#28237)

* fix(email): Fix bug that sent emails to deleted emails

Adds functionality that deletes UserOption instances when the email
is deleted. Also, adds functionality that checks when sending an email
if the UserOption email was deleted, it ignores it and deletes the
`UserOption` instance

src/sentry/api/endpoints/organization_member_details.py

@@ -21,8 +21,10 @@
     InviteStatus,
     OrganizationMember,
     OrganizationMemberTeam,
+    Project,
     Team,
     TeamStatus,
+    UserOption,
 )
 from sentry.utils import metrics, ratelimits
 
@@ -278,6 +280,17 @@ def delete(self, request, organization, member_id):
                 user=om.user, auth_provider__organization=organization
             ).delete()
 
+            # Delete instances of `UserOption` that are scoped to the projects within the
+            # organization when corresponding member is removed from org
+            proj_list = Project.objects.filter(organization=organization).values_list(
+                "id", flat=True
+            )
+            uo_list = UserOption.objects.filter(
+                user=om.user, project_id__in=proj_list, key="mail:email"
+            )
+            for uo in uo_list:
+                uo.delete()
+
             om.delete()
 
         self.create_audit_entry(

src/sentry/api/endpoints/user_emails.py

@@ -213,6 +213,9 @@ def delete(self, request, user):
         email = validator.validated_data["email"]
         primary_email = UserEmail.get_primary_email(user)
         del_email = UserEmail.objects.filter(user=user, email__iexact=email).first()
+        del_useroption_email_list = UserOption.objects.filter(
+            user=user, key="mail:email", value=email
+        )
 
         # Don't allow deleting primary email?
         if primary_email == del_email:
@@ -221,6 +224,9 @@ def delete(self, request, user):
         if del_email:
             del_email.delete()
 
+        for useroption in del_useroption_email_list:
+            useroption.delete()
+
         logger.info(
             "user.email.remove",
             extra={"user_id": user.id, "ip_address": request.META["REMOTE_ADDR"], "email": email},

src/sentry/utils/email.py

@@ -21,7 +21,7 @@
 
 from sentry import options
 from sentry.logging import LoggingFormat
-from sentry.models import Activity, Group, GroupEmailThread, Project, User, UserOption
+from sentry.models import Activity, Group, GroupEmailThread, Project, User, UserEmail, UserOption
 from sentry.utils import metrics
 from sentry.utils.compat import map
 from sentry.utils.safe import safe_execute
@@ -168,8 +168,12 @@ def get_email_addresses(user_ids: Iterable[int], project: Project = None) -> Map
     if project:
         queryset = UserOption.objects.filter(project=project, user__in=pending, key="mail:email")
         for option in (o for o in queryset if o.value and not is_fake_email(o.value)):
-            results[option.user_id] = option.value
-            pending.discard(option.user_id)
+            if UserEmail.objects.filter(user=option.user, email=option.value).exists():
+                results[option.user_id] = option.value
+                pending.discard(option.user_id)
+            else:
+                pending.discard(option.user_id)
+                option.delete()
 
     if pending:
         queryset = User.objects.filter(pk__in=pending, is_active=True)

tests/sentry/api/endpoints/test_organization_member_details.py

@@ -10,6 +10,7 @@
     Organization,
     OrganizationMember,
     OrganizationMemberTeam,
+    UserOption,
 )
 from sentry.testutils import APITestCase
 from sentry.utils.compat import map
@@ -342,6 +343,31 @@ def test_simple(self):
 
         assert not OrganizationMember.objects.filter(id=member_om.id).exists()
 
+    def test_simple_related_user_options_are_deleted(self):
+        """
+        Test that ensures that when a member is removed from an org, their corresponding
+        `UserOption` instances for that the projects in that org are deleted as well
+        """
+        org = self.create_organization()
+        project2 = self.create_project(organization=org)
+        member = self.create_user("ahmed@ahmed.io")
+        u1 = UserOption.objects.create(
+            user=member, project=self.project, key="mail:email", value="ahmed@ahmed.io"
+        )
+        u2 = UserOption.objects.create(
+            user=member, project=project2, key="mail:email", value="ahmed@ahmed.io"
+        )
+
+        member_om = self.create_member(organization=self.organization, user=member, role="member")
+
+        self.get_success_response(self.organization.slug, member_om.id)
+
+        assert not OrganizationMember.objects.filter(id=member_om.id).exists()
+        assert not UserOption.objects.filter(id=u1.id).exists()
+        # Ensure that `UserOption` for a user in a different org does not get deleted when that
+        # same member is deleted from another org
+        assert UserOption.objects.filter(id=u2.id).exists()
+
     def test_invalid_id(self):
         member = self.create_user("bar@example.com")
         self.create_member(organization=self.organization, user=member, role="member")

tests/sentry/api/endpoints/test_user_emails.py

@@ -1,6 +1,6 @@
 from django.urls import reverse
 
-from sentry.models import User, UserEmail
+from sentry.models import User, UserEmail, UserOption
 from sentry.testutils import APITestCase
 
 
@@ -74,6 +74,20 @@ def test_remove_email(self):
         assert response.status_code == 204, response.data
         assert not len(UserEmail.objects.filter(user=self.user, email="altemail1@example.com"))
 
+    def test_remove_email_also_deletes_user_option_with_same_email(self):
+        mail_to_del = "altemail1@example.com"
+        UserEmail.objects.create(user=self.user, email=mail_to_del)
+        UserOption.objects.create(
+            user=self.user, project=self.project, key="mail:email", value=mail_to_del
+        )
+
+        response = self.client.delete(self.url, data={"email": mail_to_del})
+        assert response.status_code == 204, response.data
+        assert not len(UserEmail.objects.filter(user=self.user, email=mail_to_del))
+        assert not len(
+            UserOption.objects.filter(user=self.user, key="mail:email", value=mail_to_del)
+        )
+
     def test_cant_remove_primary_email(self):
         response = self.client.delete(self.url, data={"email": "foo@example.com"})
         assert response.status_code == 400

tests/sentry/incidents/test_action_handlers.py

@@ -24,7 +24,7 @@
     IncidentStatusMethod,
     TriggerStatus,
 )
-from sentry.models import Integration, NotificationSetting, PagerDutyService, UserOption
+from sentry.models import Integration, NotificationSetting, PagerDutyService, UserEmail, UserOption
 from sentry.notifications.types import NotificationSettingOptionValues, NotificationSettingTypes
 from sentry.testutils import TestCase
 from sentry.types.integrations import ExternalProviders
@@ -104,6 +104,10 @@ def test_user_email_routing(self):
             user=self.user, project=self.project, key="mail:email", value=new_email
         )
 
+        useremail = UserEmail.objects.get(email=self.user.email)
+        useremail.email = new_email
+        useremail.save()
+
         action = self.create_alert_rule_trigger_action(
             target_type=AlertRuleTriggerAction.TargetType.USER,
             target_identifier=str(self.user.id),
@@ -113,9 +117,14 @@ def test_user_email_routing(self):
         assert handler.get_targets() == [(self.user.id, new_email)]
 
     def test_team_email_routing(self):
-        new_user = self.create_user()
-
         new_email = "marcos@sentry.io"
+
+        new_user = self.create_user(new_email)
+
+        useremail = UserEmail.objects.get(email=self.user.email)
+        useremail.email = new_email
+        useremail.save()
+
         UserOption.objects.create(
             user=self.user, project=self.project, key="mail:email", value=new_email
         )

tests/sentry/mail/test_adapter.py

@@ -26,6 +26,7 @@
     Repository,
     Rule,
     User,
+    UserEmail,
     UserOption,
     UserReport,
 )
@@ -50,7 +51,7 @@
 from sentry.testutils.helpers.datetime import before_now, iso_format
 from sentry.types.integrations import ExternalProviders
 from sentry.utils.compat import mock
-from sentry.utils.email import MessageBuilder
+from sentry.utils.email import MessageBuilder, get_email_addresses
 from sentry_plugins.opsgenie.plugin import OpsGeniePlugin
 
 
@@ -298,6 +299,55 @@ def test_notify_users_does_email(self, mock_func):
         self.assertEquals(notification.get_reference(), group)
         assert notification.get_subject() == "BAR-1 - hello world"
 
+    @mock.patch("sentry.notifications.notify.notify", side_effect=send_notification)
+    def test_email_notification_is_not_sent_to_deleted_email(self, mock_func):
+        """
+        Test that ensures if we still have some stale emails in UserOption, then upon attempting
+        to send an email notification to those emails, these stale `UserOption` instances are
+        deleted
+        """
+        # Initial Creation
+        user = self.create_user(email="foo@bar.dodo", is_active=True)
+        self.create_member(user=user, organization=self.organization, teams=[self.team])
+
+        UserOption.objects.create(
+            user=user, key="mail:email", value="foo@bar.dodo", project=self.project
+        )
+
+        # New secondary email is created
+        useremail = UserEmail.objects.create(user=user, email="ahmed@ahmed.io", is_verified=True)
+
+        # Set secondary email to be primary
+        user.email = useremail.email
+        user.save()
+
+        # Delete first email
+        old_useremail = UserEmail.objects.get(email="foo@bar.dodo")
+        old_useremail.delete()
+
+        event_manager = EventManager({"message": "hello world", "level": "error"})
+        event_manager.normalize()
+        event_data = event_manager.get_data()
+        event_type = get_event_type(event_data)
+        event_data["type"] = event_type.key
+        event_data["metadata"] = event_type.get_metadata(event_data)
+
+        event = event_manager.save(self.project.id)
+
+        with self.tasks():
+            AlertRuleNotification(Notification(event=event), ActionTargetType.ISSUE_OWNERS).send()
+
+        assert mock_func.call_count == 1
+
+        args, kwargs = mock_func.call_args
+        notification = args[1]
+
+        user_ids = []
+        for user in list(notification.get_participants().values())[0]:
+            user_ids.append(user.id)
+        assert list(get_email_addresses(user_ids, self.project).values())[1] == "ahmed@ahmed.io"
+        assert not len(UserOption.objects.filter(key="mail:email", value="foo@bar.dodo"))
+
     @mock.patch("sentry.notifications.notify.notify", side_effect=send_notification)
     def test_multiline_error(self, mock_func):
         event_manager = EventManager({"message": "hello world\nfoo bar", "level": "error"})