Merge pull request #1809 from alphagov/revisit-proxy-header-check

Revisit proxy header check

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()

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)
 

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

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]