From bbc444699a6032ef855959d81aee905775be9862 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 14 Feb 2022 12:22:54 +0000 Subject: [PATCH] Return 400 if references missing from cancel broadcast MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If someone tries to cancel a broadcast but the references don’t match and existing broadcast we correctly return a 404. If they don’t provide any references then we get an exception. This commit catches the missing references and returns a 400. I think this is more appropriate because it’s malformed request, rather than a well-formed request that doesn’t match our data. It also lets us write a more specific and helpful error message. --- app/broadcast_message/translators.py | 5 ++- app/v2/broadcast/post_broadcast.py | 5 +++ .../v2/broadcast/sample_cap_xml_documents.py | 12 ++++++- tests/app/v2/broadcast/test_post_broadcast.py | 32 ++++++++++++++++--- 4 files changed, 47 insertions(+), 7 deletions(-) diff --git a/app/broadcast_message/translators.py b/app/broadcast_message/translators.py index be75696a2..bb999fccb 100644 --- a/app/broadcast_message/translators.py +++ b/app/broadcast_message/translators.py @@ -7,7 +7,10 @@ def cap_xml_to_dict(cap_xml): return { "msgType": cap.alert.msgType.text, "reference": cap.alert.identifier.text, - "references": cap.alert.references.text, # references to previous events belonging to the same alert + "references": ( + # references to previous events belonging to the same alert + cap.alert.references.text if cap.alert.references else None + ), "cap_event": cap.alert.info.event.text, "category": cap.alert.info.category.text, "expires": cap.alert.info.expires.text, diff --git a/app/v2/broadcast/post_broadcast.py b/app/v2/broadcast/post_broadcast.py index 2be55cc30..e747076df 100644 --- a/app/v2/broadcast/post_broadcast.py +++ b/app/v2/broadcast/post_broadcast.py @@ -48,6 +48,11 @@ def create_broadcast(): validate(broadcast_json, post_broadcast_schema) if broadcast_json["msgType"] == "Cancel": + if broadcast_json["references"] is None: + raise BadRequestError( + message='Missing ', + status_code=400, + ) broadcast_message = _cancel_or_reject_broadcast( broadcast_json["references"].split(","), authenticated_service.id diff --git a/tests/app/v2/broadcast/sample_cap_xml_documents.py b/tests/app/v2/broadcast/sample_cap_xml_documents.py index 8eed119d2..146919125 100644 --- a/tests/app/v2/broadcast/sample_cap_xml_documents.py +++ b/tests/app/v2/broadcast/sample_cap_xml_documents.py @@ -45,7 +45,7 @@ WAINFLEET_CANCEL = """ Flood warning service Public - www.gov.uk/environment-agency,50385fcb0ab7aa447bbd46d848ce8466E,2020-02-16T23:01:13-00:00 + {} en-GB Met @@ -70,6 +70,16 @@ WAINFLEET_CANCEL = """ """ +WAINFLEET_CANCEL_WITH_REFERENCES = WAINFLEET_CANCEL.format( + "www.gov.uk/environment-agency,50385fcb0ab7aa447bbd46d848ce8466E,2020-02-16T23:01:13-00:00" +) +WAINFLEET_CANCEL_WITH_MISSING_REFERENCES = WAINFLEET_CANCEL.format( + "" +) +WAINFLEET_CANCEL_WITH_EMPTY_REFERENCES = WAINFLEET_CANCEL.format( + "" +) + UPDATE = """ PAAQ-4-mg5a94 diff --git a/tests/app/v2/broadcast/test_post_broadcast.py b/tests/app/v2/broadcast/test_post_broadcast.py index c42435fcf..e6e2c89be 100644 --- a/tests/app/v2/broadcast/test_post_broadcast.py +++ b/tests/app/v2/broadcast/test_post_broadcast.py @@ -156,7 +156,7 @@ def test_valid_cancel_broadcast_request_calls_validate_and_update_broadcast_mess # cancel broadcast response_for_cancel = client.post( path='/v2/broadcast', - data=sample_cap_xml_documents.WAINFLEET_CANCEL, + data=sample_cap_xml_documents.WAINFLEET_CANCEL_WITH_REFERENCES, headers=[('Content-Type', 'application/cap+xml'), auth_header], ) assert response_for_cancel.status_code == 201 @@ -167,9 +167,29 @@ def test_valid_cancel_broadcast_request_calls_validate_and_update_broadcast_mess ) +@pytest.mark.parametrize('cap_xml_document, expected_status, expected_error', ( + ( + sample_cap_xml_documents.WAINFLEET_CANCEL_WITH_REFERENCES, + 404, + [{'error': 'NoResultFound', 'message': 'No result found'}], + ), + ( + sample_cap_xml_documents.WAINFLEET_CANCEL_WITH_EMPTY_REFERENCES, + 404, + [{'error': 'NoResultFound', 'message': 'No result found'}], + ), + ( + sample_cap_xml_documents.WAINFLEET_CANCEL_WITH_MISSING_REFERENCES, + 400, + [{'error': 'BadRequestError', 'message': 'Missing '}], + ), +)) def test_cancel_request_does_not_cancel_broadcast_if_reference_does_not_match( client, - sample_broadcast_service + sample_broadcast_service, + cap_xml_document, + expected_status, + expected_error, ): auth_header = create_service_authorization_header(service_id=sample_broadcast_service.id) @@ -191,11 +211,13 @@ def test_cancel_request_does_not_cancel_broadcast_if_reference_does_not_match( # try to cancel broadcast, but reference doesn't match response_for_cancel = client.post( path='/v2/broadcast', - data=sample_cap_xml_documents.WAINFLEET_CANCEL, + data=cap_xml_document, headers=[('Content-Type', 'application/cap+xml'), auth_header], ) + response_for_cancel_json = json.loads(response_for_cancel.get_data(as_text=True)) - assert response_for_cancel.status_code == 404 + assert response_for_cancel.status_code == expected_status + assert response_for_cancel_json["errors"] == expected_error def test_cancel_request_does_not_cancel_broadcast_if_service_id_does_not_match( @@ -224,7 +246,7 @@ def test_cancel_request_does_not_cancel_broadcast_if_service_id_does_not_match( auth_header_2 = create_service_authorization_header(service_id=sample_broadcast_service_2.id) response_for_cancel = client.post( path='/v2/broadcast', - data=sample_cap_xml_documents.WAINFLEET_CANCEL, + data=sample_cap_xml_documents.WAINFLEET_CANCEL_WITH_REFERENCES, headers=[('Content-Type', 'application/cap+xml'), auth_header_2], )