refactor: adjust mail ingestion api (ietf-tools#7523)

Author: jennifer-richards

ietf/api/tests.py

@@ -10,6 +10,7 @@
 
 from importlib import import_module
 from pathlib import Path
+from random import randrange
 
 from django.apps import apps
 from django.conf import settings
@@ -1072,15 +1073,29 @@ def test_ingest_email(
         self.assertEqual(r.status_code, 400)
         self.assertFalse(any(m.called for m in mocks))
 
-        # test that valid requests call handlers appropriately
+        # bad destination
         message_b64 = base64.b64encode(b"This is a message").decode()
+        r = self.client.post(
+            url,
+            {"dest": "not-a-destination", "message": message_b64},
+            content_type="application/json",
+            headers={"X-Api-Key": "valid-token"},
+        )
+        self.assertEqual(r.status_code, 200)
+        self.assertEqual(r.headers["Content-Type"], "application/json")
+        self.assertEqual(json.loads(r.content), {"result": "bad_dest"})
+        self.assertFalse(any(m.called for m in mocks))
+
+        # test that valid requests call handlers appropriately
         r = self.client.post(
             url,
             {"dest": "iana-review", "message": message_b64},
             content_type="application/json",
             headers={"X-Api-Key": "valid-token"},
         )
         self.assertEqual(r.status_code, 200)
+        self.assertEqual(r.headers["Content-Type"], "application/json")
+        self.assertEqual(json.loads(r.content), {"result": "ok"})
         self.assertTrue(mock_iana_ingest.called)
         self.assertEqual(mock_iana_ingest.call_args, mock.call(b"This is a message"))
         self.assertFalse(any(m.called for m in (mocks - {mock_iana_ingest})))
@@ -1093,20 +1108,44 @@ def test_ingest_email(
             headers={"X-Api-Key": "valid-token"},
         )
         self.assertEqual(r.status_code, 200)
+        self.assertEqual(r.headers["Content-Type"], "application/json")
+        self.assertEqual(json.loads(r.content), {"result": "ok"})
         self.assertTrue(mock_ipr_ingest.called)
         self.assertEqual(mock_ipr_ingest.call_args, mock.call(b"This is a message"))
         self.assertFalse(any(m.called for m in (mocks - {mock_ipr_ingest})))
         mock_ipr_ingest.reset_mock()
 
+        # bad nomcom-feedback dest
+        for bad_nomcom_dest in [
+            "nomcom-feedback",  # no suffix
+            "nomcom-feedback-",  # no year
+            "nomcom-feedback-squid",  # not a year,
+            "nomcom-feedback-2024-2025",  # also not a year
+        ]:
+            r = self.client.post(
+                url,
+                {"dest": bad_nomcom_dest, "message": message_b64},
+                content_type="application/json",
+                headers={"X-Api-Key": "valid-token"},
+            )
+            self.assertEqual(r.status_code, 200)
+            self.assertEqual(r.headers["Content-Type"], "application/json")
+            self.assertEqual(json.loads(r.content), {"result": "bad_dest"})
+            self.assertFalse(any(m.called for m in mocks))
+
+        # good nomcom-feedback dest
+        random_year = randrange(100000)
         r = self.client.post(
             url,
-            {"dest": "nomcom-feedback", "message": message_b64, "year": 2024},  # arbitrary year
+            {"dest": f"nomcom-feedback-{random_year}", "message": message_b64},
             content_type="application/json",
             headers={"X-Api-Key": "valid-token"},
         )
         self.assertEqual(r.status_code, 200)
+        self.assertEqual(r.headers["Content-Type"], "application/json")
+        self.assertEqual(json.loads(r.content), {"result": "ok"})
         self.assertTrue(mock_nomcom_ingest.called)
-        self.assertEqual(mock_nomcom_ingest.call_args, mock.call(b"This is a message", 2024))
+        self.assertEqual(mock_nomcom_ingest.call_args, mock.call(b"This is a message", random_year))
         self.assertFalse(any(m.called for m in (mocks - {mock_nomcom_ingest})))
         mock_nomcom_ingest.reset_mock()
 
@@ -1118,7 +1157,9 @@ def test_ingest_email(
             content_type="application/json",
             headers={"X-Api-Key": "valid-token"},
         )
-        self.assertEqual(r.status_code, 400)
+        self.assertEqual(r.status_code, 200)
+        self.assertEqual(r.headers["Content-Type"], "application/json")
+        self.assertEqual(json.loads(r.content), {"result": "bad_msg"})
         self.assertTrue(mock_iana_ingest.called)
         self.assertEqual(mock_iana_ingest.call_args, mock.call(b"This is a message"))
         self.assertFalse(any(m.called for m in (mocks - {mock_iana_ingest})))
@@ -1138,7 +1179,9 @@ def test_ingest_email(
                 content_type="application/json",
                 headers={"X-Api-Key": "valid-token"},
             )
-        self.assertEqual(r.status_code, 400)
+        self.assertEqual(r.status_code, 200)
+        self.assertEqual(r.headers["Content-Type"], "application/json")
+        self.assertEqual(json.loads(r.content), {"result": "bad_msg"})
         self.assertTrue(mock_iana_ingest.called)
         self.assertEqual(mock_iana_ingest.call_args, mock.call(b"This is a message"))
         self.assertFalse(any(m.called for m in (mocks - {mock_iana_ingest})))
@@ -1167,7 +1210,9 @@ def test_ingest_email(
                 content_type="application/json",
                 headers={"X-Api-Key": "valid-token"},
             )
-        self.assertEqual(r.status_code, 400)
+        self.assertEqual(r.status_code, 200)
+        self.assertEqual(r.headers["Content-Type"], "application/json")
+        self.assertEqual(json.loads(r.content), {"result": "bad_msg"})
         self.assertTrue(mock_iana_ingest.called)
         self.assertEqual(mock_iana_ingest.call_args, mock.call(b"This is a message"))
         self.assertFalse(any(m.called for m in (mocks - {mock_iana_ingest})))
@@ -1192,7 +1237,9 @@ def test_ingest_email(
                 content_type="application/json",
                 headers={"X-Api-Key": "valid-token"},
             )
-        self.assertEqual(r.status_code, 400)
+        self.assertEqual(r.status_code, 200)
+        self.assertEqual(r.headers["Content-Type"], "application/json")
+        self.assertEqual(json.loads(r.content), {"result": "bad_msg"})
         self.assertTrue(mock_iana_ingest.called)
         self.assertEqual(mock_iana_ingest.call_args, mock.call(b"This is a message"))
         self.assertFalse(any(m.called for m in (mocks - {mock_iana_ingest})))

ietf/api/views.py

@@ -8,6 +8,7 @@
 import pytz
 import re
 
+from contextlib import suppress
 from django.conf import settings
 from django.contrib.auth import authenticate
 from django.contrib.auth.decorators import login_required
@@ -533,32 +534,13 @@ def role_holder_addresses(request):
         "type": "object",
         "properties": {
             "dest": {
-                "enum": [
-                    "iana-review",
-                    "ipr-response",
-                    "nomcom-feedback",
-                ]
+                "type": "string",
             },
             "message": {
                 "type": "string",  # base64-encoded mail message
             },
         },
         "required": ["dest", "message"],
-        "if": {
-            # If dest == "nomcom-feedback"...
-            "properties": {
-                "dest": {"const": "nomcom-feedback"},
-            }
-        },
-        "then": {
-            # ... then also require year, an integer, be present
-            "properties": {
-                "year": {
-                    "type": "integer",
-                },
-            },
-            "required": ["year"],
-        },
     }
 )
 
