From 3968d5b7664035d5dd3729d32db1c8dde8a4b0d3 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 19 Jun 2019 16:39:13 +0100 Subject: [PATCH] Allow org team members to see team and usage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- app/main/views/dashboard.py | 2 +- app/main/views/manage_users.py | 2 +- app/models/user.py | 15 +++- app/templates/main_nav.html | 12 +-- tests/app/main/test_permissions.py | 115 +++++++++++++++++++++++++++++ tests/conftest.py | 9 ++- 6 files changed, 143 insertions(+), 12 deletions(-) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 9d608efca..346d15fc2 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -143,7 +143,7 @@ def template_usage(service_id): @main.route("/services//usage") @login_required -@user_has_permissions('manage_service') +@user_has_permissions('manage_service', allow_org_user=True) def usage(service_id): year, current_financial_year = requested_and_current_financial_year(request) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 42c3c752b..58cc4846a 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -31,7 +31,7 @@ from app.utils import is_gov_user, redact_mobile_number, user_has_permissions @main.route("/services//users") @login_required -@user_has_permissions() +@user_has_permissions(allow_org_user=True) def manage_users(service_id): return render_template( 'views/manage-users.html', diff --git a/app/models/user.py b/app/models/user.py index d156fa904..e0e46e620 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -174,7 +174,7 @@ class User(JSONModel, UserMixin): def platform_admin(self): return self._platform_admin and not session.get('disable_platform_admin_view', False) - def has_permissions(self, *permissions, restrict_admin_usage=False): + def has_permissions(self, *permissions, restrict_admin_usage=False, allow_org_user=False): unknown_permissions = set(permissions) - all_permissions if unknown_permissions: raise TypeError('{} are not valid permissions'.format(list(unknown_permissions))) @@ -195,12 +195,19 @@ class User(JSONModel, UserMixin): if org_id: return self.belongs_to_organisation(org_id) - if not permissions: - return self.belongs_to_service(service_id) + if not permissions and self.belongs_to_service(service_id): + return True - return any( + if any( self.has_permission_for_service(service_id, permission) for permission in permissions + ): + return True + + from app.models.service import Service + + return allow_org_user and self.belongs_to_organisation( + Service.from_id(service_id).organisation_id ) def has_permission_for_service(self, service_id, permission): diff --git a/app/templates/main_nav.html b/app/templates/main_nav.html index 543828537..8037b77ff 100644 --- a/app/templates/main_nav.html +++ b/app/templates/main_nav.html @@ -6,15 +6,17 @@ {% if current_user.has_permissions('view_activity') %}
  • Dashboard
    • {% endif %} + {% if current_user.has_permissions() %}
  • Templates
    • - {% if not current_user.has_permissions('view_activity') %} -
  • Sent messages
    • - {% if current_service.has_jobs %} -
  • Uploaded files
    • + {% if not current_user.has_permissions('view_activity') %} +
  • Sent messages
    • + {% if current_service.has_jobs %} +
  • Uploaded files
    • + {% endif %} {% endif %} {% endif %}
  • Team members
    • - {% if current_user.has_permissions('manage_service') %} + {% if current_user.has_permissions('manage_service', allow_org_user=True) %}
  • Usage
    • {% endif %} {% if current_user.has_permissions('manage_api_keys', 'manage_service') %} diff --git a/tests/app/main/test_permissions.py b/tests/app/main/test_permissions.py index b44baf833..fca24395b 100644 --- a/tests/app/main/test_permissions.py +++ b/tests/app/main/test_permissions.py @@ -8,6 +8,13 @@ from app.models.roles_and_permissions import ( translate_permissions_from_db_to_admin_roles, ) from app.utils import user_has_permissions +from tests import service_json +from tests.conftest import ( + ORGANISATION_ID, + ORGANISATION_TWO_ID, + SERVICE_ONE_ID, + SERVICE_TWO_ID, +) def _test_permissions( @@ -37,6 +44,7 @@ def _test_permissions( def test_user_has_permissions_on_endpoint_fail( client, mocker, + mock_get_service, ): user = _user_with_permissions() mocker.patch('app.user_api_client.get_user', return_value=user) @@ -116,6 +124,7 @@ def test_platform_admin_user_can_not_access_page( client, platform_admin_user, mocker, + mock_get_service, ): mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) _test_permissions( @@ -254,3 +263,109 @@ 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'} + + +@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, + mock_get_service_organisation, + 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, + mock_get_service_organisation, +): + 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') + ] == [ + 'Team members', + 'Usage', + ] diff --git a/tests/conftest.py b/tests/conftest.py index 4230af60b..39d56b010 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -691,6 +691,7 @@ def mock_update_service_raise_httperror_duplicate_name(mocker): SERVICE_ONE_ID = "596364a0-858e-42c8-9062-a8fe822260eb" SERVICE_TWO_ID = "147ad62a-2951-4fa1-9ca0-093cd1a52c52" ORGANISATION_ID = "c011fa40-4cbe-4524-b415-dde2f421bd9c" +ORGANISATION_TWO_ID = "d9b5be73-0b36-4210-9d89-8f1a5c2fef26" TEMPLATE_ONE_ID = "b22d7d94-2197-4a7d-a8e7-fd5f9770bf48" USER_ONE_ID = "7b395b52-c6c1-469c-9d61-54166461c1ab" @@ -1393,6 +1394,7 @@ def active_user_no_api_key_permission(fake_uuid): 'auth_type': 'sms_auth', 'organisations': [], 'current_session_id': None, + 'services': [SERVICE_ONE_ID], } @@ -1415,6 +1417,8 @@ def active_user_no_settings_permission(fake_uuid): 'platform_admin': False, 'auth_type': 'sms_auth', 'current_session_id': None, + 'services': [SERVICE_ONE_ID], + 'organisations': [], } @@ -1432,6 +1436,7 @@ def api_user_locked(fake_uuid): 'organisations': [], 'current_session_id': None, 'platform_admin': False, + 'services': [], } return user_data @@ -1451,6 +1456,7 @@ def api_user_request_password_reset(fake_uuid): 'organisations': [], 'current_session_id': None, 'platform_admin': False, + 'services': [], } return user_data @@ -1470,6 +1476,7 @@ def api_user_changed_password(fake_uuid): 'organisations': [], 'current_session_id': None, 'platform_admin': False, + 'services': [], } return user_data @@ -2148,7 +2155,7 @@ def mock_no_inbound_number_for_service(mocker): @pytest.fixture(scope='function') def mock_has_permissions(mocker): - def _has_permission(*permissions, restrict_admin_usage=False): + def _has_permission(*permissions, restrict_admin_usage=False, allow_org_user=False): return True return mocker.patch(