From 256b76506225b0d13aa2a1f81043d4e8e1ce573c Mon Sep 17 00:00:00 2001 From: Will Pearson Date: Thu, 9 Apr 2026 15:54:25 +0100 Subject: [PATCH 1/5] Pass encrypt key to check_token This turns on the feature alphagov/notifications-utils#1336 By passing in environment variables. We start checking for encrypted tokens before we encrypt them. The plan is to keep the signing code around for at least a year, so that we don't have eny tokens in things like unsubscribe links. Encryption has been added everywhere for simplicities sake. --- app/commands.py | 1 + app/config.py | 2 ++ app/one_click_unsubscribe/rest.py | 6 +++++- app/organisation/invite_rest.py | 6 +++++- app/service_invite/rest.py | 6 +++++- 5 files changed, 18 insertions(+), 3 deletions(-) diff --git a/app/commands.py b/app/commands.py index f3779d0692..4d27e673dc 100644 --- a/app/commands.py +++ b/app/commands.py @@ -845,6 +845,7 @@ def functional_test_fixtures(): SQLALCHEMY_DATABASE_URI REDIS_URL SECRET_KEY + TOKEN_SECRET_KEY INTERNAL_CLIENT_API_KEYS ADMIN_BASE_URL API_HOST_NAME diff --git a/app/config.py b/app/config.py index 2672b514ca..3f5315d17f 100644 --- a/app/config.py +++ b/app/config.py @@ -108,6 +108,7 @@ class Config: # encyption secret/salt SECRET_KEY = os.getenv("SECRET_KEY") + TOKEN_SECRET_KEY = os.getenv("TOKEN_SECRET_KEY") DANGEROUS_SALT = os.getenv("DANGEROUS_SALT") # DB conection string @@ -614,6 +615,7 @@ class Development(Config): } SECRET_KEY = "dev-notify-secret-key" + TOKEN_SECRET_KEY = "5YNWU0e_pN5ZyaSZvBd5uZb_sZlrVDFeOjiea6dq4zQ=" DANGEROUS_SALT = "dev-notify-salt" MMG_INBOUND_SMS_AUTH = ["testkey"] diff --git a/app/one_click_unsubscribe/rest.py b/app/one_click_unsubscribe/rest.py index 5c0197ff2b..0b77641ad2 100644 --- a/app/one_click_unsubscribe/rest.py +++ b/app/one_click_unsubscribe/rest.py @@ -27,7 +27,11 @@ def one_click_unsubscribe(notification_id, token): try: email_address = check_token( - token, current_app.config["SECRET_KEY"], current_app.config["DANGEROUS_SALT"], max_age_seconds + token, + current_app.config["SECRET_KEY"], + current_app.config["DANGEROUS_SALT"], + max_age_seconds, + current_app.config["TOKEN_SECRET_KEY"], ) except BadData as e: errors = {"unsubscribe request": "This is not a valid unsubscribe link."} diff --git a/app/organisation/invite_rest.py b/app/organisation/invite_rest.py index 0b2200e9ef..ce3d620320 100644 --- a/app/organisation/invite_rest.py +++ b/app/organisation/invite_rest.py @@ -126,7 +126,11 @@ def validate_invitation_token(token): try: invited_user_id = check_token( - token, current_app.config["SECRET_KEY"], current_app.config["DANGEROUS_SALT"], max_age_seconds + token, + current_app.config["SECRET_KEY"], + current_app.config["DANGEROUS_SALT"], + max_age_seconds, + current_app.config["TOKEN_SECRET_KEY"], ) except SignatureExpired as e: errors = { diff --git a/app/service_invite/rest.py b/app/service_invite/rest.py index 7f331f3f15..920f6b0127 100644 --- a/app/service_invite/rest.py +++ b/app/service_invite/rest.py @@ -105,7 +105,11 @@ def validate_service_invitation_token(token): try: invited_user_id = check_token( - token, current_app.config["SECRET_KEY"], current_app.config["DANGEROUS_SALT"], max_age_seconds + token, + current_app.config["SECRET_KEY"], + current_app.config["DANGEROUS_SALT"], + max_age_seconds, + current_app.config["TOKEN_SECRET_KEY"], ) except SignatureExpired as e: errors = { From dfa01fad8168dfbb8102af2c1988947bd10ad562 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 17 Apr 2026 13:22:05 +0100 Subject: [PATCH 2/5] Upgrade Flask-Migrate to version 4 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://github.com/miguelgrinberg/Flask-Migrate/blob/main/CHANGES.md Don’t see anything which should affect us. Breaking changes seem to be dropping older versions of Python and Flask-SQLAlchemy --- requirements.in | 2 +- requirements.txt | 2 +- requirements_for_test.txt | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/requirements.in b/requirements.in index afc1d3d0ef..76b0488aa4 100644 --- a/requirements.in +++ b/requirements.in @@ -7,7 +7,7 @@ kombu @ git+https://github.com/celery/kombu.git@860e40a6c904c4d8551577d9f4e8c00f Flask-Bcrypt~=1.0 flask-marshmallow~=1.4 -Flask-Migrate~=3.1 +Flask-Migrate~=4.1 flask-sqlalchemy~=3.1 click-datetime~=0.2 gunicorn[eventlet]~=25.1 diff --git a/requirements.txt b/requirements.txt index 0bd789c6ba..89078b6e44 100644 --- a/requirements.txt +++ b/requirements.txt @@ -86,7 +86,7 @@ flask-bcrypt==1.0.1 # via -r requirements.in flask-marshmallow==1.4.0 # via -r requirements.in -flask-migrate==3.1.0 +flask-migrate==4.1.0 # via -r requirements.in flask-redis==0.4.0 # via notifications-utils diff --git a/requirements_for_test.txt b/requirements_for_test.txt index 7ffe6cf96a..73d607157d 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -131,7 +131,7 @@ flask-bcrypt==1.0.1 # via -r requirements.txt flask-marshmallow==1.4.0 # via -r requirements.txt -flask-migrate==3.1.0 +flask-migrate==4.1.0 # via -r requirements.txt flask-redis==0.4.0 # via From 6f9f890c7ecd047d920514c66c26bff45da9c0f0 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Wed, 22 Apr 2026 15:30:19 +0100 Subject: [PATCH 3/5] Remove duplicated workers from entrypoint.sh `api-worker-jobs-save` and `api-worker-jobs-save-documents` were listed twice in the entrypoint. These removes the second places there were listed, which are the ones that aren't used. --- entrypoint.sh | 6 ------ 1 file changed, 6 deletions(-) diff --git a/entrypoint.sh b/entrypoint.sh index 9b4ad147e4..a1a61c2ebe 100755 --- a/entrypoint.sh +++ b/entrypoint.sh @@ -39,12 +39,6 @@ case "$1" in api-worker-jobs-save-documents) exec $COMMON_CMD database-tasks-documents ;; - api-worker-jobs-save) - exec $COMMON_CMD database-tasks,job-tasks - ;; - api-worker-jobs-save-documents) - exec $COMMON_CMD database-tasks,job-tasks - ;; api-worker-research) exec $COMMON_CMD research-mode-tasks ;; From 3165a08187c43e9de025fffe1cfa844dae4c7a9c Mon Sep 17 00:00:00 2001 From: Ben Corlett Date: Thu, 2 Apr 2026 09:08:33 +0100 Subject: [PATCH 4/5] Handle sanitised database restores in functional/performance fixtures Make fixture creation resilient when running against a restored sanitised database, while keeping canonical fixture names stable (Functional Tests Org, Functional Tests, Performance Tests). Key changes: - Detect and rotate sanitised org/service rows that occupy canonical fixture names. - Recreate canonical fixture org/service rows deterministically after rotation. - Reclaim unique resources sanitised data may still own (domain, inbound number, inbound-linked SMS sender). - Repair invalid API key secrets to prevent BadSignature failures in auth/serialisation paths. - Keep fixture setup idempotent and safe to run repeatedly across environments. Also updates fixture tests to cover sanitised-row rotation, resource reclaim behaviour, and API key repair logic. --- app/functional_tests_fixtures/__init__.py | 231 +++++++++++++++++- .../test_functional_test_fixtures.py | 148 ++++++++++- 2 files changed, 373 insertions(+), 6 deletions(-) diff --git a/app/functional_tests_fixtures/__init__.py b/app/functional_tests_fixtures/__init__.py index 697d230135..3fcaf57372 100644 --- a/app/functional_tests_fixtures/__init__.py +++ b/app/functional_tests_fixtures/__init__.py @@ -4,8 +4,10 @@ import boto3 from flask import current_app +from itsdangerous.exc import BadSignature from sqlalchemy.exc import NoResultFound +from app import db from app.constants import ( EDIT_FOLDER_PERMISSIONS, EMAIL_AUTH, @@ -33,7 +35,6 @@ dao_create_organisation, dao_get_organisation_by_id, dao_get_organisations_by_partial_name, - dao_update_organisation, ) from app.dao.permissions_dao import permission_dao from app.dao.service_callback_api_dao import get_service_callback_api_by_callback_type, save_service_callback_api @@ -64,6 +65,7 @@ ) from app.dao.users_dao import get_user_by_email, save_model_user from app.models import ( + Domain, InboundNumber, InboundSms, Organisation, @@ -71,6 +73,7 @@ Service, ServiceCallbackApi, ServiceEmailReplyTo, + ServiceSmsSender, User, ) from app.schemas import api_key_schema, template_schema @@ -101,6 +104,86 @@ def _upload_env_to_ssm(ssm_upload_path, env_config, description): ) from exc +def _build_sanitised_renamed_name(name): + # Keep the original fixture name discoverable while making space for a clean canonical row. + suffix = f" [sanitised-{datetime.utcnow().strftime('%Y%m%d%H%M%S')}]" + max_name_length = 255 + if len(name) + len(suffix) <= max_name_length: + return f"{name}{suffix}" + return f"{name[: max_name_length - len(suffix)]}{suffix}" + + +def _is_sanitised_org(organisation): + # These values are written by the sanitise scripts and are a stable signal that this is a copied prod org. + return ( + organisation.request_to_go_live_notes == "redacted" + or organisation.notes == "redacted" + or organisation.agreement_signed_on_behalf_of_name == "redacted" + or organisation.agreement_signed_on_behalf_of_email_address == "redacted@redacted.com" + ) + + +def _is_sanitised_service(service): + # These fields are rewritten during sanitisation; if they are present on canonical fixture names we rotate them. + return ( + service.contact_link == "redacted.gov.uk" + or service.notes == "redacted" + or service.billing_contact_names == "redacted" + or service.billing_contact_email_addresses == "redacted@redacted.com" + or service.email_sender_local_part.endswith(".redacted") + ) + + +def _rename_sanitised_fixture_org(org_name): + organisations = dao_get_organisations_by_partial_name(org_name) + + candidate = None + for organisation in organisations: + if organisation.name == org_name: + candidate = organisation + break + + if candidate is None or not _is_sanitised_org(candidate): + return + + # We rename rather than mutate in place so fixtures can recreate a known-good canonical org deterministically. + old_name = candidate.name + candidate.name = _build_sanitised_renamed_name(candidate.name) + db.session.add(candidate) + db.session.commit() + current_app.logger.info("Renamed sanitised fixture organisation '%s' to '%s'", old_name, candidate.name) + + +def _rename_sanitised_fixture_service(service_name): + services = get_services_by_partial_name(service_name) + + candidate = None + for service in services: + if service.name == service_name: + candidate = service + break + + if candidate is None or not _is_sanitised_service(candidate): + return + + # Unique constraints on inbound numbers/senders can block fixture creation, so detach these before rotating name. + if candidate.inbound_number is not None: + candidate.inbound_number.service_id = None + db.session.add(candidate.inbound_number) + + for sms_sender in candidate.service_sms_senders: + if sms_sender.inbound_number_id is not None: + sms_sender.inbound_number_id = None + sms_sender.archived = True + db.session.add(sms_sender) + + old_name = candidate.name + candidate.name = _build_sanitised_renamed_name(candidate.name) + db.session.add(candidate) + db.session.commit() + current_app.logger.info("Renamed sanitised fixture service '%s' to '%s'", old_name, candidate.name) + + """ This function creates a set of database fixtures that functional tests use to run against. @@ -185,6 +268,14 @@ def _create_db_objects( ) -> tuple[dict[str, str], dict[str, str]]: current_app.logger.info("Creating functional test fixtures for %s:", environment) + functional_service_name = "Functional Tests" + performance_service_name = "Performance Tests" + + # First clear out sanitised copies that may be occupying canonical fixture names. + _rename_sanitised_fixture_org(org_name) + _rename_sanitised_fixture_service(functional_service_name) + _rename_sanitised_fixture_service(performance_service_name) + current_app.logger.info("--> Ensure organisation exists") org = _create_organiation(email_domain, org_name) @@ -210,17 +301,22 @@ def _create_db_objects( ) current_app.logger.info("--> Ensure service exists") - service = _create_service(org.id, service_admin_user) + service = _create_service(org.id, service_admin_user, service_name=functional_service_name) performance_service = _create_service( org.id, service_admin_user, - service_name="Performance Tests", + service_name=performance_service_name, rate_limit=performance_test_rate_limit, sms_message_limit=performance_test_sms_limit, letter_message_limit=performance_test_letter_limit, email_message_limit=performance_test_email_limit, ) + # Repair invalid key payloads early so later auth/serialization paths do not fail with BadSignature. + _repair_invalid_service_api_keys(service.id) + _repair_invalid_service_api_keys(performance_service.id) + _repair_invalid_service_api_keys(govuk_service_id) + current_app.logger.info("--> Ensure users are added to service") dao_add_user_to_service(service, service_admin_user) dao_add_user_to_service(service, email_auth_user) @@ -504,7 +600,30 @@ def _create_organiation(email_domain, org_name): dao_create_organisation(org) - dao_update_organisation(org.id, domains=[email_domain], can_approve_own_go_live_requests=True) + org.name = org_name + org.active = True + org.crown = False + org.organisation_type = "central" + org.can_approve_own_go_live_requests = True + db.session.add(org) + + normalised_email_domain = email_domain.lower() + existing_domain = Domain.query.filter_by(domain=normalised_email_domain).one_or_none() + if existing_domain is None: + db.session.add(Domain(domain=normalised_email_domain, organisation_id=org.id)) + elif existing_domain.organisation_id != org.id: + # Domain ownership is unique across orgs; reclaim it so fixture users resolve to this fixture org. + previous_org_id = existing_domain.organisation_id + existing_domain.organisation_id = org.id + db.session.add(existing_domain) + current_app.logger.info( + "Reassigned domain %s from organisation %s to fixture organisation %s", + normalised_email_domain, + previous_org_id, + org.id, + ) + + db.session.commit() return org @@ -542,6 +661,10 @@ def _create_service( ) dao_create_service(service, user) + service.active = True + service.restricted = False + service.organisation_id = org_id + service.organisation_type = "central" service.rate_limit = rate_limit service.sms_message_limit = sms_message_limit service.letter_message_limit = letter_message_limit @@ -568,6 +691,23 @@ def _create_api_key(name, service_id, user_id, key_type="normal"): api_keys = get_model_api_keys(service_id=service_id) for key in api_keys: if key.name == name: + if key.expiry_date is not None: + continue + + try: + _ = key.secret + except BadSignature: + # Keep key identity stable and rotate the secret in place to avoid dangling references. + current_app.logger.warning( + "Fixture api key %s for service %s had invalid signature; rotating in place", + name, + service_id, + ) + key.secret = uuid4() + db.session.add(key) + db.session.commit() + return key + return key request = {"created_by": user_id, "key_type": key_type, "name": name} @@ -580,10 +720,63 @@ def _create_api_key(name, service_id, user_id, key_type="normal"): return valid_api_key +def _repair_invalid_service_api_keys(service_id): + # Use get_model_api_keys so we cover the same key set auth paths read (active + recently expired). + api_keys = get_model_api_keys(service_id=service_id) + repaired_count = 0 + + for key in api_keys: + try: + _ = key.secret + except BadSignature: + key.secret = uuid4() + db.session.add(key) + repaired_count += 1 + + if repaired_count > 0: + db.session.commit() + current_app.logger.info( + "Rotated %s invalid api key secret(s) for fixture service %s", + repaired_count, + service_id, + ) + + def _create_inbound_numbers(service_id, user_id, number="07700900500", provider="mmg"): inbound_number = dao_get_inbound_number_for_service(service_id=service_id) + if inbound_number is not None and inbound_number.number == number: + return inbound_number.id + + inbound_number_by_number = InboundNumber.query.filter_by(number=number).one_or_none() + if inbound_number_by_number is not None: + # Number is globally unique; reclaim it if sanitised data left it attached to another service. + if inbound_number is not None and inbound_number.id != inbound_number_by_number.id: + inbound_number.service_id = None + db.session.add(inbound_number) + + previous_service_id = inbound_number_by_number.service_id + inbound_number_by_number.service_id = service_id + inbound_number_by_number.active = True + db.session.add(inbound_number_by_number) + db.session.commit() + + if previous_service_id and previous_service_id != service_id: + current_app.logger.info( + "Reassigned inbound number %s from service %s to fixture service %s", + number, + previous_service_id, + service_id, + ) + + return inbound_number_by_number.id + if inbound_number is not None: + inbound_number.number = number + inbound_number.provider = provider + inbound_number.active = True + db.session.add(inbound_number) + db.session.commit() return inbound_number.id inbound_number = InboundNumber() @@ -751,6 +944,36 @@ def _create_service_sms_senders(service_id, sms_sender, is_default, inbound_numb for service_sms_sender in service_sms_senders: if service_sms_sender.sms_sender == sms_sender: return service_sms_sender + if inbound_number_id and service_sms_sender.inbound_number_id == inbound_number_id: + return service_sms_sender + + if inbound_number_id: + existing_inbound_sender = ServiceSmsSender.query.filter_by(inbound_number_id=inbound_number_id).one_or_none() + if existing_inbound_sender is not None: + # inbound_number_id is unique; move the sender row over instead of creating a conflicting duplicate. + if is_default: + for service_sms_sender in service_sms_senders: + if service_sms_sender.id != existing_inbound_sender.id and service_sms_sender.is_default: + service_sms_sender.is_default = False + db.session.add(service_sms_sender) + + previous_service_id = existing_inbound_sender.service_id + existing_inbound_sender.service_id = service_id + existing_inbound_sender.sms_sender = sms_sender + existing_inbound_sender.is_default = is_default + existing_inbound_sender.archived = False + db.session.add(existing_inbound_sender) + db.session.commit() + + if previous_service_id != service_id: + current_app.logger.info( + "Reassigned inbound sms sender for number %s from service %s to fixture service %s", + sms_sender, + previous_service_id, + service_id, + ) + + return existing_inbound_sender return dao_add_sms_sender_for_service(service_id, sms_sender, is_default, inbound_number_id) diff --git a/tests/app/functional_test_fixtures/test_functional_test_fixtures.py b/tests/app/functional_test_fixtures/test_functional_test_fixtures.py index 8e68ab070d..aafa12b8a1 100644 --- a/tests/app/functional_test_fixtures/test_functional_test_fixtures.py +++ b/tests/app/functional_test_fixtures/test_functional_test_fixtures.py @@ -6,8 +6,21 @@ from moto import mock_aws from app import db -from app.functional_tests_fixtures import _create_db_objects, _create_user, apply_fixtures -from app.models import Service, Template +from app.functional_tests_fixtures import ( + _create_api_key, + _create_db_objects, + _create_user, + _repair_invalid_service_api_keys, + apply_fixtures, +) +from app.models import ApiKey, InboundNumber, Organisation, Service, ServiceSmsSender, Template +from tests.app.db import ( + create_api_key, + create_inbound_number, + create_organisation, + create_service, + create_service_sms_sender, +) from tests.conftest import set_config_values @@ -176,6 +189,137 @@ def test_create_db_objects_creates_dedicated_performance_service_with_limits_and assert template.service_id == performance_service.id +def test_create_db_objects_renames_sanitised_org_and_recreates_canonical_org(notify_api, notify_service): + sanitised_org = create_organisation(name="Functional Tests Org", domains=["digital.cabinet-office.gov.uk"]) + sanitised_org.request_to_go_live_notes = "redacted" + sanitised_org.notes = "redacted" + db.session.add(sanitised_org) + db.session.commit() + + with set_config_values( + notify_api, + { + "MMG_INBOUND_SMS_USERNAME": ["test_mmg_username"], + "MMG_INBOUND_SMS_AUTH": ["test_mmg_password"], + "INTERNAL_CLIENT_API_KEYS": {"notify-functional-tests": ["functional-tests-secret-key"]}, + "ADMIN_BASE_URL": "http://localhost:6012", + "API_HOST_NAME": "http://localhost:6011", + }, + ): + functional_vars, _ = _create_db_objects( + "fake password", + "test_request_bin_token", + "dev-env", + "notify-tests-preview", + "digital.cabinet-office.gov.uk", + "govuk_notify", + "functional_tests_service_live_key", + "functional_tests_service_test_key", + str(notify_service.id), + "Functional Tests Org", + "07700900500", + ) + + recreated_org = Organisation.query.get(functional_vars["FUNCTIONAL_TESTS_ORGANISATION_ID"]) + original_org = Organisation.query.get(sanitised_org.id) + + assert recreated_org.name == "Functional Tests Org" + assert recreated_org.id != sanitised_org.id + assert original_org.name != "Functional Tests Org" + assert original_org.name.startswith("Functional Tests Org [sanitised-") + + +def test_create_db_objects_renames_sanitised_services_and_reclaims_inbound_number(notify_api, notify_service): + sanitised_org = create_organisation(name="Legacy Sanitised Org") + sanitised_functional_service = create_service( + service_name="Functional Tests", + organisation=sanitised_org, + contact_link="redacted.gov.uk", + ) + create_service( + service_name="Performance Tests", + organisation=sanitised_org, + contact_link="redacted.gov.uk", + ) + + inbound_number = create_inbound_number(number="07700900500", service_id=sanitised_functional_service.id) + create_service_sms_sender( + service=sanitised_functional_service, + sms_sender="07700900500", + is_default=True, + inbound_number_id=inbound_number.id, + ) + + with set_config_values( + notify_api, + { + "MMG_INBOUND_SMS_USERNAME": ["test_mmg_username"], + "MMG_INBOUND_SMS_AUTH": ["test_mmg_password"], + "INTERNAL_CLIENT_API_KEYS": {"notify-functional-tests": ["functional-tests-secret-key"]}, + "ADMIN_BASE_URL": "http://localhost:6012", + "API_HOST_NAME": "http://localhost:6011", + }, + ): + functional_vars, _ = _create_db_objects( + "fake password", + "test_request_bin_token", + "dev-env", + "notify-tests-preview", + "digital.cabinet-office.gov.uk", + "govuk_notify", + "functional_tests_service_live_key", + "functional_tests_service_test_key", + str(notify_service.id), + "Functional Tests Org", + "07700900500", + ) + + recreated_functional_service = Service.query.get(functional_vars["FUNCTIONAL_TESTS_SERVICE_ID"]) + renamed_service = Service.query.get(sanitised_functional_service.id) + reassigned_inbound = InboundNumber.query.filter_by(number="07700900500").one() + inbound_sender = ServiceSmsSender.query.filter_by(inbound_number_id=reassigned_inbound.id).one() + + assert recreated_functional_service.name == "Functional Tests" + assert recreated_functional_service.id != sanitised_functional_service.id + assert renamed_service.name != "Functional Tests" + assert renamed_service.name.startswith("Functional Tests [sanitised-") + assert str(reassigned_inbound.service_id) == str(recreated_functional_service.id) + assert str(inbound_sender.service_id) == str(recreated_functional_service.id) + + +def test_create_api_key_repairs_when_existing_key_secret_is_invalid(notify_service, sample_user): + broken_key = create_api_key(notify_service, key_name="functional_tests_service_live_key") + broken_key._secret = "broken-signature-value" + db.session.add(broken_key) + db.session.commit() + + repaired_key = _create_api_key( + "functional_tests_service_live_key", + notify_service.id, + sample_user.id, + "normal", + ) + + refreshed_broken_key = ApiKey.query.get(broken_key.id) + assert refreshed_broken_key.expiry_date is None + assert repaired_key.id == broken_key.id + assert repaired_key.secret is not None + + +def test_repair_invalid_service_api_keys_repairs_recently_expired_keys(notify_service): + broken_key = create_api_key(notify_service, key_name="broken_expired_key") + broken_key.expiry_date = datetime.utcnow() + broken_key._secret = "broken-signature-value" + db.session.add(broken_key) + db.session.commit() + + _repair_invalid_service_api_keys(notify_service.id) + + refreshed_broken_key = ApiKey.query.get(broken_key.id) + assert refreshed_broken_key.expiry_date is not None + assert refreshed_broken_key.secret is not None + + @mock_aws def test_function_test_fixtures_uploads_to_ssm(notify_api, os_environ, mocker): mocker.patch( From d88d18edc37cf905ad02c4451607c0b627aa263b Mon Sep 17 00:00:00 2001 From: Wesley Hindle <76964214+WesleyHindle@users.noreply.github.com> Date: Mon, 27 Apr 2026 14:59:27 +0100 Subject: [PATCH 5/5] Revert "Add support for sanitised database restore inside the fixtures" --- app/functional_tests_fixtures/__init__.py | 231 +----------------- .../test_functional_test_fixtures.py | 148 +---------- 2 files changed, 6 insertions(+), 373 deletions(-) diff --git a/app/functional_tests_fixtures/__init__.py b/app/functional_tests_fixtures/__init__.py index 3fcaf57372..697d230135 100644 --- a/app/functional_tests_fixtures/__init__.py +++ b/app/functional_tests_fixtures/__init__.py @@ -4,10 +4,8 @@ import boto3 from flask import current_app -from itsdangerous.exc import BadSignature from sqlalchemy.exc import NoResultFound -from app import db from app.constants import ( EDIT_FOLDER_PERMISSIONS, EMAIL_AUTH, @@ -35,6 +33,7 @@ dao_create_organisation, dao_get_organisation_by_id, dao_get_organisations_by_partial_name, + dao_update_organisation, ) from app.dao.permissions_dao import permission_dao from app.dao.service_callback_api_dao import get_service_callback_api_by_callback_type, save_service_callback_api @@ -65,7 +64,6 @@ ) from app.dao.users_dao import get_user_by_email, save_model_user from app.models import ( - Domain, InboundNumber, InboundSms, Organisation, @@ -73,7 +71,6 @@ Service, ServiceCallbackApi, ServiceEmailReplyTo, - ServiceSmsSender, User, ) from app.schemas import api_key_schema, template_schema @@ -104,86 +101,6 @@ def _upload_env_to_ssm(ssm_upload_path, env_config, description): ) from exc -def _build_sanitised_renamed_name(name): - # Keep the original fixture name discoverable while making space for a clean canonical row. - suffix = f" [sanitised-{datetime.utcnow().strftime('%Y%m%d%H%M%S')}]" - max_name_length = 255 - if len(name) + len(suffix) <= max_name_length: - return f"{name}{suffix}" - return f"{name[: max_name_length - len(suffix)]}{suffix}" - - -def _is_sanitised_org(organisation): - # These values are written by the sanitise scripts and are a stable signal that this is a copied prod org. - return ( - organisation.request_to_go_live_notes == "redacted" - or organisation.notes == "redacted" - or organisation.agreement_signed_on_behalf_of_name == "redacted" - or organisation.agreement_signed_on_behalf_of_email_address == "redacted@redacted.com" - ) - - -def _is_sanitised_service(service): - # These fields are rewritten during sanitisation; if they are present on canonical fixture names we rotate them. - return ( - service.contact_link == "redacted.gov.uk" - or service.notes == "redacted" - or service.billing_contact_names == "redacted" - or service.billing_contact_email_addresses == "redacted@redacted.com" - or service.email_sender_local_part.endswith(".redacted") - ) - - -def _rename_sanitised_fixture_org(org_name): - organisations = dao_get_organisations_by_partial_name(org_name) - - candidate = None - for organisation in organisations: - if organisation.name == org_name: - candidate = organisation - break - - if candidate is None or not _is_sanitised_org(candidate): - return - - # We rename rather than mutate in place so fixtures can recreate a known-good canonical org deterministically. - old_name = candidate.name - candidate.name = _build_sanitised_renamed_name(candidate.name) - db.session.add(candidate) - db.session.commit() - current_app.logger.info("Renamed sanitised fixture organisation '%s' to '%s'", old_name, candidate.name) - - -def _rename_sanitised_fixture_service(service_name): - services = get_services_by_partial_name(service_name) - - candidate = None - for service in services: - if service.name == service_name: - candidate = service - break - - if candidate is None or not _is_sanitised_service(candidate): - return - - # Unique constraints on inbound numbers/senders can block fixture creation, so detach these before rotating name. - if candidate.inbound_number is not None: - candidate.inbound_number.service_id = None - db.session.add(candidate.inbound_number) - - for sms_sender in candidate.service_sms_senders: - if sms_sender.inbound_number_id is not None: - sms_sender.inbound_number_id = None - sms_sender.archived = True - db.session.add(sms_sender) - - old_name = candidate.name - candidate.name = _build_sanitised_renamed_name(candidate.name) - db.session.add(candidate) - db.session.commit() - current_app.logger.info("Renamed sanitised fixture service '%s' to '%s'", old_name, candidate.name) - - """ This function creates a set of database fixtures that functional tests use to run against. @@ -268,14 +185,6 @@ def _create_db_objects( ) -> tuple[dict[str, str], dict[str, str]]: current_app.logger.info("Creating functional test fixtures for %s:", environment) - functional_service_name = "Functional Tests" - performance_service_name = "Performance Tests" - - # First clear out sanitised copies that may be occupying canonical fixture names. - _rename_sanitised_fixture_org(org_name) - _rename_sanitised_fixture_service(functional_service_name) - _rename_sanitised_fixture_service(performance_service_name) - current_app.logger.info("--> Ensure organisation exists") org = _create_organiation(email_domain, org_name) @@ -301,22 +210,17 @@ def _create_db_objects( ) current_app.logger.info("--> Ensure service exists") - service = _create_service(org.id, service_admin_user, service_name=functional_service_name) + service = _create_service(org.id, service_admin_user) performance_service = _create_service( org.id, service_admin_user, - service_name=performance_service_name, + service_name="Performance Tests", rate_limit=performance_test_rate_limit, sms_message_limit=performance_test_sms_limit, letter_message_limit=performance_test_letter_limit, email_message_limit=performance_test_email_limit, ) - # Repair invalid key payloads early so later auth/serialization paths do not fail with BadSignature. - _repair_invalid_service_api_keys(service.id) - _repair_invalid_service_api_keys(performance_service.id) - _repair_invalid_service_api_keys(govuk_service_id) - current_app.logger.info("--> Ensure users are added to service") dao_add_user_to_service(service, service_admin_user) dao_add_user_to_service(service, email_auth_user) @@ -600,30 +504,7 @@ def _create_organiation(email_domain, org_name): dao_create_organisation(org) - org.name = org_name - org.active = True - org.crown = False - org.organisation_type = "central" - org.can_approve_own_go_live_requests = True - db.session.add(org) - - normalised_email_domain = email_domain.lower() - existing_domain = Domain.query.filter_by(domain=normalised_email_domain).one_or_none() - if existing_domain is None: - db.session.add(Domain(domain=normalised_email_domain, organisation_id=org.id)) - elif existing_domain.organisation_id != org.id: - # Domain ownership is unique across orgs; reclaim it so fixture users resolve to this fixture org. - previous_org_id = existing_domain.organisation_id - existing_domain.organisation_id = org.id - db.session.add(existing_domain) - current_app.logger.info( - "Reassigned domain %s from organisation %s to fixture organisation %s", - normalised_email_domain, - previous_org_id, - org.id, - ) - - db.session.commit() + dao_update_organisation(org.id, domains=[email_domain], can_approve_own_go_live_requests=True) return org @@ -661,10 +542,6 @@ def _create_service( ) dao_create_service(service, user) - service.active = True - service.restricted = False - service.organisation_id = org_id - service.organisation_type = "central" service.rate_limit = rate_limit service.sms_message_limit = sms_message_limit service.letter_message_limit = letter_message_limit @@ -691,23 +568,6 @@ def _create_api_key(name, service_id, user_id, key_type="normal"): api_keys = get_model_api_keys(service_id=service_id) for key in api_keys: if key.name == name: - if key.expiry_date is not None: - continue - - try: - _ = key.secret - except BadSignature: - # Keep key identity stable and rotate the secret in place to avoid dangling references. - current_app.logger.warning( - "Fixture api key %s for service %s had invalid signature; rotating in place", - name, - service_id, - ) - key.secret = uuid4() - db.session.add(key) - db.session.commit() - return key - return key request = {"created_by": user_id, "key_type": key_type, "name": name} @@ -720,63 +580,10 @@ def _create_api_key(name, service_id, user_id, key_type="normal"): return valid_api_key -def _repair_invalid_service_api_keys(service_id): - # Use get_model_api_keys so we cover the same key set auth paths read (active + recently expired). - api_keys = get_model_api_keys(service_id=service_id) - repaired_count = 0 - - for key in api_keys: - try: - _ = key.secret - except BadSignature: - key.secret = uuid4() - db.session.add(key) - repaired_count += 1 - - if repaired_count > 0: - db.session.commit() - current_app.logger.info( - "Rotated %s invalid api key secret(s) for fixture service %s", - repaired_count, - service_id, - ) - - def _create_inbound_numbers(service_id, user_id, number="07700900500", provider="mmg"): inbound_number = dao_get_inbound_number_for_service(service_id=service_id) - if inbound_number is not None and inbound_number.number == number: - return inbound_number.id - - inbound_number_by_number = InboundNumber.query.filter_by(number=number).one_or_none() - if inbound_number_by_number is not None: - # Number is globally unique; reclaim it if sanitised data left it attached to another service. - if inbound_number is not None and inbound_number.id != inbound_number_by_number.id: - inbound_number.service_id = None - db.session.add(inbound_number) - - previous_service_id = inbound_number_by_number.service_id - inbound_number_by_number.service_id = service_id - inbound_number_by_number.active = True - db.session.add(inbound_number_by_number) - db.session.commit() - - if previous_service_id and previous_service_id != service_id: - current_app.logger.info( - "Reassigned inbound number %s from service %s to fixture service %s", - number, - previous_service_id, - service_id, - ) - - return inbound_number_by_number.id - if inbound_number is not None: - inbound_number.number = number - inbound_number.provider = provider - inbound_number.active = True - db.session.add(inbound_number) - db.session.commit() return inbound_number.id inbound_number = InboundNumber() @@ -944,36 +751,6 @@ def _create_service_sms_senders(service_id, sms_sender, is_default, inbound_numb for service_sms_sender in service_sms_senders: if service_sms_sender.sms_sender == sms_sender: return service_sms_sender - if inbound_number_id and service_sms_sender.inbound_number_id == inbound_number_id: - return service_sms_sender - - if inbound_number_id: - existing_inbound_sender = ServiceSmsSender.query.filter_by(inbound_number_id=inbound_number_id).one_or_none() - if existing_inbound_sender is not None: - # inbound_number_id is unique; move the sender row over instead of creating a conflicting duplicate. - if is_default: - for service_sms_sender in service_sms_senders: - if service_sms_sender.id != existing_inbound_sender.id and service_sms_sender.is_default: - service_sms_sender.is_default = False - db.session.add(service_sms_sender) - - previous_service_id = existing_inbound_sender.service_id - existing_inbound_sender.service_id = service_id - existing_inbound_sender.sms_sender = sms_sender - existing_inbound_sender.is_default = is_default - existing_inbound_sender.archived = False - db.session.add(existing_inbound_sender) - db.session.commit() - - if previous_service_id != service_id: - current_app.logger.info( - "Reassigned inbound sms sender for number %s from service %s to fixture service %s", - sms_sender, - previous_service_id, - service_id, - ) - - return existing_inbound_sender return dao_add_sms_sender_for_service(service_id, sms_sender, is_default, inbound_number_id) diff --git a/tests/app/functional_test_fixtures/test_functional_test_fixtures.py b/tests/app/functional_test_fixtures/test_functional_test_fixtures.py index aafa12b8a1..8e68ab070d 100644 --- a/tests/app/functional_test_fixtures/test_functional_test_fixtures.py +++ b/tests/app/functional_test_fixtures/test_functional_test_fixtures.py @@ -6,21 +6,8 @@ from moto import mock_aws from app import db -from app.functional_tests_fixtures import ( - _create_api_key, - _create_db_objects, - _create_user, - _repair_invalid_service_api_keys, - apply_fixtures, -) -from app.models import ApiKey, InboundNumber, Organisation, Service, ServiceSmsSender, Template -from tests.app.db import ( - create_api_key, - create_inbound_number, - create_organisation, - create_service, - create_service_sms_sender, -) +from app.functional_tests_fixtures import _create_db_objects, _create_user, apply_fixtures +from app.models import Service, Template from tests.conftest import set_config_values @@ -189,137 +176,6 @@ def test_create_db_objects_creates_dedicated_performance_service_with_limits_and assert template.service_id == performance_service.id -def test_create_db_objects_renames_sanitised_org_and_recreates_canonical_org(notify_api, notify_service): - sanitised_org = create_organisation(name="Functional Tests Org", domains=["digital.cabinet-office.gov.uk"]) - sanitised_org.request_to_go_live_notes = "redacted" - sanitised_org.notes = "redacted" - db.session.add(sanitised_org) - db.session.commit() - - with set_config_values( - notify_api, - { - "MMG_INBOUND_SMS_USERNAME": ["test_mmg_username"], - "MMG_INBOUND_SMS_AUTH": ["test_mmg_password"], - "INTERNAL_CLIENT_API_KEYS": {"notify-functional-tests": ["functional-tests-secret-key"]}, - "ADMIN_BASE_URL": "http://localhost:6012", - "API_HOST_NAME": "http://localhost:6011", - }, - ): - functional_vars, _ = _create_db_objects( - "fake password", - "test_request_bin_token", - "dev-env", - "notify-tests-preview", - "digital.cabinet-office.gov.uk", - "govuk_notify", - "functional_tests_service_live_key", - "functional_tests_service_test_key", - str(notify_service.id), - "Functional Tests Org", - "07700900500", - ) - - recreated_org = Organisation.query.get(functional_vars["FUNCTIONAL_TESTS_ORGANISATION_ID"]) - original_org = Organisation.query.get(sanitised_org.id) - - assert recreated_org.name == "Functional Tests Org" - assert recreated_org.id != sanitised_org.id - assert original_org.name != "Functional Tests Org" - assert original_org.name.startswith("Functional Tests Org [sanitised-") - - -def test_create_db_objects_renames_sanitised_services_and_reclaims_inbound_number(notify_api, notify_service): - sanitised_org = create_organisation(name="Legacy Sanitised Org") - sanitised_functional_service = create_service( - service_name="Functional Tests", - organisation=sanitised_org, - contact_link="redacted.gov.uk", - ) - create_service( - service_name="Performance Tests", - organisation=sanitised_org, - contact_link="redacted.gov.uk", - ) - - inbound_number = create_inbound_number(number="07700900500", service_id=sanitised_functional_service.id) - create_service_sms_sender( - service=sanitised_functional_service, - sms_sender="07700900500", - is_default=True, - inbound_number_id=inbound_number.id, - ) - - with set_config_values( - notify_api, - { - "MMG_INBOUND_SMS_USERNAME": ["test_mmg_username"], - "MMG_INBOUND_SMS_AUTH": ["test_mmg_password"], - "INTERNAL_CLIENT_API_KEYS": {"notify-functional-tests": ["functional-tests-secret-key"]}, - "ADMIN_BASE_URL": "http://localhost:6012", - "API_HOST_NAME": "http://localhost:6011", - }, - ): - functional_vars, _ = _create_db_objects( - "fake password", - "test_request_bin_token", - "dev-env", - "notify-tests-preview", - "digital.cabinet-office.gov.uk", - "govuk_notify", - "functional_tests_service_live_key", - "functional_tests_service_test_key", - str(notify_service.id), - "Functional Tests Org", - "07700900500", - ) - - recreated_functional_service = Service.query.get(functional_vars["FUNCTIONAL_TESTS_SERVICE_ID"]) - renamed_service = Service.query.get(sanitised_functional_service.id) - reassigned_inbound = InboundNumber.query.filter_by(number="07700900500").one() - inbound_sender = ServiceSmsSender.query.filter_by(inbound_number_id=reassigned_inbound.id).one() - - assert recreated_functional_service.name == "Functional Tests" - assert recreated_functional_service.id != sanitised_functional_service.id - assert renamed_service.name != "Functional Tests" - assert renamed_service.name.startswith("Functional Tests [sanitised-") - assert str(reassigned_inbound.service_id) == str(recreated_functional_service.id) - assert str(inbound_sender.service_id) == str(recreated_functional_service.id) - - -def test_create_api_key_repairs_when_existing_key_secret_is_invalid(notify_service, sample_user): - broken_key = create_api_key(notify_service, key_name="functional_tests_service_live_key") - broken_key._secret = "broken-signature-value" - db.session.add(broken_key) - db.session.commit() - - repaired_key = _create_api_key( - "functional_tests_service_live_key", - notify_service.id, - sample_user.id, - "normal", - ) - - refreshed_broken_key = ApiKey.query.get(broken_key.id) - assert refreshed_broken_key.expiry_date is None - assert repaired_key.id == broken_key.id - assert repaired_key.secret is not None - - -def test_repair_invalid_service_api_keys_repairs_recently_expired_keys(notify_service): - broken_key = create_api_key(notify_service, key_name="broken_expired_key") - broken_key.expiry_date = datetime.utcnow() - broken_key._secret = "broken-signature-value" - db.session.add(broken_key) - db.session.commit() - - _repair_invalid_service_api_keys(notify_service.id) - - refreshed_broken_key = ApiKey.query.get(broken_key.id) - assert refreshed_broken_key.expiry_date is not None - assert refreshed_broken_key.secret is not None - - @mock_aws def test_function_test_fixtures_uploads_to_ssm(notify_api, os_environ, mocker): mocker.patch(