From 0f87ffe0934686c402cb315765cb3a35952df9f6 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 15 Jul 2021 11:18:36 +0100 Subject: [PATCH 1/5] Move inline import to top of file Usually we have imports at the top. It looks like the reason for them being inline was to avoid a circular import, but we can also avoid this by not importing everything from the app module. Since we're about to add more imports from event_handlers, now is a good time to refactor them. Note this matches how we import the event handlers in every other module. --- app/event_handlers.py | 2 +- app/models/user.py | 3 +-- tests/app/main/views/test_accept_invite.py | 6 +++--- tests/app/main/views/test_register.py | 2 +- 4 files changed, 6 insertions(+), 7 deletions(-) diff --git a/app/event_handlers.py b/app/event_handlers.py index 8c141cd59..9ecd68b28 100644 --- a/app/event_handlers.py +++ b/app/event_handlers.py @@ -1,6 +1,6 @@ from flask import request -from app import events_api_client +from app.notify_client.events_api_client import events_api_client EVENT_SCHEMAS = { "sucessful_login": {"user_id"}, diff --git a/app/models/user.py b/app/models/user.py index ff9cc1387..03cfb7e94 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -4,6 +4,7 @@ from notifications_python_client.errors import HTTPError from notifications_utils.timezones import utc_string_to_aware_gmt_datetime from werkzeug.utils import cached_property +from app.event_handlers import create_add_user_to_service_event from app.models import JSONModel, ModelList from app.models.organisation import Organisation from app.models.roles_and_permissions import ( @@ -401,8 +402,6 @@ class User(JSONModel, UserMixin): session['current_session_id'] = self.current_session_id def add_to_service(self, service_id, permissions, folder_permissions, invited_by_id): - from app.event_handlers import create_add_user_to_service_event - try: user_api_client.add_user_to_service( service_id, diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index 6f84a082d..cdf71c616 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -46,7 +46,7 @@ def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard( ): expected_service = service_one['id'] expected_permissions = {'view_activity', 'send_messages', 'manage_service', 'manage_api_keys'} - mock_audit_event = mocker.patch('app.event_handlers.create_add_user_to_service_event') + mock_audit_event = mocker.patch('app.models.user.create_add_user_to_service_event') response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken')) @@ -270,7 +270,7 @@ def test_accept_invite_redirects_if_api_raises_an_error_that_they_are_already_pa mock_no_users_for_service, mock_get_user, ): - mock_audit_event = mocker.patch('app.event_handlers.create_add_user_to_service_event') + mock_audit_event = mocker.patch('app.models.user.create_add_user_to_service_event') mocker.patch('app.user_api_client.add_user_to_service', side_effect=HTTPError( response=Mock( @@ -574,7 +574,7 @@ def test_new_invited_user_verifies_and_added_to_service( mock_create_event, mocker, ): - mock_audit_event = mocker.patch('app.event_handlers.create_add_user_to_service_event') + mock_audit_event = mocker.patch('app.models.user.create_add_user_to_service_event') # visit accept token page response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken')) diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index 81d83f802..8c83c258d 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -334,7 +334,7 @@ def test_register_from_email_auth_invite( fake_uuid, mocker, ): - mock_audit_event = mocker.patch('app.event_handlers.create_add_user_to_service_event') + mock_audit_event = mocker.patch('app.models.user.create_add_user_to_service_event') sample_invite['auth_type'] = 'email_auth' sample_invite['email_address'] = invite_email_address From 2241b119b0d3407412909d65f2074f35f8f9b600 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 15 Jul 2021 11:33:13 +0100 Subject: [PATCH 2/5] Split (has_)permissions_for_service method This avoids duplicating the code to get user permissions ("admin roles") for a service, which we'll need in the next commit. --- app/models/user.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/models/user.py b/app/models/user.py index 03cfb7e94..aaaf827c7 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -215,8 +215,7 @@ class User(JSONModel, UserMixin): return True if any( - self.has_permission_for_service(service_id, permission) - for permission in permissions + self.permissions_for_service(service_id) & set(permissions) ): return True @@ -226,6 +225,9 @@ class User(JSONModel, UserMixin): Service.from_id(service_id).organisation_id ) + def permissions_for_service(self, service_id): + return self._permissions.get(service_id, set()) + def has_permission_for_service(self, service_id, permission): return permission in self._permissions.get(service_id, []) From 171f911237547897ca808cd4d921ac361b937d4e Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 15 Jul 2021 11:57:33 +0100 Subject: [PATCH 3/5] Audit when user permissions are changed I've used the term "admin_roles" in the event data to try and show that these are not the permissions we store in the DB. This is the name we use for the abstracted form of permissions in the Admin app. While we could store the DB permissions, that would be a bit more effort and arguably it's clearer to keep the event data consistent with the options the user actually saw / chose. --- app/event_handlers.py | 5 +++++ app/main/views/manage_users.py | 1 + app/models/user.py | 14 ++++++++++++-- tests/app/models/test_user.py | 23 ++++++++++++++++++++++- tests/app/test_event_handlers.py | 14 ++++++++++++++ 5 files changed, 54 insertions(+), 3 deletions(-) diff --git a/app/event_handlers.py b/app/event_handlers.py index 9ecd68b28..63a7acebe 100644 --- a/app/event_handlers.py +++ b/app/event_handlers.py @@ -8,6 +8,7 @@ EVENT_SCHEMAS = { "update_user_mobile_number": {"user_id", "updated_by_id", "original_mobile_number", "new_mobile_number"}, "remove_user_from_service": {"user_id", "removed_by_id", "service_id"}, "add_user_to_service": {"user_id", "invited_by_id", "service_id"}, + "set_user_permissions": {"user_id", "service_id", "original_admin_roles", "new_admin_roles", "set_by_id"}, "archive_user": {"user_id", "archived_by_id"}, "change_broadcast_account_type": {"service_id", "changed_by_id", "service_mode", "broadcast_channel", "provider_restriction"}, # noqa: E501 (length) "archive_service": {"service_id", "archived_by_id"}, @@ -36,6 +37,10 @@ def create_add_user_to_service_event(**kwargs): _send_event('add_user_to_service', **kwargs) +def create_set_user_permissions_event(**kwargs): + _send_event('set_user_permissions', **kwargs) + + def create_archive_user_event(**kwargs): _send_event('archive_user', **kwargs) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 4333baf23..78e7a0f46 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -141,6 +141,7 @@ def edit_user_permissions(service_id, user_id): service_id, permissions=form.permissions, folder_permissions=form.folder_permissions.data, + set_by_id=current_user.id, ) # Only change the auth type if this is supported for a service. If a user logs in with a # security key, we generally don't want them to be able to use something less secure. diff --git a/app/models/user.py b/app/models/user.py index aaaf827c7..98c6770d1 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -4,7 +4,10 @@ from notifications_python_client.errors import HTTPError from notifications_utils.timezones import utc_string_to_aware_gmt_datetime from werkzeug.utils import cached_property -from app.event_handlers import create_add_user_to_service_event +from app.event_handlers import ( + create_add_user_to_service_event, + create_set_user_permissions_event, +) from app.models import JSONModel, ModelList from app.models.organisation import Organisation from app.models.roles_and_permissions import ( @@ -122,13 +125,20 @@ class User(JSONModel, UserMixin): datetime_string ) - def set_permissions(self, service_id, permissions, folder_permissions): + def set_permissions(self, service_id, permissions, folder_permissions, set_by_id): user_api_client.set_user_permissions( self.id, service_id, permissions=permissions, folder_permissions=folder_permissions, ) + create_set_user_permissions_event( + user_id=self.id, + service_id=service_id, + original_admin_roles=self.permissions_for_service(service_id), + new_admin_roles=permissions, + set_by_id=set_by_id, + ) def logged_in_elsewhere(self): return session.get('current_session_id') != self.current_session_id diff --git a/tests/app/models/test_user.py b/tests/app/models/test_user.py index e70b512f2..fe038b64e 100644 --- a/tests/app/models/test_user.py +++ b/tests/app/models/test_user.py @@ -1,7 +1,7 @@ import pytest from app.models.user import AnonymousUser, InvitedOrgUser, InvitedUser, User -from tests.conftest import USER_ONE_ID +from tests.conftest import SERVICE_ONE_ID, USER_ONE_ID def test_anonymous_user(notify_admin): @@ -135,3 +135,24 @@ def test_invited_org_user_from_session_uses_id(client, mocker, mock_get_invited_ def test_invited_org_user_from_session_returns_none_if_nothing_present(client, mocker): mocker.patch.dict('app.models.user.session', values={}, clear=True) assert InvitedOrgUser.from_session() is None + + +def test_set_permissions(client, mocker, active_user_view_permissions, fake_uuid): + mock_api = mocker.patch('app.models.user.user_api_client.set_user_permissions') + mock_event = mocker.patch('app.models.user.create_set_user_permissions_event') + + User(active_user_view_permissions).set_permissions( + service_id=SERVICE_ONE_ID, + permissions={'manage_templates'}, + folder_permissions=[], + set_by_id=fake_uuid, + ) + + mock_api.assert_called_once() + mock_event.assert_called_once_with( + service_id=SERVICE_ONE_ID, + user_id=active_user_view_permissions['id'], + original_admin_roles={'view_activity'}, + new_admin_roles={'manage_templates'}, + set_by_id=fake_uuid, + ) diff --git a/tests/app/test_event_handlers.py b/tests/app/test_event_handlers.py index 29d511992..8def2653a 100644 --- a/tests/app/test_event_handlers.py +++ b/tests/app/test_event_handlers.py @@ -10,6 +10,7 @@ from app.event_handlers import ( create_mobile_number_change_event, create_remove_user_from_service_event, create_resume_service_event, + create_set_user_permissions_event, create_suspend_service_event, on_user_logged_in, ) @@ -129,3 +130,16 @@ def test_resume_service(client, mock_events): create_resume_service_event(**kwargs) mock_events.assert_called_with('resume_service', event_dict(**kwargs)) + + +def test_set_user_permissions(client, mock_events): + kwargs = { + "user_id": str(uuid.uuid4()), + "service_id": str(uuid.uuid4()), + "original_admin_roles": set("manage_templates"), + "new_admin_roles": set("view_activity"), + "set_by_id": str(uuid.uuid4()), + } + + create_set_user_permissions_event(**kwargs) + mock_events.assert_called_with('set_user_permissions', event_dict(**kwargs)) From 9fafc092f72a4fcd554c6edbda48d8f92348100c Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 15 Jul 2021 12:13:42 +0100 Subject: [PATCH 4/5] Audit permissions when adding a user to a service This is useful information to store for the event, which would be lost if someone subsequently changed them. Rather than updating lots of mock assertions, I've replaced them with a single test / assert at a lower level, which is consistent with auditing being a non-critical function. --- app/event_handlers.py | 2 +- app/models/user.py | 1 + tests/app/main/views/test_accept_invite.py | 15 --------------- tests/app/main/views/test_register.py | 6 ------ tests/app/models/test_user.py | 20 ++++++++++++++++++++ tests/app/test_event_handlers.py | 3 ++- 6 files changed, 24 insertions(+), 23 deletions(-) diff --git a/app/event_handlers.py b/app/event_handlers.py index 63a7acebe..45a6c6f10 100644 --- a/app/event_handlers.py +++ b/app/event_handlers.py @@ -7,7 +7,7 @@ EVENT_SCHEMAS = { "update_user_email": {"user_id", "updated_by_id", "original_email_address", "new_email_address"}, "update_user_mobile_number": {"user_id", "updated_by_id", "original_mobile_number", "new_mobile_number"}, "remove_user_from_service": {"user_id", "removed_by_id", "service_id"}, - "add_user_to_service": {"user_id", "invited_by_id", "service_id"}, + "add_user_to_service": {"user_id", "invited_by_id", "service_id", "admin_roles"}, "set_user_permissions": {"user_id", "service_id", "original_admin_roles", "new_admin_roles", "set_by_id"}, "archive_user": {"user_id", "archived_by_id"}, "change_broadcast_account_type": {"service_id", "changed_by_id", "service_mode", "broadcast_channel", "provider_restriction"}, # noqa: E501 (length) diff --git a/app/models/user.py b/app/models/user.py index 98c6770d1..fac181cf5 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -425,6 +425,7 @@ class User(JSONModel, UserMixin): user_id=self.id, invited_by_id=invited_by_id, service_id=service_id, + admin_roles=permissions, ) except HTTPError as exception: if exception.status_code == 400 and 'already part of service' in exception.message: diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index cdf71c616..73e8fcd9b 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -46,7 +46,6 @@ def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard( ): expected_service = service_one['id'] expected_permissions = {'view_activity', 'send_messages', 'manage_service', 'manage_api_keys'} - mock_audit_event = mocker.patch('app.models.user.create_add_user_to_service_event') response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken')) @@ -62,11 +61,6 @@ def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard( assert response.status_code == 302 assert response.location == url_for('main.service_dashboard', service_id=expected_service, _external=True) - mock_audit_event.assert_called_once_with( - invited_by_id=service_one['users'][0], - service_id=SERVICE_ONE_ID, - user_id=api_user_active['id'], - ) @pytest.mark.parametrize('trial_mode, expected_endpoint', ( @@ -270,8 +264,6 @@ def test_accept_invite_redirects_if_api_raises_an_error_that_they_are_already_pa mock_no_users_for_service, mock_get_user, ): - mock_audit_event = mocker.patch('app.models.user.create_add_user_to_service_event') - mocker.patch('app.user_api_client.add_user_to_service', side_effect=HTTPError( response=Mock( status_code=400, @@ -285,7 +277,6 @@ def test_accept_invite_redirects_if_api_raises_an_error_that_they_are_already_pa response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken'), follow_redirects=False) assert response.location == url_for('main.service_dashboard', service_id=SERVICE_ONE_ID, _external=True) - assert not mock_audit_event.called def test_existing_signed_out_user_accept_invite_redirects_to_sign_in( @@ -574,8 +565,6 @@ def test_new_invited_user_verifies_and_added_to_service( mock_create_event, mocker, ): - mock_audit_event = mocker.patch('app.models.user.create_add_user_to_service_event') - # visit accept token page response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken')) assert response.status_code == 302 @@ -611,10 +600,6 @@ def test_new_invited_user_verifies_and_added_to_service( mock_check_verify_code.assert_called_once_with(new_user_id, '12345', 'sms') assert service_one['id'] == session['service_id'] - mock_audit_event.assert_called_once_with(invited_by_id=service_one['users'][0], - service_id=service_one['id'], - user_id=new_user_id) - raw_html = response.data.decode('utf-8') page = BeautifulSoup(raw_html, 'html.parser') assert page.find('h1').text == 'Dashboard' diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index 8c83c258d..74e42743d 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -334,8 +334,6 @@ def test_register_from_email_auth_invite( fake_uuid, mocker, ): - mock_audit_event = mocker.patch('app.models.user.create_add_user_to_service_event') - sample_invite['auth_type'] = 'email_auth' sample_invite['email_address'] = invite_email_address with client.session_transaction() as session: @@ -373,10 +371,6 @@ def test_register_from_email_auth_invite( assert current_user.is_authenticated assert mock_add_user_to_service.called - mock_audit_event.assert_called_once_with(invited_by_id=service_one['users'][0], - service_id=sample_invite['service'], - user_id=fake_uuid) - with client.session_transaction() as session: # invited user details are still there so they can get added to the service assert session['invited_user_id'] == sample_invite['id'] diff --git a/tests/app/models/test_user.py b/tests/app/models/test_user.py index fe038b64e..df6fb9c91 100644 --- a/tests/app/models/test_user.py +++ b/tests/app/models/test_user.py @@ -156,3 +156,23 @@ def test_set_permissions(client, mocker, active_user_view_permissions, fake_uuid new_admin_roles={'manage_templates'}, set_by_id=fake_uuid, ) + + +def test_add_to_service(client, mocker, api_user_active, fake_uuid): + mock_api = mocker.patch('app.models.user.user_api_client.add_user_to_service') + mock_event = mocker.patch('app.models.user.create_add_user_to_service_event') + + User(api_user_active).add_to_service( + service_id=SERVICE_ONE_ID, + permissions={'manage_templates'}, + folder_permissions=[], + invited_by_id=fake_uuid, + ) + + mock_api.assert_called_once() + mock_event.assert_called_once_with( + service_id=SERVICE_ONE_ID, + user_id=api_user_active['id'], + invited_by_id=fake_uuid, + admin_roles={'manage_templates'}, + ) diff --git a/tests/app/test_event_handlers.py b/tests/app/test_event_handlers.py index 8def2653a..10ff5518e 100644 --- a/tests/app/test_event_handlers.py +++ b/tests/app/test_event_handlers.py @@ -49,7 +49,8 @@ def test_create_add_user_to_service_event_calls_events_api(client, mock_events): kwargs = { "user_id": str(uuid.uuid4()), "invited_by_id": str(uuid.uuid4()), - "service_id": str(uuid.uuid4()) + "service_id": str(uuid.uuid4()), + "admin_roles": {'manage_templates'}, } create_add_user_to_service_event(**kwargs) From 832422fc666d3cade6dd4c1c5c61f318732bf662 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 21 Jul 2021 16:19:56 +0100 Subject: [PATCH 5/5] Replace "admin roles" with "ui permissions" In response to: [1]. While this does introduce a new term ("admin roles" is still used elsewhere in the code), I plan to fix this in a follow-up PR (it turned out to be quite a big change to do on this branch). [1]: https://github.com/alphagov/notifications-admin/pull/3970#discussion_r673292339 --- app/event_handlers.py | 4 ++-- app/models/user.py | 6 +++--- tests/app/models/test_user.py | 6 +++--- tests/app/test_event_handlers.py | 6 +++--- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/app/event_handlers.py b/app/event_handlers.py index 45a6c6f10..3b8313daa 100644 --- a/app/event_handlers.py +++ b/app/event_handlers.py @@ -7,8 +7,8 @@ EVENT_SCHEMAS = { "update_user_email": {"user_id", "updated_by_id", "original_email_address", "new_email_address"}, "update_user_mobile_number": {"user_id", "updated_by_id", "original_mobile_number", "new_mobile_number"}, "remove_user_from_service": {"user_id", "removed_by_id", "service_id"}, - "add_user_to_service": {"user_id", "invited_by_id", "service_id", "admin_roles"}, - "set_user_permissions": {"user_id", "service_id", "original_admin_roles", "new_admin_roles", "set_by_id"}, + "add_user_to_service": {"user_id", "invited_by_id", "service_id", "ui_permissions"}, + "set_user_permissions": {"user_id", "service_id", "original_ui_permissions", "new_ui_permissions", "set_by_id"}, "archive_user": {"user_id", "archived_by_id"}, "change_broadcast_account_type": {"service_id", "changed_by_id", "service_mode", "broadcast_channel", "provider_restriction"}, # noqa: E501 (length) "archive_service": {"service_id", "archived_by_id"}, diff --git a/app/models/user.py b/app/models/user.py index fac181cf5..a03ecf712 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -135,8 +135,8 @@ class User(JSONModel, UserMixin): create_set_user_permissions_event( user_id=self.id, service_id=service_id, - original_admin_roles=self.permissions_for_service(service_id), - new_admin_roles=permissions, + original_ui_permissions=self.permissions_for_service(service_id), + new_ui_permissions=permissions, set_by_id=set_by_id, ) @@ -425,7 +425,7 @@ class User(JSONModel, UserMixin): user_id=self.id, invited_by_id=invited_by_id, service_id=service_id, - admin_roles=permissions, + ui_permissions=permissions, ) except HTTPError as exception: if exception.status_code == 400 and 'already part of service' in exception.message: diff --git a/tests/app/models/test_user.py b/tests/app/models/test_user.py index df6fb9c91..71f858c21 100644 --- a/tests/app/models/test_user.py +++ b/tests/app/models/test_user.py @@ -152,8 +152,8 @@ def test_set_permissions(client, mocker, active_user_view_permissions, fake_uuid mock_event.assert_called_once_with( service_id=SERVICE_ONE_ID, user_id=active_user_view_permissions['id'], - original_admin_roles={'view_activity'}, - new_admin_roles={'manage_templates'}, + original_ui_permissions={'view_activity'}, + new_ui_permissions={'manage_templates'}, set_by_id=fake_uuid, ) @@ -174,5 +174,5 @@ def test_add_to_service(client, mocker, api_user_active, fake_uuid): service_id=SERVICE_ONE_ID, user_id=api_user_active['id'], invited_by_id=fake_uuid, - admin_roles={'manage_templates'}, + ui_permissions={'manage_templates'}, ) diff --git a/tests/app/test_event_handlers.py b/tests/app/test_event_handlers.py index 10ff5518e..2cf8b50bf 100644 --- a/tests/app/test_event_handlers.py +++ b/tests/app/test_event_handlers.py @@ -50,7 +50,7 @@ def test_create_add_user_to_service_event_calls_events_api(client, mock_events): "user_id": str(uuid.uuid4()), "invited_by_id": str(uuid.uuid4()), "service_id": str(uuid.uuid4()), - "admin_roles": {'manage_templates'}, + "ui_permissions": {'manage_templates'}, } create_add_user_to_service_event(**kwargs) @@ -137,8 +137,8 @@ def test_set_user_permissions(client, mock_events): kwargs = { "user_id": str(uuid.uuid4()), "service_id": str(uuid.uuid4()), - "original_admin_roles": set("manage_templates"), - "new_admin_roles": set("view_activity"), + "original_ui_permissions": set("manage_templates"), + "new_ui_permissions": set("view_activity"), "set_by_id": str(uuid.uuid4()), }