From 463f1eefaf5aa6c9a1febb9235499a9b628e334b Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Tue, 27 Mar 2018 17:37:09 +0100 Subject: [PATCH 1/2] 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 2/2] 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]