From 6528a7f3ddd51d19424df07ab5e9c77da529d167 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Tue, 11 Jun 2024 13:57:41 -0300 Subject: [PATCH 1/9] refactor: make nomcom year part of destination --- ietf/api/views.py | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/ietf/api/views.py b/ietf/api/views.py index 3c3ea0d5a9..5358f4536c 100644 --- a/ietf/api/views.py +++ b/ietf/api/views.py @@ -536,7 +536,7 @@ def role_holder_addresses(request): "enum": [ "iana-review", "ipr-response", - "nomcom-feedback", + "nomcom-feedback-2024", ] }, "message": { @@ -544,21 +544,6 @@ def role_holder_addresses(request): }, }, "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"], - }, } ) @@ -662,9 +647,8 @@ def _err(code, text): iana_ingest_review_email(message) elif dest == "ipr-response": ipr_ingest_response_email(message) - elif dest == "nomcom-feedback": - year = payload["year"] - nomcom_ingest_feedback_email(message, year) + elif dest == "nomcom-feedback-2024": + nomcom_ingest_feedback_email(message, 2024) else: # Should never get here - json schema validation should enforce the enum log.unreachable(date="2024-04-04") From 2198a7416bb1fcbad1a256c081945c18fb9159b4 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Tue, 11 Jun 2024 14:19:58 -0300 Subject: [PATCH 2/9] test: update test --- ietf/api/tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ietf/api/tests.py b/ietf/api/tests.py index 66bc7a3be7..5f620182ef 100644 --- a/ietf/api/tests.py +++ b/ietf/api/tests.py @@ -1100,7 +1100,7 @@ def test_ingest_email( r = self.client.post( url, - {"dest": "nomcom-feedback", "message": message_b64, "year": 2024}, # arbitrary year + {"dest": "nomcom-feedback-2024", "message": message_b64}, content_type="application/json", headers={"X-Api-Key": "valid-token"}, ) From b38939193718a0ea664ed85adb02e70e3df90599 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Tue, 11 Jun 2024 18:27:10 -0300 Subject: [PATCH 3/9] refactor: parse, not hardcode, nomcom year --- ietf/api/views.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/ietf/api/views.py b/ietf/api/views.py index 5358f4536c..6047083894 100644 --- a/ietf/api/views.py +++ b/ietf/api/views.py @@ -647,8 +647,11 @@ def _err(code, text): iana_ingest_review_email(message) elif dest == "ipr-response": ipr_ingest_response_email(message) - elif dest == "nomcom-feedback-2024": - nomcom_ingest_feedback_email(message, 2024) + elif dest.startswith("nomcom-feedback-"): + maybe_year = dest[len("nomcom-feedback-"):] + if maybe_year.isdecimal(): + year = int(maybe_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") From 84b8ef901798145ad814a905814879192bc9e892 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Tue, 11 Jun 2024 18:27:30 -0300 Subject: [PATCH 4/9] test: test bad email destinations --- ietf/api/tests.py | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/ietf/api/tests.py b/ietf/api/tests.py index 5f620182ef..ab12fc36a3 100644 --- a/ietf/api/tests.py +++ b/ietf/api/tests.py @@ -1072,8 +1072,18 @@ 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, 400) + 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}, @@ -1098,6 +1108,23 @@ def test_ingest_email( 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, 400) + self.assertFalse(any(m.called for m in mocks)) + + # good nomcom-feedback dest r = self.client.post( url, {"dest": "nomcom-feedback-2024", "message": message_b64}, From efe8640186ec067318a3884894ba12cdc0e44848 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Tue, 11 Jun 2024 21:23:01 -0300 Subject: [PATCH 5/9] fix: relax schema, don't use unset year --- ietf/api/tests.py | 8 +++++--- ietf/api/views.py | 20 +++++++++----------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/ietf/api/tests.py b/ietf/api/tests.py index ab12fc36a3..e535a22dfd 100644 --- a/ietf/api/tests.py +++ b/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 @@ -1121,19 +1122,20 @@ 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, 400, f"{bad_nomcom_dest} is an invalid 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-2024", "message": message_b64}, + {"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.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() diff --git a/ietf/api/views.py b/ietf/api/views.py index 6047083894..d5dbdbdbf6 100644 --- a/ietf/api/views.py +++ b/ietf/api/views.py @@ -533,11 +533,7 @@ def role_holder_addresses(request): "type": "object", "properties": { "dest": { - "enum": [ - "iana-review", - "ipr-response", - "nomcom-feedback-2024", - ] + "type": "string", }, "message": { "type": "string", # base64-encoded mail message @@ -642,24 +638,26 @@ def _err(code, text): return _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.startswith("nomcom-feedback-"): maybe_year = dest[len("nomcom-feedback-"):] if maybe_year.isdecimal(): - year = int(maybe_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 + 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) + if not valid_dest: + return _err(400, "Invalid dest") + return HttpResponse(status=200) From ff0bf16b13e217c8816110ea7c603031040fa334 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Tue, 11 Jun 2024 21:32:58 -0300 Subject: [PATCH 6/9] feat: distinguish config / user errors --- ietf/api/views.py | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/ietf/api/views.py b/ietf/api/views.py index d5dbdbdbf6..5366914ed9 100644 --- a/ietf/api/views.py +++ b/ietf/api/views.py @@ -611,31 +611,40 @@ 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 @@ -655,9 +664,9 @@ def _err(code, text): error_email = err.as_emailmessage() if error_email is not None: send_smtp(error_email) - return _err(400, err.msg) + return _api_response("bad_msg") if not valid_dest: - return _err(400, "Invalid dest") + return _api_response("bad_dest") - return HttpResponse(status=200) + return _api_response("ok") From efd5710d1887557cd6be28a505089257f96ebb22 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Tue, 11 Jun 2024 21:42:26 -0300 Subject: [PATCH 7/9] test: update test to match --- ietf/api/tests.py | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/ietf/api/tests.py b/ietf/api/tests.py index e535a22dfd..fd8eb52cd6 100644 --- a/ietf/api/tests.py +++ b/ietf/api/tests.py @@ -1081,7 +1081,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_dest"}) self.assertFalse(any(m.called for m in mocks)) # test that valid requests call handlers appropriately @@ -1092,6 +1094,8 @@ 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_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}))) @@ -1104,6 +1108,8 @@ 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}))) @@ -1122,7 +1128,9 @@ def test_ingest_email( content_type="application/json", headers={"X-Api-Key": "valid-token"}, ) - self.assertEqual(r.status_code, 400, f"{bad_nomcom_dest} is an invalid dest") + 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 @@ -1134,6 +1142,8 @@ 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_nomcom_ingest.called) 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}))) @@ -1147,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}))) @@ -1167,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}))) @@ -1196,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}))) @@ -1221,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}))) From ae0e91e04af0d90411103fea6798cdaf457152ee Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Sat, 15 Jun 2024 16:00:58 -0300 Subject: [PATCH 8/9] fix: handle send_smtp exception cleanly --- ietf/api/views.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ietf/api/views.py b/ietf/api/views.py index 5366914ed9..861961aba5 100644 --- a/ietf/api/views.py +++ b/ietf/api/views.py @@ -663,7 +663,10 @@ def _api_response(result): except EmailIngestionError as err: error_email = err.as_emailmessage() if error_email is not None: - send_smtp(error_email) + try: + send_smtp(error_email) + except Exception as err: + pass # send_smtp logs its own exception, ignore it here return _api_response("bad_msg") if not valid_dest: From bf07e281f3ca8925c52f952dd466c7c561ffa52d Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Sat, 15 Jun 2024 16:04:09 -0300 Subject: [PATCH 9/9] refactor: use suppress() instead of try... --- ietf/api/views.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/ietf/api/views.py b/ietf/api/views.py index 861961aba5..6aaed4b6a9 100644 --- a/ietf/api/views.py +++ b/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 @@ -663,10 +664,8 @@ def _api_response(result): except EmailIngestionError as err: error_email = err.as_emailmessage() if error_email is not None: - try: + with suppress(Exception): # send_smtp logs its own exceptions, ignore them here send_smtp(error_email) - except Exception as err: - pass # send_smtp logs its own exception, ignore it here return _api_response("bad_msg") if not valid_dest: