From 12c70a3a32766547407e36a902bd55bdd2ca5a7b Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Mon, 27 Sep 2021 14:38:03 -0700 Subject: [PATCH 1/9] move parse_email to utils --- src/sentry/integrations/vsts/webhooks.py | 6 +----- src/sentry/utils/email/__init__.py | 3 ++- src/sentry/utils/email/address.py | 8 ++++++++ 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/sentry/integrations/vsts/webhooks.py b/src/sentry/integrations/vsts/webhooks.py index e491bb929e5205..cf609cdde0da80 100644 --- a/src/sentry/integrations/vsts/webhooks.py +++ b/src/sentry/integrations/vsts/webhooks.py @@ -11,12 +11,11 @@ from sentry.integrations.utils import sync_group_assignee_inbound from sentry.models import Identity, Integration, OrganizationIntegration from sentry.models.apitoken import generate_token +from sentry.utils.email import parse_email from .client import VstsApiClient UNSET = object() -# Pull email from the string: u'lauryn ' -EMAIL_PARSER = re.compile(r"<(.*)>") logger = logging.getLogger("sentry.integrations") PROVIDER_KEY = "vsts" @@ -186,9 +185,6 @@ def handle_status_change( installation.sync_status_inbound(external_issue_key, data) - def parse_email(self, email: str) -> str: - # TODO(mgaeta): This is too brittle and doesn't pass types. - return EMAIL_PARSER.search(email).group(1) # type: ignore def create_subscription( self, instance: Optional[str], identity_data: Mapping[str, Any], oauth_redirect_url: str diff --git a/src/sentry/utils/email/__init__.py b/src/sentry/utils/email/__init__.py index 03036417051ef9..af0dfd6908bde6 100644 --- a/src/sentry/utils/email/__init__.py +++ b/src/sentry/utils/email/__init__.py @@ -7,6 +7,7 @@ "group_id_to_email", "inline_css", "is_smtp_enabled", + "parse_email", "ListResolver", "MessageBuilder", "PreviewBackend", @@ -20,7 +21,7 @@ user emails in the database. """ -from .address import email_to_group_id, group_id_to_email +from .address import email_to_group_id, group_id_to_email, parse_email from .backend import PreviewBackend, is_smtp_enabled from .faker import create_fake_email from .list_resolver import ListResolver diff --git a/src/sentry/utils/email/address.py b/src/sentry/utils/email/address.py index 07ecf0f4ad71f9..cf76cdfc14cfea 100644 --- a/src/sentry/utils/email/address.py +++ b/src/sentry/utils/email/address.py @@ -1,3 +1,4 @@ +import re from email.utils import parseaddr from django.conf import settings @@ -11,6 +12,8 @@ # This is just a tuple of (email, email-domain) _from_email_domain_cache = (None, None) +# Pull email from the string: u'lauryn ' +EMAIL_PARSER = re.compile(r"<(.*)>") signer = _CaseInsensitiveSigner() @@ -54,3 +57,8 @@ def domain_from_email(email): def is_valid_email_address(value: str) -> bool: return not settings.INVALID_EMAIL_ADDRESS_PATTERN.search(value) + + +def parse_email(email: str) -> str: + # TODO(mgaeta): This is too brittle and doesn't pass types. + return EMAIL_PARSER.search(email).group(1) # type: ignore From fa96373c471795bf61e6dd913fe93a9c3d35539a Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Mon, 8 Nov 2021 13:17:02 -0800 Subject: [PATCH 2/9] f-strings --- src/sentry/utils/email/manager.py | 2 +- src/sentry/utils/email/message_builder.py | 2 +- src/sentry/utils/email/signer.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/sentry/utils/email/manager.py b/src/sentry/utils/email/manager.py index 89a9659c03a361..96f722429530cf 100644 --- a/src/sentry/utils/email/manager.py +++ b/src/sentry/utils/email/manager.py @@ -35,7 +35,7 @@ def get_email_addresses(user_ids: Iterable[int], project: Project = None) -> Map if pending: logger.warning( - "Could not resolve email addresses for user IDs in %r, discarding...", pending + f"Could not resolve email addresses for user IDs in {pending}, discarding..." ) return results diff --git a/src/sentry/utils/email/message_builder.py b/src/sentry/utils/email/message_builder.py index 016c7a927362b3..0a11b0e748ce1a 100644 --- a/src/sentry/utils/email/message_builder.py +++ b/src/sentry/utils/email/message_builder.py @@ -211,7 +211,7 @@ def send_async(self, to=None, cc=None, bcc=None): extra = {"message_type": self.type} loggable = [v for k, v in self.context.items() if hasattr(v, "id")] for context in loggable: - extra["%s_id" % type(context).__name__.lower()] = context.id + extra[f"{type(context).__name__.lower()}_id"] = context.id log_mail_queued = partial(logger.info, "mail.queued", extra=extra) for message in messages: diff --git a/src/sentry/utils/email/signer.py b/src/sentry/utils/email/signer.py index f285b3a4855853..3ce2411a55d108 100644 --- a/src/sentry/utils/email/signer.py +++ b/src/sentry/utils/email/signer.py @@ -26,7 +26,7 @@ def unsign(self, signed_value): # See: https://github.com/django/django/blob/1.6.11/django/core/signing.py#L165-L172 signed_value = force_str(signed_value) if self.sep not in signed_value: - raise BadSignature('No "%s" found in value' % self.sep) + raise BadSignature(f'No "{self.sep}" found in value') value, sig = signed_value.rsplit(self.sep, 1) if constant_time_compare(sig.lower(), self.signature(value)): return force_text(value) From 18278f17e33d3db9a86fadc1c8097487bd3e1979 Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Mon, 8 Nov 2021 13:18:58 -0800 Subject: [PATCH 3/9] Add type annotations --- mypy.ini | 5 ++ src/sentry/utils/email/address.py | 10 ++- src/sentry/utils/email/backend.py | 14 ++- src/sentry/utils/email/faker.py | 4 +- src/sentry/utils/email/list_resolver.py | 15 +++- src/sentry/utils/email/manager.py | 6 +- src/sentry/utils/email/message_builder.py | 102 +++++++++++++--------- src/sentry/utils/email/send.py | 25 ++++-- src/sentry/utils/email/signer.py | 18 ++-- src/sentry/utils/strings.py | 4 +- 10 files changed, 133 insertions(+), 70 deletions(-) diff --git a/mypy.ini b/mypy.ini index c06213e612e10d..7fcaf63fbc1b25 100644 --- a/mypy.ini +++ b/mypy.ini @@ -66,6 +66,7 @@ files = src/sentry/api/bases/external_actor.py, src/sentry/utils/avatar.py, src/sentry/utils/codecs.py, src/sentry/utils/dates.py, + src/sentry/utils/email/, src/sentry/utils/jwt.py, src/sentry/utils/kvstore, src/sentry/utils/time_window.py, @@ -104,6 +105,8 @@ ignore_missing_imports = True ignore_missing_imports = True [mypy-jsonschema] ignore_missing_imports = True +[mypy-lxml] +ignore_missing_imports = True [mypy-mistune.*] ignore_missing_imports = True [mypy-rb.*] @@ -112,5 +115,7 @@ ignore_missing_imports = True ignore_missing_imports = True [mypy-sentry_relay.*] ignore_missing_imports = True +[mypy-toronado] +ignore_missing_imports = True [mypy-zstandard] ignore_missing_imports = True diff --git a/src/sentry/utils/email/address.py b/src/sentry/utils/email/address.py index cf76cdfc14cfea..7c8ba1d5b6efc6 100644 --- a/src/sentry/utils/email/address.py +++ b/src/sentry/utils/email/address.py @@ -1,3 +1,5 @@ +from __future__ import annotations + import re from email.utils import parseaddr @@ -18,7 +20,7 @@ signer = _CaseInsensitiveSigner() -def get_from_email_domain(): +def get_from_email_domain() -> str: global _from_email_domain_cache from_ = options.get("mail.from") if not _from_email_domain_cache[0] == from_: @@ -26,7 +28,7 @@ def get_from_email_domain(): return _from_email_domain_cache[1] -def email_to_group_id(address): +def email_to_group_id(address: str) -> int: """ Email address should be in the form of: {group_id}+{signature}@example.com @@ -36,7 +38,7 @@ def email_to_group_id(address): return int(force_bytes(signer.unsign(signed_data))) -def group_id_to_email(group_id): +def group_id_to_email(group_id: int) -> str: signed_data = signer.sign(str(group_id)) return "@".join( ( @@ -46,7 +48,7 @@ def group_id_to_email(group_id): ) -def domain_from_email(email): +def domain_from_email(email: str) -> str: email = parseaddr(email)[1] try: return email.split("@", 1)[1] diff --git a/src/sentry/utils/email/backend.py b/src/sentry/utils/email/backend.py index cddb863e49dd5a..824bc147f7effd 100644 --- a/src/sentry/utils/email/backend.py +++ b/src/sentry/utils/email/backend.py @@ -1,20 +1,26 @@ +from __future__ import annotations + import subprocess import tempfile +from typing import Any, Sequence from django.conf import settings +from django.core.mail import EmailMultiAlternatives from django.core.mail.backends.base import BaseEmailBackend from sentry import options +Backend = Any + -def is_smtp_enabled(backend=None): +def is_smtp_enabled(backend: Backend | None = None) -> bool: """Check if the current backend is SMTP based.""" if backend is None: backend = get_mail_backend() return backend not in settings.SENTRY_SMTP_DISABLED_BACKENDS -def get_mail_backend(): +def get_mail_backend() -> Backend: backend = options.get("mail.backend") try: return settings.SENTRY_EMAIL_BACKEND_ALIASES[backend] @@ -22,7 +28,7 @@ def get_mail_backend(): return backend -class PreviewBackend(BaseEmailBackend): +class PreviewBackend(BaseEmailBackend): # type: ignore """ Email backend that can be used in local development to open messages in the local mail client as they are sent. @@ -30,7 +36,7 @@ class PreviewBackend(BaseEmailBackend): Probably only works on OS X. """ - def send_messages(self, email_messages): + def send_messages(self, email_messages: Sequence[EmailMultiAlternatives]) -> int: for message in email_messages: content = bytes(message.message()) preview = tempfile.NamedTemporaryFile( diff --git a/src/sentry/utils/email/faker.py b/src/sentry/utils/email/faker.py index cc8e577f466990..4eb4e83d83281b 100644 --- a/src/sentry/utils/email/faker.py +++ b/src/sentry/utils/email/faker.py @@ -3,7 +3,7 @@ FAKE_EMAIL_TLD = ".sentry-fake" -def create_fake_email(unique_id, namespace): +def create_fake_email(unique_id: str, namespace: str) -> str: """ Generate a fake email of the form: {unique_id}@{namespace}{FAKE_EMAIL_TLD} @@ -12,6 +12,6 @@ def create_fake_email(unique_id, namespace): return f"{unique_id}@{namespace}{FAKE_EMAIL_TLD}" -def is_fake_email(email): +def is_fake_email(email: str) -> bool: """Returns True if the provided email matches the fake email pattern.""" return email.endswith(FAKE_EMAIL_TLD) diff --git a/src/sentry/utils/email/list_resolver.py b/src/sentry/utils/email/list_resolver.py index b82d887f7b1569..fdf267b267acea 100644 --- a/src/sentry/utils/email/list_resolver.py +++ b/src/sentry/utils/email/list_resolver.py @@ -1,7 +1,14 @@ +from __future__ import annotations + +from collections import Mapping +from typing import Any, Callable, Generic + +from sentry.db.models import Model +from sentry.db.models.manager import M from sentry.utils.strings import is_valid_dot_atom -class ListResolver: +class ListResolver(Generic[M]): """ Manages the generation of RFC 2919 compliant list-id strings from varying objects types. @@ -12,7 +19,9 @@ class UnregisteredTypeError(Exception): Error raised when attempting to build a list-id from an unregistered object type. """ - def __init__(self, namespace, type_handlers): + def __init__( + self, namespace: str, type_handlers: Mapping[type[Model], Callable[[M], Any]] + ) -> None: assert is_valid_dot_atom(namespace) # The list-id-namespace that will be used when generating the list-id @@ -26,7 +35,7 @@ def __init__(self, namespace, type_handlers): # values. self.__type_handlers = type_handlers - def __call__(self, instance): + def __call__(self, instance: M) -> str: """ Build a list-id string from an instance. diff --git a/src/sentry/utils/email/manager.py b/src/sentry/utils/email/manager.py index 96f722429530cf..f5beb26511e2a8 100644 --- a/src/sentry/utils/email/manager.py +++ b/src/sentry/utils/email/manager.py @@ -1,3 +1,5 @@ +from __future__ import annotations + import logging from typing import Iterable, Mapping @@ -8,7 +10,9 @@ logger = logging.getLogger("sentry.mail") -def get_email_addresses(user_ids: Iterable[int], project: Project = None) -> Mapping[int, str]: +def get_email_addresses( + user_ids: Iterable[int], project: Project | None = None +) -> Mapping[int, str]: """ Find the best email addresses for a collection of users. If a project is provided, prefer their project-specific notification preferences. diff --git a/src/sentry/utils/email/message_builder.py b/src/sentry/utils/email/message_builder.py index 0a11b0e748ce1a..721c004fb08d5d 100644 --- a/src/sentry/utils/email/message_builder.py +++ b/src/sentry/utils/email/message_builder.py @@ -1,10 +1,12 @@ +from __future__ import annotations + import logging import os import time from functools import partial from operator import attrgetter from random import randrange -from typing import Iterable, Optional +from typing import Any, Callable, Iterable, Mapping, MutableMapping, Sequence import lxml import toronado @@ -12,6 +14,7 @@ from django.utils.encoding import force_text from sentry import options +from sentry.db.models import Model from sentry.logging import LoggingFormat from sentry.models import Activity, Group, GroupEmailThread, Project from sentry.utils import metrics @@ -25,7 +28,7 @@ logger = logging.getLogger("sentry.mail") -default_list_type_handlers = { +default_list_type_handlers: Mapping[type[Model], Callable[[Model], Any]] = { Activity: attrgetter("project.slug", "project.organization.slug"), Project: attrgetter("slug", "organization.slug"), Group: attrgetter("project.slug", "organization.slug"), @@ -42,7 +45,7 @@ # Slightly modified version of Django's `django.core.mail.message:make_msgid` # because we need to override the domain. If we ever upgrade to django 1.8, we # can/should replace this. -def make_msgid(domain): +def make_msgid(domain: str) -> str: """Returns a string suitable for RFC 2822 compliant Message-ID, e.g: <20020201195627.33539.96671@nightshade.la.mastaler.com> Optional idstring if given is a string used to strengthen the @@ -63,75 +66,78 @@ def inline_css(value: str) -> str: toronado.inline(tree) # CSS media query support is inconsistent when the DOCTYPE declaration is # missing, so we force it to HTML5 here. - return lxml.html.tostring(tree, doctype="", encoding=None).decode("utf-8") + html: str = lxml.html.tostring(tree, doctype="", encoding=None).decode("utf-8") + return html class MessageBuilder: def __init__( self, - subject, - context=None, - template=None, - html_template=None, - body="", - html_body=None, - headers=None, - reference=None, - reply_reference=None, - from_email=None, - type=None, - ): + subject: str, + context: Mapping[str, Any] | None = None, + template: str | None = None, + html_template: str | None = None, + body: str = "", + html_body: str | None = None, + headers: Mapping[str, Any] | None = None, + reference: Model | None = None, + reply_reference: Model | None = None, + from_email: str | None = None, + type: str | None = None, + ) -> None: assert not (body and template) assert not (html_body and html_template) assert context or not (template or html_template) - if headers is None: - headers = {} - self.subject = subject self.context = context or {} self.template = template self.html_template = html_template self._txt_body = body self._html_body = html_body - self.headers = headers + self.headers: MutableMapping[str, Any] = {**(headers or {})} self.reference = reference # The object that generated this message self.reply_reference = reply_reference # The object this message is replying about self.from_email = from_email or options.get("mail.from") - self._send_to = set() + self._send_to: set[str] = set() self.type = type if type else "generic" - if reference is not None and "List-Id" not in headers: + if reference is not None and "List-Id" not in self.headers: try: - headers["List-Id"] = make_listid_from_instance(reference) + self.headers["List-Id"] = make_listid_from_instance(reference) except ListResolver.UnregisteredTypeError as error: logger.debug(str(error)) except AssertionError as error: logger.warning(str(error)) - def __render_html_body(self) -> str: - html_body = None + def __render_html_body(self) -> str | None: if self.html_template: html_body = render_to_string(self.html_template, self.context) else: html_body = self._html_body - if html_body is not None: - return inline_css(html_body) + if html_body is None: + return None + + return inline_css(html_body) def __render_text_body(self) -> str: if self.template: - return render_to_string(self.template, self.context) + body: str = render_to_string(self.template, self.context) + return body return self._txt_body - def add_users(self, user_ids: Iterable[int], project: Optional[Project] = None) -> None: + def add_users(self, user_ids: Iterable[int], project: Project | None = None) -> None: self._send_to.update(list(get_email_addresses(user_ids, project).values())) - def build(self, to, reply_to=None, cc=None, bcc=None): - if self.headers is None: - headers = {} - else: - headers = self.headers.copy() + def build( + self, + to: str, + reply_to: Iterable[str] | None = None, + cc: Iterable[str] | None = None, + bcc: Iterable[str] | None = None, + ) -> EmailMultiAlternatives: + headers = {**self.headers} if options.get("mail.enable-replies") and "X-Sentry-Reply-To" in headers: reply_to = headers["X-Sentry-Reply-To"] @@ -151,7 +157,7 @@ def build(self, to, reply_to=None, cc=None, bcc=None): if self.reply_reference is not None: reference = self.reply_reference - subject = "Re: %s" % subject + subject = f"Re: {subject}" else: reference = self.reference @@ -181,7 +187,12 @@ def build(self, to, reply_to=None, cc=None, bcc=None): return msg - def get_built_messages(self, to=None, cc=None, bcc=None): + def get_built_messages( + self, + to: list[str] | None = None, + cc: Iterable[str] | None = None, + bcc: Iterable[str] | None = None, + ) -> Sequence[EmailMultiAlternatives]: send_to = set(to or ()) send_to.update(self._send_to) results = [ @@ -191,24 +202,35 @@ def get_built_messages(self, to=None, cc=None, bcc=None): logger.debug("Did not build any messages, no users to send to.") return results - def format_to(self, to): + def format_to(self, to: list[str]) -> str: if not to: return "" if len(to) > MAX_RECIPIENTS: to = to[:MAX_RECIPIENTS] + [f"and {len(to[MAX_RECIPIENTS:])} more."] return ", ".join(to) - def send(self, to=None, cc=None, bcc=None, fail_silently=False): + def send( + self, + to: list[str] | None = None, + cc: Iterable[str] | None = None, + bcc: Iterable[str] | None = None, + fail_silently: bool = False, + ) -> int: return send_messages( self.get_built_messages(to, cc=cc, bcc=bcc), fail_silently=fail_silently ) - def send_async(self, to=None, cc=None, bcc=None): + def send_async( + self, + to: list[str] | None = None, + cc: Iterable[str] | None = None, + bcc: Iterable[str] | None = None, + ) -> None: from sentry.tasks.email import send_email fmt = options.get("system.logging-format") messages = self.get_built_messages(to, cc=cc, bcc=bcc) - extra = {"message_type": self.type} + extra: MutableMapping[str, str | tuple[str]] = {"message_type": self.type} loggable = [v for k, v in self.context.items() if hasattr(v, "id")] for context in loggable: extra[f"{type(context).__name__.lower()}_id"] = context.id diff --git a/src/sentry/utils/email/send.py b/src/sentry/utils/email/send.py index 91b074a9656705..f005ad4392bba7 100644 --- a/src/sentry/utils/email/send.py +++ b/src/sentry/utils/email/send.py @@ -1,6 +1,8 @@ import logging +from typing import Any, Sequence from django.core import mail +from django.core.mail import EmailMultiAlternatives from sentry import options from sentry.utils import metrics @@ -10,9 +12,10 @@ logger = logging.getLogger("sentry.mail") -def send_messages(messages, fail_silently=False): +def send_messages(messages: Sequence[EmailMultiAlternatives], fail_silently: bool = False) -> int: connection = get_connection(fail_silently=fail_silently) - sent = connection.send_messages(messages) + # Explicitly typing to satisfy mypy. + sent: int = connection.send_messages(messages) metrics.incr("email.sent", len(messages), skip_internal=False) for message in messages: extra = { @@ -23,7 +26,7 @@ def send_messages(messages, fail_silently=False): return sent -def get_connection(fail_silently=False): +def get_connection(fail_silently: bool = False) -> Any: """Gets an SMTP connection using our OptionsStore.""" return mail.get_connection( backend=get_mail_backend(), @@ -38,17 +41,25 @@ def get_connection(fail_silently=False): ) -def send_mail(subject, message, from_email, recipient_list, fail_silently=False, **kwargs): +def send_mail( + subject: str, + message: str, + from_email: str, + recipient_list: Sequence[str], + fail_silently: bool = False, + **kwargs: Any, +) -> int: """ Wrapper that forces sending mail through our connection. Uses EmailMessage class which has more options than the simple send_mail """ - email = mail.EmailMessage( + # Explicitly typing to satisfy mypy. + sent: int = mail.EmailMessage( subject, message, from_email, recipient_list, connection=get_connection(fail_silently=fail_silently), **kwargs, - ) - return email.send(fail_silently=fail_silently) + ).send(fail_silently=fail_silently) + return sent diff --git a/src/sentry/utils/email/signer.py b/src/sentry/utils/email/signer.py index 3ce2411a55d108..1c8bf1dca4fe04 100644 --- a/src/sentry/utils/email/signer.py +++ b/src/sentry/utils/email/signer.py @@ -3,7 +3,7 @@ from django.utils.encoding import force_str, force_text -class _CaseInsensitiveSigner(Signer): +class _CaseInsensitiveSigner(Signer): # type: ignore """ Generate a signature that is comprised of only lowercase letters. @@ -17,17 +17,21 @@ class _CaseInsensitiveSigner(Signer): and sends the value as all lowercase. """ - def signature(self, value): - sig = super().signature(value) + def signature(self, value: str) -> str: + # Explicitly typing to satisfy mypy. + sig: str = super().signature(value) return sig.lower() - def unsign(self, signed_value): + def unsign(self, signed_value: str) -> str: # This `unsign` is identical to subclass except for the lower-casing # See: https://github.com/django/django/blob/1.6.11/django/core/signing.py#L165-L172 signed_value = force_str(signed_value) if self.sep not in signed_value: raise BadSignature(f'No "{self.sep}" found in value') value, sig = signed_value.rsplit(self.sep, 1) - if constant_time_compare(sig.lower(), self.signature(value)): - return force_text(value) - raise BadSignature('Signature "%s" does not match' % sig) + if not constant_time_compare(sig.lower(), self.signature(value)): + raise BadSignature(f'Signature "{sig}" does not match') + + # Explicitly typing to satisfy mypy. + val: str = force_text(value) + return val diff --git a/src/sentry/utils/strings.py b/src/sentry/utils/strings.py index e3a7984a2f4ef2..bae81d6e5a91d8 100644 --- a/src/sentry/utils/strings.py +++ b/src/sentry/utils/strings.py @@ -164,7 +164,7 @@ def tokens_from_name(value, remove_digits=False): valid_dot_atom_characters = frozenset(string.ascii_letters + string.digits + ".!#$%&'*+-/=?^_`{|}~") -def is_valid_dot_atom(value): +def is_valid_dot_atom(value: str) -> bool: """Validate an input string as an RFC 2822 dot-atom-text value.""" return ( isinstance(value, str) # must be a string type @@ -174,7 +174,7 @@ def is_valid_dot_atom(value): ) # can only contain valid characters -def count_sprintf_parameters(string): +def count_sprintf_parameters(string: str) -> int: """Counts the number of sprintf parameters in a string.""" return len(_sprintf_placeholder_re.findall(string)) From dce061f6b065e8e25e4030f423ef1fa702bdd3f0 Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Mon, 8 Nov 2021 13:19:54 -0800 Subject: [PATCH 4/9] Make tuple optional instead of a tuple of optionals --- src/sentry/utils/email/address.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sentry/utils/email/address.py b/src/sentry/utils/email/address.py index 7c8ba1d5b6efc6..6170a2158aa760 100644 --- a/src/sentry/utils/email/address.py +++ b/src/sentry/utils/email/address.py @@ -12,7 +12,7 @@ # cache the domain_from_email calculation # This is just a tuple of (email, email-domain) -_from_email_domain_cache = (None, None) +_from_email_domain_cache: tuple[str, str] | None = None # Pull email from the string: u'lauryn ' EMAIL_PARSER = re.compile(r"<(.*)>") @@ -23,7 +23,7 @@ def get_from_email_domain() -> str: global _from_email_domain_cache from_ = options.get("mail.from") - if not _from_email_domain_cache[0] == from_: + if _from_email_domain_cache is None or not _from_email_domain_cache[0] == from_: _from_email_domain_cache = (from_, domain_from_email(from_)) return _from_email_domain_cache[1] From 38c974ab93ce62f8de0a46031a83ed6a6761bb79 Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Mon, 8 Nov 2021 14:18:17 -0800 Subject: [PATCH 5/9] fix tests --- src/sentry/integrations/vsts/webhooks.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/sentry/integrations/vsts/webhooks.py b/src/sentry/integrations/vsts/webhooks.py index cf609cdde0da80..c046afd3371e9e 100644 --- a/src/sentry/integrations/vsts/webhooks.py +++ b/src/sentry/integrations/vsts/webhooks.py @@ -1,5 +1,4 @@ import logging -import re from typing import Any, Mapping, Optional from django.utils.crypto import constant_time_compare @@ -139,7 +138,7 @@ def handle_assign_to( new_value = assigned_to.get("newValue") if new_value is not None: try: - email = self.parse_email(new_value) + email = parse_email(new_value) except AttributeError as e: logger.info( "vsts.failed-to-parse-email-in-handle-assign-to", @@ -185,7 +184,6 @@ def handle_status_change( installation.sync_status_inbound(external_issue_key, data) - def create_subscription( self, instance: Optional[str], identity_data: Mapping[str, Any], oauth_redirect_url: str ) -> Response: From 731a390d53fb9f5666b234bfa05942392ea5814e Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Mon, 8 Nov 2021 16:24:41 -0800 Subject: [PATCH 6/9] fix brittle regex --- src/sentry/utils/email/address.py | 8 ++++---- tests/sentry/utils/email/test_address.py | 19 ++++++++++++++++++- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/src/sentry/utils/email/address.py b/src/sentry/utils/email/address.py index 6170a2158aa760..268f73b98b95c8 100644 --- a/src/sentry/utils/email/address.py +++ b/src/sentry/utils/email/address.py @@ -14,8 +14,8 @@ # This is just a tuple of (email, email-domain) _from_email_domain_cache: tuple[str, str] | None = None -# Pull email from the string: u'lauryn ' -EMAIL_PARSER = re.compile(r"<(.*)>") +# Pull email from the string: "lauryn " +EMAIL_PARSER = re.compile(r"<([^>]*)>") signer = _CaseInsensitiveSigner() @@ -62,5 +62,5 @@ def is_valid_email_address(value: str) -> bool: def parse_email(email: str) -> str: - # TODO(mgaeta): This is too brittle and doesn't pass types. - return EMAIL_PARSER.search(email).group(1) # type: ignore + matches = EMAIL_PARSER.search(email) + return matches.group(1) if matches else "" diff --git a/tests/sentry/utils/email/test_address.py b/tests/sentry/utils/email/test_address.py index f456f46038d920..b976b4fb166517 100644 --- a/tests/sentry/utils/email/test_address.py +++ b/tests/sentry/utils/email/test_address.py @@ -1,5 +1,5 @@ from sentry.testutils import TestCase -from sentry.utils.email.address import get_from_email_domain, is_valid_email_address +from sentry.utils.email.address import get_from_email_domain, is_valid_email_address, parse_email class GetFromEmailDomainTest(TestCase): @@ -20,3 +20,20 @@ def test_is_valid_email_address_number_at_qqcom(self): def test_is_valid_email_address_normal_human_email_address(self): assert is_valid_email_address("dcramer@gmail.com") is True + + +class ParseEmailTest(TestCase): + def test_empty(self): + assert parse_email("") == "" + + def test_no_email(self): + assert parse_email("marcos@sentry.io") == "" + + def test_empty_email(self): + assert parse_email("<>") == "" + + def test_two_emails(self): + assert parse_email("") == "a" + + def test_simple(self): + assert parse_email("lauryn ") == "lauryn@sentry.io" From 6edd32d3e365d547c08778f87045a149bf34fa6b Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Mon, 8 Nov 2021 16:35:09 -0800 Subject: [PATCH 7/9] fix list_resolver attrgetter types --- src/sentry/utils/email/list_resolver.py | 4 ++-- src/sentry/utils/email/message_builder.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/sentry/utils/email/list_resolver.py b/src/sentry/utils/email/list_resolver.py index fdf267b267acea..125c118306f41c 100644 --- a/src/sentry/utils/email/list_resolver.py +++ b/src/sentry/utils/email/list_resolver.py @@ -1,7 +1,7 @@ from __future__ import annotations from collections import Mapping -from typing import Any, Callable, Generic +from typing import Callable, Generic, Iterable from sentry.db.models import Model from sentry.db.models.manager import M @@ -20,7 +20,7 @@ class UnregisteredTypeError(Exception): """ def __init__( - self, namespace: str, type_handlers: Mapping[type[Model], Callable[[M], Any]] + self, namespace: str, type_handlers: Mapping[type[Model], Callable[[M], Iterable[str]]] ) -> None: assert is_valid_dot_atom(namespace) diff --git a/src/sentry/utils/email/message_builder.py b/src/sentry/utils/email/message_builder.py index 721c004fb08d5d..ab9761f140064c 100644 --- a/src/sentry/utils/email/message_builder.py +++ b/src/sentry/utils/email/message_builder.py @@ -28,7 +28,7 @@ logger = logging.getLogger("sentry.mail") -default_list_type_handlers: Mapping[type[Model], Callable[[Model], Any]] = { +default_list_type_handlers: Mapping[type[Model], Callable[[Model], Iterable[str]]] = { Activity: attrgetter("project.slug", "project.organization.slug"), Project: attrgetter("slug", "organization.slug"), Group: attrgetter("project.slug", "organization.slug"), From 094b7775e9e52ead2060fd40cf529144a3e853c3 Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Mon, 8 Nov 2021 16:35:22 -0800 Subject: [PATCH 8/9] fix header types --- src/sentry/utils/email/message_builder.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/utils/email/message_builder.py b/src/sentry/utils/email/message_builder.py index ab9761f140064c..004758df4aaee4 100644 --- a/src/sentry/utils/email/message_builder.py +++ b/src/sentry/utils/email/message_builder.py @@ -79,7 +79,7 @@ def __init__( html_template: str | None = None, body: str = "", html_body: str | None = None, - headers: Mapping[str, Any] | None = None, + headers: Mapping[str, str] | None = None, reference: Model | None = None, reply_reference: Model | None = None, from_email: str | None = None, From b80a70a65b7402a45a98ed1a49723d2f50c662da Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Mon, 8 Nov 2021 16:38:39 -0800 Subject: [PATCH 9/9] Make to an Iterable --- src/sentry/utils/email/message_builder.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/sentry/utils/email/message_builder.py b/src/sentry/utils/email/message_builder.py index 004758df4aaee4..69060d4f24e8ef 100644 --- a/src/sentry/utils/email/message_builder.py +++ b/src/sentry/utils/email/message_builder.py @@ -189,7 +189,7 @@ def build( def get_built_messages( self, - to: list[str] | None = None, + to: Iterable[str] | None = None, cc: Iterable[str] | None = None, bcc: Iterable[str] | None = None, ) -> Sequence[EmailMultiAlternatives]: @@ -211,7 +211,7 @@ def format_to(self, to: list[str]) -> str: def send( self, - to: list[str] | None = None, + to: Iterable[str] | None = None, cc: Iterable[str] | None = None, bcc: Iterable[str] | None = None, fail_silently: bool = False, @@ -222,7 +222,7 @@ def send( def send_async( self, - to: list[str] | None = None, + to: Iterable[str] | None = None, cc: Iterable[str] | None = None, bcc: Iterable[str] | None = None, ) -> None: