From 31afd65e7158906d9d0a422f0924a6580cc65c14 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 19 Jun 2019 14:28:12 +0100 Subject: [PATCH 1/5] Refactor permissions checking to use methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s a bit more concise to use these methods, rather than access the lists directly. And because it’s easier to read it will make later refactoring less bothersome. --- app/models/user.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/app/models/user.py b/app/models/user.py index 0a1fb3ed2..d156fa904 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -193,11 +193,15 @@ class User(JSONModel, UserMixin): return True if org_id: - return org_id in self.organisation_ids + return self.belongs_to_organisation(org_id) + if not permissions: - return service_id in self.service_ids - if service_id: - return any(x in self._permissions.get(service_id, []) for x in permissions) + return self.belongs_to_service(service_id) + + return any( + self.has_permission_for_service(service_id, permission) + for permission in permissions + ) def has_permission_for_service(self, service_id, permission): return permission in self._permissions.get(service_id, []) From 3968d5b7664035d5dd3729d32db1c8dde8a4b0d3 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 19 Jun 2019 16:39:13 +0100 Subject: [PATCH 2/5] 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( From afedf431e0960af7bd57f63705ecb76bc995e8b4 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 20 Jun 2019 10:35:55 +0100 Subject: [PATCH 3/5] Link to all services from organisation Because a member of the organisation can now see some pages for any live service they should be able to click into each one. --- app/main/views/organisations.py | 6 ------ .../views/organisations/organisation/index.html | 8 ++------ .../organisations/organisation/trial-mode-services.html | 2 +- tests/app/main/views/organisations/test_organisation.py | 9 +++++---- 4 files changed, 8 insertions(+), 17 deletions(-) diff --git a/app/main/views/organisations.py b/app/main/views/organisations.py index 44d4c068d..78ebfec2d 100644 --- a/app/main/views/organisations.py +++ b/app/main/views/organisations.py @@ -70,13 +70,8 @@ def add_organisation(): @login_required @user_has_permissions() def organisation_dashboard(org_id): - for service in current_organisation.live_services: - has_permission = current_user.has_permission_for_service(service['id'], 'view_activity') - service.update({'has_permission_to_view': has_permission}) - return render_template( 'views/organisations/organisation/index.html', - organisation_services=current_organisation.live_services ) @@ -87,7 +82,6 @@ def organisation_trial_mode_services(org_id): return render_template( 'views/organisations/organisation/trial-mode-services.html', search_form=SearchByNameForm(), - services=current_organisation.trial_services ) diff --git a/app/templates/views/organisations/organisation/index.html b/app/templates/views/organisations/organisation/index.html index 1fd844422..c0ae8a5cd 100644 --- a/app/templates/views/organisations/organisation/index.html +++ b/app/templates/views/organisations/organisation/index.html @@ -10,13 +10,9 @@ Services
      - {% for service in organisation_services %} + {% for service in current_org.live_services %}
    • - {% if service.has_permission_to_view or current_user.platform_admin %} - {{ service['name'] }} - {% else %} - {{ service['name'] }} - {% endif %} + {{ service['name'] }}
      • {% endfor %}
    diff --git a/app/templates/views/organisations/organisation/trial-mode-services.html b/app/templates/views/organisations/organisation/trial-mode-services.html index 74c734c23..5e8d0e1cf 100644 --- a/app/templates/views/organisations/organisation/trial-mode-services.html +++ b/app/templates/views/organisations/organisation/trial-mode-services.html @@ -12,7 +12,7 @@ {{ live_search(target_selector='.browse-list-item', show=True, form=search_form, label='Search by name') }}
      - {% for service in services %} + {% for service in current_org.trial_services %}
    • {{ service['name'] }}
      • diff --git a/tests/app/main/views/organisations/test_organisation.py b/tests/app/main/views/organisations/test_organisation.py index cb4fb0bed..634182ec8 100644 --- a/tests/app/main/views/organisations/test_organisation.py +++ b/tests/app/main/views/organisations/test_organisation.py @@ -9,6 +9,7 @@ from tests import organisation_json, service_json from tests.conftest import ( ORGANISATION_ID, SERVICE_ONE_ID, + SERVICE_TWO_ID, active_user_with_permissions, normalize_spaces, platform_admin_user, @@ -104,11 +105,11 @@ def test_organisation_services_shows_live_services_only( mocker.patch( 'app.organisations_client.get_organisation_services', return_value=[ - service_json(id_='1', name='1', restricted=False, active=True), # live + service_json(id_=SERVICE_ONE_ID, name='1', restricted=False, active=True), # live service_json(id_='2', name='2', restricted=True, active=True), # trial service_json(id_='3', name='3', restricted=True, active=False), # trial, now archived service_json(id_='4', name='4', restricted=False, active=False), # was live, now archived - service_json(id_=SERVICE_ONE_ID, name='5', restricted=False, active=True), # live, member of + service_json(id_=SERVICE_TWO_ID, name='5', restricted=False, active=True), # live, member of ] ) @@ -120,8 +121,8 @@ def test_organisation_services_shows_live_services_only( assert normalize_spaces(services[0].text) == '1' assert normalize_spaces(services[1].text) == '5' - assert services[0].find('a') is None - assert services[1].find('a')['href'] == url_for('main.service_dashboard', service_id=SERVICE_ONE_ID) + assert services[0].find('a')['href'] == url_for('main.service_dashboard', service_id=SERVICE_ONE_ID) + assert services[1].find('a')['href'] == url_for('main.service_dashboard', service_id=SERVICE_TWO_ID) def test_organisation_trial_mode_services_shows_all_non_live_services( From 873272160c675be330e0384c1fb966170a236505 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 20 Jun 2019 10:58:27 +0100 Subject: [PATCH 4/5] Update content on invite page to reflect changes What org team members can see has changed slightly, so we should update the hint text accordingly. --- .../organisation/users/invite-org-user.html | 28 +++++++++---------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/app/templates/views/organisations/organisation/users/invite-org-user.html b/app/templates/views/organisations/organisation/users/invite-org-user.html index 14fb4dbe3..2768fcd39 100644 --- a/app/templates/views/organisations/organisation/users/invite-org-user.html +++ b/app/templates/views/organisations/organisation/users/invite-org-user.html @@ -15,22 +15,20 @@ "Invite a team member", back_link=url_for('.manage_org_users', org_id=current_org.id) ) }} -
      - {% call form_wrapper(class="column-three-quarters") %} - {{ textbox(form.email_address, width='1-1', safe_error_message=True) }} + {% call form_wrapper() %} + {{ textbox(form.email_address, width='1-1', safe_error_message=True) }} -
      -

      - All team members can see: -

      -
        -
      • dashboard
        • -
      • who the other team members are
        • -
      -
      +
      +

      + {{ current_org.name }} team members can: +

      +
        +
      • see usage for and team members for each service
        • +
      • invite other team members
        • +
      +
      - {{ page_footer('Send invitation email') }} - {% endcall %} -
      + {{ page_footer('Send invitation email') }} + {% endcall %} {% endblock %} From e6733d7e24e93dd29859b0c980596826c561de22 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 20 Jun 2019 10:59:54 +0100 Subject: [PATCH 5/5] Link to usage, not dashboard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Organisation team members only have access to the dashboard if they’re also a member of that service. They always have access to the usage page, so let’s link there instead. --- app/templates/org_nav.html | 2 +- app/templates/views/organisations/organisation/index.html | 6 +++--- tests/app/main/views/organisations/test_organisation.py | 6 +++--- tests/app/test_navigation.py | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/templates/org_nav.html b/app/templates/org_nav.html index fd3cc0c2d..a9f518e11 100644 --- a/app/templates/org_nav.html +++ b/app/templates/org_nav.html @@ -1,6 +1,6 @@