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/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/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/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/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/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 bc043e351..0a9a1a564 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 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 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 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 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 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 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()] 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, 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]