diff --git a/ietf/ipr/mail.py b/ietf/ipr/mail.py index 842426d820..167b11956c 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", "") + 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 0b15fb0651..616cade5c4 100644 --- a/ietf/ipr/management/commands/process_email.py +++ b/ietf/ipr/management/commands/process_email.py @@ -9,7 +9,7 @@ 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 @@ def handle(self, *args, **options): 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 3c70567fd8..d72018f10b 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.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 @@ def test_notify_generic(self): ) 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 @@ def send_ipr_email_helper(self): 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 @@ def send_ipr_email_helper(self): 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 @@ def test_process_response_email_with_invalid_encoding(self): 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 @@ def test_process_response_email_uninteresting_with_invalid_encoding(self): 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 @@ def test_ingest_response_email(self, mock_process_response_email): 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 @@ def test_ingest_response_email(self, mock_process_response_email): 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 42d485ccad..8f0b9cf3f2 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)