Revert "Audit api key id when cancelling broadcast via api"

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=None, api_key_id=None):
+def validate_and_update_broadcast_message_status(broadcast_message, new_status, updating_user):
     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,7 +39,6 @@ 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,17 +2322,15 @@ class BroadcastMessage(db.Model):
     approved_by = db.relationship('User', foreign_keys=[approved_by_id])
     cancelled_by = db.relationship('User', foreign_keys=[cancelled_by_id])
 
-    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])
+    api_key_id = db.Column(UUID(as_uuid=True), db.ForeignKey('api_keys.id'), nullable=True)
+    api_key = db.relationship('ApiKey')
 
     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 created_by_api_key_id is not null")
+    CheckConstraint("created_by_id is not null or 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,
-            created_by_api_key_id=api_user.id,
+            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,
-        api_key_id=api_user.id
+        updating_user=None
     )
     return broadcast_message
 

migrations/versions/0363_cancelled_by_api_key.py

@@ -1,60 +0,0 @@
-"""
-
-Revision ID: 0363_cancelled_by_api_key
-Revises: 0362_broadcast_msg_event
-Create Date: 2022-01-25 18:05:27.750234
-
-"""
-from alembic import op
-import sqlalchemy as sa
-from sqlalchemy.dialects import postgresql
-
-revision = '0363_cancelled_by_api_key'
-down_revision = '0362_broadcast_msg_event'
-
-
-def upgrade():
-    # ### commands auto generated by Alembic - please adjust! ###
-    op.add_column('broadcast_message', sa.Column('created_by_api_key_id', postgresql.UUID(as_uuid=True), nullable=True))
-    op.add_column(
-        'broadcast_message', sa.Column('cancelled_by_api_key_id', postgresql.UUID(as_uuid=True), nullable=True)
-    )
-    op.drop_constraint('broadcast_message_api_key_id_fkey', 'broadcast_message', type_='foreignkey')
-    op.create_foreign_key(
-        'broadcast_message_created_by_api_key_id_fkey',
-        'broadcast_message',
-        'api_keys',
-        ['created_by_api_key_id'],
-        ['id']
-    )
-    op.create_foreign_key(
-        'broadcast_message_cancelled_by_api_key_id_fkey',
-        'broadcast_message',
-        'api_keys',
-        ['cancelled_by_api_key_id'],
-        ['id']
-    )
-    op.create_check_constraint(
-        "ck_broadcast_message_created_by_not_null",
-        "broadcast_message",
-        "created_by_id is not null or created_by_api_key_id is not null"
-    )
-    op.get_bind()
-    op.execute("UPDATE broadcast_message SET created_by_api_key_id=api_key_id")  # move data over
-    # ### end Alembic commands ###
-
-
-def downgrade():
-    # ### commands auto generated by Alembic - please adjust! ###
-    op.drop_constraint(
-        "ck_broadcast_message_created_by_not_null",
-        "broadcast_message"
-    )
-    op.drop_constraint('broadcast_message_created_by_api_key_id_fkey', 'broadcast_message', type_='foreignkey')
-    op.drop_constraint('broadcast_message_cancelled_by_api_key_id_fkey', 'broadcast_message', type_='foreignkey')
-    op.create_foreign_key('broadcast_message_api_key_id_fkey', 'broadcast_message', 'api_keys', ['api_key_id'], ['id'])
-    op.get_bind()
-    op.execute("UPDATE broadcast_message SET api_key_id=created_by_api_key_id")  # move data over
-    op.drop_column('broadcast_message', 'cancelled_by_api_key_id')
-    op.drop_column('broadcast_message', 'created_by_api_key_id')
-    # ### end Alembic commands ###

tests/app/broadcast_message/test_utils.py

@@ -9,12 +9,7 @@ from app.models import (
     BroadcastEventMessageType,
     BroadcastStatusType,
 )
-from tests.app.db import (
-    create_api_key,
-    create_broadcast_message,
-    create_template,
-    create_user,
-)
+from tests.app.db import create_broadcast_message, create_template, create_user
 
 
 def test_validate_and_update_broadcast_message_status_stores_approved_by_and_approved_at_and_queues_task(
@@ -54,9 +49,11 @@ def test_validate_and_update_broadcast_message_status_stores_approved_by_and_app
     assert alert_event.transmitted_content == {"body": "emergency broadcast"}
 
 
-def test_validate_and_update_broadcast_message_status_for_cancelling_broadcast_from_admin_interface(
+@pytest.mark.parametrize("cancel_route", ["admin_interface", "api_call"])
+def test_validate_and_update_broadcast_message_status_for_cancelling_broadcast(
     sample_broadcast_service,
     mocker,
+    cancel_route
 ):
     template = create_template(sample_broadcast_service, BROADCAST_TYPE, content='emergency broadcast')
     broadcast_message = create_broadcast_message(
@@ -67,18 +64,19 @@ def test_validate_and_update_broadcast_message_status_for_cancelling_broadcast_f
             "simple_polygons": [[[51.30, 0.7], [51.28, 0.8], [51.25, -0.7]]]
         }
     )
-    canceller = sample_broadcast_service.created_by
-
+    if cancel_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.CANCELLED, updating_user=canceller, api_key_id=None
+        broadcast_message, BroadcastStatusType.CANCELLED, canceller
     )
 
     assert broadcast_message.status == BroadcastStatusType.CANCELLED
     assert broadcast_message.cancelled_at is not None
-    assert broadcast_message.cancelled_by_id == canceller.id
-    assert broadcast_message.cancelled_by_api_key_id is None
+    assert broadcast_message.cancelled_by_id == (canceller.id if canceller else None)
 
     assert len(broadcast_message.events) == 1
     alert_event = broadcast_message.events[0]
@@ -89,73 +87,12 @@ def test_validate_and_update_broadcast_message_status_for_cancelling_broadcast_f
     assert alert_event.message_type == BroadcastEventMessageType.CANCEL
 
 
-def test_validate_and_update_broadcast_message_status_for_cancelling_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.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(
-        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, updating_user=sample_broadcast_service.created_by
-    )
-
-    assert broadcast_message.status == BroadcastStatusType.REJECTED
-    assert broadcast_message.cancelled_at is None
-    assert broadcast_message.cancelled_by_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("reject_route", ["admin_interface", "api_call"])
 def test_validate_and_update_broadcast_message_status_for_rejecting_broadcast(
     sample_broadcast_service,
-    mocker
+    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,
@@ -165,16 +102,19 @@ 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, api_key_id=api_key.id
+        broadcast_message, BroadcastStatusType.REJECTED, canceller
     )
 
     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

tests/app/v2/broadcast/test_post_broadcast.py

@@ -7,7 +7,6 @@ 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
 
@@ -131,7 +130,6 @@ 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
@@ -160,11 +158,7 @@ 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,
-        api_key_id=api_key.id
-    )
+    mock_update.assert_called_once_with(broadcast_message, expected_status, updating_user=None)
 
 
 def test_cancel_request_does_not_cancel_broadcast_if_reference_does_not_match(