From 171f911237547897ca808cd4d921ac361b937d4e Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 15 Jul 2021 11:57:33 +0100 Subject: [PATCH] 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))