darkhelm/notifications-admin
1
0
Fork 0
mirror of https://github.com/GSA/notifications-admin.git synced 2026-02-05 10:53:28 -05:00
Code Issues Packages Projects Releases Wiki Activity

Store all data about uploaded letters in the S3 metadata

Browse Source 
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.
This commit is contained in:
Katie Smith
2019-10-07 15:22:11 +01:00
parent 28c901d5b9
commit 11543e15be
5 changed files with 71 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files

37
app/main/views/uploads.py
View File

@@ -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/<service_id>/preview-letter/<file_id>")
@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/<service_id>/preview-letter-image/<file_id>")
@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/<service_id>/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)

19
app/s3_client/s3_letter_upload_client.py
View File

@@ -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']

1
app/templates/views/uploads/preview.html
View File

@@ -28,7 +28,6 @@
service_id=current_service.id,
)}}" class='page-footer'>
<input type="hidden" name="csrf_token" value="{{ csrf_token() }}" />
<input type="hidden" name="filename" value="{{ original_filename }}" />
<input type="hidden" name="file_id" value="{{ file_id }}" />
<button type="submit" class="button">Send 1 letter</button>
</form>

33
tests/app/main/views/test_uploads.py
View File

@@ -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'

9
tests/app/s3_client/test_s3_letter_upload_client.py
View File

@@ -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']
)