Merge pull request #3452 from alphagov/revert-3449-revert-3440-audit-api-key-id-when-cancelling-broadcast-via-api

Audit api key id when cancelling broadcast via api - second try

app/broadcast_message/utils.py

@@ -13,7 +13,7 @@ from app.models import (
 )
 
 
-def validate_and_update_broadcast_message_status(broadcast_message, new_status, updating_user):
+def validate_and_update_broadcast_message_status(broadcast_message, new_status, updating_user=None, api_key_id=None):
     if new_status not in BroadcastStatusType.ALLOWED_STATUS_TRANSITIONS[broadcast_message.status]:
         raise InvalidRequest(
             f'Cannot move broadcast_message {broadcast_message.id} from {broadcast_message.status} to {new_status}',
@@ -39,6 +39,7 @@ def validate_and_update_broadcast_message_status(broadcast_message, new_status,
     if new_status == BroadcastStatusType.CANCELLED:
         broadcast_message.cancelled_at = datetime.utcnow()
         broadcast_message.cancelled_by = updating_user
+        broadcast_message.cancelled_by_api_key_id = api_key_id
 
     current_app.logger.info(
         f'broadcast_message {broadcast_message.id} moving from {broadcast_message.status} to {new_status}'

app/models.py

@@ -2322,15 +2322,17 @@ class BroadcastMessage(db.Model):
     approved_by = db.relationship('User', foreign_keys=[approved_by_id])
     cancelled_by = db.relationship('User', foreign_keys=[cancelled_by_id])
 
-    api_key_id = db.Column(UUID(as_uuid=True), db.ForeignKey('api_keys.id'), nullable=True)
-    api_key = db.relationship('ApiKey')
+    created_by_api_key_id = db.Column(UUID(as_uuid=True), db.ForeignKey('api_keys.id'), nullable=True)
+    cancelled_by_api_key_id = db.Column(UUID(as_uuid=True), db.ForeignKey('api_keys.id'), nullable=True)
+    created_by_api_key = db.relationship('ApiKey', foreign_keys=[created_by_api_key_id])
+    cancelled_by_api_key = db.relationship('ApiKey', foreign_keys=[cancelled_by_api_key_id])
 
     reference = db.Column(db.String(255), nullable=True)
     cap_event = db.Column(db.String(255), nullable=True)
 
     stubbed = db.Column(db.Boolean, nullable=False)
 
-    CheckConstraint("created_by_id is not null or api_key_id is not null")
+    CheckConstraint("created_by_id is not null or created_by_api_key_id is not null")
 
     @property
     def personalisation(self):

app/v2/broadcast/post_broadcast.py

@@ -81,7 +81,7 @@ def create_broadcast():
                 'simple_polygons': simple_polygons.as_coordinate_pairs_lat_long,
             },
             status=BroadcastStatusType.PENDING_APPROVAL,
-            api_key_id=api_user.id,
+            created_by_api_key_id=api_user.id,
             stubbed=authenticated_service.restricted
             # The client may pass in broadcast_json['expires'] but it’s
             # simpler for now to ignore it and have the rules around expiry
@@ -111,7 +111,7 @@ def _cancel_or_reject_broadcast(references_to_original_broadcast, service_id):
     validate_and_update_broadcast_message_status(
         broadcast_message,
         new_status,
-        updating_user=None
+        api_key_id=api_user.id
     )
     return broadcast_message
 

tests/app/broadcast_message/test_utils.py

@@ -9,7 +9,12 @@ from app.models import (
     BroadcastEventMessageType,
     BroadcastStatusType,
 )
-from tests.app.db import create_broadcast_message, create_template, create_user
+from tests.app.db import (
+    create_api_key,
+    create_broadcast_message,
+    create_template,
+    create_user,
+)
 
 
 def test_validate_and_update_broadcast_message_status_stores_approved_by_and_approved_at_and_queues_task(
@@ -49,11 +54,9 @@ def test_validate_and_update_broadcast_message_status_stores_approved_by_and_app
     assert alert_event.transmitted_content == {"body": "emergency broadcast"}
 
 
-@pytest.mark.parametrize("cancel_route", ["admin_interface", "api_call"])
-def test_validate_and_update_broadcast_message_status_for_cancelling_broadcast(
+def test_validate_and_update_broadcast_message_status_for_cancelling_broadcast_from_admin_interface(
     sample_broadcast_service,
     mocker,
-    cancel_route
 ):
     template = create_template(sample_broadcast_service, BROADCAST_TYPE, content='emergency broadcast')
     broadcast_message = create_broadcast_message(
@@ -64,19 +67,18 @@ def test_validate_and_update_broadcast_message_status_for_cancelling_broadcast(
             "simple_polygons": [[[51.30, 0.7], [51.28, 0.8], [51.25, -0.7]]]
         }
     )
-    if cancel_route == "admin_interface":
-        canceller = sample_broadcast_service.created_by
-    else:
-        canceller = None
+    canceller = sample_broadcast_service.created_by
+
     mock_task = mocker.patch('app.celery.broadcast_message_tasks.send_broadcast_event.apply_async')
 
     validate_and_update_broadcast_message_status(
-        broadcast_message, BroadcastStatusType.CANCELLED, canceller
+        broadcast_message, BroadcastStatusType.CANCELLED, updating_user=canceller, api_key_id=None
     )
 
     assert broadcast_message.status == BroadcastStatusType.CANCELLED
     assert broadcast_message.cancelled_at is not None
-    assert broadcast_message.cancelled_by_id == (canceller.id if canceller else None)
+    assert broadcast_message.cancelled_by_id == canceller.id
+    assert broadcast_message.cancelled_by_api_key_id is None
 
     assert len(broadcast_message.events) == 1
     alert_event = broadcast_message.events[0]
@@ -87,11 +89,43 @@ def test_validate_and_update_broadcast_message_status_for_cancelling_broadcast(
     assert alert_event.message_type == BroadcastEventMessageType.CANCEL
 
 
-@pytest.mark.parametrize("reject_route", ["admin_interface", "api_call"])
-def test_validate_and_update_broadcast_message_status_for_rejecting_broadcast(
+def test_validate_and_update_broadcast_message_status_for_cancelling_broadcast_from_API_call(
     sample_broadcast_service,
     mocker,
-    reject_route
+):
+    api_key = create_api_key(service=sample_broadcast_service)
+    template = create_template(sample_broadcast_service, BROADCAST_TYPE, content='emergency broadcast')
+    broadcast_message = create_broadcast_message(
+        template,
+        status=BroadcastStatusType.BROADCASTING,
+        areas={
+            "ids": ["london"],
+            "simple_polygons": [[[51.30, 0.7], [51.28, 0.8], [51.25, -0.7]]]
+        }
+    )
+    mock_task = mocker.patch('app.celery.broadcast_message_tasks.send_broadcast_event.apply_async')
+
+    validate_and_update_broadcast_message_status(
+        broadcast_message, BroadcastStatusType.CANCELLED, updating_user=None, api_key_id=api_key.id
+    )
+
+    assert broadcast_message.status == BroadcastStatusType.CANCELLED
+    assert broadcast_message.cancelled_at is not None
+    assert broadcast_message.cancelled_by_id is None
+    assert broadcast_message.cancelled_by_api_key_id == api_key.id
+
+    assert len(broadcast_message.events) == 1
+    alert_event = broadcast_message.events[0]
+
+    mock_task.assert_called_once_with(kwargs={'broadcast_event_id': str(alert_event.id)}, queue='broadcast-tasks')
+
+    assert alert_event.service_id == sample_broadcast_service.id
+    assert alert_event.message_type == BroadcastEventMessageType.CANCEL
+
+
+def test_validate_and_update_broadcast_message_status_for_rejecting_broadcast_via_admin_interface(
+    sample_broadcast_service,
+    mocker
 ):
     template = create_template(sample_broadcast_service, BROADCAST_TYPE, content='emergency broadcast')
     broadcast_message = create_broadcast_message(
@@ -102,14 +136,10 @@ def test_validate_and_update_broadcast_message_status_for_rejecting_broadcast(
             "simple_polygons": [[[51.30, 0.7], [51.28, 0.8], [51.25, -0.7]]]
         }
     )
-    if reject_route == "admin_interface":
-        canceller = sample_broadcast_service.created_by
-    else:
-        canceller = None
     mock_task = mocker.patch('app.celery.broadcast_message_tasks.send_broadcast_event.apply_async')
 
     validate_and_update_broadcast_message_status(
-        broadcast_message, BroadcastStatusType.REJECTED, canceller
+        broadcast_message, BroadcastStatusType.REJECTED, updating_user=sample_broadcast_service.created_by
     )
 
     assert broadcast_message.status == BroadcastStatusType.REJECTED
@@ -121,6 +151,36 @@ def test_validate_and_update_broadcast_message_status_for_rejecting_broadcast(
     assert len(broadcast_message.events) == 0
 
 
+def test_validate_and_update_broadcast_message_status_for_rejecting_broadcast_from_API_call(
+    sample_broadcast_service,
+    mocker
+):
+    api_key = create_api_key(service=sample_broadcast_service)
+    template = create_template(sample_broadcast_service, BROADCAST_TYPE, content='emergency broadcast')
+    broadcast_message = create_broadcast_message(
+        template,
+        status=BroadcastStatusType.PENDING_APPROVAL,
+        areas={
+            "ids": ["london"],
+            "simple_polygons": [[[51.30, 0.7], [51.28, 0.8], [51.25, -0.7]]]
+        }
+    )
+    mock_task = mocker.patch('app.celery.broadcast_message_tasks.send_broadcast_event.apply_async')
+
+    validate_and_update_broadcast_message_status(
+        broadcast_message, BroadcastStatusType.REJECTED, api_key_id=api_key.id
+    )
+
+    assert broadcast_message.status == BroadcastStatusType.REJECTED
+    assert broadcast_message.cancelled_at is None
+    assert broadcast_message.cancelled_by_id is None
+    assert broadcast_message.cancelled_by_api_key_id is None
+    assert broadcast_message.updated_at is not None
+
+    assert not mock_task.called
+    assert len(broadcast_message.events) == 0
+
+
 @pytest.mark.parametrize('current_status, new_status', [
     (BroadcastStatusType.DRAFT, BroadcastStatusType.DRAFT),
     (BroadcastStatusType.DRAFT, BroadcastStatusType.BROADCASTING),

tests/app/v2/broadcast/test_post_broadcast.py

@@ -7,6 +7,7 @@ from app.dao.broadcast_message_dao import (
     dao_get_broadcast_message_by_id_and_service_id,
 )
 from tests import create_service_authorization_header
+from tests.app.db import create_api_key
 
 from . import sample_cap_xml_documents
 
@@ -130,6 +131,7 @@ def test_valid_cancel_broadcast_request_calls_validate_and_update_broadcast_mess
     is_approved,
     expected_status
 ):
+    api_key = create_api_key(service=sample_broadcast_service)
     auth_header = create_service_authorization_header(service_id=sample_broadcast_service.id)
 
     # create a broadcast
@@ -158,7 +160,11 @@ def test_valid_cancel_broadcast_request_calls_validate_and_update_broadcast_mess
         headers=[('Content-Type', 'application/cap+xml'), auth_header],
     )
     assert response_for_cancel.status_code == 201
-    mock_update.assert_called_once_with(broadcast_message, expected_status, updating_user=None)
+    mock_update.assert_called_once_with(
+        broadcast_message,
+        expected_status,
+        api_key_id=api_key.id
+    )
 
 
 def test_cancel_request_does_not_cancel_broadcast_if_reference_does_not_match(