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(