diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index 2080d7e9f..fbaf77240 100644 --- a/app/main/views/uploads.py +++ b/app/main/views/uploads.py @@ -7,7 +7,6 @@ from functools import partial from io import BytesIO from zipfile import BadZipFile -from botocore.exceptions import ClientError from flask import ( abort, current_app, @@ -39,6 +38,7 @@ from app.main import main from app.main.forms import CsvUploadForm, LetterUploadPostageForm, PDFUploadForm from app.models.contact_list import ContactList from app.s3_client.s3_letter_upload_client import ( + LetterNotFoundError, backup_original_letter_to_s3, get_letter_metadata, get_letter_pdf_and_metadata, @@ -267,17 +267,18 @@ def uploaded_letter_preview(service_id, file_id): try: metadata = get_letter_metadata(service_id, file_id) - except ClientError as e: - # if the file's not there, it's probably because we've already created the notification and the letter has been - # moved to the normal letters-pdf bucket. So lets just bounce out to the notification page - if e.response['Error']['Code'] == 'NoSuchKey': - return redirect(url_for( - '.view_notification', - service_id=service_id, - notification_id=file_id, - )) - else: - raise + except LetterNotFoundError as e: + current_app.logger.warning(e) + + # If the file is missing it could be because this is a duplicate + # request, the notification already exists and the file has been + # moved to a different bucket. Note that the ID of a precompiled + # notification is always set to the file_id. + return redirect(url_for( + '.view_notification', + service_id=service_id, + notification_id=file_id, + )) original_filename = metadata.get('filename') page_count = metadata.get('page_count') @@ -352,7 +353,20 @@ def send_uploaded_letter(service_id, file_id): if not current_service.has_permission('letter'): abort(403) - metadata = get_letter_metadata(service_id, file_id) + try: + metadata = get_letter_metadata(service_id, file_id) + except LetterNotFoundError as e: + current_app.logger.error(e) + + # If the file is missing it could be because this is a duplicate + # request, the notification already exists and the file has been + # moved to a different bucket. Note that the ID of a precompiled + # notification is always set to the file_id. + return redirect(url_for( + '.view_notification', + service_id=service_id, + notification_id=file_id, + )) 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 861a62604..4bb479fdb 100644 --- a/app/s3_client/s3_letter_upload_client.py +++ b/app/s3_client/s3_letter_upload_client.py @@ -1,11 +1,16 @@ import json import urllib +import botocore from boto3 import resource from flask import current_app from notifications_utils.s3 import s3upload as utils_s3upload +class LetterNotFoundError(Exception): + pass + + def get_transient_letter_file_location(service_id, upload_id): return 'service-{}/{}.pdf'.format(service_id, upload_id) @@ -69,19 +74,24 @@ class LetterMetadata: return value +def get_letter_s3_object(service_id, file_id): + try: + file_location = get_transient_letter_file_location(service_id, file_id) + s3 = resource('s3') + return s3.Object(current_app.config['TRANSIENT_UPLOADED_LETTERS'], file_location).get() + except botocore.exceptions.ClientError as e: + if e.response['Error']['Code'] == 'NoSuchKey': + raise LetterNotFoundError(f'Letter not found for service {service_id} and file {file_id}') + + raise + + 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() - + s3_object = get_letter_s3_object(service_id, file_id) 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') - s3_object = s3.Object(current_app.config['TRANSIENT_UPLOADED_LETTERS'], file_location).get() - + s3_object = get_letter_s3_object(service_id, file_id) return LetterMetadata(s3_object['Metadata']) diff --git a/requirements_for_test.txt b/requirements_for_test.txt index 8a7c1427e..180a8c8ad 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -9,6 +9,7 @@ freezegun==1.1.0 flake8==4.0.1 flake8-bugbear==21.9.2 flake8-print==4.0.0 +moto==3.0.4 requests-mock==1.9.3 # used for creating manifest file locally jinja2-cli[yaml]==0.7.0 diff --git a/tests/app/main/views/uploads/test_upload_letter.py b/tests/app/main/views/uploads/test_upload_letter.py index a0d13afc9..ea1269269 100644 --- a/tests/app/main/views/uploads/test_upload_letter.py +++ b/tests/app/main/views/uploads/test_upload_letter.py @@ -1,12 +1,14 @@ from unittest.mock import ANY, Mock import pytest -from botocore.exceptions import ClientError from flask import make_response, url_for from requests import RequestException from app.formatters import normalize_spaces -from app.s3_client.s3_letter_upload_client import LetterMetadata +from app.s3_client.s3_letter_upload_client import ( + LetterMetadata, + LetterNotFoundError, +) from tests.conftest import SERVICE_ONE_ID @@ -465,11 +467,11 @@ def test_uploaded_letter_preview_redirects_if_file_not_in_s3( client_request, fake_uuid ): - boto_error_json = {'Error': {'Code': 'NoSuchKey', 'Message': 'The specified key does not exist.'}} mocker.patch( 'app.main.views.uploads.get_letter_metadata', - side_effect=ClientError(boto_error_json, 'operation_name') + side_effect=LetterNotFoundError ) + client_request.get( 'main.uploaded_letter_preview', service_id=SERVICE_ONE_ID, @@ -646,6 +648,33 @@ def test_send_uploaded_letter_sends_letter_and_redirects_to_notification_page( ) +def test_send_uploaded_letter_redirects_if_file_not_in_s3( + mocker, + client_request, + fake_uuid, + service_one, +): + mocker.patch( + 'app.main.views.uploads.get_letter_metadata', + side_effect=LetterNotFoundError + ) + + service_one['permissions'] = ['letter', 'upload_letters'] + + client_request.post( + 'main.send_uploaded_letter', + service_id=SERVICE_ONE_ID, + file_id=fake_uuid, + _data={'filename': 'my_file.pdf'}, + _expected_redirect=url_for( + 'main.view_notification', + service_id=SERVICE_ONE_ID, + notification_id=fake_uuid, + _external=True + ) + ) + + @pytest.mark.parametrize('permissions', [ ['email'], ['sms'], 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 163b28ca6..86aadb2d1 100644 --- a/tests/app/s3_client/test_s3_letter_upload_client.py +++ b/tests/app/s3_client/test_s3_letter_upload_client.py @@ -1,11 +1,17 @@ import urllib import uuid +import boto3 +import botocore +import pytest from flask import current_app +from moto import mock_s3 from app.s3_client.s3_letter_upload_client import ( LetterMetadata, + LetterNotFoundError, backup_original_letter_to_s3, + get_letter_metadata, upload_letter_to_s3, ) @@ -89,3 +95,26 @@ def test_lettermetadata_unquotes_special_keys(): metadata = LetterMetadata({"filename": "%C2%A3hello", "recipient": "%C2%A3hi"}) assert metadata.get("filename") == "£hello" assert metadata.get("recipient") == "£hi" + + +@mock_s3 +@pytest.mark.parametrize('will_raise_custom_error,expected_exception', [ + (True, LetterNotFoundError), + (False, botocore.exceptions.ClientError) +]) +def test_get_letter_s3_object_raises_custom_error( + will_raise_custom_error, + expected_exception +): + bucket_name = current_app.config['TRANSIENT_UPLOADED_LETTERS'] + s3 = boto3.client('s3', region_name='eu-west-1') + + # bucket not existing will trigger some other error + if will_raise_custom_error: + s3.create_bucket( + Bucket=bucket_name, + CreateBucketConfiguration={'LocationConstraint': 'eu-west-1'} + ) + + with pytest.raises(expected_exception): + get_letter_metadata('service', 'file')