From e1a8219eb1ac51beb69ec295fdfb8ecddf1f1806 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 25 Jan 2022 18:09:03 +0000 Subject: [PATCH 1/4] DB Migration to allow auditing api key id when cancelling broadcast via API. --- app/models.py | 9 +++-- .../versions/0363_cancelled_by_api_key.py | 39 +++++++++++++++++++ 2 files changed, 45 insertions(+), 3 deletions(-) create mode 100644 migrations/versions/0363_cancelled_by_api_key.py diff --git a/app/models.py b/app/models.py index 2486a4045..ddba7e158 100644 --- a/app/models.py +++ b/app/models.py @@ -2313,15 +2313,18 @@ 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") + CheckConstraint("cancelled_by_id is not null or cancelled_by_api_key_id is not null") @property def personalisation(self): diff --git a/migrations/versions/0363_cancelled_by_api_key.py b/migrations/versions/0363_cancelled_by_api_key.py new file mode 100644 index 000000000..b9611e875 --- /dev/null +++ b/migrations/versions/0363_cancelled_by_api_key.py @@ -0,0 +1,39 @@ +""" + +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.get_bind() + op.execute("UPDATE broadcast_message SET created_by_api_key_id=api_key_id") # move data over + op.drop_column('broadcast_message', 'api_key_id') + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.add_column('broadcast_message', sa.Column('api_key_id', postgresql.UUID(), autoincrement=False, nullable=True)) + 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 ### From 82f08f230cba8bfbe42315b84b44cac967eec2c2 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 26 Jan 2022 15:35:00 +0000 Subject: [PATCH 2/4] Save api key id when cancelling broadcast by API call This is so that we can audit who cancelled the broadcast if there are any issues. --- app/broadcast_message/utils.py | 3 +- app/v2/broadcast/post_broadcast.py | 4 +- tests/app/broadcast_message/test_utils.py | 96 +++++++++++++++---- tests/app/v2/broadcast/test_post_broadcast.py | 8 +- 4 files changed, 89 insertions(+), 22 deletions(-) diff --git a/app/broadcast_message/utils.py b/app/broadcast_message/utils.py index 1673ab2bb..cb3a0b412 100644 --- a/app/broadcast_message/utils.py +++ b/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}' diff --git a/app/v2/broadcast/post_broadcast.py b/app/v2/broadcast/post_broadcast.py index a597c2aa0..2be55cc30 100644 --- a/app/v2/broadcast/post_broadcast.py +++ b/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 diff --git a/tests/app/broadcast_message/test_utils.py b/tests/app/broadcast_message/test_utils.py index a02ac66bf..db12b0d0b 100644 --- a/tests/app/broadcast_message/test_utils.py +++ b/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( + 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), diff --git a/tests/app/v2/broadcast/test_post_broadcast.py b/tests/app/v2/broadcast/test_post_broadcast.py index e767d33d2..c42435fcf 100644 --- a/tests/app/v2/broadcast/test_post_broadcast.py +++ b/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( From 0737eceddb10737100e3205ef5f7584cdece0e23 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 2 Feb 2022 17:21:58 +0000 Subject: [PATCH 3/4] Include check constraint in migration script Add check constraint that created_by_id should not be null, unless created_by_api_key_id is not null to the migration script. It is already in the models file. Also remove check constraint for cancelled_by_id from models, as this field would only be filled for broadcasts with cancelled status. Also add some spacing in that migration script so it is easier to read. --- app/models.py | 1 - .../versions/0363_cancelled_by_api_key.py | 29 +++++++++++++++++-- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/app/models.py b/app/models.py index ddba7e158..5b34d868f 100644 --- a/app/models.py +++ b/app/models.py @@ -2324,7 +2324,6 @@ class BroadcastMessage(db.Model): stubbed = db.Column(db.Boolean, nullable=False) CheckConstraint("created_by_id is not null or created_by_api_key_id is not null") - CheckConstraint("cancelled_by_id is not null or cancelled_by_api_key_id is not null") @property def personalisation(self): diff --git a/migrations/versions/0363_cancelled_by_api_key.py b/migrations/versions/0363_cancelled_by_api_key.py index b9611e875..ef4e432e9 100644 --- a/migrations/versions/0363_cancelled_by_api_key.py +++ b/migrations/versions/0363_cancelled_by_api_key.py @@ -16,10 +16,29 @@ 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.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_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 op.drop_column('broadcast_message', 'api_key_id') @@ -29,6 +48,10 @@ def upgrade(): def downgrade(): # ### commands auto generated by Alembic - please adjust! ### op.add_column('broadcast_message', sa.Column('api_key_id', postgresql.UUID(), autoincrement=False, nullable=True)) + 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']) From f2bef7392ed84f4bce4ef8ef130d47348422029b Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 8 Feb 2022 15:57:28 +0000 Subject: [PATCH 4/4] api_key_id column will have to be dropped in a separate PR This is to avoid situation where we deploy migration already but don't deploy code yet and they are not compatible for a while which leads to errors. --- migrations/versions/0363_cancelled_by_api_key.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/migrations/versions/0363_cancelled_by_api_key.py b/migrations/versions/0363_cancelled_by_api_key.py index ef4e432e9..7df591382 100644 --- a/migrations/versions/0363_cancelled_by_api_key.py +++ b/migrations/versions/0363_cancelled_by_api_key.py @@ -41,13 +41,11 @@ def upgrade(): ) op.get_bind() op.execute("UPDATE broadcast_message SET created_by_api_key_id=api_key_id") # move data over - op.drop_column('broadcast_message', 'api_key_id') # ### end Alembic commands ### def downgrade(): # ### commands auto generated by Alembic - please adjust! ### - op.add_column('broadcast_message', sa.Column('api_key_id', postgresql.UUID(), autoincrement=False, nullable=True)) op.drop_constraint( "ck_broadcast_message_created_by_not_null", "broadcast_message"