Merge pull request #3450 from alphagov/non-ascii-metadata

Non ascii metadata

app/main/views/uploads.py

@@ -1,7 +1,6 @@
 import base64
 import itertools
 import json
-import urllib
 import uuid
 from datetime import datetime
 from functools import partial
@@ -255,14 +254,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

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,15 +44,27 @@ def upload_letter_to_s3(
     )
 
 
+class LetterMetadata:
+    KEYS_TO_DECODE = ["filename", "recipient"]
+
+    def __init__(self, metadata):
+        self._metadata = metadata
+
+    def get(self, key, default=None):
+        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):
     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()
-    metadata = s3_object['Metadata']
 
-    return pdf, metadata
+    return pdf, LetterMetadata(s3_object['Metadata'])
 
 
 def get_letter_metadata(service_id, file_id):
@@ -58,4 +72,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'])

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
@@ -10,6 +9,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,
@@ -333,12 +333,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
@@ -399,12 +399,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
@@ -518,9 +518,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()
@@ -562,9 +562,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(
@@ -621,11 +621,12 @@ 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({
-    mocker.patch('app.main.views.uploads.get_letter_metadata', return_value={
+        'filename': 'my_encoded_filename%C2%A3.pdf',
-        'filename': 'my_letter.pdf', 'page_count': '1', 'status': 'valid',
+        'page_count': '1',
-        'recipient': urllib.parse.quote(recipient)
+        'status': 'valid',
-        })
+        'recipient': 'Bugs Bunny%0A123 Big Hole%0DLooney Town'  # 'Bugs Bunny%0A123 Big Hole\rLooney Town' url encoded
+    }))
 
     service_one['restricted'] = False
     client_request.login(active_user_with_permissions, service=service_one)
@@ -634,13 +635,9 @@ 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'
+    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'})
@@ -652,8 +649,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={
+    mocker.patch('app.main.views.uploads.get_letter_metadata', return_value=LetterMetadata({
-        'filename': 'my_letter.pdf', 'page_count': '1', 'status': 'valid', 'recipient': 'The Queen'})
+        '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(
@@ -770,7 +767,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')
@@ -823,8 +820,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={'filename': 'my_file.pdf', 'page_count': '3', 'status': 'invalid',
+        return_value=LetterMetadata(
-                      'message': 'error', 'invalid_pages': '[1]'}
+            {
+                'filename': 'my_file.pdf', 'page_count': '3', 'status': 'invalid',
+                'message': 'error', 'invalid_pages': '[1]'
+            }
+        )
     )
 
     service_one['permissions'] = ['letter', 'upload_letters']
@@ -848,7 +849,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', (

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"