From 5f0843a17594adad90c2871954519ae53a6466ba Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Fri, 23 Mar 2018 10:58:05 +0000 Subject: [PATCH 1/9] Update pytest from 3.4.2 to 3.5.0 --- requirements_for_test.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements_for_test.txt b/requirements_for_test.txt index 45bb4d004..391a8ce5a 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -1,7 +1,7 @@ -r requirements.txt flake8==3.5.0 moto==1.3.1 -pytest==3.4.2 +pytest==3.5.0 pytest-env==0.6.2 pytest-mock==1.7.1 pytest-cov==2.5.1 From 463f1eefaf5aa6c9a1febb9235499a9b628e334b Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Tue, 27 Mar 2018 17:37:09 +0100 Subject: [PATCH 2/9] Move proxy header check to auth-requiring endpoints The main drive behind this is to allow us to enable http healthchecks on the `/_status` endpoint. The healthcheck requests are happening directly on the instances without going to the proxy to get the header properly set. In any case, endpoints like `/_status` should be generally accessible by anything without requiring any form of authorization. --- app/__init__.py | 2 -- app/authentication/auth.py | 5 ++++ .../app/authentication/test_authentication.py | 30 +++++++++++++++++-- 3 files changed, 33 insertions(+), 4 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index c56ec476c..81eeec6e0 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -219,8 +219,6 @@ def init_app(app): def record_user_agent(): statsd_client.incr("user-agent.{}".format(process_user_agent(request.headers.get('User-Agent', None)))) - app.before_request(request_helper.check_proxy_header_before_request) - @app.before_request def record_request_details(): g.start = monotonic() diff --git a/app/authentication/auth.py b/app/authentication/auth.py index ea42791ee..f878ccda7 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -1,6 +1,7 @@ from flask import request, _request_ctx_stack, current_app, g from notifications_python_client.authentication import decode_jwt_token, get_token_issuer from notifications_python_client.errors import TokenDecodeError, TokenExpiredError, TokenIssuerError +from notifications_utils import request_helper from sqlalchemy.exc import DataError from sqlalchemy.orm.exc import NoResultFound @@ -48,6 +49,8 @@ def requires_no_auth(): def requires_admin_auth(): + request_helper.check_proxy_header_before_request() + auth_token = get_auth_token(request) client = __get_token_issuer(auth_token) @@ -59,6 +62,8 @@ def requires_admin_auth(): def requires_auth(): + request_helper.check_proxy_header_before_request() + auth_token = get_auth_token(request) client = __get_token_issuer(auth_token) diff --git a/tests/app/authentication/test_authentication.py b/tests/app/authentication/test_authentication.py index 4677ecce3..e6d78d317 100644 --- a/tests/app/authentication/test_authentication.py +++ b/tests/app/authentication/test_authentication.py @@ -325,11 +325,11 @@ def __create_token(service_id): @pytest.mark.parametrize('check_proxy_header,header_value,expected_status', [ (True, 'key_1', 200), - (True, 'wrong_key', 403), + (True, 'wrong_key', 200), (False, 'key_1', 200), (False, 'wrong_key', 200), ]) -def test_route_correct_secret_key(notify_api, check_proxy_header, header_value, expected_status): +def test_proxy_key_non_auth_endpoint(notify_api, check_proxy_header, header_value, expected_status): with set_config_values(notify_api, { 'ROUTE_SECRET_KEY_1': 'key_1', 'ROUTE_SECRET_KEY_2': '', @@ -344,3 +344,29 @@ def test_route_correct_secret_key(notify_api, check_proxy_header, header_value, ] ) assert response.status_code == expected_status + + +@pytest.mark.parametrize('check_proxy_header,header_value,expected_status', [ + (True, 'key_1', 200), + (True, 'wrong_key', 403), + (False, 'key_1', 200), + (False, 'wrong_key', 200), +]) +def test_proxy_key_on_admin_auth_endpoint(notify_api, check_proxy_header, header_value, expected_status): + token = create_jwt_token(current_app.config['ADMIN_CLIENT_SECRET'], current_app.config['ADMIN_CLIENT_USER_NAME']) + + with set_config_values(notify_api, { + 'ROUTE_SECRET_KEY_1': 'key_1', + 'ROUTE_SECRET_KEY_2': '', + 'CHECK_PROXY_HEADER': check_proxy_header, + }): + + with notify_api.test_client() as client: + response = client.get( + path='/service', + headers=[ + ('X-Custom-Forwarder', header_value), + ('Authorization', 'Bearer {}'.format(token)) + ] + ) + assert response.status_code == expected_status From 6f1e4c76d56b92f93a7a57b28169c47b3c4e7454 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Tue, 27 Mar 2018 17:41:05 +0100 Subject: [PATCH 3/9] Make test context managers more reliable Sometimes, when a test using one of the set_config[_values] context managers failed or raised an exception it would cause the context to not be able to revert its config changes, resulting in a 'spooky action at a distance' where random tests would start to fail for non-obvious reasons. --- tests/conftest.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 0a5dc77c9..7ed1ba895 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -141,8 +141,10 @@ def pytest_generate_tests(metafunc): def set_config(app, name, value): old_val = app.config.get(name) app.config[name] = value - yield - app.config[name] = old_val + try: + yield + finally: + app.config[name] = old_val @contextmanager @@ -153,7 +155,8 @@ def set_config_values(app, dict): old_values[key] = app.config.get(key) app.config[key] = dict[key] - yield - - for key in dict: - app.config[key] = old_values[key] + try: + yield + finally: + for key in dict: + app.config[key] = old_values[key] From 52aeec22476b1978d06d6fc0e44c790d48923d63 Mon Sep 17 00:00:00 2001 From: venusbb Date: Thu, 29 Mar 2018 11:38:07 +0100 Subject: [PATCH 4/9] bug fix: use date only when comparing unique records rather than datetime --- app/celery/reporting_tasks.py | 5 +++- tests/app/celery/test_reporting_tasks.py | 37 ++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/app/celery/reporting_tasks.py b/app/celery/reporting_tasks.py index 960264fc6..f3cf640cf 100644 --- a/app/celery/reporting_tasks.py +++ b/app/celery/reporting_tasks.py @@ -84,15 +84,18 @@ def create_nightly_billing(day_start=None): for data in transit_data: update_count = FactBilling.query.filter( - FactBilling.bst_date == process_day, + FactBilling.bst_date == datetime.date(process_day), FactBilling.template_id == data.template_id, + FactBilling.service_id == data.service_id, FactBilling.provider == data.sent_by, # This could be zero - this is a bug that needs to be fixed. FactBilling.rate_multiplier == data.rate_multiplier, FactBilling.notification_type == data.notification_type, + FactBilling.international == data.international ).update( {"notifications_sent": data.notifications_sent, "billable_units": data.billable_units}, synchronize_session=False) + if update_count == 0: billing_record = FactBilling( bst_date=process_day, diff --git a/tests/app/celery/test_reporting_tasks.py b/tests/app/celery/test_reporting_tasks.py index b9ce112ec..9bf2daa99 100644 --- a/tests/app/celery/test_reporting_tasks.py +++ b/tests/app/celery/test_reporting_tasks.py @@ -404,3 +404,40 @@ def test_create_nightly_billing_use_BST( assert len(records) == 2 assert records[0].bst_date == date(2018, 3, 25) assert records[-1].bst_date == date(2018, 3, 26) + + +@freeze_time('2018-01-15T03:30:00') +def test_create_nightly_billing_update_when_record_exists( + notify_db, + notify_db_session, + sample_service, + sample_template, + mocker): + + mocker.patch('app.celery.reporting_tasks.get_rate', side_effect=mocker_get_rate) + + sample_notification( + notify_db, + notify_db_session, + created_at=datetime.now() - timedelta(days=1), + service=sample_service, + template=sample_template, + status='delivered', + sent_by=None, + international=False, + rate_multiplier=1.0, + billable_units=1, + ) + + records = FactBilling.query.all() + assert len(records) == 0 + + create_nightly_billing() + records = FactBilling.query.order_by(FactBilling.bst_date).all() + + assert len(records) == 1 + assert records[0].bst_date == date(2018, 1, 14) + + # run again, make sure create_nightly_billing() updates with no error + create_nightly_billing() + assert len(records) == 1 From 0e6907aba72ee59b1385543b40f41dcb535eb700 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 29 Mar 2018 14:28:06 +0100 Subject: [PATCH 5/9] Refactor letter utils tests - tests were failing as creating duplicate buckets --- tests/app/letters/test_letter_utils.py | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/tests/app/letters/test_letter_utils.py b/tests/app/letters/test_letter_utils.py index 400c545dc..5a8be234d 100644 --- a/tests/app/letters/test_letter_utils.py +++ b/tests/app/letters/test_letter_utils.py @@ -169,37 +169,33 @@ def test_move_scanned_letter_pdf_to_processing_bucket( @freeze_time(FROZEN_DATE_TIME) def test_move_failed_pdf_error(notify_api): filename = 'test.pdf' - source_bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME'] - target_bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME'] + bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME'] conn = boto3.resource('s3', region_name='eu-west-1') - source_bucket = conn.create_bucket(Bucket=source_bucket_name) - target_bucket = conn.create_bucket(Bucket=target_bucket_name) + bucket = conn.create_bucket(Bucket=bucket_name) s3 = boto3.client('s3', region_name='eu-west-1') - s3.put_object(Bucket=source_bucket_name, Key=filename, Body=b'pdf_content') + s3.put_object(Bucket=bucket_name, Key=filename, Body=b'pdf_content') move_failed_pdf(filename, ScanErrorType.ERROR) - assert 'ERROR/' + filename in [o.key for o in target_bucket.objects.all()] - assert filename not in [o.key for o in source_bucket.objects.all()] + assert 'ERROR/' + filename in [o.key for o in bucket.objects.all()] + assert filename not in [o.key for o in bucket.objects.all()] @mock_s3 @freeze_time(FROZEN_DATE_TIME) def test_move_failed_pdf_scan_failed(notify_api): filename = 'test.pdf' - source_bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME'] - target_bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME'] + bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME'] conn = boto3.resource('s3', region_name='eu-west-1') - source_bucket = conn.create_bucket(Bucket=source_bucket_name) - target_bucket = conn.create_bucket(Bucket=target_bucket_name) + bucket = conn.create_bucket(Bucket=bucket_name) s3 = boto3.client('s3', region_name='eu-west-1') - s3.put_object(Bucket=source_bucket_name, Key=filename, Body=b'pdf_content') + s3.put_object(Bucket=bucket_name, Key=filename, Body=b'pdf_content') move_failed_pdf(filename, ScanErrorType.FAILURE) - assert 'FAILURE/' + filename in [o.key for o in target_bucket.objects.all()] - assert filename not in [o.key for o in source_bucket.objects.all()] + assert 'FAILURE/' + filename in [o.key for o in bucket.objects.all()] + assert filename not in [o.key for o in bucket.objects.all()] From 7524402b561184926e61c86c8122944887c0fd80 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 29 Mar 2018 14:28:42 +0100 Subject: [PATCH 6/9] Only hide the notify tag on precompiled on first page --- app/template/rest.py | 12 ++++----- tests/app/template/test_rest.py | 48 +++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 7 deletions(-) diff --git a/app/template/rest.py b/app/template/rest.py index 9db0bed57..8ad07d423 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -200,7 +200,6 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file template = dao_get_template_by_id(notification.template_id) if template.is_precompiled_letter: - try: pdf_file = get_letter_pdf(notification) @@ -215,9 +214,9 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file content = base64.b64encode(pdf_file).decode('utf-8') if file_type == 'png': - try: - page_number = page if page else "0" + page_number = page if page else "1" + pdf_page = extract_page_from_pdf(BytesIO(pdf_file), int(page_number) - 1) content = base64.b64encode(pdf_page).decode('utf-8') except PdfReadError as e: @@ -227,12 +226,12 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file status_code=500 ) - url = '{}/precompiled-preview.png'.format( - current_app.config['TEMPLATE_PREVIEW_API_HOST'] + url = '{}/precompiled-preview.png{}'.format( + current_app.config['TEMPLATE_PREVIEW_API_HOST'], + '?hide_notify=true' if page_number == '1' else '' ) content = _get_png_preview(url, content, notification.id, json=False) - else: template_for_letter_print = { @@ -256,7 +255,6 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file file_type, '?page={}'.format(page) if page else '' ) - content = _get_png_preview(url, data, notification.id, json=True) return jsonify({"content": content}) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 14145014e..9d043a19c 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -1008,6 +1008,54 @@ def test_preview_letter_template_precompiled_png_file_type( assert base64.b64decode(resp['content']) == png_content +@pytest.mark.parametrize('page_number,expect_preview_url', [ + ('', 'http://localhost/notifications-template-preview/precompiled-preview.png?hide_notify=true'), + ('1', 'http://localhost/notifications-template-preview/precompiled-preview.png?hide_notify=true'), + ('2', 'http://localhost/notifications-template-preview/precompiled-preview.png') +]) +def test_preview_letter_template_precompiled_png_file_type_hide_notify_tag_only_on_first_page( + notify_api, + client, + admin_request, + sample_service, + mocker, + page_number, + expect_preview_url +): + + template = create_template(sample_service, + template_type='letter', + template_name='Pre-compiled PDF', + subject='Pre-compiled PDF', + hidden=True) + + notification = create_notification(template) + + with set_config_values(notify_api, { + 'TEMPLATE_PREVIEW_API_HOST': 'http://localhost/notifications-template-preview', + 'TEMPLATE_PREVIEW_API_KEY': 'test-key' + }): + pdf_content = b'\x00\x01' + png_content = b'\x00\x02' + encoded = base64.b64encode(png_content).decode('utf-8') + + mocker.patch('app.template.rest.get_letter_pdf', return_value=pdf_content) + mocker.patch('app.template.rest.extract_page_from_pdf', return_value=png_content) + mock_get_png_preview = mocker.patch('app.template.rest._get_png_preview', return_value=encoded) + + admin_request.get( + 'template.preview_letter_template_by_notification_id', + service_id=notification.service_id, + notification_id=notification.id, + file_type='png', + page=page_number + ) + + mock_get_png_preview.assert_called_once_with( + expect_preview_url, encoded, notification.id, json=False + ) + + def test_preview_letter_template_precompiled_png_template_preview_500_error( notify_api, client, From 8f1f546f6950739e79ee53fc62200743242fe2b7 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Thu, 29 Mar 2018 14:57:19 +0100 Subject: [PATCH 7/9] Increase pool size to 15 permanent connections At the same time, decrease the number of workers from 5 to 4. Effect on max db connections will be the same - although with a higher "resting" number of connections. Before: 12 (instances) * 5 (workers) * 20 (10 permanent + 10 overflow) = 1200 After: 12 (instances) * 4 (workers) * 25 (15 permanent + 10 overflow) = 1200 --- gunicorn_config.py | 2 +- manifest-api-base.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/gunicorn_config.py b/gunicorn_config.py index b3e7ddf90..c74012969 100644 --- a/gunicorn_config.py +++ b/gunicorn_config.py @@ -2,7 +2,7 @@ import os import sys import traceback -workers = 5 +workers = 4 worker_class = "eventlet" worker_connections = 256 errorlog = "/home/vcap/logs/gunicorn_error.log" diff --git a/manifest-api-base.yml b/manifest-api-base.yml index e8197575f..28ca9ef5a 100644 --- a/manifest-api-base.yml +++ b/manifest-api-base.yml @@ -9,7 +9,7 @@ env: CW_APP_NAME: api # required by cf run-task FLASK_APP: application.py - SQLALCHEMY_POOL_SIZE: 10 + SQLALCHEMY_POOL_SIZE: 15 # Credentials variables ADMIN_BASE_URL: null From c9b297a3d47fab5635e54ee8ad72da1afaff2de9 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 3 Apr 2018 12:10:45 +0100 Subject: [PATCH 8/9] Bump utils to 25.2.3 To bring in: - [x] https://github.com/alphagov/notifications-utils/pull/436 Changes: - https://github.com/alphagov/notifications-utils/compare/25.2.2...25.2.3 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 24316ae9c..5e85a803a 100644 --- a/requirements.txt +++ b/requirements.txt @@ -23,6 +23,6 @@ notifications-python-client==4.8.1 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@25.2.2#egg=notifications-utils==25.2.2 +git+https://github.com/alphagov/notifications-utils.git@25.2.3#egg=notifications-utils==25.2.3 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 From f1abce22ae016cf369c8efa467afc04bc0a7101c Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Tue, 3 Apr 2018 12:31:52 +0100 Subject: [PATCH 9/9] Logging refactor to make debugging easier. Before the filename needed to be known. Added the notification id to the logging message so that the notification can be traced through the logging system by knowing the notification id, making it easier to debug. Also changed to raise an exception so that alerts are generated. This way we should get an email to say that there has been an error. --- app/celery/letters_pdf_tasks.py | 11 ++++++++--- app/errors.py | 6 ++++++ tests/app/celery/test_letters_pdf_tasks.py | 9 +++++++-- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 9c05ab223..0d4f77fef 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -21,6 +21,7 @@ from app.dao.notifications_dao import ( dao_get_notifications_by_references, dao_update_notifications_by_reference, ) +from app.errors import VirusScanError from app.letters.utils import ( get_reference_from_filename, move_scanned_pdf_to_test_or_live_pdf_bucket, @@ -162,9 +163,9 @@ def letter_in_created_state(filename): @notify_celery.task(name='process-virus-scan-passed') def process_virus_scan_passed(filename): - current_app.logger.info('Virus scan passed: {}'.format(filename)) reference = get_reference_from_filename(filename) notification = dao_get_notification_by_reference(reference) + current_app.logger.info('notification id {} Virus scan passed: {}'.format(notification.id, filename)) is_test_key = notification.key_type == KEY_TYPE_TEST move_scanned_pdf_to_test_or_live_pdf_bucket( @@ -179,9 +180,9 @@ def process_virus_scan_passed(filename): @notify_celery.task(name='process-virus-scan-failed') def process_virus_scan_failed(filename): - current_app.logger.exception('Virus scan failed: {}'.format(filename)) move_failed_pdf(filename, ScanErrorType.FAILURE) reference = get_reference_from_filename(filename) + notification = dao_get_notification_by_reference(reference) updated_count = update_letter_pdf_status(reference, NOTIFICATION_VIRUS_SCAN_FAILED) if updated_count != 1: @@ -191,12 +192,14 @@ def process_virus_scan_failed(filename): ) ) + raise VirusScanError('notification id {} Virus scan failed: {}'.format(notification.id, filename)) + @notify_celery.task(name='process-virus-scan-error') def process_virus_scan_error(filename): - current_app.logger.exception('Virus scan error: {}'.format(filename)) move_failed_pdf(filename, ScanErrorType.ERROR) reference = get_reference_from_filename(filename) + notification = dao_get_notification_by_reference(reference) updated_count = update_letter_pdf_status(reference, NOTIFICATION_TECHNICAL_FAILURE) if updated_count != 1: @@ -206,6 +209,8 @@ def process_virus_scan_error(filename): ) ) + raise VirusScanError('notification id {} Virus scan error: {}'.format(notification.id, filename)) + def update_letter_pdf_status(reference, status): return dao_update_notifications_by_reference( diff --git a/app/errors.py b/app/errors.py index c2461f3cd..c5df8892f 100644 --- a/app/errors.py +++ b/app/errors.py @@ -10,6 +10,12 @@ from jsonschema import ValidationError as JsonSchemaValidationError from app.authentication.auth import AuthError +class VirusScanError(Exception): + def __init__(self, message): + + super().__init__(message) + + class InvalidRequest(Exception): code = None fields = [] diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 9d07e7239..055b711d9 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -10,6 +10,7 @@ from celery.exceptions import MaxRetriesExceededError from requests import RequestException from sqlalchemy.orm.exc import NoResultFound +from app.errors import VirusScanError from app.variables import Retention from app.celery.letters_pdf_tasks import ( create_letters_pdf, @@ -343,8 +344,10 @@ def test_process_letter_task_check_virus_scan_failed(sample_letter_notification, sample_letter_notification.status = 'pending-virus-check' mock_move_failed_pdf = mocker.patch('app.celery.letters_pdf_tasks.move_failed_pdf') - process_virus_scan_failed(filename) + with pytest.raises(VirusScanError) as e: + process_virus_scan_failed(filename) + assert "Virus scan failed:" in str(e) mock_move_failed_pdf.assert_called_once_with(filename, ScanErrorType.FAILURE) assert sample_letter_notification.status == NOTIFICATION_VIRUS_SCAN_FAILED @@ -354,7 +357,9 @@ def test_process_letter_task_check_virus_scan_error(sample_letter_notification, sample_letter_notification.status = 'pending-virus-check' mock_move_failed_pdf = mocker.patch('app.celery.letters_pdf_tasks.move_failed_pdf') - process_virus_scan_error(filename) + with pytest.raises(VirusScanError) as e: + process_virus_scan_error(filename) + assert "Virus scan error:" in str(e) mock_move_failed_pdf.assert_called_once_with(filename, ScanErrorType.ERROR) assert sample_letter_notification.status == NOTIFICATION_TECHNICAL_FAILURE