From 11543e15be51282d7210cdd51fac0b43f4d76355 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Mon, 7 Oct 2019 15:22:11 +0100 Subject: [PATCH] Store all data about uploaded letters in the S3 metadata We had been storing whether or not a file was valid in the S3 metadata, but using the query string of the URL to store the original filename and the page count. This meant that if you tried to view the preview letter page without the query string you would see a `500`. It was possible for this to happen if you were signed out of Notify while on the preview page - you would be redirected back to the preview page but without the query string, causing an error. --- app/main/views/uploads.py | 37 +++++++++++-------- app/s3_client/s3_letter_upload_client.py | 19 ++++++++-- app/templates/views/uploads/preview.html | 1 - tests/app/main/views/test_uploads.py | 33 +++++++++++++---- .../s3_client/test_s3_letter_upload_client.py | 9 ++++- 5 files changed, 71 insertions(+), 28 deletions(-) diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index 82b0373fc..31d3a4f46 100644 --- a/app/main/views/uploads.py +++ b/app/main/views/uploads.py @@ -20,6 +20,7 @@ from app.extensions import antivirus_client from app.main import main from app.main.forms import PDFUploadForm from app.s3_client.s3_letter_upload_client import ( + get_letter_metadata, get_letter_pdf_and_metadata, get_transient_letter_file_location, upload_letter_to_s3, @@ -43,6 +44,7 @@ def upload_letter(service_id): if form.validate_on_submit(): pdf_file_bytes = form.file.data.read() + original_filename = form.file.data.filename virus_free = antivirus_client.scan(BytesIO(pdf_file_bytes)) if not virus_free: @@ -67,22 +69,29 @@ def upload_letter(service_id): except RequestException as ex: if ex.response is not None and ex.response.status_code == 400: status = 'invalid' - upload_letter_to_s3(pdf_file_bytes, file_location, status) + upload_letter_to_s3( + pdf_file_bytes, + file_location=file_location, + status=status, + page_count=page_count, + filename=original_filename) else: raise ex else: status = 'valid' file_contents = base64.b64decode(response.json()['file'].encode()) - upload_letter_to_s3(file_contents, file_location, status) + upload_letter_to_s3( + file_contents, + file_location=file_location, + status=status, + page_count=page_count, + filename=original_filename) return redirect( url_for( 'main.uploaded_letter_preview', service_id=current_service.id, file_id=upload_id, - original_filename=form.file.data.filename, - page_count=page_count, - status=status, ) ) @@ -97,9 +106,10 @@ def invalid_upload_error(message): @main.route("/services//preview-letter/") @user_has_permissions('send_messages') def uploaded_letter_preview(service_id, file_id): - original_filename = request.args.get('original_filename') - page_count = request.args.get('page_count') - status = request.args.get('status') + metadata = get_letter_metadata(service_id, file_id) + original_filename = metadata.get('filename') + page_count = metadata.get('page_count') + status = metadata.get('status') template_dict = service_api_client.get_precompiled_template(service_id) @@ -126,8 +136,7 @@ def uploaded_letter_preview(service_id, file_id): @main.route("/services//preview-letter-image/") @user_has_permissions('send_messages') def view_letter_upload_as_preview(service_id, file_id): - file_location = get_transient_letter_file_location(service_id, file_id) - pdf_file, metadata = get_letter_pdf_and_metadata(file_location) + pdf_file, metadata = get_letter_pdf_and_metadata(service_id, file_id) page = request.args.get('page') @@ -140,14 +149,12 @@ def view_letter_upload_as_preview(service_id, file_id): @main.route("/services//upload-letter/send", methods=['POST']) @user_has_permissions('send_messages', restrict_admin_usage=True) def send_uploaded_letter(service_id): - filename = request.form['filename'] - file_id = request.form['file_id'] - if not (current_service.has_permission('letter') and current_service.has_permission('upload_letters')): abort(403) - file_location = get_transient_letter_file_location(service_id, file_id) - _, metadata = get_letter_pdf_and_metadata(file_location) + file_id = request.form['file_id'] + metadata = get_letter_metadata(service_id, file_id) + filename = metadata.get('filename') if metadata.get('status') != 'valid': abort(403) diff --git a/app/s3_client/s3_letter_upload_client.py b/app/s3_client/s3_letter_upload_client.py index 27848f105..8bdeece7b 100644 --- a/app/s3_client/s3_letter_upload_client.py +++ b/app/s3_client/s3_letter_upload_client.py @@ -7,17 +7,22 @@ def get_transient_letter_file_location(service_id, upload_id): return 'service-{}/{}.pdf'.format(service_id, upload_id) -def upload_letter_to_s3(data, file_location, status): +def upload_letter_to_s3(data, *, file_location, status, page_count, filename): utils_s3upload( filedata=data, region=current_app.config['AWS_REGION'], bucket_name=current_app.config['TRANSIENT_UPLOADED_LETTERS'], file_location=file_location, - metadata={'status': status} + metadata={ + 'status': status, + 'page_count': str(page_count), + 'filename': filename, + } ) -def get_letter_pdf_and_metadata(file_location): +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() @@ -25,3 +30,11 @@ def get_letter_pdf_and_metadata(file_location): metadata = s3_object['Metadata'] return pdf, metadata + + +def get_letter_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() + + return s3_object['Metadata'] diff --git a/app/templates/views/uploads/preview.html b/app/templates/views/uploads/preview.html index c8d954887..5642540e0 100644 --- a/app/templates/views/uploads/preview.html +++ b/app/templates/views/uploads/preview.html @@ -28,7 +28,6 @@ service_id=current_service.id, )}}" class='page-footer'> - diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py index b9b532f84..e58031e0f 100644 --- a/tests/app/main/views/test_uploads.py +++ b/tests/app/main/views/test_uploads.py @@ -33,6 +33,8 @@ def test_post_upload_letter_redirects_for_valid_file(mocker, client_request): return_value=Mock(content='The sanitised content', json=lambda: {'file': 'VGhlIHNhbml0aXNlZCBjb250ZW50'}) ) mock_s3 = mocker.patch('app.main.views.uploads.upload_letter_to_s3') + mocker.patch('app.main.views.uploads.get_letter_metadata', return_value={ + 'filename': 'tests/test_pdf_files/one_page_pdf.pdf', 'page_count': '1', 'status': 'valid'}) mocker.patch('app.main.views.uploads.service_api_client.get_precompiled_template') with open('tests/test_pdf_files/one_page_pdf.pdf', 'rb') as file: @@ -46,14 +48,15 @@ def test_post_upload_letter_redirects_for_valid_file(mocker, client_request): mock_s3.assert_called_once_with( b'The sanitised content', - 'service-{}/fake-uuid.pdf'.format(SERVICE_ONE_ID), - 'valid', + file_location='service-{}/fake-uuid.pdf'.format(SERVICE_ONE_ID), + status='valid', + page_count=1, + filename='tests/test_pdf_files/one_page_pdf.pdf' ) assert page.find('h1').text == 'tests/test_pdf_files/one_page_pdf.pdf' assert not page.find(id='validation-error-message') - assert page.find('input', {'type': 'hidden', 'name': 'filename', 'value': 'tests/test_pdf_files/one_page_pdf.pdf'}) assert page.find('input', {'type': 'hidden', 'name': 'file_id', 'value': 'fake-uuid'}) assert page.find('button', {'type': 'submit'}).text == 'Send 1 letter' @@ -73,6 +76,8 @@ def test_post_upload_letter_shows_letter_preview_for_valid_file(mocker, client_r ) 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={ + 'filename': 'tests/test_pdf_files/one_page_pdf.pdf', 'page_count': '3', 'status': 'valid'}) mocker.patch('app.main.views.uploads.service_api_client.get_precompiled_template', return_value=letter_template) with open('tests/test_pdf_files/one_page_pdf.pdf', 'rb') as file: @@ -170,6 +175,8 @@ def test_post_upload_letter_with_invalid_file(mocker, client_request): mock_sanitise_response.raise_for_status.side_effect = RequestException(response=Mock(status_code=400)) 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={ + 'filename': 'tests/test_pdf_files/one_page_pdf.pdf', 'page_count': '1', 'status': 'invalid'}) with open('tests/test_pdf_files/one_page_pdf.pdf', 'rb') as file: file_contents = file.read() @@ -184,8 +191,10 @@ def test_post_upload_letter_with_invalid_file(mocker, client_request): mock_s3.assert_called_once_with( file_contents, - 'service-{}/fake-uuid.pdf'.format(SERVICE_ONE_ID), - 'invalid', + file_location='service-{}/fake-uuid.pdf'.format(SERVICE_ONE_ID), + status='invalid', + page_count=1, + filename='tests/test_pdf_files/one_page_pdf.pdf' ) assert page.find('h1').text == 'tests/test_pdf_files/one_page_pdf.pdf' @@ -209,6 +218,8 @@ def test_post_upload_letter_shows_letter_preview_for_invalid_file(mocker, client mock_sanitise_response.raise_for_status.side_effect = RequestException(response=Mock(status_code=400)) 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={ + 'filename': 'tests/test_pdf_files/one_page_pdf.pdf', 'page_count': '1', 'status': 'invalid'}) with open('tests/test_pdf_files/one_page_pdf.pdf', 'rb') as file: page = client_request.post( @@ -253,6 +264,8 @@ def test_post_upload_letter_does_not_upload_to_s3_if_template_preview_raises_unk def test_uploaded_letter_preview(mocker, client_request): 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'}) page = client_request.get( 'main.uploaded_letter_preview', @@ -268,8 +281,11 @@ def test_uploaded_letter_preview(mocker, client_request): def test_send_uploaded_letter_sends_letter_and_redirects_to_notification_page(mocker, service_one, client_request): - mocker.patch('app.main.views.uploads.get_letter_pdf_and_metadata', return_value=('file', {'status': 'valid'})) + metadata = {'filename': 'my_file.pdf', 'page_count': '1', 'status': 'valid'} + + 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') + mocker.patch('app.main.views.uploads.get_letter_metadata', return_value=metadata) service_one['permissions'] = ['letter', 'upload_letters'] file_id = 'abcd-1234' @@ -315,8 +331,11 @@ def test_send_uploaded_letter_when_service_does_not_have_correct_permissions( def test_send_uploaded_letter_when_metadata_states_pdf_is_invalid(mocker, service_one, client_request): - mocker.patch('app.main.views.uploads.get_letter_pdf_and_metadata', return_value=('file', {'status': 'invalid'})) 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'} + ) service_one['permissions'] = ['letter', 'upload_letters'] file_id = 'abcd-1234' 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 c30ac61b5..5e87a64b2 100644 --- a/tests/app/s3_client/test_s3_letter_upload_client.py +++ b/tests/app/s3_client/test_s3_letter_upload_client.py @@ -6,12 +6,17 @@ from app.s3_client.s3_letter_upload_client import upload_letter_to_s3 def test_upload_letter_to_s3(mocker): s3_mock = mocker.patch('app.s3_client.s3_letter_upload_client.utils_s3upload') - upload_letter_to_s3('pdf_data', 'service_id/upload_id.pdf', 'valid') + upload_letter_to_s3( + 'pdf_data', + file_location='service_id/upload_id.pdf', + status='valid', + page_count=3, + filename='my_doc') s3_mock.assert_called_once_with( bucket_name=current_app.config['TRANSIENT_UPLOADED_LETTERS'], file_location='service_id/upload_id.pdf', filedata='pdf_data', - metadata={'status': 'valid'}, + metadata={'status': 'valid', 'page_count': '3', 'filename': 'my_doc'}, region=current_app.config['AWS_REGION'] )