From df82b514d1805afad4e20e8431877dfe1bec1950 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 13 Apr 2021 16:50:26 +0100 Subject: [PATCH 1/2] Don't create broadcast event unless necessary If we're not going to send a broadcast, we don't need to create the BroadcastEvent in the database. The BroadcastMessage contains all the data we need - the BroadcastEvent is not used. Not creating the event when we won't send the broadcast (e.g. when the broadcast message was created when the service was in trial mode) adds an extra layer of security. --- app/broadcast_message/rest.py | 44 ++++++++++++------------ tests/app/broadcast_message/test_rest.py | 11 ++---- 2 files changed, 24 insertions(+), 31 deletions(-) diff --git a/app/broadcast_message/rest.py b/app/broadcast_message/rest.py index 859f2636d..f909bc958 100644 --- a/app/broadcast_message/rest.py +++ b/app/broadcast_message/rest.py @@ -192,29 +192,29 @@ def _create_broadcast_event(broadcast_message): """ Creates a broadcast event, stores it in the database, and triggers the task to send the CAP XML off """ - msg_types = { - BroadcastStatusType.BROADCASTING: BroadcastEventMessageType.ALERT, - BroadcastStatusType.CANCELLED: BroadcastEventMessageType.CANCEL, - } - - event = BroadcastEvent( - service=broadcast_message.service, - broadcast_message=broadcast_message, - message_type=msg_types[broadcast_message.status], - transmitted_content={"body": broadcast_message.content}, - transmitted_areas=broadcast_message.areas, - # TODO: Probably move this somewhere more standalone too and imply that it shouldn't change. Should it include - # a service based identifier too? eg "flood-warnings@notifications.service.gov.uk" or similar - transmitted_sender='notifications.service.gov.uk', - - # TODO: Should this be set to now? Or the original starts_at? - transmitted_starts_at=broadcast_message.starts_at, - transmitted_finishes_at=broadcast_message.finishes_at, - ) - - dao_save_object(event) - if not broadcast_message.stubbed: + msg_types = { + BroadcastStatusType.BROADCASTING: BroadcastEventMessageType.ALERT, + BroadcastStatusType.CANCELLED: BroadcastEventMessageType.CANCEL, + } + + event = BroadcastEvent( + service=broadcast_message.service, + broadcast_message=broadcast_message, + message_type=msg_types[broadcast_message.status], + transmitted_content={"body": broadcast_message.content}, + transmitted_areas=broadcast_message.areas, + # TODO: Probably move this somewhere more standalone too and imply that it shouldn't change. Should it + # include a service based identifier too? eg "flood-warnings@notifications.service.gov.uk" or similar + transmitted_sender='notifications.service.gov.uk', + + # TODO: Should this be set to now? Or the original starts_at? + transmitted_starts_at=broadcast_message.starts_at, + transmitted_finishes_at=broadcast_message.finishes_at, + ) + + dao_save_object(event) + send_broadcast_event.apply_async( kwargs={'broadcast_event_id': str(event.id)}, queue=QueueNames.BROADCASTS diff --git a/tests/app/broadcast_message/test_rest.py b/tests/app/broadcast_message/test_rest.py index b067b8bc4..440b4f81c 100644 --- a/tests/app/broadcast_message/test_rest.py +++ b/tests/app/broadcast_message/test_rest.py @@ -591,17 +591,10 @@ def test_update_broadcast_message_status_updates_details_but_does_not_queue_task assert response['approved_at'] is not None assert response['approved_by_id'] == str(approver.id) - assert len(bm.events) == 1 - alert_event = bm.events[0] - + # The broadcast can be approved, but does not create a broadcast_event in the database or put a task on the queue + assert len(bm.events) == 0 assert len(mock_task.mock_calls) == 0 - assert alert_event.service_id == sample_broadcast_service.id - assert alert_event.transmitted_areas == bm.areas - assert alert_event.message_type == BroadcastEventMessageType.ALERT - assert alert_event.transmitted_finishes_at == bm.finishes_at - assert alert_event.transmitted_content == {"body": "emergency broadcast"} - def test_update_broadcast_message_status_creates_event_with_correct_content_if_broadcast_has_no_template( admin_request, From 59978fd99a8483d083779f53632794dbccf5ca8c Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 13 Apr 2021 18:36:31 +0100 Subject: [PATCH 2/2] Check if a service is live before sending a broadcast We only want to send a broadcast if the broadcast message is not stubbed and the service is live at the point at which the broadcast event should be created. This is to prevent the situation where a broadcast service is switched to live / trial mode in between the message being created and approved (we log an error if this happens). A stubbed broadcast message with a trial mode service at the point of approval is not an issue - trial mode services can approve their own broadcasts. In this situation, we don't create the broadcast event but also don't need to log an error. --- app/broadcast_message/rest.py | 17 ++++++++++++++--- tests/app/broadcast_message/test_rest.py | 20 ++++++++++++++------ 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/app/broadcast_message/rest.py b/app/broadcast_message/rest.py index f909bc958..975959154 100644 --- a/app/broadcast_message/rest.py +++ b/app/broadcast_message/rest.py @@ -190,16 +190,19 @@ def update_broadcast_message_status(service_id, broadcast_message_id): def _create_broadcast_event(broadcast_message): """ - Creates a broadcast event, stores it in the database, and triggers the task to send the CAP XML off + If the service is live and the broadcast message is not stubbed, creates a broadcast event, stores it in the + database, and triggers the task to send the CAP XML off. """ - if not broadcast_message.stubbed: + service = broadcast_message.service + + if not broadcast_message.stubbed and not service.restricted: msg_types = { BroadcastStatusType.BROADCASTING: BroadcastEventMessageType.ALERT, BroadcastStatusType.CANCELLED: BroadcastEventMessageType.CANCEL, } event = BroadcastEvent( - service=broadcast_message.service, + service=service, broadcast_message=broadcast_message, message_type=msg_types[broadcast_message.status], transmitted_content={"body": broadcast_message.content}, @@ -219,3 +222,11 @@ def _create_broadcast_event(broadcast_message): kwargs={'broadcast_event_id': str(event.id)}, queue=QueueNames.BROADCASTS ) + elif broadcast_message.stubbed != service.restricted: + # It's possible for a service to create a broadcast in trial mode, and then approve it after the + # service is live (or vice versa). We don't think it's safe to send such broadcasts, as the service + # has changed since they were created. Log an error instead. + current_app.logger.error( + f'Broadcast event not created. Stubbed status of broadcast message was {broadcast_message.stubbed}' + f' but service was {"in trial mode" if service.restricted else "live"}' + ) diff --git a/tests/app/broadcast_message/test_rest.py b/tests/app/broadcast_message/test_rest.py index 440b4f81c..92abfde4f 100644 --- a/tests/app/broadcast_message/test_rest.py +++ b/tests/app/broadcast_message/test_rest.py @@ -1,5 +1,4 @@ import uuid -from unittest.mock import ANY import pytest from freezegun import freeze_time @@ -562,23 +561,32 @@ def test_update_broadcast_message_status_stores_approved_by_and_approved_at_and_ assert alert_event.transmitted_content == {"body": "emergency broadcast"} -def test_update_broadcast_message_status_updates_details_but_does_not_queue_task_for_stubbed_broadcast_message( +@pytest.mark.parametrize('broadcast_message_stubbed, service_restricted_before_approval', [ + (True, True), + (True, False), + (False, True), +]) +def test_update_broadcast_message_status_updates_details_but_does_not_queue_task_if_bm_is_stubbed_or_service_not_live( admin_request, sample_broadcast_service, - mocker + mocker, + broadcast_message_stubbed, + service_restricted_before_approval, ): - sample_broadcast_service.restricted = True + sample_broadcast_service.restricted = broadcast_message_stubbed t = create_template(sample_broadcast_service, BROADCAST_TYPE, content='emergency broadcast') bm = create_broadcast_message( t, status=BroadcastStatusType.PENDING_APPROVAL, areas={"areas": ["london"], "simple_polygons": [[[51.30, 0.7], [51.28, 0.8], [51.25, -0.7]]]}, - stubbed=True + stubbed=broadcast_message_stubbed ) approver = create_user(email='approver@gov.uk') sample_broadcast_service.users.append(approver) mock_task = mocker.patch('app.celery.broadcast_message_tasks.send_broadcast_event.apply_async') + sample_broadcast_service.restricted = service_restricted_before_approval + response = admin_request.post( 'broadcast_message.update_broadcast_message_status', _data={'status': BroadcastStatusType.BROADCASTING, 'created_by': str(approver.id)}, @@ -707,7 +715,7 @@ def test_update_broadcast_message_status_allows_trial_mode_services_to_approve_o assert response['approved_at'] is not None assert response['created_by_id'] == str(t.created_by_id) assert response['approved_by_id'] == str(t.created_by_id) - mock_task.assert_called_once_with(kwargs={'broadcast_event_id': ANY}, queue='broadcast-tasks') + assert not mock_task.called def test_update_broadcast_message_status_rejects_approval_from_user_not_on_that_service(