darkhelm/notifications-admin
1
0
Fork 0
mirror of https://github.com/GSA/notifications-admin.git synced 2026-08-20 14:29:51 -04:00
Code Issues Packages Projects Releases Wiki Activity
Files
9d4095074db040aa97cf9e12f3ecf2f499d89e3b
notifications-admin/tests/app/main/test_permissions.py

320 lines
9.9 KiB
Python
Raw Normal View History

Ensure all service route have permission decorators 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
2019-06-27 14:23:03 +01:00
import ast
import inspect
Require IDs to be UUIDs in URLS We mostly rely on the API returning a 404 to generate 404s for trying to get things with non-UUID IDs. This is fine, except our tests often mock these API calls. So it could look like everything is working fine, except the thing your passing in might never be a valid UUID, and thus would 404 in a non-test environment. So this commit: 1. uses the `uuid` URL converter everywhere there’s something that looks like an ID in a URL parameter 2. adds a test which automates checking for 1.
2019-11-04 11:07:55 +00:00
import re
Ensure all service route have permission decorators 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
2019-06-27 14:23:03 +01:00
User permissions added with test.
2016-02-19 16:38:04 +00:00
import pytest
Relocate unit tests for user permission util Previously these were lumped together with integration-level tests for specific endpoints, which test the decorator was applied to the endpoint in question.
2021-06-09 15:38:28 +01:00
from flask import current_app
Enforce order and style of imports Done using isort[1], with the following command: ``` isort -rc ./app ./tests ``` Adds linting to the `run_tests.sh` script to stop badly-sorted imports getting re-introduced. Chosen style is ‘Vertical Hanging Indent’ with trailing commas, because I think it gives the cleanest diffs, eg: ``` from third_party import ( lib1, lib2, lib3, lib4, ) ``` 1. https://pypi.python.org/pypi/isort
2018-02-20 11:22:17 +00:00
Make user API client return JSON, not a model The data flow of other bits of our application looks like this: ``` API (returns JSON) ⬇ API client (returns a built in type, usually `dict`) ⬇ Model (returns an instance, eg of type `Service`) ⬇ View (returns HTML) ``` The user API client was architected weirdly, in that it returned a model directly, like this: ``` API (returns JSON) ⬇ API client (returns a model, of type `User`, `InvitedUser`, etc) ⬇ View (returns HTML) ``` This mixing of different layers of the application is bad because it makes it hard to write model code that doesn’t have circular dependencies. As our application gets more complicated we will be relying more on models to manage this complexity, so we should make it easy, not hard to write them. It also means that most of our mocking was of the User model, not just the underlying JSON. So it would have been easy to introduce subtle bugs to the user model, because it wasn’t being comprehensively tested. A lot of the changed lines of code in this commit mean changing the tests to mock only the JSON, which means that the model layer gets implicitly tested. For those reasons this commit changes the user API client to return JSON, not an instance of `User` or other models.
2019-05-23 15:27:35 +01:00
from app.models.roles_and_permissions import (
map roles and db permissions in the db, we have several rows for single permissions - we separate `send_messages` into `send_texts`, `send_emails` and `send_letters`, and also `manage_service` into `manage_users` and `manage_settings`. But on the front end we don't do anything with this distinction. It's unhelpful for us to have to think about permissions as groups of things when we can never split them up at all. So we should combine them. This commit makes sure: * when user models are read (from JSON direct from the API), we should transform them from db permissions into roles. * when permissions are persisted (editing permissions, and creating invites), we should send db permissions to the API. All other interaction with permissions (should just be the endpoint decorator and checks in html templates generally) should use admin roles.
2018-02-28 15:37:49 +00:00
translate_permissions_from_admin_roles_to_db,
translate_permissions_from_db_to_admin_roles,
)
Allow org team members to see team and usage Organisation team members will be ultimately interested in the detailed usage of each service, but shouldn't necessarily have access to the personal data of that services users. So we should allow these organisation team members to navigate to live services usage page from the organisation page. They may need to contact the team so they should also be able to view the team members page. So they'll then see just usage and team members pages. If they are actually a team member of the service they're viewing, then they'll see the full range of options as usual. This commit implement the above by adding an extra flag to the `user.has_permissions` decorator which allows certain pages to be marked as viewable by an organisation user. The default (for all other existing pages) is that organisation users don’t have permission.
2019-06-19 16:39:13 +01:00
from tests import service_json
from tests.conftest import (
ORGANISATION_ID,
ORGANISATION_TWO_ID,
SERVICE_ONE_ID,
SERVICE_TWO_ID,
)
User permissions added with test.
2016-02-19 16:38:04 +00:00
map roles and db permissions in the db, we have several rows for single permissions - we separate `send_messages` into `send_texts`, `send_emails` and `send_letters`, and also `manage_service` into `manage_users` and `manage_settings`. But on the front end we don't do anything with this distinction. It's unhelpful for us to have to think about permissions as groups of things when we can never split them up at all. So we should combine them. This commit makes sure: * when user models are read (from JSON direct from the API), we should transform them from db permissions into roles. * when permissions are persisted (editing permissions, and creating invites), we should send db permissions to the API. All other interaction with permissions (should just be the endpoint decorator and checks in html templates generally) should use admin roles.
2018-02-28 15:37:49 +00:00
def test_translate_permissions_from_db_to_admin_roles():
db_perms = ['send_texts', 'send_emails', 'send_letters', 'manage_templates', 'some_unknown_permission']
roles = translate_permissions_from_db_to_admin_roles(db_perms)
assert roles == {'send_messages', 'manage_templates', 'some_unknown_permission'}
def test_translate_permissions_from_admin_roles_to_db():
roles = ['send_messages', 'manage_templates', 'some_unknown_permission']
db_perms = translate_permissions_from_admin_roles_to_db(roles)
assert db_perms == {'send_texts', 'send_emails', 'send_letters', 'manage_templates', 'some_unknown_permission'}
Allow org team members to see team and usage Organisation team members will be ultimately interested in the detailed usage of each service, but shouldn't necessarily have access to the personal data of that services users. So we should allow these organisation team members to navigate to live services usage page from the organisation page. They may need to contact the team so they should also be able to view the team members page. So they'll then see just usage and team members pages. If they are actually a team member of the service they're viewing, then they'll see the full range of options as usual. This commit implement the above by adding an extra flag to the `user.has_permissions` decorator which allows certain pages to be marked as viewable by an organisation user. The default (for all other existing pages) is that organisation users don’t have permission.
2019-06-19 16:39:13 +01:00
@pytest.mark.parametrize(
'user_services, user_organisations, expected_status, organisation_checked',
(
([SERVICE_ONE_ID], [], 200, False),
([SERVICE_ONE_ID, SERVICE_TWO_ID], [], 200, False),
([], [ORGANISATION_ID], 200, True),
([SERVICE_ONE_ID], [ORGANISATION_ID], 200, False),
([], [], 403, True),
([SERVICE_TWO_ID], [], 403, True),
([SERVICE_TWO_ID], [ORGANISATION_ID], 200, True),
([SERVICE_ONE_ID, SERVICE_TWO_ID], [ORGANISATION_ID], 200, False),
([], [ORGANISATION_TWO_ID], 403, True),
([], [ORGANISATION_ID, ORGANISATION_TWO_ID], 200, True),
)
)
def test_services_pages_that_org_users_are_allowed_to_see(
client_request,
mocker,
api_user_active,
mock_get_usage,
mock_get_billable_units,
mock_get_free_sms_fragment_limit,
mock_get_service,
mock_get_invites_for_service,
mock_get_users_by_service,
mock_get_template_folders,
Cache organisation name in Redis A lot of pages in the admin app are now generated entirely from Redis, without touching the API. The one remaining API call that a lot of pages make, when the user is platform admin or a member of an organisation, is to get the name of the current service’s organisation. This commit adds some code to start caching that as well, which should speed up page load times for when we’re clicking around the admin app (it’s typically 100ms just to get the organisation, and more than that when the API is under load). This means changing the service model to get the organisation from the API by ID, not by service ID. Otherwise it would be very hard to clear the cache if the name of the organisation ever changed. We can’t cache the whole organisation because it has a `count_of_live_services` field which can change at any time, without an update being made.
2020-03-20 08:42:33 +00:00
mock_get_organisation,
Allow org team members to see team and usage Organisation team members will be ultimately interested in the detailed usage of each service, but shouldn't necessarily have access to the personal data of that services users. So we should allow these organisation team members to navigate to live services usage page from the organisation page. They may need to contact the team so they should also be able to view the team members page. So they'll then see just usage and team members pages. If they are actually a team member of the service they're viewing, then they'll see the full range of options as usual. This commit implement the above by adding an extra flag to the `user.has_permissions` decorator which allows certain pages to be marked as viewable by an organisation user. The default (for all other existing pages) is that organisation users don’t have permission.
2019-06-19 16:39:13 +01:00
mock_has_jobs,
user_services,
user_organisations,
expected_status,
organisation_checked,
):
api_user_active['services'] = user_services
api_user_active['organisations'] = user_organisations
api_user_active['permissions'] = {
service_id: ['manage_service']
for service_id in user_services
}
service = service_json(
name='SERVICE WITH ORG',
id_=SERVICE_ONE_ID,
users=[api_user_active['id']],
organisation_id=ORGANISATION_ID,
)
mock_get_service = mocker.patch(
'app.notify_client.service_api_client.service_api_client.get_service',
return_value={'data': service}
)
client_request.login(
api_user_active,
service=service if SERVICE_ONE_ID in user_services else None,
)
endpoints = (
'main.usage',
'main.manage_users',
)
for endpoint in endpoints:
client_request.get(
endpoint,
service_id=SERVICE_ONE_ID,
_expected_status=expected_status,
)
assert mock_get_service.called is organisation_checked
def test_service_navigation_for_org_user(
client_request,
mocker,
api_user_active,
mock_get_usage,
mock_get_billable_units,
mock_get_free_sms_fragment_limit,
mock_get_service,
mock_get_invites_for_service,
mock_get_users_by_service,
Cache organisation name in Redis A lot of pages in the admin app are now generated entirely from Redis, without touching the API. The one remaining API call that a lot of pages make, when the user is platform admin or a member of an organisation, is to get the name of the current service’s organisation. This commit adds some code to start caching that as well, which should speed up page load times for when we’re clicking around the admin app (it’s typically 100ms just to get the organisation, and more than that when the API is under load). This means changing the service model to get the organisation from the API by ID, not by service ID. Otherwise it would be very hard to clear the cache if the name of the organisation ever changed. We can’t cache the whole organisation because it has a `count_of_live_services` field which can change at any time, without an update being made.
2020-03-20 08:42:33 +00:00
mock_get_organisation,
Allow org team members to see team and usage Organisation team members will be ultimately interested in the detailed usage of each service, but shouldn't necessarily have access to the personal data of that services users. So we should allow these organisation team members to navigate to live services usage page from the organisation page. They may need to contact the team so they should also be able to view the team members page. So they'll then see just usage and team members pages. If they are actually a team member of the service they're viewing, then they'll see the full range of options as usual. This commit implement the above by adding an extra flag to the `user.has_permissions` decorator which allows certain pages to be marked as viewable by an organisation user. The default (for all other existing pages) is that organisation users don’t have permission.
2019-06-19 16:39:13 +01:00
):
api_user_active['services'] = []
api_user_active['organisations'] = [ORGANISATION_ID]
service = service_json(
id_=SERVICE_ONE_ID,
organisation_id=ORGANISATION_ID,
)
mocker.patch(
'app.service_api_client.get_service',
return_value={'data': service}
)
client_request.login(api_user_active, service=service)
page = client_request.get(
'main.usage',
service_id=SERVICE_ONE_ID,
)
assert [
item.text.strip() for item in page.select('nav.navigation a')
] == [
'Usage',
Swap order of usage and team members for org users Org users, when looking at the page for their org, see: > Usage > Team members When they click into a service it switches to: > Team members > Usage This is jarring. It should stay consistent. I think it that _Usage_ then _Team members_ is the natural way of ordering the navigation at the organisation level, so let’s follow that through to the service level. This does mean that if someone is a member of both an organisation and a service that the nav will jump (because it’ll switch to the existing, service-level order of _Team members_ then _Usage_) but it’s going to jump anyway because you get all the extra navigation items when you’re a member of a service.
2020-02-03 11:35:29 +00:00
'Team members',
Allow org team members to see team and usage Organisation team members will be ultimately interested in the detailed usage of each service, but shouldn't necessarily have access to the personal data of that services users. So we should allow these organisation team members to navigate to live services usage page from the organisation page. They may need to contact the team so they should also be able to view the team members page. So they'll then see just usage and team members pages. If they are actually a team member of the service they're viewing, then they'll see the full range of options as usual. This commit implement the above by adding an extra flag to the `user.has_permissions` decorator which allows certain pages to be marked as viewable by an organisation user. The default (for all other existing pages) is that organisation users don’t have permission.
2019-06-19 16:39:13 +01:00
]
Ensure all service route have permission decorators 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
2019-06-27 14:23:03 +01:00
Make test more robust Now tests: - that the user doesn’t have the usage menu item when they’re not a member of the organisation (i.e. the counterfactual) - that the user still gets a 403 when they try to view the usage page if they’re not a member of the organisation
2020-02-11 11:08:35 +00:00
@pytest.mark.parametrize('user_organisations, expected_menu_items, expected_status', [
(
[],
(
'Templates',
'Sent messages',
Remove jobs from the dashboard We’ve done this already for services with the upload letters permission. And all services can upload letters now. But we’re still returning it in the JSON response we use to AJAX-ify the page. Since the jobs response can query stats for up to 50 jobs at a time this puts some load on the API/database. Hopefully this might drop that load a bit.
2020-03-16 12:08:38 +00:00
'Uploads',
Make test more robust Now tests: - that the user doesn’t have the usage menu item when they’re not a member of the organisation (i.e. the counterfactual) - that the user still gets a 403 when they try to view the usage page if they’re not a member of the organisation
2020-02-11 11:08:35 +00:00
'Team members',
),
403,
),
(
[ORGANISATION_ID],
(
'Templates',
'Sent messages',
Remove jobs from the dashboard We’ve done this already for services with the upload letters permission. And all services can upload letters now. But we’re still returning it in the JSON response we use to AJAX-ify the page. Since the jobs response can query stats for up to 50 jobs at a time this puts some load on the API/database. Hopefully this might drop that load a bit.
2020-03-16 12:08:38 +00:00
'Uploads',
Make test more robust Now tests: - that the user doesn’t have the usage menu item when they’re not a member of the organisation (i.e. the counterfactual) - that the user still gets a 403 when they try to view the usage page if they’re not a member of the organisation
2020-02-11 11:08:35 +00:00
'Team members',
'Usage',
),
200,
),
])
def test_service_user_without_manage_service_permission_can_see_usage_page_when_org_user(
Add test for org user who is also a service user Org users should always be able to see the usage page, even if they don’t have the `manage_service` permission for the service.
2020-02-03 16:05:46 +00:00
client_request,
mocker,
active_caseworking_user,
mock_has_no_jobs,
mock_get_usage,
mock_get_billable_units,
mock_get_free_sms_fragment_limit,
mock_get_service,
mock_get_invites_for_service,
mock_get_users_by_service,
Cache organisation name in Redis A lot of pages in the admin app are now generated entirely from Redis, without touching the API. The one remaining API call that a lot of pages make, when the user is platform admin or a member of an organisation, is to get the name of the current service’s organisation. This commit adds some code to start caching that as well, which should speed up page load times for when we’re clicking around the admin app (it’s typically 100ms just to get the organisation, and more than that when the API is under load). This means changing the service model to get the organisation from the API by ID, not by service ID. Otherwise it would be very hard to clear the cache if the name of the organisation ever changed. We can’t cache the whole organisation because it has a `count_of_live_services` field which can change at any time, without an update being made.
2020-03-20 08:42:33 +00:00
mock_get_organisation,
Make test more robust Now tests: - that the user doesn’t have the usage menu item when they’re not a member of the organisation (i.e. the counterfactual) - that the user still gets a 403 when they try to view the usage page if they’re not a member of the organisation
2020-02-11 11:08:35 +00:00
mock_get_service_templates,
mock_get_template_folders,
Allow users of the API to search templates by ID For someone who has retrieved a template ID from their system the only way to find it in Notify is: - hack the URL - click through every template, visually inspecting the ID shown on the page until you find the right one Neither of these is ideal. This commit adds searching by ID, for those services who have an API integration. This means we don’t need to confuse teams who aren’t using the API by talking about IDs. This is similar to how we let these teams search for notifications by reference[1] 1. https://github.com/alphagov/notifications-admin/pull/3223/files
2021-04-13 13:59:51 +01:00
mock_get_api_keys,
Make test more robust Now tests: - that the user doesn’t have the usage menu item when they’re not a member of the organisation (i.e. the counterfactual) - that the user still gets a 403 when they try to view the usage page if they’re not a member of the organisation
2020-02-11 11:08:35 +00:00
user_organisations,
expected_status,
expected_menu_items,
Add test for org user who is also a service user Org users should always be able to see the usage page, even if they don’t have the `manage_service` permission for the service.
2020-02-03 16:05:46 +00:00
):
active_caseworking_user['services'] = [SERVICE_ONE_ID]
Make test more robust Now tests: - that the user doesn’t have the usage menu item when they’re not a member of the organisation (i.e. the counterfactual) - that the user still gets a 403 when they try to view the usage page if they’re not a member of the organisation
2020-02-11 11:08:35 +00:00
active_caseworking_user['organisations'] = user_organisations
Add test for org user who is also a service user Org users should always be able to see the usage page, even if they don’t have the `manage_service` permission for the service.
2020-02-03 16:05:46 +00:00
service = service_json(
id_=SERVICE_ONE_ID,
organisation_id=ORGANISATION_ID,
)
mocker.patch(
'app.service_api_client.get_service',
return_value={'data': service}
)
client_request.login(active_caseworking_user, service=service)
page = client_request.get(
Make test more robust Now tests: - that the user doesn’t have the usage menu item when they’re not a member of the organisation (i.e. the counterfactual) - that the user still gets a 403 when they try to view the usage page if they’re not a member of the organisation
2020-02-11 11:08:35 +00:00
'main.choose_template',
Add test for org user who is also a service user Org users should always be able to see the usage page, even if they don’t have the `manage_service` permission for the service.
2020-02-03 16:05:46 +00:00
service_id=SERVICE_ONE_ID,
)
Make test more robust Now tests: - that the user doesn’t have the usage menu item when they’re not a member of the organisation (i.e. the counterfactual) - that the user still gets a 403 when they try to view the usage page if they’re not a member of the organisation
2020-02-11 11:08:35 +00:00
assert tuple(
Add test for org user who is also a service user Org users should always be able to see the usage page, even if they don’t have the `manage_service` permission for the service.
2020-02-03 16:05:46 +00:00
item.text.strip() for item in page.select('nav.navigation a')
Make test more robust Now tests: - that the user doesn’t have the usage menu item when they’re not a member of the organisation (i.e. the counterfactual) - that the user still gets a 403 when they try to view the usage page if they’re not a member of the organisation
2020-02-11 11:08:35 +00:00
) == expected_menu_items
client_request.get(
'main.usage',
service_id=SERVICE_ONE_ID,
_expected_status=expected_status,
)
Add test for org user who is also a service user Org users should always be able to see the usage page, even if they don’t have the `manage_service` permission for the service.
2020-02-03 16:05:46 +00:00
Ensure all service route have permission decorators 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
2019-06-27 14:23:03 +01:00
def get_name_of_decorator_from_ast_node(node):
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)
Have permissions decorators check user signed in Rather than force us to write the decorators in a specific order let’s just have one decorator call the other. This should make fewer lines of code, and fewer annoying test failures. It also means that the same way of raising a `401` (through the `current_app` method) is used everywhere.
2019-07-01 15:22:08 +01:00
if isinstance(node, ast.Attribute):
return node.value.id
Enforce order of permissions decorators At the moment we mostly have `user_has_permissions` execute first. It shouldn’t matter, but it feels right for us to check that a user is logged in before we check their permissions to a service. Otherwise a malicious user could (maybe) check if a service ID belongs to a real service, and go on to do something malicious with that information. This commit adds some extra test code to enforce that the order is always the same. N.B. decorators in Python execute from closest to furthest (from the line on which the function is defined).
2019-07-01 13:45:21 +01:00
return '{}.{}'.format(node.func.value.id, node.func.attr)
Ensure all service route have permission decorators 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
2019-06-27 14:23:03 +01:00
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'
Have permissions decorators check user signed in Rather than force us to write the decorators in a specific order let’s just have one decorator call the other. This should make fewer lines of code, and fewer annoying test failures. It also means that the same way of raising a `401` (through the `current_app` method) is used everywhere.
2019-07-01 15:22:08 +01:00
def get_routes_and_decorators(argument_name=None):
Ensure all service route have permission decorators 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
2019-06-27 14:23:03 +01:00
import app.main.views as views
for module_name, module in inspect.getmembers(views):
for function_name, function in inspect.getmembers(module):
Have permissions decorators check user signed in Rather than force us to write the decorators in a specific order let’s just have one decorator call the other. This should make fewer lines of code, and fewer annoying test failures. It also means that the same way of raising a `401` (through the `current_app` method) is used everywhere.
2019-07-01 15:22:08 +01:00
if inspect.isfunction(function):
Ensure all service route have permission decorators 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
2019-06-27 14:23:03 +01:00
decorators = list(get_decorators_for_function(function))
Have permissions decorators check user signed in Rather than force us to write the decorators in a specific order let’s just have one decorator call the other. This should make fewer lines of code, and fewer annoying test failures. It also means that the same way of raising a `401` (through the `current_app` method) is used everywhere.
2019-07-01 15:22:08 +01:00
if 'main.route' in decorators and (
not argument_name or
argument_name in inspect.signature(function).parameters.keys()
):
Ensure all service route have permission decorators 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
2019-06-27 14:23:03 +01:00
yield '{}.{}'.format(module_name, function_name), decorators
Enforce order of permissions decorators At the moment we mostly have `user_has_permissions` execute first. It shouldn’t matter, but it feels right for us to check that a user is logged in before we check their permissions to a service. Otherwise a malicious user could (maybe) check if a service ID belongs to a real service, and go on to do something malicious with that information. This commit adds some extra test code to enforce that the order is always the same. N.B. decorators in Python execute from closest to furthest (from the line on which the function is defined).
2019-07-01 13:45:21 +01:00
def format_decorators(decorators, indent=8):
return '\n'.join(
'{}@{}'.format(' ' * indent, decorator)
for decorator in decorators
)
Ensure all service route have permission decorators 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
2019-06-27 14:23:03 +01:00
def test_code_to_extract_decorators_works_with_known_examples():
assert (
'templates.choose_template',
Add a specific route for template_type=all `all` is not a real template type, so for links to template folders that apply to all template types we have a URL that looks like: ``` /services/<uuid:service_id>/templates ``` However Flask only generates this url when `url_for` is called with `template_type=None`. If called with `template_type=all` then Flask will generate a URL like ``` /services/<uuid:service_id>/templates/all ``` However attempting to load this URL will now 404, since `all` is not a template type recognised by the regex introduced in https://github.com/alphagov/notifications-admin/pull/3176 It would be nice to not have URLs with `all` in them at all, but since people might have bookmarked them we need to support them indefinitely. Also considered but decided against adding `all` to the set of template types because it might cause other problems, for example attempting to create a new template with a type of `all` would never work.
2019-11-13 13:38:02 +00:00
['main.route', 'main.route', 'main.route', 'main.route', 'main.route', 'main.route', 'user_has_permissions'],
Ensure all service route have permission decorators 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
2019-06-27 14:23:03 +01:00
) in list(
Have permissions decorators check user signed in Rather than force us to write the decorators in a specific order let’s just have one decorator call the other. This should make fewer lines of code, and fewer annoying test failures. It also means that the same way of raising a `401` (through the `current_app` method) is used everywhere.
2019-07-01 15:22:08 +01:00
get_routes_and_decorators(SERVICE_ID_ARGUMENT)
Ensure all service route have permission decorators 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
2019-06-27 14:23:03 +01:00
)
assert (
'organisations.organisation_dashboard',
Have permissions decorators check user signed in Rather than force us to write the decorators in a specific order let’s just have one decorator call the other. This should make fewer lines of code, and fewer annoying test failures. It also means that the same way of raising a `401` (through the `current_app` method) is used everywhere.
2019-07-01 15:22:08 +01:00
['main.route', 'user_has_permissions'],
Ensure all service route have permission decorators 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
2019-06-27 14:23:03 +01:00
) in list(
Have permissions decorators check user signed in Rather than force us to write the decorators in a specific order let’s just have one decorator call the other. This should make fewer lines of code, and fewer annoying test failures. It also means that the same way of raising a `401` (through the `current_app` method) is used everywhere.
2019-07-01 15:22:08 +01:00
get_routes_and_decorators(ORGANISATION_ID_ARGUMENT)
)
assert (
'platform_admin.platform_admin',
['main.route', 'user_is_platform_admin'],
) in list(
get_routes_and_decorators()
Ensure all service route have permission decorators 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
2019-06-27 14:23:03 +01:00
)
Have permissions decorators check user signed in Rather than force us to write the decorators in a specific order let’s just have one decorator call the other. This should make fewer lines of code, and fewer annoying test failures. It also means that the same way of raising a `401` (through the `current_app` method) is used everywhere.
2019-07-01 15:22:08 +01:00
def test_routes_have_permissions_decorators():
Ensure all service route have permission decorators 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
2019-06-27 14:23:03 +01:00
for endpoint, decorators in (
Have permissions decorators check user signed in Rather than force us to write the decorators in a specific order let’s just have one decorator call the other. This should make fewer lines of code, and fewer annoying test failures. It also means that the same way of raising a `401` (through the `current_app` method) is used everywhere.
2019-07-01 15:22:08 +01:00
list(get_routes_and_decorators(SERVICE_ID_ARGUMENT)) +
list(get_routes_and_decorators(ORGANISATION_ID_ARGUMENT))
Ensure all service route have permission decorators 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
2019-06-27 14:23:03 +01:00
):
Enforce order of permissions decorators At the moment we mostly have `user_has_permissions` execute first. It shouldn’t matter, but it feels right for us to check that a user is logged in before we check their permissions to a service. Otherwise a malicious user could (maybe) check if a service ID belongs to a real service, and go on to do something malicious with that information. This commit adds some extra test code to enforce that the order is always the same. N.B. decorators in Python execute from closest to furthest (from the line on which the function is defined).
2019-07-01 13:45:21 +01:00
file, function = endpoint.split('.')
Have permissions decorators check user signed in Rather than force us to write the decorators in a specific order let’s just have one decorator call the other. This should make fewer lines of code, and fewer annoying test failures. It also means that the same way of raising a `401` (through the `current_app` method) is used everywhere.
2019-07-01 15:22:08 +01:00
assert 'user_is_logged_in' not in decorators, (
'@user_is_logged_in used on service or organisation specific endpoint\n'
'Use @user_has_permissions() or @user_is_platform_admin only\n'
'app/main/views/{}.py::{}\n'
).format(file, function)
Ensure all service route have permission decorators 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
2019-06-27 14:23:03 +01:00
if 'user_is_platform_admin' in decorators:
Have permissions decorators check user signed in Rather than force us to write the decorators in a specific order let’s just have one decorator call the other. This should make fewer lines of code, and fewer annoying test failures. It also means that the same way of raising a `401` (through the `current_app` method) is used everywhere.
2019-07-01 15:22:08 +01:00
continue
assert 'user_has_permissions' in decorators, (
'Missing @user_has_permissions decorator\n'
'Use @user_has_permissions() or @user_is_platform_admin instead\n'
'app/main/views/{}.py::{}\n'
).format(file, function)
Do extra code style checks with flake8-bugbear Flake8 Bugbear checks for some extra things that aren’t code style errors, but are likely to introduce bugs or unexpected behaviour. A good example is having mutable default function arguments, which get shared between every call to the function and therefore mutating a value in one place can unexpectedly cause it to change in another. This commit enables all the extra warnings provided by Flake8 Bugbear, except for the line length one (because we already lint for that separately). It disables: - _B003: Assigning to os.environ_ because I don’t really understand this - _B306: BaseException.message is removed in Python 3_ because I think our exceptions have a custom structure that means the `.message` attribute is still present
2019-11-01 10:43:01 +00:00
for _endpoint, decorators in get_routes_and_decorators():
Have permissions decorators check user signed in Rather than force us to write the decorators in a specific order let’s just have one decorator call the other. This should make fewer lines of code, and fewer annoying test failures. It also means that the same way of raising a `401` (through the `current_app` method) is used everywhere.
2019-07-01 15:22:08 +01:00
assert 'login_required' not in decorators, (
'@login_required found\n'
'For consistency, use @user_is_logged_in() instead (from app.utils)\n'
'app/main/views/{}.py::{}\n'
).format(file, function)
if 'user_is_platform_admin' in decorators:
assert 'user_has_permissions' not in decorators, (
'@user_has_permissions and @user_is_platform_admin decorating same function\n'
'You can only use one of these at a time\n'
'app/main/views/{}.py::{}\n'
).format(file, function)
assert 'user_is_logged_in' not in decorators, (
'@user_is_logged_in used with @user_is_platform_admin\n'
'Use @user_is_platform_admin only\n'
'app/main/views/{}.py::{}\n'
).format(file, function)
Require IDs to be UUIDs in URLS We mostly rely on the API returning a 404 to generate 404s for trying to get things with non-UUID IDs. This is fine, except our tests often mock these API calls. So it could look like everything is working fine, except the thing your passing in might never be a valid UUID, and thus would 404 in a non-test environment. So this commit: 1. uses the `uuid` URL converter everywhere there’s something that looks like an ID in a URL parameter 2. adds a test which automates checking for 1.
2019-11-04 11:07:55 +00:00
def test_routes_require_uuids(client_request):
for rule in current_app.url_map.iter_rules():
for param in re.findall('<([^>]*)>', rule.rule):
if '_id' in param and not param.startswith('uuid:'):
pytest.fail((
'Should be <uuid:{}> in {}'
).format(param, rule.rule))
Reference in New Issue Copy Permalink