From 255ce158b78f16fb4536491d493992691e53466e Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 9 Dec 2016 15:44:58 +0000 Subject: [PATCH 1/3] block inactive services from making stateful changes in the NotifyAdminAPIClient, which all api traffic goes through, return 403 for any stateful requests (post, put and delete), if the following criteria have been met: * a current_service is set (this prevents checks being carried out on non-service related updates, eg editing user details) * the service is not active * the current user is not a platform admin so platform admins can still update anything. Note: Without any specific error handling, the user will see a generic 403 page. This is fine, probably - it's a relatively niche case that you'll be editing a service you can't get to anyway --- app/notify_client/__init__.py | 28 ++++++- .../test_notify_admin_api_client.py | 74 +++++++++++++++++++ 2 files changed, 101 insertions(+), 1 deletion(-) create mode 100644 tests/app/notify_client/test_notify_admin_api_client.py diff --git a/app/notify_client/__init__.py b/app/notify_client/__init__.py index 609798de5..b4d1c02e0 100644 --- a/app/notify_client/__init__.py +++ b/app/notify_client/__init__.py @@ -1,5 +1,5 @@ from flask_login import current_user -from flask import has_request_context, request +from flask import has_request_context, request, abort from notifications_python_client.base import BaseAPIClient from notifications_python_client.version import __version__ @@ -26,3 +26,29 @@ class NotifyAdminAPIClient(BaseAPIClient): return headers headers['NotifyRequestID'] = request.request_id return headers + + def check_inactive_service(self): + # this file is imported in app/__init__.py before current_service is initialised, so need to import later + # to prevent cyclical imports + from app import current_service + + # if the current service is inactive and the user isn't a platform admin, we should block them from making any + # stateful modifications to that service + print('\n\n\n') + print(current_service) + print(current_user) + if current_service and not current_service['active'] and not current_user.platform_admin: + abort(403) + + + def post(self, *args, **kwargs): + self.check_inactive_service() + return super().post(*args, **kwargs) + + def put(self, *args, **kwargs): + self.check_inactive_service() + return super().put(*args, **kwargs) + + def delete(self, *args, **kwargs): + self.check_inactive_service() + return super().delete(*args, **kwargs) diff --git a/tests/app/notify_client/test_notify_admin_api_client.py b/tests/app/notify_client/test_notify_admin_api_client.py new file mode 100644 index 000000000..c74c954bf --- /dev/null +++ b/tests/app/notify_client/test_notify_admin_api_client.py @@ -0,0 +1,74 @@ +import uuid +import pytest +from unittest.mock import patch + +import werkzeug + +from tests import service_json +from tests.conftest import api_user_active, platform_admin_user +from app.notify_client import NotifyAdminAPIClient + + +@pytest.mark.parametrize('method', [ + 'put', + 'post', + 'delete' +]) +@pytest.mark.parametrize('user', [ + api_user_active(str(uuid.uuid4())), + platform_admin_user(str(uuid.uuid4())) +], ids=['api_user', 'platform_admin']) +@pytest.mark.parametrize('service', [ + service_json(active=True), + None +], ids=['active_service', 'no_service']) +def test_active_service_can_be_modified(app_, method, user, service): + api_client = NotifyAdminAPIClient('api_key', 'base_url', 'service_id') + + with app_.test_request_context() as request_context, app_.test_client() as client: + client.login(user) + request_context.service = service + + with patch.object(api_client, 'request') as request: + ret = getattr(api_client, method)('url', 'data') + + assert request.called + assert ret == request.return_value + + +@pytest.mark.parametrize('method', [ + 'put', + 'post', + 'delete' +]) +def test_inactive_service_cannot_be_modified_by_normal_user(app_, api_user_active, method): + api_client = NotifyAdminAPIClient('api_key', 'base_url', 'service_id') + + with app_.test_request_context() as request_context, app_.test_client() as client: + client.login(api_user_active) + request_context.service = service_json(active=False) + + with patch.object(api_client, 'request') as request: + with pytest.raises(werkzeug.exceptions.Forbidden): + getattr(api_client, method)('url', 'data') + + assert not request.called + + +@pytest.mark.parametrize('method', [ + 'put', + 'post', + 'delete' +]) +def test_inactive_service_can_be_modified_by_platform_admin(app_, platform_admin_user, method): + api_client = NotifyAdminAPIClient('api_key', 'base_url', 'service_id') + + with app_.test_request_context() as request_context, app_.test_client() as client: + client.login(platform_admin_user) + request_context.service = service_json(active=False) + + with patch.object(api_client, 'request') as request: + ret = getattr(api_client, method)('url', 'data') + + assert request.called + assert ret == request.return_value From d56b7b9a583ae3b95a7b12ebc384e073c2e2fcd9 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 9 Dec 2016 15:44:58 +0000 Subject: [PATCH 2/3] block inactive services from making stateful changes in the NotifyAdminAPIClient, which all api traffic goes through, return 403 for any stateful requests (post, put and delete), if the following criteria have been met: * a current_service is set (this prevents checks being carried out on non-service related updates, eg editing user details) * the service is not active * the current user is not a platform admin so platform admins can still update anything. Note: Without any specific error handling, the user will see a generic 403 page. This is fine, probably - it's a relatively niche case that you'll be editing a service you can't get to anyway --- app/notify_client/__init__.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/notify_client/__init__.py b/app/notify_client/__init__.py index b4d1c02e0..fb0cb7f19 100644 --- a/app/notify_client/__init__.py +++ b/app/notify_client/__init__.py @@ -34,13 +34,9 @@ class NotifyAdminAPIClient(BaseAPIClient): # if the current service is inactive and the user isn't a platform admin, we should block them from making any # stateful modifications to that service - print('\n\n\n') - print(current_service) - print(current_user) if current_service and not current_service['active'] and not current_user.platform_admin: abort(403) - def post(self, *args, **kwargs): self.check_inactive_service() return super().post(*args, **kwargs) From 2419f0d8a9c42a76a43c6ec6afe5a47e55b74e50 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 9 Dec 2016 15:54:13 +0000 Subject: [PATCH 3/3] write tests for @minglis 's code :slightly_frowning_face: --- .../test_notify_admin_api_client.py | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/app/notify_client/test_notify_admin_api_client.py b/tests/app/notify_client/test_notify_admin_api_client.py index c74c954bf..2e0fe9ef5 100644 --- a/tests/app/notify_client/test_notify_admin_api_client.py +++ b/tests/app/notify_client/test_notify_admin_api_client.py @@ -72,3 +72,25 @@ def test_inactive_service_can_be_modified_by_platform_admin(app_, platform_admin assert request.called assert ret == request.return_value + + +def test_generate_headers_sets_standard_headers(): + api_client = NotifyAdminAPIClient('api_key', 'base_url', 'service_id') + + # with patch('app.notify_client.has_request_context', return_value=False): + headers = api_client.generate_headers('api_token') + + assert set(headers.keys()) == {'Authorization', 'Content-type', 'User-agent'} + assert headers['Authorization'] == 'Bearer api_token' + assert headers['Content-type'] == 'application/json' + assert headers['User-agent'].startswith('NOTIFY-API-PYTHON-CLIENT') + + +def test_generate_headers_sets_request_id_if_in_request_context(app_): + api_client = NotifyAdminAPIClient('api_key', 'base_url', 'service_id') + + with app_.test_request_context() as request_context: + headers = api_client.generate_headers('api_token') + + assert set(headers.keys()) == {'Authorization', 'Content-type', 'User-agent', 'NotifyRequestID'} + assert headers['NotifyRequestID'] == request_context.request.request_id