From 91f2da8b68ddd8663d625751e16bc238e23b1a63 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 27 Jun 2019 14:23:03 +0100 Subject: [PATCH] Ensure all service route have permission decorators MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We accidentally miss these sometimes. This code adds a test which inspects the code to automatically check that any function which: - handles a route - accepts a service_id For each function it checks that each of these routes have the permissions decorator we’d expect. Most of the introspection/AST code is adapted from here: https://mvdwoord.github.io/exploration/2017/08/18/ast_explore.html --- app/main/views/agreement.py | 3 + app/main/views/dashboard.py | 2 + app/main/views/jobs.py | 3 + app/main/views/manage_users.py | 1 + app/main/views/notifications.py | 1 + app/main/views/platform_admin.py | 2 + tests/app/main/test_permissions.py | 72 +++++++++++++++++++++ tests/app/main/views/test_platform_admin.py | 6 +- 8 files changed, 87 insertions(+), 3 deletions(-) diff --git a/app/main/views/agreement.py b/app/main/views/agreement.py index 96008c5ac..2a1989d31 100644 --- a/app/main/views/agreement.py +++ b/app/main/views/agreement.py @@ -23,6 +23,7 @@ def agreement(): @main.route('/services//agreement') @login_required +@user_has_permissions('manage_service') def service_agreement(service_id): return render_template( 'views/agreement/service-{}.html'.format(current_service.organisation.as_jinja_template), @@ -41,6 +42,7 @@ def service_download_agreement(service_id): @main.route('/services//agreement/accept', methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_service') def service_accept_agreement(service_id): if not current_service.organisation: @@ -64,6 +66,7 @@ def service_accept_agreement(service_id): @main.route('/services//agreement/confirm', methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_service') def service_confirm_agreement(service_id): if ( diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 346d15fc2..923a4fa6f 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -43,6 +43,7 @@ from app.utils import ( # to view history @main.route("/services//history") @login_required +@user_has_permissions() def temp_service_history(service_id): data = service_api_client.get_service_history(service_id)['data'] return render_template('views/temp-history.html', @@ -78,6 +79,7 @@ def service_dashboard(service_id): @main.route("/services//dashboard.json") +@login_required @user_has_permissions('view_activity') def service_dashboard_updates(service_id): return jsonify(**get_dashboard_partials(service_id)) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index c6fa2c76d..46a6dfddf 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -163,6 +163,7 @@ def cancel_job(service_id, job_id): @main.route("/services//jobs/.json") @user_has_permissions() +@login_required def view_job_updates(service_id, job_id): job = job_api_client.get_job(service_id, job_id)['data'] @@ -205,6 +206,7 @@ def view_notifications(service_id, message_type=None): @main.route('/services//notifications.json', methods=['GET', 'POST']) @main.route('/services//notifications/.json', methods=['GET', 'POST']) @user_has_permissions() +@login_required def get_notifications_as_json(service_id, message_type=None): return jsonify(get_notifications( service_id, message_type, status_override=request.args.get('status') @@ -213,6 +215,7 @@ def get_notifications_as_json(service_id, message_type=None): @main.route('/services//notifications/.csv', endpoint="view_notifications_csv") @user_has_permissions() +@login_required def get_notifications(service_id, message_type, status_override=None): # TODO get the api to return count of pages as well. page = get_page_from_request() diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 58cc4846a..ff462b82f 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -269,6 +269,7 @@ def confirm_edit_user_mobile_number(service_id, user_id): @main.route("/services//cancel-invited-user/", methods=['GET']) @user_has_permissions('manage_service') +@login_required def cancel_invited_user(service_id, invited_user_id): current_service.cancel_invite(invited_user_id) diff --git a/app/main/views/notifications.py b/app/main/views/notifications.py index 4341e53aa..8dc2db67d 100644 --- a/app/main/views/notifications.py +++ b/app/main/views/notifications.py @@ -220,6 +220,7 @@ def view_letter_notification_as_preview(service_id, notification_id, filetype): @main.route("/services//notification/.json") @user_has_permissions('view_activity', 'send_messages') +@login_required def view_notification_updates(service_id, notification_id): return jsonify(**get_single_notification_partials( notification_api_client.get_notification(service_id, notification_id) diff --git a/app/main/views/platform_admin.py b/app/main/views/platform_admin.py index a5f335bc2..d5809b3ab 100644 --- a/app/main/views/platform_admin.py +++ b/app/main/views/platform_admin.py @@ -33,6 +33,7 @@ from app.utils import ( generate_next_dict, generate_previous_dict, get_page_from_request, + user_has_permissions, user_is_platform_admin, ) @@ -339,6 +340,7 @@ def platform_admin_letter_validation_preview(): @main.route("/services//letter-validation-preview", methods=["GET", "POST"]) @login_required +@user_has_permissions() def service_letter_validation_preview(service_id): return letter_validation_preview(from_platform_admin=False) diff --git a/tests/app/main/test_permissions.py b/tests/app/main/test_permissions.py index fca24395b..aa320e5e6 100644 --- a/tests/app/main/test_permissions.py +++ b/tests/app/main/test_permissions.py @@ -1,3 +1,6 @@ +import ast +import inspect + import pytest from flask import request from werkzeug.exceptions import Forbidden, Unauthorized @@ -369,3 +372,72 @@ def test_service_navigation_for_org_user( 'Team members', 'Usage', ] + + +def get_name_of_decorator_from_ast_node(node): + if isinstance(node, ast.Attribute): + return '{}.{}'.format( + get_name_of_decorator_from_ast_node(node.value), + node.attr, + ) + if isinstance(node, ast.Name): + return str(node.id) + if isinstance(node, ast.Call) and isinstance(node.func, ast.Name): + return get_name_of_decorator_from_ast_node(node.func) + return node.func.attr + + +def get_decorators_for_function(function): + for node in ast.walk(ast.parse(inspect.getsource(function))): + if isinstance(node, ast.FunctionDef): + for decorator in node.decorator_list: + yield get_name_of_decorator_from_ast_node(decorator) + + +SERVICE_ID_ARGUMENT = 'service_id' +ORGANISATION_ID_ARGUMENT = 'org_id' + + +def get_routes_and_decorators_with_argument(argument_name): + import app.main.views as views + for module_name, module in inspect.getmembers(views): + for function_name, function in inspect.getmembers(module): + if ( + inspect.isfunction(function) and + argument_name in inspect.signature(function).parameters.keys() + ): + decorators = list(get_decorators_for_function(function)) + if 'route' in decorators: + yield '{}.{}'.format(module_name, function_name), decorators + + +def test_code_to_extract_decorators_works_with_known_examples(): + assert ( + 'templates.choose_template', + ['route', 'route', 'route', 'route', 'login_required', 'user_has_permissions'], + ) in list( + get_routes_and_decorators_with_argument(SERVICE_ID_ARGUMENT) + ) + assert ( + 'organisations.organisation_dashboard', + ['route', 'login_required', 'user_has_permissions'], + ) in list( + get_routes_and_decorators_with_argument(ORGANISATION_ID_ARGUMENT) + ) + + +def test_service_routes_have_decorator(): + + for endpoint, decorators in ( + list(get_routes_and_decorators_with_argument(SERVICE_ID_ARGUMENT)) + + list(get_routes_and_decorators_with_argument(ORGANISATION_ID_ARGUMENT)) + ): + if 'user_is_platform_admin' in decorators: + required_decorators = {'login_required'} + else: + required_decorators = {'login_required', 'user_has_permissions'} + + for required_decorator in required_decorators: + assert required_decorator in decorators, ( + 'Missing {} decorator on app/main/views/{}.py::{}' + ).format(required_decorator, *endpoint.split('.')) diff --git a/tests/app/main/views/test_platform_admin.py b/tests/app/main/views/test_platform_admin.py index ac05a3e7e..8b88ba547 100644 --- a/tests/app/main/views/test_platform_admin.py +++ b/tests/app/main/views/test_platform_admin.py @@ -19,7 +19,7 @@ from app.main.views.platform_admin import ( sum_service_usage, ) from tests import service_json -from tests.conftest import mock_get_user, normalize_spaces +from tests.conftest import SERVICE_ONE_ID, mock_get_user, normalize_spaces @pytest.mark.parametrize('endpoint', [ @@ -767,7 +767,7 @@ def test_service_letter_validation_preview_renders_correctly( mock_has_no_jobs ): - page = client_request.get('main.service_letter_validation_preview', service_id="service_1") + page = client_request.get('main.service_letter_validation_preview', service_id=SERVICE_ONE_ID) assert page.find('h1').text.strip() == "Letter validation preview" assert page.find_all('input', class_='file-upload-field') @@ -780,7 +780,7 @@ def test_service_letter_validation_preview_returns_400_if_file_is_too_big( ): with open('tests/test_pdf_files/big.pdf', 'rb') as file: - page = client_request.post('main.service_letter_validation_preview', service_id="service_1", + page = client_request.post('main.service_letter_validation_preview', service_id=SERVICE_ONE_ID, _data=dict( pdf_file=file, ),