From d14f33ea700c8cdaece2de46a1f751675046d460 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 1 Mar 2018 11:34:53 +0000 Subject: [PATCH] has_permissions() now checks user's orgs for view args view args are parameters within the route. for example, `/organisation//users`. If there is an org_id, then check that the user is part of that organisation (users.organisations is a list of all orgs that user is a member of). * platform admins ignore this check if restrict_admin_usage=False * if an endpoint has both org_id and service_id, org_id takes precedence, but we should probably revisit this if we ever need to create such an endpoint. * you now call `@user_has_permissions()` with no arguments for organisation endpoints - we can look at this if we decide we want more clarity. * you should never call user_has_permissions without any arguments for endpoints that aren't organisation-based. We'll raise NotImplementedError if you do. --- app/__init__.py | 22 ++++++------- app/notify_client/models.py | 15 +++++++-- tests/app/main/test_permissions.py | 50 ++++++++++++++++++++++++++---- 3 files changed, 66 insertions(+), 21 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 2cd412af4..c9d2242f6 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -418,19 +418,17 @@ def load_organisation_before_request(): _request_ctx_stack.top.organisation = None if request.view_args: - org_id = request.view_args.get('org_id', session.get('org_id')) - else: - org_id = session.get('org_id') + org_id = request.view_args.get('org_id') - if org_id: - try: - _request_ctx_stack.top.organisation = organisations_client.get_organisation(org_id) - except HTTPError as exc: - # if org id isn't real, then 404 rather than 500ing later because we expect org to be set - if exc.status_code == 404: - abort(404) - else: - raise + if org_id: + try: + _request_ctx_stack.top.organisation = organisations_client.get_organisation(org_id) + except HTTPError as exc: + # if org id isn't real, then 404 rather than 500ing later because we expect org to be set + if exc.status_code == 404: + abort(404) + else: + raise def save_service_after_request(response): diff --git a/app/notify_client/models.py b/app/notify_client/models.py index 091f3a68a..2523bd286 100644 --- a/app/notify_client/models.py +++ b/app/notify_client/models.py @@ -32,6 +32,10 @@ def _get_service_id_from_view_args(): return request.view_args.get('service_id', None) +def _get_org_id_from_view_args(): + return request.view_args.get('org_id', None) + + def translate_permissions_from_db_to_admin_roles(permissions): """ Given a list of database permissions, return a set of roles @@ -123,9 +127,14 @@ class User(UserMixin): # Service id is always set on the request for service specific views. service_id = _get_service_id_from_view_args() - if service_id in self._permissions: - return any(x in self._permissions[service_id] for x in permissions) - return False + org_id = _get_org_id_from_view_args() + + if org_id: + return org_id in self.organisations + elif service_id: + return any(x in self._permissions.get(service_id, []) for x in permissions) + else: + return False def has_permission_for_service(self, service_id, permission): return permission in self._permissions.get(service_id, []) diff --git a/tests/app/main/test_permissions.py b/tests/app/main/test_permissions.py index c10f76d3b..3f5b885ed 100644 --- a/tests/app/main/test_permissions.py +++ b/tests/app/main/test_permissions.py @@ -44,7 +44,7 @@ def test_user_has_permissions_on_endpoint_fail( client, user, ['send_messages'], - False) + will_succeed=False) def test_user_has_permissions_success( @@ -57,7 +57,7 @@ def test_user_has_permissions_success( client, user, ['manage_service'], - True) + will_succeed=True) def test_user_has_permissions_or( @@ -70,7 +70,7 @@ def test_user_has_permissions_or( client, user, ['send_messages', 'manage_service'], - True) + will_succeed=True) def test_user_has_permissions_multiple( @@ -96,7 +96,7 @@ def test_exact_permissions( client, user, ['manage_service', 'manage_templates'], - True) + will_succeed=True) def test_platform_admin_user_can_access_page_that_has_no_permissions( @@ -109,7 +109,7 @@ def test_platform_admin_user_can_access_page_that_has_no_permissions( client, platform_admin_user, [], - True) + will_succeed=True) def test_platform_admin_user_can_not_access_page( @@ -138,6 +138,43 @@ def test_no_user_returns_401_unauth( will_succeed=False) +def test_user_has_permissions_for_organisation( + client, + mocker, +): + user = _user_with_permissions() + user.organisations = ['org_1', 'org_2'] + mocker.patch('app.user_api_client.get_user', return_value=user) + client.login(user) + + request.view_args = {'org_id': 'org_2'} + + @user_has_permissions() + def index(): + pass + + index() + + +def test_user_doesnt_have_permissions_for_organisation( + client, + mocker, +): + user = _user_with_permissions() + user.organisations = ['org_1', 'org_2'] + mocker.patch('app.user_api_client.get_user', return_value=user) + client.login(user) + + request.view_args = {'org_id': 'org_3'} + + @user_has_permissions() + def index(): + pass + + with pytest.raises(Forbidden): + index() + + def _user_with_permissions(): from app.notify_client.user_api_client import User @@ -149,7 +186,8 @@ def _user_with_permissions(): 'state': 'active', 'failed_login_count': 0, 'permissions': {'foo': ['manage_users', 'manage_templates', 'manage_settings']}, - 'platform_admin': False + 'platform_admin': False, + 'organisations': ['org_1', 'org_2'], } user = User(user_data) return user