fix: drop, not bounce, uninteresting ipr emails (ietf-tools#8057)

* fix: drop, not bounce, uninteresting ipr emails

* chore: log to address

* chore: unused import

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
+

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'

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,43 +749,55 @@ 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
 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 @@ 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,15 +829,23 @@ 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
         self.assertTrue(mock_process_response_email.called)
         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)

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)