From 90ff64b1cfc18c134195c83e4805b5b4cecbc5dc Mon Sep 17 00:00:00 2001 From: Richard Parke Date: Tue, 3 Feb 2026 19:18:02 +0000 Subject: [PATCH] added validation for retention period document-download-api already validates at the point of sending, but we should try and catch any errors at the point of file creation too. --- app/constants.py | 4 +++ app/schema_validation/__init__.py | 23 +++++++++++++- .../template_email_files_schemas.py | 4 +-- .../app/dao/test_template_email_files_dao.py | 6 ++-- tests/app/db.py | 2 +- tests/app/template_email_files/test_rest.py | 30 +++++++++++-------- 6 files changed, 50 insertions(+), 19 deletions(-) diff --git a/app/constants.py b/app/constants.py index d45660e4e7..ef360c74a9 100644 --- a/app/constants.py +++ b/app/constants.py @@ -413,3 +413,7 @@ class CacheKeys: # Date and time SECONDS_IN_1_MINUTE = 60 + +# Document Download +DOCUMENT_DOWNLOAD_RETENTION_UPPER_LIMIT_IN_WEEKS = 78 +DOCUMENT_DOWNLOAD_RETENTION_LOWER_LIMIT_IN_WEEKS = 1 diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index eecf85c3ca..8974cf5723 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -9,6 +9,11 @@ from notifications_utils.recipient_validation.errors import InvalidEmailError, InvalidPhoneError from notifications_utils.recipient_validation.phone_number import PhoneNumber +from app.constants import ( + DOCUMENT_DOWNLOAD_RETENTION_LOWER_LIMIT_IN_WEEKS, + DOCUMENT_DOWNLOAD_RETENTION_UPPER_LIMIT_IN_WEEKS, +) + format_checker = FormatChecker() @@ -88,7 +93,12 @@ def validate_schema_retention_period(instance): if isinstance(instance, str): period = instance.strip().lower() match = re.match(r"^(\d+) weeks?$", period) - if match and 1 <= int(match.group(1)) <= 78: + if ( + match + and DOCUMENT_DOWNLOAD_RETENTION_LOWER_LIMIT_IN_WEEKS + <= int(match.group(1)) + <= DOCUMENT_DOWNLOAD_RETENTION_UPPER_LIMIT_IN_WEEKS + ): return True raise ValidationError( @@ -126,6 +136,17 @@ def send_a_file_confirm_email_before_download(instance): ) +@format_checker.checks("send_file_via_ui_retention_period", raises=ValidationError) +def validate_send_file_via_ui_retention_period(instance): + if isinstance(instance, int): + return ( + DOCUMENT_DOWNLOAD_RETENTION_LOWER_LIMIT_IN_WEEKS + <= instance + <= DOCUMENT_DOWNLOAD_RETENTION_UPPER_LIMIT_IN_WEEKS + ) + raise ValidationError(f"Unsuported value for for retention period: {instance}. Supported values are 1 to 78") + + def validate(json_to_validate, schema): validator = Draft7Validator(schema, format_checker=format_checker) errors = list(validator.iter_errors(json_to_validate)) diff --git a/app/template_email_files/template_email_files_schemas.py b/app/template_email_files/template_email_files_schemas.py index 2aa30e4c71..a0cd24c2f4 100644 --- a/app/template_email_files/template_email_files_schemas.py +++ b/app/template_email_files/template_email_files_schemas.py @@ -10,7 +10,7 @@ "filename": {"type": "string"}, "link_text": {"type": "string"}, "service": uuid, - "retention_period": {"type": "integer"}, + "retention_period": {"type": "integer", "format": "send_file_via_ui_retention_period"}, "validate_users_email": {"type": "boolean"}, "created_by_id": uuid, }, @@ -26,7 +26,7 @@ "filename": {"type": "string"}, "link_text": {"type": "string"}, "service": uuid, - "retention_period": {"type": "integer"}, + "retention_period": {"type": "integer", "format": "send_file_via_ui_retention_period"}, "validate_users_email": {"type": "boolean"}, "template_version": {"type": "integer"}, "archived_by_id": uuid, diff --git a/tests/app/dao/test_template_email_files_dao.py b/tests/app/dao/test_template_email_files_dao.py index 159f4ef213..cd3cb4458e 100644 --- a/tests/app/dao/test_template_email_files_dao.py +++ b/tests/app/dao/test_template_email_files_dao.py @@ -22,7 +22,7 @@ def test_create_template_email_files_dao(sample_email_template, sample_service): "id": "d963f496-b075-4e13-90ae-1f009feddbc6", "filename": "example.pdf", "link_text": "click this link!", - "retention_period": 90, + "retention_period": 45, "validate_users_email": True, "template_id": str(sample_email_template.id), "template_version": int(sample_email_template.version), @@ -36,7 +36,7 @@ def test_create_template_email_files_dao(sample_email_template, sample_service): assert str(template_email_file.id) == "d963f496-b075-4e13-90ae-1f009feddbc6" assert template_email_file.filename == "example.pdf" assert template_email_file.link_text == "click this link!" - assert template_email_file.retention_period == 90 + assert template_email_file.retention_period == 45 assert template_email_file.validate_users_email assert template_email_file.version == 1 assert template_email_file.created_by_id == sample_service.users[0].id @@ -138,7 +138,7 @@ def test_dao_get_template_email_files_by_template_id_historical(sample_email_tem assert template_email_file_fetched.id == sample_template_email_file.id assert template_email_file_fetched.filename == sample_template_email_file.filename assert template_email_file_fetched.link_text == sample_template_email_file.link_text - assert template_email_file_fetched.retention_period == 90 # we want the initial retention period for historical + assert template_email_file_fetched.retention_period == 45 # we want the initial retention period for historical assert template_email_file_fetched.validate_users_email == sample_template_email_file.validate_users_email assert template_email_file_fetched.template_version == template_version_to_get assert template_email_file_fetched.created_by_id == sample_template_email_file.created_by_id diff --git a/tests/app/db.py b/tests/app/db.py index 90407d4731..b20040a31d 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -197,7 +197,7 @@ def create_template_email_file( created_by_id, filename="example.pdf", link_text="follow this link", - retention_period=90, + retention_period=45, validate_users_email=True, ): data = { diff --git a/tests/app/template_email_files/test_rest.py b/tests/app/template_email_files/test_rest.py index 816d06fbc3..9dc28cfb1c 100644 --- a/tests/app/template_email_files/test_rest.py +++ b/tests/app/template_email_files/test_rest.py @@ -13,7 +13,7 @@ def test_create_template_email_file_happy_path(sample_service, sample_email_temp data = { "filename": "example.pdf", "link_text": "click this link!", - "retention_period": 90, + "retention_period": 45, "validate_users_email": True, "created_by_id": str(sample_service.users[0].id), } @@ -27,7 +27,7 @@ def test_create_template_email_file_happy_path(sample_service, sample_email_temp # test response contains created email file data assert response["data"]["filename"] == "example.pdf" - assert response["data"]["retention_period"] == 90 + assert response["data"]["retention_period"] == 45 assert response["data"]["link_text"] == "click this link!" assert response["data"]["validate_users_email"] assert response["data"]["template_id"] == str(sample_email_template.id) @@ -37,7 +37,7 @@ def test_create_template_email_file_happy_path(sample_service, sample_email_temp # test that email file gets persisted into the database template_email_file = TemplateEmailFile.query.get(str(response["data"]["id"])) assert template_email_file.filename == "example.pdf" - assert template_email_file.retention_period == 90 + assert template_email_file.retention_period == 45 assert template_email_file.link_text == "click this link!" assert template_email_file.validate_users_email assert template_email_file.template_id == sample_email_template.id @@ -53,7 +53,7 @@ def test_create_template_email_file_fails_if_template_not_email_type( data = { "filename": "example.pdf", "link_text": "click this link!", - "retention_period": 90, + "retention_period": 45, "validate_users_email": True, "created_by_id": str(sample_service.users[0].id), } @@ -73,7 +73,7 @@ def test_create_template_email_file_fails_if_template_does_not_exist(sample_serv data = { "filename": "example.pdf", "link_text": "click this link!", - "retention_period": 90, + "retention_period": 45, "validate_users_email": True, "created_by_id": str(sample_service.users[0].id), } @@ -95,7 +95,7 @@ def test_create_template_email_file_fails_if_template_already_has_file_with_same data = { "filename": filename, "link_text": "click this link!", - "retention_period": 90, + "retention_period": 45, "validate_users_email": True, "created_by_id": str(sample_service.users[0].id), } @@ -154,14 +154,20 @@ def test_create_template_email_file_creates_file_with_latest_template_version( "retention_period": "not an integer", "validate_users_email": True, }, - [{"error": "ValidationError", "message": "retention_period not an integer is not of type integer"}], + [ + {"error": "ValidationError", "message": "retention_period not an integer is not of type integer"}, + { + "error": "ValidationError", + "message": "retention_period Unsuported value for for retention period: not an integer. Supported values are 1 to 78", # noqa: E501 + }, + ], ), ( { "id": "d963f496-b075-4e13-90ae-1f009feddbc6", "filename": "example.pdf", "link_text": "click this link!", - "retention_period": 90, + "retention_period": 45, "validate_users_email": "this is a string", }, [{"error": "ValidationError", "message": "validate_users_email this is a string is not of type boolean"}], @@ -195,7 +201,7 @@ def test_create_template_email_file_raises_exception_for_invalid_data( { "filename": "example.pdf", "link_text": "example.pdf", - "retention_period": 90, + "retention_period": 45, "validate_users_email": True, }, ), @@ -203,7 +209,7 @@ def test_create_template_email_file_raises_exception_for_invalid_data( { "filename": "example.pdf", "link_text": "example.pdf", - "retention_period": 90, + "retention_period": 45, "validate_users_email": True, }, { @@ -309,7 +315,7 @@ def test_update_template_email_file( {"id": str(sample_template_email_file.id), "version": 1} ) assert template_email_file_history_version_one.link_text == "follow this link" - assert template_email_file_history_version_one.retention_period == 90 + assert template_email_file_history_version_one.retention_period == 45 assert template_email_file_history_version_one.validate_users_email is True assert template_email_file_history_version_one.template_version == 2 assert template_email_file_history_version_one.version == 1 @@ -358,7 +364,7 @@ def test_archive_template_email_file(client, sample_service, sample_email_templa data = { "filename": "example.pdf", "link_text": "click this link!", - "retention_period": 90, + "retention_period": 45, "validate_users_email": True, "template_id": str(sample_email_template.id), "created_by_id": str(sample_service.users[0].id),