From 43bcb56ff4489aa3a19cbf91e9576639e5230382 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Thu, 29 Jul 2021 09:56:17 +0100
Subject: [PATCH] Revoke API keys when changing broadcast settings
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On a regular Notify service anyone with permission can create an API
key. If this service then is given permission to send emergency alerts
it will have an API key which can create emergency alerts. This feels
dangerous.

Secondly, if a service which legitimately has an API key for sending
alerts in training mode is changed to live mode you now have an API key
which people will think isn’t going to create a real alert but actually
will. This feels really dangerous.

Neither of these scenarios are things we should be doing, but having
them possible still makes me feel uncomfortable.

This commit revokes all API keys for a service when its broadcast
settings change, same way we remove all permissions for its users.
---
 app/dao/broadcast_service_dao.py |  8 ++++++++
 tests/app/service/test_rest.py   | 22 ++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/app/dao/broadcast_service_dao.py b/app/dao/broadcast_service_dao.py
index 40611bbe2..546b6ed91 100644
--- a/app/dao/broadcast_service_dao.py
+++ b/app/dao/broadcast_service_dao.py
@@ -9,6 +9,7 @@ from app.models import (
     EMAIL_AUTH_TYPE,
     INVITE_PENDING,
     VIEW_ACTIVITY,
+    ApiKey,
     InvitedUser,
     Organisation,
     Permission,
@@ -67,6 +68,13 @@ def set_broadcast_service_type(service, service_mode, broadcast_channel, provide
         status=INVITE_PENDING
     ).update({'permissions': VIEW_ACTIVITY})
 
+    # Revoke any API keys to avoid a regular API key being used to send alerts
+    ApiKey.query.filter_by(
+        service_id=service.id
+    ).update({
+        ApiKey.expiry_date: datetime.utcnow()
+    })
+
     # Add service to organisation
     organisation = Organisation.query.filter_by(
         id=current_app.config['BROADCAST_ORGANISATION_ID']
diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py
index f80063dd6..a74ff05c9 100644
--- a/tests/app/service/test_rest.py
+++ b/tests/app/service/test_rest.py
@@ -4170,3 +4170,25 @@ def test_set_as_broadcast_service_removes_user_permissions(
 
     # Permissions for other services remain
     assert service_user.get_permissions(service_id=sample_service_full_permissions.id) == ['send_emails']
+
+
+def test_set_as_broadcast_service_revokes_api_keys(
+    admin_request,
+    broadcast_organisation,
+    sample_service,
+    sample_service_full_permissions,
+):
+    api_key_1 = create_api_key(service=sample_service)
+    api_key_2 = create_api_key(service=sample_service_full_permissions)
+
+    admin_request.post(
+        'service.set_as_broadcast_service',
+        service_id=sample_service.id,
+        _data={
+            'broadcast_channel': 'government',
+            'service_mode': 'live',
+            'provider_restriction': 'all',
+        }
+    )
+    assert api_key_1.expiry_date < datetime.utcnow()
+    assert api_key_2.expiry_date is None