From 9708b09ba3d77550fc19425a22332694942ed11e Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 5 Oct 2020 18:57:21 +0100 Subject: [PATCH 1/2] Tailor message-too-long error message depending on the notification type. Up until now, only sms messages could get message-too-long error, but now we also need to validate the size of email messages, so the message content needs to be tailored to the notification type. --- app/notifications/validators.py | 17 +++++++--- requirements-app.txt | 2 +- requirements.txt | 6 ++-- tests/app/notifications/test_validators.py | 33 ++++++++++++++----- .../service/test_send_one_off_notification.py | 2 +- 5 files changed, 42 insertions(+), 18 deletions(-) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 91747d14d..a7eeb635a 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -144,10 +144,19 @@ def check_if_service_can_send_to_number(service, number): return international_phone_info -def check_content_char_count(template_with_content): +def check_message_is_not_too_long(template_with_content): if template_with_content.is_message_too_long(): - message = f"Text messages cannot be longer than {SMS_CHAR_COUNT_LIMIT} characters. " \ - f"Your message is {template_with_content.content_count_without_prefix} characters" + message = "Your message is too long." + if template_with_content.template_type == SMS_TYPE: + message = ( + f"Text messages cannot be longer than {SMS_CHAR_COUNT_LIMIT} characters. " + f"Your message is {template_with_content.content_count_without_prefix} characters long." + ) + elif template_with_content.template_type == EMAIL_TYPE: + message = ( + f"Emails cannot be longer than 7500000 bytes. " + f"Your message is {template_with_content.content_size_in_bytes} bytes." + ) raise BadRequestError(message=message) @@ -173,7 +182,7 @@ def validate_template(template_id, personalisation, service, notification_type): check_notification_content_is_not_empty(template_with_content) - check_content_char_count(template_with_content) + check_message_is_not_too_long(template_with_content) return template, template_with_content diff --git a/requirements-app.txt b/requirements-app.txt index ded61f2c9..e9d24d4ad 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -29,7 +29,7 @@ notifications-python-client==5.7.0 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@43.0.0#egg=notifications-utils==43.0.0 +git+https://github.com/alphagov/notifications-utils.git@43.1.0#egg=notifications-utils==43.1.0 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.8.0 diff --git a/requirements.txt b/requirements.txt index cae9c641a..244235f30 100644 --- a/requirements.txt +++ b/requirements.txt @@ -31,7 +31,7 @@ notifications-python-client==5.7.0 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@43.0.0#egg=notifications-utils==43.0.0 +git+https://github.com/alphagov/notifications-utils.git@43.1.0#egg=notifications-utils==43.1.0 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.8.0 @@ -42,14 +42,14 @@ alembic==1.4.3 amqp==1.4.9 anyjson==0.3.3 attrs==20.2.0 -awscli==1.18.164 +awscli==1.18.167 bcrypt==3.2.0 billiard==3.3.0.23 bleach==3.1.4 blinker==1.4 boto==2.49.0 boto3==1.10.38 -botocore==1.19.4 +botocore==1.19.7 certifi==2020.6.20 chardet==3.0.4 click==7.1.2 diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index de6ce99ce..79142f210 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -13,7 +13,7 @@ from app.models import ( ) from app.notifications.process_notifications import create_content_for_notification from app.notifications.validators import ( - check_content_char_count, + check_message_is_not_too_long, check_if_service_can_send_files_by_email, check_notification_content_is_not_empty, check_service_over_daily_message_limit, @@ -319,36 +319,51 @@ def test_service_can_send_to_recipient_fails_when_mobile_number_is_not_on_team(s @pytest.mark.parametrize('char_count', [612, 0, 494, 200, 918]) @pytest.mark.parametrize('show_prefix', [True, False]) @pytest.mark.parametrize('template_type', ['sms', 'email', 'letter']) -def test_check_content_char_count_passes(notify_db_session, show_prefix, char_count, template_type): +def test_check_message_is_not_too_long_passes(notify_db_session, show_prefix, char_count, template_type): service = create_service(prefix_sms=show_prefix) t = create_template(service=service, content='a' * char_count, template_type=template_type) template = templates_dao.dao_get_template_by_id_and_service_id(template_id=t.id, service_id=service.id) template_with_content = get_template_instance(template=template.__dict__, values={}) - assert check_content_char_count(template_with_content) is None + assert check_message_is_not_too_long(template_with_content) is None @pytest.mark.parametrize('char_count', [919, 6000]) @pytest.mark.parametrize('show_prefix', [True, False]) -def test_check_content_char_count_fails(notify_db_session, show_prefix, char_count): +def test_check_message_is_not_too_long_for_too_long_sms_messages(notify_db_session, show_prefix, char_count): with pytest.raises(BadRequestError) as e: service = create_service(prefix_sms=show_prefix) t = create_template(service=service, content='a' * char_count, template_type='sms') template = templates_dao.dao_get_template_by_id_and_service_id(template_id=t.id, service_id=service.id) template_with_content = get_template_instance(template=template.__dict__, values={}) - check_content_char_count(template_with_content) + check_message_is_not_too_long(template_with_content) assert e.value.status_code == 400 assert e.value.message == f'Text messages cannot be longer than {SMS_CHAR_COUNT_LIMIT} characters. ' \ - f'Your message is {char_count} characters' + f'Your message is {char_count} characters.' + assert e.value.fields == [] + + +def test_check_message_is_not_too_long_for_too_big_email_messages(notify_db_session): + with pytest.raises(BadRequestError) as e: + service = create_service() + t = create_template(service=service, content='a' * 7500000, template_type='email') + template = templates_dao.dao_get_template_by_id_and_service_id(template_id=t.id, service_id=service.id) + template_with_content = get_template_instance(template=template.__dict__, values={}) + check_message_is_not_too_long(template_with_content) + assert e.value.status_code == 400 + assert e.value.message == ( + f"Email messages cannot be longer than 7500000 bytes. " + f"Your message is 7500081 bytes." + ) assert e.value.fields == [] @pytest.mark.parametrize('template_type', ['email', 'letter']) -def test_check_content_char_count_passes_for_long_email_or_letter(sample_service, template_type): +def test_check_message_is_not_too_long_passes_for_long_email_or_letter(sample_service, template_type): t = create_template(service=sample_service, content='a' * 1000, template_type=template_type) template = templates_dao.dao_get_template_by_id_and_service_id(template_id=t.id, service_id=t.service_id) template_with_content = get_template_instance(template=template.__dict__, values={}) - assert check_content_char_count(template_with_content) is None + assert check_message_is_not_too_long(template_with_content) is None def test_check_notification_content_is_not_empty_passes(notify_api, mocker, sample_service): @@ -394,7 +409,7 @@ def test_validate_template_calls_all_validators(mocker, fake_uuid, sample_servic 'app.notifications.validators.create_content_for_notification', return_value="content" ) mock_check_not_empty = mocker.patch('app.notifications.validators.check_notification_content_is_not_empty') - mock_check_message_is_too_long = mocker.patch('app.notifications.validators.check_content_char_count') + mock_check_message_is_too_long = mocker.patch('app.notifications.validators.check_message_is_not_too_long') template, template_with_content = validate_template(template.id, {}, sample_service, "email") mock_check_type.assert_called_once_with("email", "email") diff --git a/tests/app/service/test_send_one_off_notification.py b/tests/app/service/test_send_one_off_notification.py index 2f6e1ee60..b956d700a 100644 --- a/tests/app/service/test_send_one_off_notification.py +++ b/tests/app/service/test_send_one_off_notification.py @@ -318,7 +318,7 @@ def test_send_one_off_notification_raises_if_message_too_long(persist_mock, noti send_one_off_notification(service.id, post_data) assert e.value.message == f'Text messages cannot be longer than {SMS_CHAR_COUNT_LIMIT} characters. ' \ - f'Your message is {1029} characters' + f'Your message is {1029} characters.' def test_send_one_off_notification_fails_if_created_by_other_service(sample_template): From 41d1cf453dad3f25e41c57123e9211204be77f3b Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 14 Oct 2020 10:59:13 +0100 Subject: [PATCH 2/2] Update limit to 1MB and update tests SES rejects email messages bigger than 10485760 bytes (just over 10 MB per message (after base64 encoding)): https://docs.aws.amazon.com/ses/latest/DeveloperGuide/quotas.html#limits-message Base64 is apparently wasteful because we use just 64 different values per byte, whereas a byte can represent 256 different characters. That is, we use bytes (which are 8-bit words) as 6-bit words. There is a waste of 2 bits for each 8 bits of transmission data. To send three bytes of information (3 times 8 is 24 bits), you need to use four bytes (4 times 6 is again 24 bits). Thus the base64 version of a file is 4/3 larger than it might be. So we use 33% more storage than we could. https://lemire.me/blog/2019/01/30/what-is-the-space-overhead-of-base64-encoding/ That brings down our max safe size to 7.5 MB == 7500000 bytes before base64 encoding But this is not the end! The message we send to SES is structured as follows: "Message": { 'Subject': { 'Data': subject, }, 'Body': {'Text': {'Data': body}, 'Html': {'Data': html_body}} }, Which means that we are sending the contents of email message twice in one request: once in plain text and once with html tags. That means our plain text content needs to be much shorter to make sure we fit within the limit, especially since HTML body can be much byte-heavier than plain text body. Hence, we decided to put the limit at 1MB, which is equivalent of between 250 and 500 pages of text. That's still an extremely long email, and should be sufficient for all normal use, while at the same time giving us safe margin while sending the emails through Amazon SES. --- app/notifications/validators.py | 2 +- tests/app/notifications/test_validators.py | 8 ++++---- tests/app/service/test_send_one_off_notification.py | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index a7eeb635a..745155f39 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -154,7 +154,7 @@ def check_message_is_not_too_long(template_with_content): ) elif template_with_content.template_type == EMAIL_TYPE: message = ( - f"Emails cannot be longer than 7500000 bytes. " + f"Emails cannot be longer than 1000000 bytes. " f"Your message is {template_with_content.content_size_in_bytes} bytes." ) raise BadRequestError(message=message) diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index 79142f210..93727b48f 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -338,21 +338,21 @@ def test_check_message_is_not_too_long_for_too_long_sms_messages(notify_db_sessi check_message_is_not_too_long(template_with_content) assert e.value.status_code == 400 assert e.value.message == f'Text messages cannot be longer than {SMS_CHAR_COUNT_LIMIT} characters. ' \ - f'Your message is {char_count} characters.' + f'Your message is {char_count} characters long.' assert e.value.fields == [] def test_check_message_is_not_too_long_for_too_big_email_messages(notify_db_session): with pytest.raises(BadRequestError) as e: service = create_service() - t = create_template(service=service, content='a' * 7500000, template_type='email') + t = create_template(service=service, content='a' * 1000000, template_type='email') template = templates_dao.dao_get_template_by_id_and_service_id(template_id=t.id, service_id=service.id) template_with_content = get_template_instance(template=template.__dict__, values={}) check_message_is_not_too_long(template_with_content) assert e.value.status_code == 400 assert e.value.message == ( - f"Email messages cannot be longer than 7500000 bytes. " - f"Your message is 7500081 bytes." + f"Emails cannot be longer than 1000000 bytes. " + f"Your message is 1000081 bytes." ) assert e.value.fields == [] diff --git a/tests/app/service/test_send_one_off_notification.py b/tests/app/service/test_send_one_off_notification.py index b956d700a..cfc025157 100644 --- a/tests/app/service/test_send_one_off_notification.py +++ b/tests/app/service/test_send_one_off_notification.py @@ -318,7 +318,7 @@ def test_send_one_off_notification_raises_if_message_too_long(persist_mock, noti send_one_off_notification(service.id, post_data) assert e.value.message == f'Text messages cannot be longer than {SMS_CHAR_COUNT_LIMIT} characters. ' \ - f'Your message is {1029} characters.' + f'Your message is {1029} characters long.' def test_send_one_off_notification_fails_if_created_by_other_service(sample_template):