@@ -630,49 +612,63 @@ def as_emailmessage(self) -> Optional[EmailMessage]:
 @requires_api_token
 @csrf_exempt
 def ingest_email(request):
+    """Ingest incoming email
+    
+    Returns a 4xx or 5xx status code if the HTTP request was invalid or something went
+    wrong while processing it. If the request was valid, returns a 200. This may or may
+    not indicate that the message was accepted.
+    """
 
-    def _err(code, text):
+    def _http_err(code, text):
         return HttpResponse(text, status=code, content_type="text/plain")
 
+    def _api_response(result):
+        return JsonResponse(data={"result": result})
+
     if request.method != "POST":
-        return _err(405, "Method not allowed")
+        return _http_err(405, "Method not allowed")
 
     if request.content_type != "application/json":
-        return _err(415, "Content-Type must be application/json")
+        return _http_err(415, "Content-Type must be application/json")
 
     # Validate
     try:
         payload = json.loads(request.body)
         _response_email_json_validator.validate(payload)
     except json.decoder.JSONDecodeError as err:
-        return _err(400, f"JSON parse error at line {err.lineno} col {err.colno}: {err.msg}")
+        return _http_err(400, f"JSON parse error at line {err.lineno} col {err.colno}: {err.msg}")
     except jsonschema.exceptions.ValidationError as err:
-        return _err(400, f"JSON schema error at {err.json_path}: {err.message}")
+        return _http_err(400, f"JSON schema error at {err.json_path}: {err.message}")
     except Exception:
-        return _err(400, "Invalid request format")
+        return _http_err(400, "Invalid request format")
 
     try:
         message = base64.b64decode(payload["message"], validate=True)
     except binascii.Error:
-        return _err(400, "Invalid message: bad base64 encoding")
+        return _http_err(400, "Invalid message: bad base64 encoding")
 
     dest = payload["dest"]
+    valid_dest = False
     try:
         if dest == "iana-review":
+            valid_dest = True
             iana_ingest_review_email(message)
         elif dest == "ipr-response":
+            valid_dest = True
             ipr_ingest_response_email(message)
-        elif dest == "nomcom-feedback":
-            year = payload["year"]
-            nomcom_ingest_feedback_email(message, year)
-        else:
-            # Should never get here - json schema validation should enforce the enum
-            log.unreachable(date="2024-04-04")
-            return _err(400, "Invalid dest")  # return something reasonable if we got here unexpectedly
+        elif dest.startswith("nomcom-feedback-"):
+            maybe_year = dest[len("nomcom-feedback-"):]
+            if maybe_year.isdecimal():
+                valid_dest = True
+                nomcom_ingest_feedback_email(message, int(maybe_year))
     except EmailIngestionError as err:
         error_email = err.as_emailmessage()
         if error_email is not None:
-            send_smtp(error_email)
-        return _err(400, err.msg)
+            with suppress(Exception): # send_smtp logs its own exceptions, ignore them here
+                send_smtp(error_email)
+        return _api_response("bad_msg")
+
+    if not valid_dest:
+        return _api_response("bad_dest")
 
-    return HttpResponse(status=200)
+    return _api_response("ok")