From 4fef2861c6c5705e6648e2249e878defc7e4816b Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 18 Feb 2022 14:37:09 +0000 Subject: [PATCH 1/2] Catch error if letter does not exist on send This repeats the pattern we already have for previewing a letter, where we assume the error is because the notification has already been sent and redirect the user to see it. I've improved the original pattern a bit: - I've DRYed-up the low-level boto code and moved the error handler there so it can be reused. - I've introduced a custom exception, which the calling code can choose to log. - I've introduced the moto library, which we use elsewhere, to make it easier to test S3 code. I've used an error level log when sending a notification - now that we have a more descriptive log, we can verify the assumption is true and then make an informed decision to downgrade the log. In future we may want to merge this handler with the similar code in utils [1], but we'll need to be careful as the utils handler is superficial - it doesn't check the reason for the error. [1]: https://github.com/alphagov/notifications-utils/blob/bce0f4e596af9451212d0026c8ecc27b636eed38/notifications_utils/s3.py#L52 --- app/main/views/uploads.py | 32 ++++++++++------ app/s3_client/s3_letter_upload_client.py | 28 +++++++++----- requirements_for_test.txt | 1 + .../main/views/uploads/test_upload_letter.py | 37 +++++++++++++++++-- .../s3_client/test_s3_letter_upload_client.py | 29 +++++++++++++++ 5 files changed, 103 insertions(+), 24 deletions(-) diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index 2080d7e9f..0a7a1c0f4 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,16 @@ def uploaded_letter_preview(service_id, file_id): try: metadata = get_letter_metadata(service_id, file_id) - except ClientError as e: + except LetterNotFoundError as e: + current_app.logger.warning(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 + 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 +351,18 @@ 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'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 + 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') From 3effb75045b84e940c70b4e0fbfe353fe87263c6 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 23 Feb 2022 17:25:35 +0000 Subject: [PATCH 2/2] Clarify purpose of file_id in redirects In response to: [1]. [1]: https://github.com/alphagov/notifications-admin/pull/4159/files#r813050941 --- app/main/views/uploads.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index 0a7a1c0f4..fbaf77240 100644 --- a/app/main/views/uploads.py +++ b/app/main/views/uploads.py @@ -270,8 +270,10 @@ def uploaded_letter_preview(service_id, file_id): except LetterNotFoundError as e: current_app.logger.warning(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 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, @@ -356,8 +358,10 @@ def send_uploaded_letter(service_id, file_id): except LetterNotFoundError as e: current_app.logger.error(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 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,