From 7c6ae4003439a8478c64f87005fab394b875b05c Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 22 Apr 2021 14:42:54 +0100 Subject: [PATCH 1/2] Normalise broadcast content before validating length MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This changes the content length validation of the internal API to match the validation of the public broadcast API[1]. This removes the length check from JSONSchema, which isn’t sophisticated enough to deal with things like normalising newlines or handling different encodings. The admin app should catch these errors before they’re raised here, but it’s best to be belt and braces. 1.https://github.com/alphagov/notifications-api/blob/7ab0403ae7ab9c9a17b3fb6595c8c8d2f8b32cbb/app/v2/broadcast/post_broadcast.py#L53-L63 --- .../broadcast_message_schema.py | 2 +- app/broadcast_message/rest.py | 14 ++++++++ tests/app/broadcast_message/test_rest.py | 34 +++++++++++++++++++ 3 files changed, 49 insertions(+), 1 deletion(-) diff --git a/app/broadcast_message/broadcast_message_schema.py b/app/broadcast_message/broadcast_message_schema.py index 09eff203b..fc4527511 100644 --- a/app/broadcast_message/broadcast_message_schema.py +++ b/app/broadcast_message/broadcast_message_schema.py @@ -15,7 +15,7 @@ create_broadcast_message_schema = { 'finishes_at': {'type': 'string', 'format': 'datetime'}, 'areas': {"type": "array", "items": {"type": "string"}}, 'simple_polygons': {"type": "array", "items": {"type": "array"}}, - 'content': {'type': 'string', 'minLength': 1, 'maxLength': 1395}, + 'content': {'type': 'string', 'minLength': 1}, 'reference': {'type': 'string', 'minLength': 1, 'maxLength': 255}, }, 'required': ['service_id', 'created_by'], diff --git a/app/broadcast_message/rest.py b/app/broadcast_message/rest.py index 31e3a5a4d..26221b89a 100644 --- a/app/broadcast_message/rest.py +++ b/app/broadcast_message/rest.py @@ -2,6 +2,7 @@ from datetime import datetime import iso8601 from flask import Blueprint, current_app, jsonify, request +from notifications_utils.template import BroadcastMessageTemplate from app.broadcast_message.broadcast_message_schema import ( create_broadcast_message_schema, @@ -116,6 +117,19 @@ def create_broadcast_message(service_id): reference = None else: template, content, reference = None, data['content'], data['reference'] + temporary_template = BroadcastMessageTemplate.from_content(content) + if temporary_template.content_too_long: + raise InvalidRequest( + ( + f'Content must be ' + f'{temporary_template.max_content_count:,.0f} ' + f'characters or fewer' + ) + ( + ' (because it could not be GSM7 encoded)' + if temporary_template.non_gsm_characters else '' + ), + status_code=400, + ) broadcast_message = BroadcastMessage( service_id=service.id, diff --git a/tests/app/broadcast_message/test_rest.py b/tests/app/broadcast_message/test_rest.py index 49550c515..438aedaf4 100644 --- a/tests/app/broadcast_message/test_rest.py +++ b/tests/app/broadcast_message/test_rest.py @@ -192,6 +192,40 @@ def test_create_broadcast_message_400s_if_json_schema_fails_validation( assert response['errors'] == expected_errors +@pytest.mark.parametrize('content, expected_status, expected_errors', ( + ('a', 201, None), + ('a' * 1_395, 201, None), + ('a\r\n' * 697, 201, None), # 1,394 chars – new lines normalised to \n + ('a' * 1_396, 400, ( + 'Content must be 1,395 characters or fewer' + )), + ('ŵ' * 615, 201, None), + ('ŵ' * 616, 400, ( + 'Content must be 615 characters or fewer ' + '(because it could not be GSM7 encoded)' + )), +)) +def test_create_broadcast_message_400s_if_content_too_long( + admin_request, + sample_broadcast_service, + content, + expected_status, + expected_errors, +): + response = admin_request.post( + 'broadcast_message.create_broadcast_message', + service_id=sample_broadcast_service.id, + _data={ + 'service_id': str(sample_broadcast_service.id), + 'created_by': str(sample_broadcast_service.created_by_id), + 'reference': 'abc123', + 'content': content, + }, + _expected_status=expected_status, + ) + assert response.get('message') == expected_errors + + @freeze_time('2020-01-01') def test_create_broadcast_message_can_be_created_from_content(admin_request, sample_broadcast_service): response = admin_request.post( From f8bca5765dcb4e164f5f4947048024a91005ba5a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 22 Apr 2021 14:42:54 +0100 Subject: [PATCH 2/2] Validate length of broadcast templates on creation This is a belt-and-braces check because the admin app already checks this. But since we do it for SMS already it makes sense to replicate it for broadcast templates. --- app/template/rest.py | 24 ++++++++++++++++++------ tests/app/template/test_rest.py | 16 +++++++++++++--- 2 files changed, 31 insertions(+), 9 deletions(-) diff --git a/app/template/rest.py b/app/template/rest.py index 0ff792660..0933ac919 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -5,7 +5,10 @@ import botocore from flask import Blueprint, current_app, jsonify, request from notifications_utils import SMS_CHAR_COUNT_LIMIT from notifications_utils.pdf import extract_page_from_pdf -from notifications_utils.template import SMSMessageTemplate +from notifications_utils.template import ( + BroadcastMessageTemplate, + SMSMessageTemplate, +) from PyPDF2.utils import PdfReadError from requests import post as requests_post from sqlalchemy.orm.exc import NoResultFound @@ -28,7 +31,13 @@ from app.dao.templates_dao import ( ) from app.errors import InvalidRequest, register_errors from app.letters.utils import get_letter_pdf_and_metadata -from app.models import LETTER_TYPE, SECOND_CLASS, SMS_TYPE, Template +from app.models import ( + BROADCAST_TYPE, + LETTER_TYPE, + SECOND_CLASS, + SMS_TYPE, + Template, +) from app.notifications.validators import check_reply_to, service_has_permission from app.schema_validation import validate from app.schemas import ( @@ -48,10 +57,13 @@ register_errors(template_blueprint) def _content_count_greater_than_limit(content, template_type): - if template_type != SMS_TYPE: - return False - template = SMSMessageTemplate({'content': content, 'template_type': template_type}) - return template.is_message_too_long() + if template_type == SMS_TYPE: + template = SMSMessageTemplate({'content': content, 'template_type': template_type}) + return template.is_message_too_long() + if template_type == BROADCAST_TYPE: + template = BroadcastMessageTemplate({'content': content, 'template_type': template_type}) + return template.is_message_too_long() + return False def validate_parent_folder(template_json): diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 79610d09c..f67075621 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -731,14 +731,24 @@ def test_should_return_404_if_no_templates_for_service_with_id(client, sample_se assert json_resp['message'] == 'No result found' -def test_create_400_for_over_limit_content(client, notify_api, sample_user, sample_service, fake_uuid): +@pytest.mark.parametrize('template_type', ( + SMS_TYPE, BROADCAST_TYPE, +)) +def test_create_400_for_over_limit_content( + client, + notify_api, + sample_user, + fake_uuid, + template_type, +): + sample_service = create_service(service_permissions=[template_type]) content = ''.join(random.choice(string.ascii_uppercase + string.digits) for _ in range(SMS_CHAR_COUNT_LIMIT + 1)) data = { 'name': 'too big template', - 'template_type': SMS_TYPE, + 'template_type': template_type, 'content': content, 'service': str(sample_service.id), - 'created_by': str(sample_user.id) + 'created_by': str(sample_service.created_by.id) } data = json.dumps(data) auth_header = create_authorization_header()