From ba80d5b7cd56e34a699f2f0b349efb60e893e83a Mon Sep 17 00:00:00 2001 From: David McDonald Date: Mon, 18 May 2020 15:15:54 +0100 Subject: [PATCH 1/5] Introduce abstraction for s3 metadata S3 metadata only supports ascii characters. Whenever we save data to it we need to make sure we encode it to save it and then decode it to display it again to users. This abstraction will act as the place for that decoding to happen so the rest of the code in our views doesn't need to care about the encoding abstraction. --- app/s3_client/s3_letter_upload_client.py | 17 ++++++++++--- tests/app/main/views/test_uploads.py | 31 ++++++++++++------------ 2 files changed, 30 insertions(+), 18 deletions(-) diff --git a/app/s3_client/s3_letter_upload_client.py b/app/s3_client/s3_letter_upload_client.py index bf263fb6f..e5ddad695 100644 --- a/app/s3_client/s3_letter_upload_client.py +++ b/app/s3_client/s3_letter_upload_client.py @@ -48,9 +48,20 @@ def get_letter_pdf_and_metadata(service_id, file_id): s3_object = s3.Object(current_app.config['TRANSIENT_UPLOADED_LETTERS'], file_location).get() pdf = s3_object['Body'].read() - metadata = s3_object['Metadata'] - return pdf, metadata + return pdf, LetterMetadata(s3_object['Metadata']) + + +class LetterMetadata: + KEYS_TO_DECODE = [] + + def __init__(self, metadata): + self._metadata = metadata + + def get(self, key, default=None): + if key in self.KEYS_TO_DECODE: + return urllib.parse.unquote(self._metadata.get(key, default)) + return self._metadata.get(key, default) def get_letter_metadata(service_id, file_id): @@ -58,4 +69,4 @@ def get_letter_metadata(service_id, file_id): s3 = resource('s3') s3_object = s3.Object(current_app.config['TRANSIENT_UPLOADED_LETTERS'], file_location).get() - return s3_object['Metadata'] + return LetterMetadata(s3_object['Metadata']) diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py index 98246762a..c77eb9bc6 100644 --- a/tests/app/main/views/test_uploads.py +++ b/tests/app/main/views/test_uploads.py @@ -10,6 +10,7 @@ from freezegun import freeze_time from requests import RequestException from app.main.views.uploads import format_recipient +from app.s3_client.s3_letter_upload_client import LetterMetadata from app.utils import normalize_spaces from tests.conftest import ( SERVICE_ONE_ID, @@ -166,12 +167,12 @@ def test_post_upload_letter_redirects_for_valid_file( ) ) mock_s3 = mocker.patch('app.main.views.uploads.upload_letter_to_s3') - mocker.patch('app.main.views.uploads.get_letter_metadata', return_value={ + mocker.patch('app.main.views.uploads.get_letter_metadata', return_value=LetterMetadata({ 'filename': 'tests/test_pdf_files/one_page_pdf.pdf', 'page_count': '1', 'status': 'valid', 'recipient': 'The Queen' - }) + })) mocker.patch('app.main.views.uploads.service_api_client.get_precompiled_template') service_one['restricted'] = False @@ -232,12 +233,12 @@ def test_post_upload_letter_shows_letter_preview_for_valid_file( ) mocker.patch('app.main.views.uploads.upload_letter_to_s3') mocker.patch('app.main.views.uploads.pdf_page_count', return_value=3) - mocker.patch('app.main.views.uploads.get_letter_metadata', return_value={ + mocker.patch('app.main.views.uploads.get_letter_metadata', return_value=LetterMetadata({ 'filename': 'tests/test_pdf_files/one_page_pdf.pdf', 'page_count': '3', 'status': 'valid', 'recipient': 'The Queen' - }) + })) mocker.patch('app.main.views.uploads.service_api_client.get_precompiled_template', return_value=letter_template) service_one['restricted'] = False @@ -351,9 +352,9 @@ def test_post_upload_letter_with_invalid_file(mocker, client_request, fake_uuid) } mocker.patch('app.main.views.uploads.sanitise_letter', return_value=mock_sanitise_response) mocker.patch('app.main.views.uploads.service_api_client.get_precompiled_template') - mocker.patch('app.main.views.uploads.get_letter_metadata', return_value={ + mocker.patch('app.main.views.uploads.get_letter_metadata', return_value=LetterMetadata({ 'filename': 'tests/test_pdf_files/one_page_pdf.pdf', 'page_count': '1', 'status': 'invalid', - 'message': 'content-outside-printable-area', 'invalid_pages': '[1]'}) + 'message': 'content-outside-printable-area', 'invalid_pages': '[1]'})) with open('tests/test_pdf_files/one_page_pdf.pdf', 'rb') as file: file_contents = file.read() @@ -395,9 +396,9 @@ def test_post_upload_letter_shows_letter_preview_for_invalid_file(mocker, client mock_sanitise_response.json = lambda: {"message": "template preview error", "recipient_address": "The Queen"} mocker.patch('app.main.views.uploads.sanitise_letter', return_value=mock_sanitise_response) mocker.patch('app.main.views.uploads.service_api_client.get_precompiled_template', return_value=letter_template) - mocker.patch('app.main.views.uploads.get_letter_metadata', return_value={ + mocker.patch('app.main.views.uploads.get_letter_metadata', return_value=LetterMetadata({ 'filename': 'tests/test_pdf_files/one_page_pdf.pdf', 'page_count': '1', 'status': 'invalid', - 'message': 'template-preview-error'}) + 'message': 'template-preview-error'})) with open('tests/test_pdf_files/one_page_pdf.pdf', 'rb') as file: page = client_request.post( @@ -455,10 +456,10 @@ def test_uploaded_letter_preview( ): mocker.patch('app.main.views.uploads.service_api_client') recipient = 'Bugs Bunny\n123 Big Hole\rLooney Town' - mocker.patch('app.main.views.uploads.get_letter_metadata', return_value={ + mocker.patch('app.main.views.uploads.get_letter_metadata', return_value=LetterMetadata({ 'filename': 'my_letter.pdf', 'page_count': '1', 'status': 'valid', 'recipient': urllib.parse.quote(recipient) - }) + })) service_one['restricted'] = False client_request.login(active_user_with_permissions, service=service_one) @@ -485,8 +486,8 @@ def test_uploaded_letter_preview_does_not_show_send_button_if_service_in_trial_m fake_uuid, ): mocker.patch('app.main.views.uploads.service_api_client') - mocker.patch('app.main.views.uploads.get_letter_metadata', return_value={ - 'filename': 'my_letter.pdf', 'page_count': '1', 'status': 'valid', 'recipient': 'The Queen'}) + mocker.patch('app.main.views.uploads.get_letter_metadata', return_value=LetterMetadata({ + 'filename': 'my_letter.pdf', 'page_count': '1', 'status': 'valid', 'recipient': 'The Queen'})) # client_request uses service_one, which is in trial mode page = client_request.get( @@ -603,7 +604,7 @@ def test_uploaded_letter_preview_image_400s_for_bad_page_type( def test_send_uploaded_letter_sends_letter_and_redirects_to_notification_page(mocker, service_one, client_request): - metadata = {'filename': 'my_file.pdf', 'page_count': '1', 'status': 'valid', 'recipient': 'address'} + metadata = LetterMetadata({'filename': 'my_file.pdf', 'page_count': '1', 'status': 'valid', 'recipient': 'address'}) mocker.patch('app.main.views.uploads.get_letter_pdf_and_metadata', return_value=('file', metadata)) mock_send = mocker.patch('app.main.views.uploads.notification_api_client.send_precompiled_letter') @@ -656,8 +657,8 @@ def test_send_uploaded_letter_when_metadata_states_pdf_is_invalid(mocker, servic mock_send = mocker.patch('app.main.views.uploads.notification_api_client.send_precompiled_letter') mocker.patch( 'app.main.views.uploads.get_letter_metadata', - return_value={'filename': 'my_file.pdf', 'page_count': '3', 'status': 'invalid', - 'message': 'error', 'invalid_pages': '[1]'} + return_value=LetterMetadata({'filename': 'my_file.pdf', 'page_count': '3', 'status': 'invalid', + 'message': 'error', 'invalid_pages': '[1]'}) ) service_one['permissions'] = ['letter', 'upload_letters'] From 1fb31a88619fd30ad941bfae61cbc20a01070253 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Mon, 18 May 2020 15:24:55 +0100 Subject: [PATCH 2/5] Remove unused args in test These args are not inputs to the function under test, neither as way of named arguments or as GET query parameters. I assume this has been leftover from a previous refactor of behaviour. --- tests/app/main/views/test_uploads.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py index c77eb9bc6..ad2cb440e 100644 --- a/tests/app/main/views/test_uploads.py +++ b/tests/app/main/views/test_uploads.py @@ -468,10 +468,6 @@ def test_uploaded_letter_preview( 'main.uploaded_letter_preview', service_id=SERVICE_ONE_ID, file_id=fake_uuid, - original_filename='my_letter.pdf', - page_count=1, - status='valid', - error={} ) assert page.find('h1').text == 'my_letter.pdf' From 7923760345e8a9914bb4588869729bbb64895768 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Mon, 18 May 2020 15:26:35 +0100 Subject: [PATCH 3/5] Ensure pdf letter filename is ascii encoded and decoded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit S3 can only handle ascii characters, therefore for filename which could include non ascii characters, for example a filename with the character '£' in it, we must encode these using urllib before saving it as s3 metadata. We then also make sure that it comes back decoded when presenting it to the user. --- app/s3_client/s3_letter_upload_client.py | 26 +++++++++++++----------- tests/app/main/views/test_uploads.py | 8 +++++--- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/app/s3_client/s3_letter_upload_client.py b/app/s3_client/s3_letter_upload_client.py index e5ddad695..0f2e5a587 100644 --- a/app/s3_client/s3_letter_upload_client.py +++ b/app/s3_client/s3_letter_upload_client.py @@ -21,10 +21,12 @@ def upload_letter_to_s3( invalid_pages=None, recipient=None ): + # Use of urllib.parse.quote encodes metadata into ascii, which is required by s3. + # Making sure data for displaying to users is decoded is taken care of by LetterMetadata metadata = { 'status': status, 'page_count': str(page_count), - 'filename': filename, + 'filename': urllib.parse.quote(filename), } if message: metadata['message'] = message @@ -42,18 +44,8 @@ def upload_letter_to_s3( ) -def get_letter_pdf_and_metadata(service_id, file_id): - file_location = get_transient_letter_file_location(service_id, file_id) - s3 = resource('s3') - s3_object = s3.Object(current_app.config['TRANSIENT_UPLOADED_LETTERS'], file_location).get() - - pdf = s3_object['Body'].read() - - return pdf, LetterMetadata(s3_object['Metadata']) - - class LetterMetadata: - KEYS_TO_DECODE = [] + KEYS_TO_DECODE = ["filename"] def __init__(self, metadata): self._metadata = metadata @@ -64,6 +56,16 @@ class LetterMetadata: return self._metadata.get(key, default) +def get_letter_pdf_and_metadata(service_id, file_id): + file_location = get_transient_letter_file_location(service_id, file_id) + s3 = resource('s3') + s3_object = s3.Object(current_app.config['TRANSIENT_UPLOADED_LETTERS'], file_location).get() + + pdf = s3_object['Body'].read() + + return pdf, LetterMetadata(s3_object['Metadata']) + + def get_letter_metadata(service_id, file_id): file_location = get_transient_letter_file_location(service_id, file_id) s3 = resource('s3') diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py index ad2cb440e..b9ac34222 100644 --- a/tests/app/main/views/test_uploads.py +++ b/tests/app/main/views/test_uploads.py @@ -457,9 +457,11 @@ def test_uploaded_letter_preview( mocker.patch('app.main.views.uploads.service_api_client') recipient = 'Bugs Bunny\n123 Big Hole\rLooney Town' mocker.patch('app.main.views.uploads.get_letter_metadata', return_value=LetterMetadata({ - 'filename': 'my_letter.pdf', 'page_count': '1', 'status': 'valid', + 'filename': 'my_encoded_filename%C2%A3.pdf', + 'page_count': '1', + 'status': 'valid', 'recipient': urllib.parse.quote(recipient) - })) + })) service_one['restricted'] = False client_request.login(active_user_with_permissions, service=service_one) @@ -470,7 +472,7 @@ def test_uploaded_letter_preview( file_id=fake_uuid, ) - assert page.find('h1').text == 'my_letter.pdf' + assert page.find('h1').text == 'my_encoded_filename£.pdf' assert page.find('div', class_='letter-sent') assert not page.find("label", {"class": "file-upload-button"}) assert page.find('button', {'class': 'page-footer__button', 'type': 'submit'}) From 387fcfda3fec26c9b05539c9128dc39904239480 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Mon, 18 May 2020 15:44:20 +0100 Subject: [PATCH 4/5] Move recipient decoding into LetterMetadata class This is for consistency with how we do it for filenames in the previous commit and moves the decoding into the `LetterMetadata` class for abstracting this behaviour. Small refactor of the LetterMetadata class needed to handle None case as recipient can be None. --- app/main/views/uploads.py | 3 --- app/s3_client/s3_letter_upload_client.py | 9 +++++---- tests/app/main/views/test_uploads.py | 14 ++++++++------ 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index 263fd43bf..d2ee17de2 100644 --- a/app/main/views/uploads.py +++ b/app/main/views/uploads.py @@ -1,7 +1,6 @@ import base64 import itertools import json -import urllib import uuid from io import BytesIO from zipfile import BadZipFile @@ -192,14 +191,12 @@ def _get_error_from_upload_form(form_errors): def format_recipient(address): ''' To format the recipient we need to: - - decode, address is url encoded - remove new line characters - remove whitespace around the lines - join the address lines, separated by a comma ''' if not address: return address - address = urllib.parse.unquote(address) stripped_address_lines_no_trailing_commas = [ line.lstrip().rstrip(' ,') for line in address.splitlines() if line diff --git a/app/s3_client/s3_letter_upload_client.py b/app/s3_client/s3_letter_upload_client.py index 0f2e5a587..2e3ff08cb 100644 --- a/app/s3_client/s3_letter_upload_client.py +++ b/app/s3_client/s3_letter_upload_client.py @@ -45,15 +45,16 @@ def upload_letter_to_s3( class LetterMetadata: - KEYS_TO_DECODE = ["filename"] + KEYS_TO_DECODE = ["filename", "recipient"] def __init__(self, metadata): self._metadata = metadata def get(self, key, default=None): - if key in self.KEYS_TO_DECODE: - return urllib.parse.unquote(self._metadata.get(key, default)) - return self._metadata.get(key, default) + value = self._metadata.get(key, default) + if value and key in self.KEYS_TO_DECODE: + value = urllib.parse.unquote(value) + return value def get_letter_pdf_and_metadata(service_id, file_id): diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py index b9ac34222..49f8b784e 100644 --- a/tests/app/main/views/test_uploads.py +++ b/tests/app/main/views/test_uploads.py @@ -1,5 +1,4 @@ import re -import urllib import uuid from io import BytesIO from unittest.mock import ANY, Mock @@ -455,12 +454,11 @@ def test_uploaded_letter_preview( fake_uuid, ): mocker.patch('app.main.views.uploads.service_api_client') - recipient = 'Bugs Bunny\n123 Big Hole\rLooney Town' mocker.patch('app.main.views.uploads.get_letter_metadata', return_value=LetterMetadata({ 'filename': 'my_encoded_filename%C2%A3.pdf', 'page_count': '1', 'status': 'valid', - 'recipient': urllib.parse.quote(recipient) + 'recipient': 'Bugs Bunny%0A123 Big Hole%0DLooney Town' # 'Bugs Bunny%0A123 Big Hole\rLooney Town' url encoded })) service_one['restricted'] = False @@ -655,8 +653,12 @@ def test_send_uploaded_letter_when_metadata_states_pdf_is_invalid(mocker, servic mock_send = mocker.patch('app.main.views.uploads.notification_api_client.send_precompiled_letter') mocker.patch( 'app.main.views.uploads.get_letter_metadata', - return_value=LetterMetadata({'filename': 'my_file.pdf', 'page_count': '3', 'status': 'invalid', - 'message': 'error', 'invalid_pages': '[1]'}) + return_value=LetterMetadata( + { + 'filename': 'my_file.pdf', 'page_count': '3', 'status': 'invalid', + 'message': 'error', 'invalid_pages': '[1]' + } + ) ) service_one['permissions'] = ['letter', 'upload_letters'] @@ -680,7 +682,7 @@ def test_send_uploaded_letter_when_metadata_states_pdf_is_invalid(mocker, servic ('', ''), ]) def test_format_recipient(original_address, expected_address): - assert format_recipient(urllib.parse.quote(original_address)) == expected_address + assert format_recipient(original_address) == expected_address @pytest.mark.parametrize('user', ( From 3185505612a45d5f975c8e94b45228cf071450d6 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Tue, 19 May 2020 11:20:03 +0100 Subject: [PATCH 5/5] Add unit test for LetterMetadata class --- .../s3_client/test_s3_letter_upload_client.py | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/tests/app/s3_client/test_s3_letter_upload_client.py b/tests/app/s3_client/test_s3_letter_upload_client.py index e03aae794..900e8b31e 100644 --- a/tests/app/s3_client/test_s3_letter_upload_client.py +++ b/tests/app/s3_client/test_s3_letter_upload_client.py @@ -2,7 +2,10 @@ import urllib from flask import current_app -from app.s3_client.s3_letter_upload_client import upload_letter_to_s3 +from app.s3_client.s3_letter_upload_client import ( + LetterMetadata, + upload_letter_to_s3, +) def test_upload_letter_to_s3(mocker): @@ -53,3 +56,17 @@ def test_upload_letter_to_s3_with_message_and_invalid_pages(mocker): }, region=current_app.config['AWS_REGION'] ) + + +def test_lettermetadata_gets_non_special_keys(): + metadata = LetterMetadata({"key": "value", "not_key_to_decode": "%C2%A3"}) + assert metadata.get("key") == "value" + assert metadata.get("other_key") is None + assert metadata.get("other_key", "default") == "default" + assert metadata.get("not_key_to_decode") == "%C2%A3" + + +def test_lettermetadata_unquotes_special_keys(): + metadata = LetterMetadata({"filename": "%C2%A3hello", "recipient": "%C2%A3hi"}) + assert metadata.get("filename") == "£hello" + assert metadata.get("recipient") == "£hi"