From a7dbde39e9efaff29c538c421e2b2100cfab4ab6 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Tue, 14 Nov 2017 14:38:26 +0000 Subject: [PATCH 01/14] Rename utils module --- app/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index c45ad9a84..c670e659d 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -25,7 +25,7 @@ from flask_wtf.csrf import CSRFError from functools import partial from notifications_python_client.errors import HTTPError -from notifications_utils import logging, request_id, formatters +from notifications_utils import logging, request_helper, formatters from notifications_utils.clients.statsd.statsd_client import StatsdClient from notifications_utils.recipients import ( validate_phone_number, @@ -95,7 +95,7 @@ def create_app(): statsd_client.init_app(application) logging.init_app(application, statsd_client) init_csrf(application) - request_id.init_app(application) + request_helper.init_app(application) service_api_client.init_app(application) user_api_client.init_app(application) From 5f0e6beb7977963a1a843f0e491e13f6f477d2d3 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Tue, 14 Nov 2017 14:40:32 +0000 Subject: [PATCH 02/14] Add before_request to check header --- app/__init__.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/__init__.py b/app/__init__.py index c670e659d..c14a17fea 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -186,6 +186,8 @@ def init_csrf(application): def init_app(application): + application.before_request(request_helper.check_proxy_header_before_request) + @application.before_request def record_start_time(): g.start = monotonic() From 557420060748dcd4e2356f2723477a6906a3cdb7 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Tue, 14 Nov 2017 14:51:17 +0000 Subject: [PATCH 03/14] Copy set_config_values from api --- tests/conftest.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/conftest.py b/tests/conftest.py index 2a1958781..860633847 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2457,6 +2457,19 @@ def set_config(app, name, value): app.config[name] = old_val +@contextmanager +def set_config_values(app, dict): + old_values = {} + + for key in dict: + old_values[key] = app.config.get(key) + app.config[key] = dict[key] + + yield + + for key in dict: + app.config[key] = old_values[key] + @pytest.fixture(scope='function') def valid_token(app_, fake_uuid): return generate_token( From 0d2adfcce21dd2efb5d781babec3e6b03464b6d5 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Tue, 14 Nov 2017 15:26:03 +0000 Subject: [PATCH 04/14] Add basic tests --- tests/app/main/test_request_header.py | 33 +++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 tests/app/main/test_request_header.py diff --git a/tests/app/main/test_request_header.py b/tests/app/main/test_request_header.py new file mode 100644 index 000000000..6111c6f26 --- /dev/null +++ b/tests/app/main/test_request_header.py @@ -0,0 +1,33 @@ +from tests.conftest import set_config_values + + +def test_route_correct_secret_key(app_, client): + with set_config_values(app_, { + 'ROUTE_SECRET_KEY_1': 'key_1', + 'ROUTE_SECRET_KEY_2': '', + 'DEBUG': False, + }): + + response = client.get( + path='/_status', + headers=[ + ('X-Custom-forwarder', 'key_1'), + ] + ) + assert response.status_code == 200 + + +def test_route_incorrect_secret_key(app_, client): + with set_config_values(app_, { + 'ROUTE_SECRET_KEY_1': 'key_1', + 'ROUTE_SECRET_KEY_2': '', + 'DEBUG': False, + }): + + response = client.get( + path='/_status', + headers=[ + ('X-Custom-forwarder', 'wrong_key'), + ] + ) + assert response.status_code == 403 From 5b978bc0f432955733dcb5097ce26bf368d420b1 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Tue, 14 Nov 2017 15:26:07 +0000 Subject: [PATCH 05/14] Bump utils version in requirements --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 9383324d6..84dc5531f 100644 --- a/requirements.txt +++ b/requirements.txt @@ -22,4 +22,4 @@ notifications-python-client==4.6.0 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@21.5.0#egg=notifications-utils==21.5.0 +git+https://github.com/alphagov/notifications-utils.git@22.0.0#egg=notifications-utils==22.0.0 From 36663bd9ac661a45e4fb37b5e08dc9c9e19ca075 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Wed, 15 Nov 2017 12:05:05 +0000 Subject: [PATCH 06/14] Load route_secret config --- app/cloudfoundry_config.py | 2 ++ app/config.py | 2 ++ 2 files changed, 4 insertions(+) diff --git a/app/cloudfoundry_config.py b/app/cloudfoundry_config.py index d04b3eb5d..6d55acf37 100644 --- a/app/cloudfoundry_config.py +++ b/app/cloudfoundry_config.py @@ -37,6 +37,8 @@ def extract_notify_config(notify_config): os.environ['ADMIN_BASE_URL'] = notify_config['credentials']['admin_base_url'] os.environ['SECRET_KEY'] = notify_config['credentials']['secret_key'] os.environ['DANGEROUS_SALT'] = notify_config['credentials']['dangerous_salt'] + os.environ['ROUTE_SECRET_KEY_1'] = notify_config['credentials']['route_secret_key_1'] + os.environ['ROUTE_SECRET_KEY_2'] = notify_config['credentials']['route_secret_key_2'] def extract_notify_aws_config(aws_config): diff --git a/app/config.py b/app/config.py index 71a1e21ad..086e84935 100644 --- a/app/config.py +++ b/app/config.py @@ -98,6 +98,8 @@ class Config(object): ] LOGO_UPLOAD_BUCKET_NAME = 'public-logos-local' + ROUTE_SECRET_KEY_1 = os.environ.get('ROUTE_SECRET_KEY_1', '') + ROUTE_SECRET_KEY_2 = os.environ.get('ROUTE_SECRET_KEY_2', '') class Development(Config): From 7d4fdba237b7482f82ca5272832f4e391de1ca90 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Wed, 15 Nov 2017 16:56:39 +0000 Subject: [PATCH 07/14] Fix PEP8 --- tests/conftest.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/conftest.py b/tests/conftest.py index e1277a0d3..6fae5774f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2473,6 +2473,7 @@ def set_config_values(app, dict): for key in dict: app.config[key] = old_values[key] + @pytest.fixture(scope='function') def valid_token(app_, fake_uuid): return generate_token( From f8b8cdece7740629864743fd56b39bf21fb7d36f Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Thu, 16 Nov 2017 16:25:40 +0000 Subject: [PATCH 08/14] No need to put the before_request down there --- app/__init__.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 7cdb9f4d8..317e395d3 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -158,13 +158,12 @@ def init_app(application): application.after_request(useful_headers_after_request) application.after_request(save_service_after_request) application.before_request(load_service_before_request) + application.before_request(request_helper.check_proxy_header_before_request) @application.context_processor def _attach_current_service(): return {'current_service': current_service} - application.before_request(request_helper.check_proxy_header_before_request) - @application.before_request def record_start_time(): g.start = monotonic() From 0e9291c9d8fb8e88e0c8e35e1f046acc163aa469 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Thu, 16 Nov 2017 16:33:21 +0000 Subject: [PATCH 09/14] Add CHECK_PROXY_HEADER flag and bump utils --- app/config.py | 4 ++++ requirements.txt | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/app/config.py b/app/config.py index 086e84935..eb2cdd5e2 100644 --- a/app/config.py +++ b/app/config.py @@ -100,6 +100,7 @@ class Config(object): LOGO_UPLOAD_BUCKET_NAME = 'public-logos-local' ROUTE_SECRET_KEY_1 = os.environ.get('ROUTE_SECRET_KEY_1', '') ROUTE_SECRET_KEY_2 = os.environ.get('ROUTE_SECRET_KEY_2', '') + CHECK_PROXY_HEADER = False class Development(Config): @@ -130,6 +131,7 @@ class Preview(Config): CSV_UPLOAD_BUCKET_NAME = 'preview-notifications-csv-upload' LOGO_UPLOAD_BUCKET_NAME = 'public-logos-preview' NOTIFY_ENVIRONMENT = 'preview' + CHECK_PROXY_HEADER = True class Staging(Config): @@ -140,6 +142,7 @@ class Staging(Config): CSV_UPLOAD_BUCKET_NAME = 'staging-notify-csv-upload' LOGO_UPLOAD_BUCKET_NAME = 'public-logos-staging' NOTIFY_ENVIRONMENT = 'staging' + CHECK_PROXY_HEADER = True class Live(Config): @@ -150,6 +153,7 @@ class Live(Config): CSV_UPLOAD_BUCKET_NAME = 'live-notifications-csv-upload' LOGO_UPLOAD_BUCKET_NAME = 'public-logos-production' NOTIFY_ENVIRONMENT = 'live' + CHECK_PROXY_HEADER = False class CloudFoundryConfig(Config): diff --git a/requirements.txt b/requirements.txt index 941f5c1a5..f73536619 100644 --- a/requirements.txt +++ b/requirements.txt @@ -21,4 +21,4 @@ notifications-python-client==4.6.0 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@22.0.0#egg=notifications-utils==22.0.0 +git+https://github.com/alphagov/notifications-utils.git@22.1.0#egg=notifications-utils==22.1.0 From 1c78b938b4488ed0cb72b7d5b94d9ac7789c488b Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Thu, 16 Nov 2017 16:33:50 +0000 Subject: [PATCH 10/14] Fix tests --- tests/app/main/test_choose_time_form.py | 6 +++--- tests/app/test_cloudfoundry_config.py | 4 ++++ tests/conftest.py | 2 +- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/app/main/test_choose_time_form.py b/tests/app/main/test_choose_time_form.py index 77e9449f0..fbe1a5330 100644 --- a/tests/app/main/test_choose_time_form.py +++ b/tests/app/main/test_choose_time_form.py @@ -5,7 +5,7 @@ from freezegun import freeze_time @freeze_time("2016-01-01 11:09:00.061258") -def test_form_contains_next_24h(): +def test_form_contains_next_24h(app_): choices = ChooseTimeForm().scheduled_for.choices @@ -34,12 +34,12 @@ def test_form_contains_next_24h(): @freeze_time("2016-01-01 11:09:00.061258") -def test_form_defaults_to_now(client): +def test_form_defaults_to_now(app_): assert ChooseTimeForm().scheduled_for.data == '' @freeze_time("2016-01-01 11:09:00.061258") -def test_form_contains_next_three_days(): +def test_form_contains_next_three_days(app_): assert ChooseTimeForm().scheduled_for.categories == [ 'Later today', 'Tomorrow', 'Sunday', 'Monday' ] diff --git a/tests/app/test_cloudfoundry_config.py b/tests/app/test_cloudfoundry_config.py index 81852d7bf..22b8f93c0 100644 --- a/tests/app/test_cloudfoundry_config.py +++ b/tests/app/test_cloudfoundry_config.py @@ -16,6 +16,8 @@ def notify_config(): 'admin_client_secret': 'admin client secret', 'secret_key': 'secret key', 'dangerous_salt': 'dangerous salt', + 'route_secret_key_1': 'key 1', + 'route_secret_key_2': 'key 2', } } @@ -118,6 +120,8 @@ def test_notify_config(): assert os.environ['ADMIN_CLIENT_SECRET'] == 'admin client secret' assert os.environ['SECRET_KEY'] == 'secret key' assert os.environ['DANGEROUS_SALT'] == 'dangerous salt' + assert os.environ['ROUTE_SECRET_KEY_1'] == 'key 1' + assert os.environ['ROUTE_SECRET_KEY_2'] == 'key 2' @pytest.mark.usefixtures('os_environ', 'cloudfoundry_environ') diff --git a/tests/conftest.py b/tests/conftest.py index 9332ba77d..a7364b646 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -32,7 +32,7 @@ from notifications_utils.url_safe_token import generate_token import json -@pytest.fixture(scope='session') +@pytest.fixture def app_(request): app = Flask('app') create_app(app) From 8786ce79f4d836348d95c5a8e4f9f4d7c4a95674 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Thu, 16 Nov 2017 16:34:10 +0000 Subject: [PATCH 11/14] Use parametrize --- tests/app/main/test_request_header.py | 35 +++++++++++---------------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/tests/app/main/test_request_header.py b/tests/app/main/test_request_header.py index 6111c6f26..e5dc6beaa 100644 --- a/tests/app/main/test_request_header.py +++ b/tests/app/main/test_request_header.py @@ -1,33 +1,26 @@ +import pytest + from tests.conftest import set_config_values -def test_route_correct_secret_key(app_, client): +@pytest.mark.parametrize('check_proxy_header,header_value,expected_code', [ + (True, 'key_1', 200), + (True, 'wrong_key', 403), + (False, 'wrong_key', 200), + (False, 'key_1', 200), +]) +def test_route_correct_secret_key(app_, check_proxy_header, header_value, expected_code): with set_config_values(app_, { 'ROUTE_SECRET_KEY_1': 'key_1', 'ROUTE_SECRET_KEY_2': '', - 'DEBUG': False, + 'CHECK_PROXY_HEADER': check_proxy_header, }): + client = app_.test_client() response = client.get( - path='/_status', + path='/_status?elb=True', headers=[ - ('X-Custom-forwarder', 'key_1'), + ('X-Custom-forwarder', header_value), ] ) - assert response.status_code == 200 - - -def test_route_incorrect_secret_key(app_, client): - with set_config_values(app_, { - 'ROUTE_SECRET_KEY_1': 'key_1', - 'ROUTE_SECRET_KEY_2': '', - 'DEBUG': False, - }): - - response = client.get( - path='/_status', - headers=[ - ('X-Custom-forwarder', 'wrong_key'), - ] - ) - assert response.status_code == 403 + assert response.status_code == expected_code From ac3cf15322fd8bf16d1c73e02b60edd82d070eb7 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Thu, 16 Nov 2017 16:35:24 +0000 Subject: [PATCH 12/14] Log failed api healthchecks --- app/status/views/healthcheck.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/status/views/healthcheck.py b/app/status/views/healthcheck.py index 4d5515d57..2eedad519 100644 --- a/app/status/views/healthcheck.py +++ b/app/status/views/healthcheck.py @@ -1,4 +1,4 @@ -from flask import jsonify, request +from flask import jsonify, request, current_app from app import (version, status_api_client) from app.status import status from notifications_python_client.errors import HTTPError @@ -12,6 +12,7 @@ def show_status(): try: api_status = status_api_client.get_status() except HTTPError as e: + current_app.logger.exception("API failed to respond") return jsonify(status="error", message=str(e.message)), 500 return jsonify( status="ok", From bde734dc751cbfd59b40c1c2f0d60229795fae4a Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Thu, 16 Nov 2017 17:02:38 +0000 Subject: [PATCH 13/14] Use test_client() as context manager --- tests/app/main/test_request_header.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/app/main/test_request_header.py b/tests/app/main/test_request_header.py index e5dc6beaa..643d33c56 100644 --- a/tests/app/main/test_request_header.py +++ b/tests/app/main/test_request_header.py @@ -16,11 +16,11 @@ def test_route_correct_secret_key(app_, check_proxy_header, header_value, expect 'CHECK_PROXY_HEADER': check_proxy_header, }): - client = app_.test_client() - response = client.get( - path='/_status?elb=True', - headers=[ - ('X-Custom-forwarder', header_value), - ] - ) + with app_.test_client() as client: + response = client.get( + path='/_status?elb=True', + headers=[ + ('X-Custom-forwarder', header_value), + ] + ) assert response.status_code == expected_code From d74b104bfe99ebc26f2f90829684d63ab6159860 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Fri, 17 Nov 2017 14:43:57 +0000 Subject: [PATCH 14/14] Bump utils version --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index f73536619..a0ebd5053 100644 --- a/requirements.txt +++ b/requirements.txt @@ -21,4 +21,4 @@ notifications-python-client==4.6.0 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@22.1.0#egg=notifications-utils==22.1.0 +git+https://github.com/alphagov/notifications-utils.git@23.0.1#egg=notifications-utils==23.0.1