From 9c56ba9a0a53b6b92aa1cc0ac97764e976bf1463 Mon Sep 17 00:00:00 2001
From: Jennifer Richards <jennifer@staff.ietf.org>
Date: Thu, 17 Oct 2024 18:23:34 -0300
Subject: [PATCH] fix: drop, not bounce, uninteresting ipr emails (#8057)
* fix: drop, not bounce, uninteresting ipr emails
* chore: log to address
* chore: unused import
---
ietf/ipr/mail.py | 35 +++++++----
ietf/ipr/management/commands/process_email.py | 4 +-
ietf/ipr/tests.py | 59 +++++++++++++------
ietf/ipr/utils.py | 13 ++--
4 files changed, 72 insertions(+), 39 deletions(-)
diff --git a/ietf/ipr/mail.py b/ietf/ipr/mail.py
index 842426d82..167b11956 100644
--- a/ietf/ipr/mail.py
+++ b/ietf/ipr/mail.py
@@ -171,31 +171,44 @@ def message_from_message(message,by=None):
)
return msg
+
+class UndeliverableIprResponseError(Exception):
+ """Response email could not be delivered and should be treated as an error"""
+
+
def process_response_email(msg):
- """Saves an incoming message. msg=string. Message "To" field is expected to
- be in the format ietf-ipr+[identifier]@ietf.org. Expect to find a message with
- a matching value in the reply_to field, associated to an IPR disclosure through
- IprEvent. Create a Message object for the incoming message and associate it to
- the original message via new IprEvent"""
+ """Save an incoming IPR response email message
+
+ Message "To" field is expected to be in the format ietf-ipr+[identifier]@ietf.org. If
+ the address or identifier is missing, the message will be silently dropped.
+
+ Expect to find a message with a matching value in the reply_to field, associated to an
+ IPR disclosure through IprEvent. If it cannot be matched, raises UndeliverableIprResponseError
+
+ Creates a Message object for the incoming message and associates it to
+ the original message via new IprEvent
+ """
message = message_from_bytes(force_bytes(msg))
to = message.get('To', '')
# exit if this isn't a response we're interested in (with plus addressing)
- local,domain = get_base_ipr_request_address().split('@')
+ local, domain = get_base_ipr_request_address().split('@')
if not re.match(r'^{}\+[a-zA-Z0-9_\-]{}@{}'.format(local,'{16}',domain),to):
- return None
-
+ _from = message.get("From", "<unknown>")
+ log(f"Ignoring IPR email without a message identifier from {_from} to {to}")
+ return
+
try:
to_message = Message.objects.get(reply_to=to)
except Message.DoesNotExist:
log('Error finding matching message ({})'.format(to))
- return None
+ raise UndeliverableIprResponseError(f"Unable to find message matching {to}")
try:
disclosure = to_message.msgevents.first().disclosure
except:
log('Error processing message ({})'.format(to))
- return None
+ raise UndeliverableIprResponseError("Error processing message for {to}")
ietf_message = message_from_message(message)
IprEvent.objects.create(
@@ -207,4 +220,4 @@ def process_response_email(msg):
)
log("Received IPR email from %s" % ietf_message.frm)
- return ietf_message
+
diff --git a/ietf/ipr/management/commands/process_email.py b/ietf/ipr/management/commands/process_email.py
index 0b15fb065..616cade5c 100644
--- a/ietf/ipr/management/commands/process_email.py
+++ b/ietf/ipr/management/commands/process_email.py
@@ -9,7 +9,7 @@ from textwrap import dedent
from django.core.management import CommandError
from ietf.utils.management.base import EmailOnFailureCommand
-from ietf.ipr.mail import process_response_email
+from ietf.ipr.mail import process_response_email, UndeliverableIprResponseError
import debug # pyflakes:ignore
@@ -31,7 +31,7 @@ class Command(EmailOnFailureCommand):
self.msg_bytes = sys.stdin.buffer.read()
try:
process_response_email(self.msg_bytes)
- except ValueError as e:
+ except (ValueError, UndeliverableIprResponseError) as e:
raise CommandError(e)
failure_subject = 'Error during ipr email processing'
diff --git a/ietf/ipr/tests.py b/ietf/ipr/tests.py
index 3c70567fd..d72018f10 100644
--- a/ietf/ipr/tests.py
+++ b/ietf/ipr/tests.py
@@ -4,6 +4,7 @@
import datetime
import mock
+import re
from pyquery import PyQuery
from urllib.parse import quote, urlparse
@@ -35,9 +36,9 @@ from ietf.ipr.factories import (
)
from ietf.ipr.forms import DraftForm, HolderIprDisclosureForm
from ietf.ipr.mail import (process_response_email, get_reply_to, get_update_submitter_emails,
- get_pseudo_submitter, get_holders, get_update_cc_addrs)
-from ietf.ipr.models import (IprDisclosureBase,GenericIprDisclosure,HolderIprDisclosure,
- ThirdPartyIprDisclosure)
+ get_pseudo_submitter, get_holders, get_update_cc_addrs, UndeliverableIprResponseError)
+from ietf.ipr.models import (IprDisclosureBase, GenericIprDisclosure, HolderIprDisclosure,
+ ThirdPartyIprDisclosure, IprEvent)
from ietf.ipr.templatetags.ipr_filters import no_revisions_message
from ietf.ipr.utils import get_genitive, get_ipr_summary, ingest_response_email
from ietf.mailtrigger.utils import gather_address_lists
@@ -712,7 +713,7 @@ I would like to revoke this declaration.
)
self.assertIn(f'{settings.IDTRACKER_BASE_URL}{urlreverse("ietf.ipr.views.showlist")}', get_payload_text(outbox[1]).replace('\n',' '))
- def send_ipr_email_helper(self):
+ def send_ipr_email_helper(self) -> tuple[str, IprEvent, HolderIprDisclosure]:
ipr = HolderIprDisclosureFactory()
url = urlreverse('ietf.ipr.views.email',kwargs={ "id": ipr.id })
self.client.login(username="secretary", password="secretary+password")
@@ -730,10 +731,11 @@ I would like to revoke this declaration.
q = Message.objects.filter(reply_to=data['reply_to'])
self.assertEqual(q.count(),1)
event = q[0].msgevents.first()
+ assert event is not None
self.assertTrue(event.response_past_due())
self.assertEqual(len(outbox), 1)
self.assertTrue('joe@test.com' in outbox[0]['To'])
- return data['reply_to'], event
+ return data['reply_to'], event, ipr
uninteresting_ipr_message_strings = [
("To: {to}\nCc: {cc}\nFrom: joe@test.com\nDate: {date}\nSubject: test\n"),
@@ -747,34 +749,46 @@ I would like to revoke this declaration.
def test_process_response_email(self):
# first send a mail
- reply_to, event = self.send_ipr_email_helper()
+ reply_to, event, _ = self.send_ipr_email_helper()
# test process response uninteresting messages
addrs = gather_address_lists('ipr_disclosure_submitted').as_strings()
for message_string in self.uninteresting_ipr_message_strings:
- result = process_response_email(
+ process_response_email(
message_string.format(
to=addrs.to,
cc=addrs.cc,
date=timezone.now().ctime()
)
)
- self.assertIsNone(result)
-
+
# test process response
message_string = """To: {}
From: joe@test.com
Date: {}
Subject: test
""".format(reply_to, timezone.now().ctime())
- result = process_response_email(message_string)
-
- self.assertIsInstance(result, Message)
+ process_response_email(message_string)
self.assertFalse(event.response_past_due())
+ # test with an unmatchable message identifier
+ bad_reply_to = re.sub(
+ r"\+.{16}@",
+ '+0123456789abcdef@',
+ reply_to,
+ )
+ self.assertNotEqual(reply_to, bad_reply_to)
+ message_string = f"""To: {bad_reply_to}
+ From: joe@test.com
+ Date: {timezone.now().ctime()}
+ Subject: test
+ """
+ with self.assertRaises(UndeliverableIprResponseError):
+ process_response_email(message_string)
+
def test_process_response_email_with_invalid_encoding(self):
"""Interesting emails with invalid encoding should be handled"""
- reply_to, _ = self.send_ipr_email_helper()
+ reply_to, _, disclosure = self.send_ipr_email_helper()
# test process response
message_string = """To: {}
From: joe@test.com
@@ -782,8 +796,8 @@ Date: {}
Subject: test
""".format(reply_to, timezone.now().ctime())
message_bytes = message_string.encode('utf8') + b'\nInvalid stuff: \xfe\xff\n'
- result = process_response_email(message_bytes)
- self.assertIsInstance(result, Message)
+ process_response_email(message_bytes)
+ result = IprEvent.objects.filter(disclosure=disclosure).first().message # newest
# \ufffd is a rhombus character with an inverse ?, used to replace invalid characters
self.assertEqual(result.body, 'Invalid stuff: \ufffd\ufffd\n\n', # not sure where the extra \n is from
'Invalid characters should be replaced with \ufffd characters')
@@ -798,8 +812,7 @@ Subject: test
cc=addrs.cc,
date=timezone.now().ctime(),
).encode('utf8') + b'\nInvalid stuff: \xfe\xff\n'
- result = process_response_email(message_bytes)
- self.assertIsNone(result)
+ process_response_email(message_bytes)
@override_settings(ADMINS=(("Some Admin", "admin@example.com"),))
@mock.patch("ietf.ipr.utils.process_response_email")
@@ -816,8 +829,8 @@ Subject: test
self.assertEqual(mock_process_response_email.call_args, mock.call(message))
mock_process_response_email.reset_mock()
- mock_process_response_email.side_effect = None
- mock_process_response_email.return_value = None # rejected message
+ mock_process_response_email.side_effect = UndeliverableIprResponseError
+ mock_process_response_email.return_value = None
with self.assertRaises(EmailIngestionError) as context:
ingest_response_email(message)
self.assertIsNone(context.exception.as_emailmessage()) # should not send an email on a clean rejection
@@ -825,6 +838,14 @@ Subject: test
self.assertEqual(mock_process_response_email.call_args, mock.call(message))
mock_process_response_email.reset_mock()
+ mock_process_response_email.side_effect = None
+ mock_process_response_email.return_value = None # ignored message
+ ingest_response_email(message) # should not raise an exception
+ self.assertIsNone(context.exception.as_emailmessage()) # should not send an email on ignored message
+ self.assertTrue(mock_process_response_email.called)
+ self.assertEqual(mock_process_response_email.call_args, mock.call(message))
+ mock_process_response_email.reset_mock()
+
# successful operation
mock_process_response_email.return_value = MessageFactory()
ingest_response_email(message)
diff --git a/ietf/ipr/utils.py b/ietf/ipr/utils.py
index 42d485cca..8f0b9cf3f 100644
--- a/ietf/ipr/utils.py
+++ b/ietf/ipr/utils.py
@@ -3,7 +3,7 @@
from textwrap import dedent
-from ietf.ipr.mail import process_response_email
+from ietf.ipr.mail import process_response_email, UndeliverableIprResponseError
from ietf.ipr.models import IprDocRel
import debug # pyflakes:ignore
@@ -92,7 +92,11 @@ def generate_draft_recursive_txt():
def ingest_response_email(message: bytes):
from ietf.api.views import EmailIngestionError # avoid circular import
try:
- result = process_response_email(message)
+ process_response_email(message)
+ except UndeliverableIprResponseError:
+ # Message was rejected due to some problem the sender can fix, so bounce but don't send
+ # an email to the admins
+ raise EmailIngestionError("IPR response rejected", email_body=None)
except Exception as err:
# Message was rejected due to an unhandled exception. This is likely something
# the admins need to address, so send them a copy of the email.
@@ -106,8 +110,3 @@ def ingest_response_email(message: bytes):
email_original_message=message,
email_attach_traceback=True,
) from err
-
- if result is None:
- # Message was rejected due to some problem the sender can fix, so bounce but don't send
- # an email to the admins
- raise EmailIngestionError("IPR response rejected", email_body=None)