diff --git a/kobo/apps/audit_log/audit_actions.py b/kobo/apps/audit_log/audit_actions.py index d8a29c4c33..45c8f52426 100644 --- a/kobo/apps/audit_log/audit_actions.py +++ b/kobo/apps/audit_log/audit_actions.py @@ -3,6 +3,7 @@ class AuditAction(models.TextChoices): ADD_MEDIA = 'add-media' + ALLOW_ANONYMOUS_SUBMISSIONS = 'allow-anonymous-submissions' ARCHIVE = 'archive' AUTH = 'auth' CONNECT_PROJECT = 'connect-project' @@ -12,6 +13,7 @@ class AuditAction(models.TextChoices): DELETE_SERVICE = 'delete-service' DEPLOY = 'deploy' DISABLE_SHARING = 'disable-sharing' + DISALLOW_ANONYMOUS_SUBMISSIONS = 'disallow-anonymous-submissions' DISCONNECT_PROJECT = 'disconnect-project' ENABLE_SHARING = 'enable-sharing' EXPORT = 'export' @@ -19,12 +21,17 @@ class AuditAction(models.TextChoices): MODIFY_IMPORTED_FIELDS = 'modify-imported-fields' MODIFY_SERVICE = 'modify-service' MODIFY_SHARING = 'modify_sharing' + MODIFY_USER_PERMISSIONS = 'modify-user-permissions' PUT_BACK = 'put-back' REDEPLOY = 'redeploy' REGISTER_SERVICE = 'register-service' REMOVE = 'remove' REPLACE_FORM = 'replace-form' + SHARE_FORM_PUBLICLY = 'share-form-publicly' + SHARE_DATA_PUBLICLY = 'share-data-publicly' UNARCHIVE = 'unarchive' + UNSHARE_FORM_PUBLICLY = 'unshare-form-publicly' + UNSHARE_DATA_PUBLICLY = 'unshare-data-publicly' UPDATE = 'update' UPDATE_CONTENT = 'update-content' UPDATE_NAME = 'update-name' diff --git a/kobo/apps/audit_log/models.py b/kobo/apps/audit_log/models.py index 140f6eb852..7e8df931fd 100644 --- a/kobo/apps/audit_log/models.py +++ b/kobo/apps/audit_log/models.py @@ -1,3 +1,5 @@ +import copy + import jsonschema from django.conf import settings from django.db import models @@ -19,6 +21,10 @@ ACCESS_LOG_SUBMISSION_AUTH_TYPE, ACCESS_LOG_SUBMISSION_GROUP_AUTH_TYPE, ACCESS_LOG_UNKNOWN_AUTH_TYPE, + PERM_ADD_SUBMISSIONS, + PERM_VIEW_ASSET, + PERM_VIEW_SUBMISSIONS, + PROJECT_HISTORY_LOG_PERMISSION_SUBTYPE, PROJECT_HISTORY_LOG_PROJECT_SUBTYPE, ) from kpi.fields.kpi_uid import UUID_LENGTH @@ -30,6 +36,18 @@ ADDED = 'added' REMOVED = 'removed' +ANONYMOUS_USER_PERMISSION_ACTIONS = { + # key: (permission, granting?), value: ph log action + # True means the permission is being granted, + # False means it's being revoked + (PERM_VIEW_ASSET, True): AuditAction.SHARE_FORM_PUBLICLY, + (PERM_VIEW_SUBMISSIONS, True): AuditAction.SHARE_DATA_PUBLICLY, + (PERM_ADD_SUBMISSIONS, True): AuditAction.ALLOW_ANONYMOUS_SUBMISSIONS, + (PERM_VIEW_ASSET, False): AuditAction.UNSHARE_FORM_PUBLICLY, + (PERM_VIEW_SUBMISSIONS, False): AuditAction.UNSHARE_DATA_PUBLICLY, + (PERM_ADD_SUBMISSIONS, False): AuditAction.DISALLOW_ANONYMOUS_SUBMISSIONS, +} + class AuditType(models.TextChoices): ACCESS = 'access' @@ -247,10 +265,7 @@ def create_from_request( elif is_loginas: # third option: loginas auth_type = ACCESS_LOG_LOGINAS_AUTH_TYPE - elif ( - hasattr(logged_in_user, 'backend') - and logged_in_user.backend is not None - ): + elif hasattr(logged_in_user, 'backend') and logged_in_user.backend is not None: # fourth option: the backend that authenticated the user auth_type = logged_in_user.backend else: @@ -336,6 +351,9 @@ def create_from_request(cls, request): 'asset-export-list': cls.create_from_export_request, 'exporttask-list': cls.create_from_v1_export, 'asset-bulk': cls.create_from_bulk_request, + 'asset-permission-assignment-bulk-assignments': cls.create_from_permissions_request, # noqa + 'asset-permission-assignment-detail': cls.create_from_permissions_request, + 'asset-permission-assignment-list': cls.create_from_permissions_request, } url_name = request.resolver_match.url_name method = url_name_to_action.get(url_name, None) @@ -456,7 +474,7 @@ def create_from_detail_request(cls, request): 'log_subtype': PROJECT_HISTORY_LOG_PROJECT_SUBTYPE, 'ip_address': get_client_ip(request), 'source': get_human_readable_client_user_agent(request), - 'latest_version_uid': updated_data['latest_version.uid'] + 'latest_version_uid': updated_data['latest_version.uid'], } changed_field_to_action_map = { @@ -668,3 +686,118 @@ def create_from_v1_export(cls, request): 'source': get_human_readable_client_user_agent(request), }, ) + + @classmethod + def create_from_permissions_request(cls, request): + logs = [] + initial_data = getattr(request, 'initial_data', None) + updated_data = getattr(request, 'updated_data', None) + asset_uid = request.resolver_match.kwargs['parent_lookup_asset'] + source_data = updated_data if updated_data else initial_data + if source_data is None: + # there was an error on the request, ignore + return + asset_id = source_data['asset.id'] + # these will be dicts of username: [permissions] (as set or list) + permissions_added = getattr(request, 'permissions_added', {}) + permissions_removed = getattr(request, 'permissions_removed', {}) + partial_permissions_added = getattr(request, 'partial_permissions_added', {}) + # basic metadata for all PH logs + base_metadata = { + 'ip_address': get_client_ip(request), + 'source': get_human_readable_client_user_agent(request), + 'asset_uid': asset_uid, + 'log_subtype': PROJECT_HISTORY_LOG_PERMISSION_SUBTYPE, + } + # we'll be bulk creating logs instead of using .create, so we have to set + # all fields manually + log_base = { + 'user': request.user, + 'object_id': asset_id, + 'app_label': Asset._meta.app_label, + 'model_name': Asset._meta.model_name, + 'log_type': AuditType.PROJECT_HISTORY, + 'user_uid': request.user.extra_details.uid, + } + # get all users whose permissions changed + for username in { + *permissions_added, + *permissions_removed, + *partial_permissions_added, + }: + user_permissions_added = permissions_added.get(username, {}) + user_permissions_removed = permissions_removed.get(username, {}) + user_partial_permissions_added = partial_permissions_added.get(username, []) + if username == 'AnonymousUser': + cls.handle_anonymous_user_permissions( + user_permissions_added, + user_permissions_removed, + user_partial_permissions_added, + base_metadata, + log_base, + logs, + ) + continue + metadata = copy.deepcopy(base_metadata) + metadata['permissions'] = { + 'username': username, + REMOVED: list(user_permissions_removed), + ADDED: list(user_permissions_added) + user_partial_permissions_added, + } + logs.append( + ProjectHistoryLog( + **log_base, + action=AuditAction.MODIFY_USER_PERMISSIONS, + metadata=metadata, + ) + ) + ProjectHistoryLog.objects.bulk_create(logs) + + @classmethod + def handle_anonymous_user_permissions( + cls, + perms_added, + perms_removed, + partial_perms_added, + base_metadata, + log_base, + logs, + ): + # go through all the usual anonymous user permissions and create + # logs if they were changed + # remove each permission as it is logged so we can see if there + # are any unusual ones left over + for combination, action in ANONYMOUS_USER_PERMISSION_ACTIONS.items(): + # ANONYMOUS_USER_PERMISSION_ACTIONS has tuples as keys + permission, was_added = combination + list_to_update = perms_added if was_added else perms_removed + if permission in list_to_update: + list_to_update.discard(permission) + logs.append( + ProjectHistoryLog( + **log_base, + action=action, + metadata=base_metadata, + ) + ) + + # this shouldn't happen, but if anonymous users are granted other permissions, + # we want to know + if ( + len(perms_removed) > 0 + or len(perms_added) > 0 + or len(partial_perms_added) > 0 + ): + metadata = copy.deepcopy(base_metadata) + metadata['permissions'] = { + 'username': 'AnonymousUser', + ADDED: list(perms_added) + partial_perms_added, + REMOVED: list(perms_removed), + } + logs.append( + ProjectHistoryLog( + **log_base, + metadata=metadata, + action=AuditAction.MODIFY_USER_PERMISSIONS, + ) + ) diff --git a/kobo/apps/audit_log/signals.py b/kobo/apps/audit_log/signals.py index daab6dc9cb..e9565f2638 100644 --- a/kobo/apps/audit_log/signals.py +++ b/kobo/apps/audit_log/signals.py @@ -1,11 +1,20 @@ +from collections import defaultdict + from celery.signals import task_success from django.contrib.auth.signals import user_logged_in from django.dispatch import receiver +from django_userforeignkey.request import get_current_request -from kpi.models import ImportTask +from kpi.constants import ASSET_TYPE_SURVEY, PERM_PARTIAL_SUBMISSIONS +from kpi.models import Asset, ImportTask from kpi.tasks import import_in_background from kpi.utils.log import logging - +from kpi.utils.object_permission import ( + post_assign_partial_perm, + post_assign_perm, + post_remove_partial_perms, + post_remove_perm, +) from .models import AccessLog, ProjectHistoryLog @@ -24,3 +33,61 @@ def create_access_log(sender, user, **kwargs): def create_ph_log_for_import(sender, result, **kwargs): task = ImportTask.objects.get(uid=result) ProjectHistoryLog.create_from_import_task(task) + + +def initialize_permission_lists_if_necessary(request): + if getattr(request, 'permissions_added', None) is None: + request.permissions_added = defaultdict(set) + if getattr(request, 'permissions_removed', None) is None: + request.permissions_removed = defaultdict(set) + if getattr(request, 'partial_permissions_added', None) is None: + request.partial_permissions_added = defaultdict(list) + + +def initialize_request(): + request = get_current_request() + if request is None: + return None + initialize_permission_lists_if_necessary(request) + return request + + +@receiver(post_assign_perm, sender=Asset) +def add_assigned_perms(sender, instance, user, codename, deny, **kwargs): + request = initialize_request() + if not request or instance.asset_type != ASSET_TYPE_SURVEY or deny: + return + request.permissions_added[user.username].add(codename) + request.permissions_removed[user.username].discard(codename) + + +@receiver(post_remove_perm, sender=Asset) +def add_removed_perms(sender, instance, user, codename, **kwargs): + request = initialize_request() + if not request or instance.asset_type != ASSET_TYPE_SURVEY: + return + request.permissions_removed[user.username].add(codename) + request.permissions_added[user.username].discard(codename) + + +@receiver(post_assign_partial_perm, sender=Asset) +def add_assigned_partial_perms(sender, instance, user, perms, **kwargs): + request = initialize_request() + if not request or instance.asset_type != ASSET_TYPE_SURVEY: + return + perms_as_list_of_dicts = [{'code': k, 'filters': v} for k, v in perms.items()] + # partial permissions are replaced rather than added + request.partial_permissions_added[user.username] = perms_as_list_of_dicts + + +@receiver(post_remove_partial_perms, sender=Asset) +def remove_partial_perms(sender, instance, user, **kwargs): + request = initialize_request() + if not request or instance.asset_type != ASSET_TYPE_SURVEY: + return + request.partial_permissions_added[user.username] = [] + # in case we somehow deleted partial permissions + # without deleting 'partial_submissions', take care of that now since + # we can't have one without the other + request.permissions_added[user.username].discard(PERM_PARTIAL_SUBMISSIONS) + request.permissions_removed[user.username].add(PERM_PARTIAL_SUBMISSIONS) diff --git a/kobo/apps/audit_log/tests/test_models.py b/kobo/apps/audit_log/tests/test_models.py index eb997a970f..369ab5eb83 100644 --- a/kobo/apps/audit_log/tests/test_models.py +++ b/kobo/apps/audit_log/tests/test_models.py @@ -13,6 +13,8 @@ from kobo.apps.audit_log.models import ( ACCESS_LOG_LOGINAS_AUTH_TYPE, ACCESS_LOG_UNKNOWN_AUTH_TYPE, + ADDED, + REMOVED, AccessLog, AuditLog, AuditType, @@ -644,3 +646,34 @@ def test_create_from_import_task_with_name_change(self): 'name': {'old': old_name, 'new': 'new_name'}, }, ) + + def test_create_from_unexpected_anonymous_permissions(self): + # Normal anonymous permissions tested elsewhere + # This test is for if somehow other permissions are assigned + factory = RequestFactory() + request = factory.post('/') + request.user = User.objects.get(username='someuser') + request.resolver_match = Mock() + request.resolver_match.kwargs = {'parent_lookup_asset': 'a12345'} + request.updated_data = { + 'asset.id': 1, + } + request.permissions_added = { + # these permissions are not allowed for anonymous users, + # pretend something went wrong/changed and they were assigned anyway + 'AnonymousUser': {'discover_asset', 'validate_submissions'} + } + ProjectHistoryLog.create_from_permissions_request( + request, + ) + self.assertEqual(ProjectHistoryLog.objects.count(), 1) + log = ProjectHistoryLog.objects.first() + self.assertEqual(log.object_id, 1) + # should create a regular 'MODIFY_USER_PERMISSIONS' log + self.assertEqual(log.action, AuditAction.MODIFY_USER_PERMISSIONS) + permissions = log.metadata['permissions'] + self.assertEqual(permissions['username'], 'AnonymousUser') + self.assertListEqual(permissions[REMOVED], []) + self.assertListEqual( + sorted(permissions[ADDED]), ['discover_asset', 'validate_submissions'] + ) diff --git a/kobo/apps/audit_log/tests/test_project_history_logs.py b/kobo/apps/audit_log/tests/test_project_history_logs.py index 3b684a85f9..bbb6f88bd6 100644 --- a/kobo/apps/audit_log/tests/test_project_history_logs.py +++ b/kobo/apps/audit_log/tests/test_project_history_logs.py @@ -16,8 +16,16 @@ from kobo.apps.audit_log.tests.test_models import BaseAuditLogTestCase from kobo.apps.hook.models import Hook from kobo.apps.kobo_auth.shortcuts import User -from kpi.constants import PROJECT_HISTORY_LOG_PROJECT_SUBTYPE -from kpi.models import Asset, AssetFile, PairedData +from kpi.constants import ( + PERM_ADD_SUBMISSIONS, + PERM_CHANGE_SUBMISSIONS, + PERM_PARTIAL_SUBMISSIONS, + PERM_VIEW_ASSET, + PERM_VIEW_SUBMISSIONS, + PROJECT_HISTORY_LOG_PERMISSION_SUBTYPE, + PROJECT_HISTORY_LOG_PROJECT_SUBTYPE, +) +from kpi.models import Asset, AssetFile, ObjectPermission, PairedData from kpi.models.asset import AssetSetting from kpi.utils.strings import to_str @@ -480,8 +488,8 @@ def test_update_qa_creates_log(self): ) self.assertEqual( - log_metadata['qa'][NEW], - request_data['advanced_features']['qual']['qual_survey'], + log_metadata['qa'][NEW], + request_data['advanced_features']['qual']['qual_survey'], ) def test_failed_qa_update_does_not_create_log(self): @@ -921,3 +929,361 @@ def test_bulk_actions(self, bulk_action, audit_action): object_id__in=[asset.id for asset in assets], action=audit_action ) self.assertEqual(project_hist_logs.count(), 2) + + def test_bulk_permission_assignment_creates_logs_for_each_user(self): + def get_user_url(username): + return reverse('api_v2:user-kpi-detail', kwargs={'username': username}) + + def get_permission_url(codename): + return reverse('api_v2:permission-detail', kwargs={'codename': codename}) + + request_data = [ + # assign simple perms to someuser and anotheruser + { + 'permission': get_permission_url(PERM_VIEW_ASSET), + 'user': get_user_url('someuser'), + }, + { + 'permission': get_permission_url(PERM_VIEW_ASSET), + 'user': get_user_url('anotheruser'), + }, + ] + self.client.post( + path=reverse( + 'api_v2:asset-permission-assignment-bulk-assignments', + kwargs={ + 'parent_lookup_asset': self.asset.uid, + }, + ), + data=request_data, + format='json', + ) + self.assertEqual(ProjectHistoryLog.objects.count(), 2) + someuser_log = ProjectHistoryLog.objects.filter( + metadata__permissions__username='someuser' + ).first() + anotheruser_log = ProjectHistoryLog.objects.filter( + metadata__permissions__username='anotheruser' + ).first() + self._check_common_metadata( + someuser_log.metadata, PROJECT_HISTORY_LOG_PERMISSION_SUBTYPE + ) + self._check_common_metadata( + anotheruser_log.metadata, PROJECT_HISTORY_LOG_PERMISSION_SUBTYPE + ) + self.assertListEqual( + someuser_log.metadata['permissions'][ADDED], [PERM_VIEW_ASSET] + ) + self.assertListEqual( + anotheruser_log.metadata['permissions'][ADDED], [PERM_VIEW_ASSET] + ) + self.assertListEqual(someuser_log.metadata['permissions'][REMOVED], []) + self.assertListEqual(anotheruser_log.metadata['permissions'][REMOVED], []) + self.assertEqual(someuser_log.action, AuditAction.MODIFY_USER_PERMISSIONS) + self.assertEqual(anotheruser_log.action, AuditAction.MODIFY_USER_PERMISSIONS) + + @data( + # permission, use bulk endpoint, expected action, expected inverse action + ( + PERM_VIEW_ASSET, + True, + AuditAction.SHARE_FORM_PUBLICLY, + AuditAction.UNSHARE_FORM_PUBLICLY, + ), + ( + PERM_VIEW_SUBMISSIONS, + True, + AuditAction.SHARE_DATA_PUBLICLY, + AuditAction.UNSHARE_DATA_PUBLICLY, + ), + ( + PERM_ADD_SUBMISSIONS, + True, + AuditAction.ALLOW_ANONYMOUS_SUBMISSIONS, + AuditAction.DISALLOW_ANONYMOUS_SUBMISSIONS, + ), + ( + PERM_VIEW_ASSET, + False, + AuditAction.SHARE_FORM_PUBLICLY, + AuditAction.UNSHARE_FORM_PUBLICLY, + ), + ( + PERM_VIEW_SUBMISSIONS, + False, + AuditAction.SHARE_DATA_PUBLICLY, + AuditAction.UNSHARE_DATA_PUBLICLY, + ), + ( + PERM_ADD_SUBMISSIONS, + False, + AuditAction.ALLOW_ANONYMOUS_SUBMISSIONS, + AuditAction.DISALLOW_ANONYMOUS_SUBMISSIONS, + ), + ) + @unpack + def test_create_permission_creates_logs_for_anonymous_user( + self, permission, use_bulk, expected_action, expected_inverse_action + ): + request_data = { + 'permission': reverse( + 'api_v2:permission-detail', kwargs={'codename': permission} + ), + 'user': reverse( + 'api_v2:user-kpi-detail', kwargs={'username': 'AnonymousUser'} + ), + } + # /bulk expects assignments to come in a list + if use_bulk: + request_data = [request_data] + endpoint = 'bulk-assignments' if use_bulk else 'list' + self.client.post( + path=reverse( + f'api_v2:asset-permission-assignment-{endpoint}', + kwargs={ + 'parent_lookup_asset': self.asset.uid, + }, + ), + data=request_data, + format='json', + ) + log = ProjectHistoryLog.objects.filter(action=expected_action).first() + self.assertEqual(log.action, expected_action) + self._check_common_metadata( + log.metadata, PROJECT_HISTORY_LOG_PERMISSION_SUBTYPE + ) + + # get the permission that was created + perm = ObjectPermission.objects.filter( + user__username='AnonymousUser', permission__codename=permission + ).first() + # request to delete the permission we just created + self.client.delete( + path=reverse( + 'api_v2:asset-permission-assignment-detail', + kwargs={'parent_lookup_asset': self.asset.uid, 'uid': perm.uid}, + ), + ) + removal_log = ProjectHistoryLog.objects.filter( + action=expected_inverse_action + ).first() + self._check_common_metadata( + removal_log.metadata, PROJECT_HISTORY_LOG_PERMISSION_SUBTYPE + ) + + # use bulk endpoint? (as opposed to -detail) + @data(True, False) + def test_modify_permissions_log_includes_implied(self, use_bulk): + request_data = { + # change_submissions implies view_submissions, add_submissions, + # and view_asset + 'permission': reverse( + 'api_v2:permission-detail', kwargs={'codename': PERM_CHANGE_SUBMISSIONS} + ), + 'user': reverse('api_v2:user-kpi-detail', kwargs={'username': 'someuser'}), + } + if use_bulk: + request_data = [request_data] + endpoint = 'bulk-assignments' if use_bulk else 'list' + self.client.post( + path=reverse( + f'api_v2:asset-permission-assignment-{endpoint}', + kwargs={ + 'parent_lookup_asset': self.asset.uid, + }, + ), + data=request_data, + format='json', + ) + self.assertEqual(ProjectHistoryLog.objects.count(), 1) + log = ProjectHistoryLog.objects.filter( + action=AuditAction.MODIFY_USER_PERMISSIONS + ).first() + self._check_common_metadata( + log.metadata, PROJECT_HISTORY_LOG_PERMISSION_SUBTYPE + ) + self.assertListEqual( + sorted(log.metadata['permissions'][ADDED]), + ['add_submissions', 'change_submissions', 'view_asset', 'view_submissions'], + ) + self.assertListEqual(log.metadata['permissions'][REMOVED], []) + self.assertEqual(log.metadata['permissions']['username'], 'someuser') + + # removing view_asset should remove view_submissions and change_submissions + view_asset_perm = ObjectPermission.objects.filter( + user__username='someuser', permission__codename=PERM_VIEW_ASSET + ).first() + self.client.delete( + path=reverse( + 'api_v2:asset-permission-assignment-detail', + kwargs={ + 'parent_lookup_asset': self.asset.uid, + 'uid': view_asset_perm.uid, + }, + ), + ) + self.assertEqual(ProjectHistoryLog.objects.count(), 2) + removal_log = ( + ProjectHistoryLog.objects.filter(action=AuditAction.MODIFY_USER_PERMISSIONS) + .order_by('-date_created') + .first() + ) + self.assertListEqual(removal_log.metadata['permissions'][ADDED], []) + self.assertListEqual( + sorted(removal_log.metadata['permissions'][REMOVED]), + ['change_submissions', 'view_asset', 'view_submissions'], + ) + self.assertEqual(removal_log.metadata['permissions']['username'], 'someuser') + + @data(True, False) + def test_create_partial_permission_creates_log(self, use_bulk): + request_data = { + # partial change_submissions implies partial view_submissions, + # add_submissions, and view_asset + 'permission': reverse( + 'api_v2:permission-detail', + kwargs={'codename': PERM_PARTIAL_SUBMISSIONS}, + ), + 'user': reverse('api_v2:user-kpi-detail', kwargs={'username': 'someuser'}), + 'partial_permissions': [ + { + 'url': reverse( + 'api_v2:permission-detail', + kwargs={'codename': PERM_CHANGE_SUBMISSIONS}, + ), + 'filters': [{'_submitted_by': 'someuser'}], + } + ], + } + if use_bulk: + request_data = [request_data] + endpoint = 'bulk-assignments' if use_bulk else 'list' + self.client.post( + path=reverse( + f'api_v2:asset-permission-assignment-{endpoint}', + kwargs={ + 'parent_lookup_asset': self.asset.uid, + }, + ), + data=request_data, + format='json', + ) + self.assertEqual(ProjectHistoryLog.objects.count(), 1) + log = ProjectHistoryLog.objects.filter( + action=AuditAction.MODIFY_USER_PERMISSIONS + ).first() + self._check_common_metadata( + log.metadata, PROJECT_HISTORY_LOG_PERMISSION_SUBTYPE + ) + # can't sort a list of strings and dicts, + # so check length and expected entries individually + added = log.metadata['permissions'][ADDED] + self.assertEqual(len(added), 6) + # adding partial permissions always adds 'add_submissions', + # both partial and full + self.assertIn(PERM_ADD_SUBMISSIONS, added) + self.assertIn(PERM_VIEW_ASSET, added) + self.assertIn(PERM_PARTIAL_SUBMISSIONS, added) + + # inherited partial permissions + self.assertIn( + { + 'code': PERM_CHANGE_SUBMISSIONS, + 'filters': [{'_submitted_by': 'someuser'}], + }, + added, + ) + self.assertIn( + {'code': PERM_VIEW_SUBMISSIONS, 'filters': [{'_submitted_by': 'someuser'}]}, + added, + ) + self.assertIn( + {'code': PERM_ADD_SUBMISSIONS, 'filters': [{'_submitted_by': 'someuser'}]}, + added, + ) + + self.assertListEqual(log.metadata['permissions'][REMOVED], []) + self.assertEqual(log.metadata['permissions']['username'], 'someuser') + + # removing view_asset should remove view_submissions and change_submissions + partial_submissions_perm = ObjectPermission.objects.filter( + user__username='someuser', permission__codename=PERM_PARTIAL_SUBMISSIONS + ).first() + self.client.delete( + path=reverse( + 'api_v2:asset-permission-assignment-detail', + kwargs={ + 'parent_lookup_asset': self.asset.uid, + 'uid': partial_submissions_perm.uid, + }, + ), + ) + self.assertEqual(ProjectHistoryLog.objects.count(), 2) + removal_log = ( + ProjectHistoryLog.objects.filter(action=AuditAction.MODIFY_USER_PERMISSIONS) + .order_by('-date_created') + .first() + ) + self.assertListEqual(removal_log.metadata['permissions'][ADDED], []) + # we don't record what exact partial permissions were lost since all of them + # are removed at once + self.assertListEqual( + sorted(removal_log.metadata['permissions'][REMOVED]), + ['partial_submissions'], + ) + self.assertEqual(removal_log.metadata['permissions']['username'], 'someuser') + + def test_no_logs_if_no_permissions_change(self): + someuser = User.objects.get(username='someuser') + # assign a permission someone already has + self.asset.assign_perm(user_obj=someuser, perm=PERM_VIEW_ASSET) + self.client.post( + path=reverse( + 'api_v2:asset-permission-assignment-list', + kwargs={ + 'parent_lookup_asset': self.asset.uid, + }, + ), + data={ + 'permission': reverse( + 'api_v2:permission-detail', kwargs={'codename': PERM_VIEW_ASSET} + ), + 'user': reverse( + 'api_v2:user-kpi-detail', kwargs={'username': 'someuser'} + ), + }, + format='json', + ) + self.assertEqual(ProjectHistoryLog.objects.count(), 0) + + def test_no_logs_if_bulk_request_fails(self): + someuser = User.objects.get(username='someuser') + self.asset.assign_perm(user_obj=someuser, perm=PERM_VIEW_ASSET) + data = [ + { + 'permission': reverse( + 'api_v2:permission-detail', kwargs={'codename': PERM_VIEW_ASSET} + ), + 'user': reverse( + 'api_v2:user-kpi-detail', kwargs={'username': 'someuser'} + ), + }, + # trying to assign a permission to an admin will fail the request + { + 'permission': reverse( + 'api_v2:permission-detail', kwargs={'codename': PERM_VIEW_ASSET} + ), + 'user': reverse('api_v2:user-kpi-detail', kwargs={'username': 'admin'}), + }, + ] + self.client.post( + path=reverse( + 'api_v2:asset-permission-assignment-bulk-assignments', + kwargs={ + 'parent_lookup_asset': self.asset.uid, + }, + ), + data=data, + format='json', + ) + self.assertEqual(ProjectHistoryLog.objects.count(), 0) diff --git a/kobo/apps/audit_log/tests/test_signals.py b/kobo/apps/audit_log/tests/test_signals.py index 7c476bb0a9..d3e6b23c0a 100644 --- a/kobo/apps/audit_log/tests/test_signals.py +++ b/kobo/apps/audit_log/tests/test_signals.py @@ -2,8 +2,9 @@ from unittest.mock import patch from allauth.account.models import EmailAddress +from ddt import data, ddt from django.contrib.auth.signals import user_logged_in -from django.test import override_settings +from django.test import RequestFactory, override_settings from django.urls import reverse from trench.utils import get_mfa_model @@ -11,7 +12,19 @@ from kobo.apps.audit_log.models import AuditLog from kobo.apps.audit_log.signals import create_access_log from kobo.apps.kobo_auth.shortcuts import User +from kpi.constants import ( + PERM_PARTIAL_SUBMISSIONS, + PERM_VIEW_ASSET, + PERM_VIEW_SUBMISSIONS, +) +from kpi.models import Asset from kpi.tests.base_test_case import BaseTestCase +from kpi.utils.object_permission import ( + post_assign_partial_perm, + post_assign_perm, + post_remove_partial_perms, + post_remove_perm, +) @contextlib.contextmanager @@ -27,13 +40,14 @@ def skip_login_access_log(): user_logged_in.connect(create_access_log) -class AuditLogSignalsTestCase(BaseTestCase): +class AccessLogsSignalsTestCase(BaseTestCase): """ - Class for testing that logins produce AuditLogs. + Class for testing that logins produce AccessLogs. - Here we just test that AuditLogs are produced, not necessarily what they contain. More tests for what they contain - are in test_models.py. Also, AuditLogs for SSO logins are tested as part of the SSO tests - to avoid copying lots of complicated setup. + Here we just test that AccessLogs are produced, not necessarily what they contain. + More tests for what they contain are in test_models.py. Also, AccessLogs for SSO + logins are tested as part of the SSO tests to avoid copying lots of + complicated setup. """ @classmethod @@ -55,7 +69,7 @@ def test_audit_log_created_on_login(self, patched_create): def test_simple_login(self): count = AuditLog.objects.count() self.assertEqual(count, 0) - user = AuditLogSignalsTestCase.user + user = AccessLogsSignalsTestCase.user data = { 'login': 'user', 'password': 'pass', @@ -69,7 +83,7 @@ def test_simple_login(self): self.assertEqual(audit_log.action, AuditAction.AUTH) def test_login_with_email_verification(self): - user = AuditLogSignalsTestCase.user + user = AccessLogsSignalsTestCase.user data = { 'login': 'user', 'password': 'pass', @@ -88,7 +102,7 @@ def test_login_with_email_verification(self): def test_mfa_login(self): mfa_object = get_mfa_model().objects.create( - user=AuditLogSignalsTestCase.user, + user=AccessLogsSignalsTestCase.user, secret='dummy_mfa_secret', name='app', is_primary=True, @@ -97,7 +111,7 @@ def test_mfa_login(self): ) mfa_object.save() email_address, _ = EmailAddress.objects.get_or_create( - user=AuditLogSignalsTestCase.user + user=AccessLogsSignalsTestCase.user ) email_address.primary = True email_address.verified = True @@ -112,7 +126,7 @@ def test_mfa_login(self): with patch( 'kobo.apps.accounts.mfa.forms.authenticate_second_step_command', - return_value=AuditLogSignalsTestCase.user, + return_value=AccessLogsSignalsTestCase.user, ): self.client.post( reverse('mfa_token'), @@ -121,12 +135,12 @@ def test_mfa_login(self): ) self.assertEqual(AuditLog.objects.count(), 1) audit_log = AuditLog.objects.first() - self.assertEqual(audit_log.user.id, AuditLogSignalsTestCase.user.id) + self.assertEqual(audit_log.user.id, AccessLogsSignalsTestCase.user.id) self.assertEqual(audit_log.action, AuditAction.AUTH) def test_loginas(self): - AuditLogSignalsTestCase.user.is_superuser = True - AuditLogSignalsTestCase.user.save() + AccessLogsSignalsTestCase.user.is_superuser = True + AccessLogsSignalsTestCase.user.save() new_user = User.objects.create_user( 'user2', 'user2@example.com', 'pass2' ) @@ -138,3 +152,95 @@ def test_loginas(self): audit_log = AuditLog.objects.first() self.assertEqual(audit_log.user.id, new_user.id) self.assertEqual(audit_log.action, AuditAction.AUTH) + + +@ddt +class ProjectHistoryLogsSignalsTestCase(BaseTestCase): + + fixtures = ['test_data'] + + def setUp(self): + super().setUp() + self.wsgi_request = RequestFactory().post('/') + self.asset = Asset.objects.get(pk=1) + # 'someuser' owns the asset, so use 'anotheruser' for testing perm assignments + self.user = User.objects.get(username='anotheruser') + request_patcher = patch( + 'kobo.apps.audit_log.signals.get_current_request', + return_value=self.wsgi_request, + ) + + request_patcher.start() + self.addCleanup(request_patcher.stop) + + @data( + post_assign_perm, + post_remove_perm, + post_assign_partial_perm, + post_remove_partial_perms, + ) + def test_receivers_add_necessary_fields_to_request(self, signal): + # use all possible parameters for each signal + # so we can re-use the test + signal.send( + sender=Asset, + instance=self.asset, + user=self.user, + codename=PERM_PARTIAL_SUBMISSIONS, + perms={PERM_VIEW_ASSET: [{'_submitted_by': 'someuser'}]}, + deny=False, + ) + self.assertTrue(hasattr(self.wsgi_request, 'permissions_added')) + self.assertTrue(hasattr(self.wsgi_request, 'permissions_removed')) + self.assertTrue(hasattr(self.wsgi_request, 'partial_permissions_added')) + + def test_add_permission(self): + self.asset.assign_perm(self.user, PERM_VIEW_ASSET) + self.assertDictEqual( + self.wsgi_request.permissions_added, {'anotheruser': {PERM_VIEW_ASSET}} + ) + + def test_remove_permission(self): + self.asset.assign_perm(self.user, PERM_VIEW_ASSET) + self.asset.remove_perm(self.user, PERM_VIEW_ASSET) + self.assertDictEqual( + self.wsgi_request.permissions_removed, {'anotheruser': {PERM_VIEW_ASSET}} + ) + + def test_add_partial_permission(self): + self.asset.assign_perm( + self.user, + PERM_PARTIAL_SUBMISSIONS, + partial_perms={PERM_VIEW_SUBMISSIONS: [{'_submitted_by': 'someuser'}]}, + ) + self.assertDictEqual( + self.wsgi_request.partial_permissions_added, + { + 'anotheruser': [ + { + 'code': PERM_VIEW_SUBMISSIONS, + 'filters': [{'_submitted_by': 'someuser'}], + } + ] + }, + ) + + def test_remove_partial_permission(self): + self.asset.assign_perm( + self.user, + PERM_PARTIAL_SUBMISSIONS, + partial_perms={PERM_VIEW_SUBMISSIONS: [{'_submitted_by': 'someuser'}]}, + ) + self.asset.remove_perm(self.user, PERM_PARTIAL_SUBMISSIONS) + self.assertDictEqual( + self.wsgi_request.partial_permissions_added, {'anotheruser': []} + ) + self.assertDictEqual( + self.wsgi_request.permissions_removed, + {'anotheruser': {PERM_PARTIAL_SUBMISSIONS}}, + ) + + def test_deny_permission(self): + self.asset.assign_perm(self.user, PERM_VIEW_ASSET, deny=True) + # nothing added since the permission was denied + self.assertDictEqual(self.wsgi_request.partial_permissions_added, {}) diff --git a/kpi/mixins/object_permission.py b/kpi/mixins/object_permission.py index 81bc8e6b6c..e782ecfa14 100644 --- a/kpi/mixins/object_permission.py +++ b/kpi/mixins/object_permission.py @@ -525,6 +525,13 @@ def assign_perm(self, user_obj, perm, deny=False, defer_recalc=False, deny=deny, inherited=False ) + post_assign_perm.send( + sender=self.__class__, + instance=self, + user=user_obj, + codename=codename, + deny=deny, + ) # Assign any applicable KC permissions if not deny and not skip_kc: assign_applicable_kc_permissions(self, user_obj, codename) @@ -544,12 +551,6 @@ def assign_perm(self, user_obj, perm, deny=False, defer_recalc=False, self._update_partial_permissions( user_obj, perm, partial_perms=partial_perms ) - post_assign_perm.send( - sender=self.__class__, - instance=self, - user=user_obj, - codename=codename, - ) # Recalculate all descendants self.recalculate_descendants_perms() @@ -714,6 +715,7 @@ def remove_perm(self, user_obj, perm, defer_recalc=False, skip_kc=False): ) direct_permissions = all_permissions.filter(inherited=False) inherited_permissions = all_permissions.filter(inherited=True) + removed_perm_count = direct_permissions.count() + inherited_permissions.count() # Resolve implied permissions, e.g. revoking view implies revoking # change implied_perms = self.get_implied_perms( @@ -724,11 +726,21 @@ def remove_perm(self, user_obj, perm, defer_recalc=False, skip_kc=False): user_obj, implied_perm, defer_recalc=True) # Delete directly assigned permissions, if any direct_permissions.delete() + if inherited_permissions.exists(): # Delete inherited permissions inherited_permissions.delete() # Add a deny permission to block future inheritance self.assign_perm(user_obj, perm, deny=True, defer_recalc=True) + + if removed_perm_count > 0: + post_remove_perm.send( + sender=self.__class__, + instance=self, + user=user_obj, + codename=codename, + ) + # Remove any applicable KC permissions if not skip_kc: remove_applicable_kc_permissions(self, user_obj, codename) @@ -740,13 +752,6 @@ def remove_perm(self, user_obj, perm, defer_recalc=False, skip_kc=False): self._update_partial_permissions(user_obj, perm, remove=True) - post_remove_perm.send( - sender=self.__class__, - instance=self, - user=user_obj, - codename=codename, - ) - # Recalculate all descendants self.recalculate_descendants_perms() diff --git a/kpi/models/asset.py b/kpi/models/asset.py index cd77893161..1bf7c2b3f3 100644 --- a/kpi/models/asset.py +++ b/kpi/models/asset.py @@ -77,7 +77,11 @@ from kpi.models.asset_user_partial_permission import AssetUserPartialPermission from kpi.models.asset_version import AssetVersion from kpi.utils.asset_content_analyzer import AssetContentAnalyzer -from kpi.utils.object_permission import get_cached_code_names +from kpi.utils.object_permission import ( + get_cached_code_names, + post_assign_partial_perm, + post_remove_partial_perms, +) from kpi.utils.sluggify import sluggify_label @@ -1377,7 +1381,13 @@ def clean_up_table(): # Because of the unique constraint, there should be only # one record that matches this query. # We don't look for record existence to avoid extra query. - self.asset_partial_permissions.filter(user_id=user.pk).delete() + deleted, _ = self.asset_partial_permissions.filter(user_id=user.pk).delete() + if deleted > 0: + post_remove_partial_perms.send( + sender=self.__class__, + instance=self, + user=user, + ) if perm == PERM_PARTIAL_SUBMISSIONS: @@ -1404,6 +1414,12 @@ def clean_up_table(): asset_id=self.pk, user_id=user.pk, defaults={'permissions': new_partial_perms}) + post_assign_partial_perm.send( + sender=self.__class__, + perms=new_partial_perms, + instance=self, + user=user, + ) # There are no real partial permissions for 'add_submissions' but # 'change_submissions' implies it. So if 'add_submissions' is in the diff --git a/kpi/utils/object_permission.py b/kpi/utils/object_permission.py index 19022decc3..b933244e6b 100644 --- a/kpi/utils/object_permission.py +++ b/kpi/utils/object_permission.py @@ -5,7 +5,7 @@ import django.dispatch from django.apps import apps from django.conf import settings -from django.contrib.auth.models import Permission, AnonymousUser +from django.contrib.auth.models import AnonymousUser, Permission from django.contrib.contenttypes.models import ContentType from django.db import models from django.shortcuts import _get_queryset @@ -13,7 +13,7 @@ from rest_framework import serializers from kobo.apps.kobo_auth.shortcuts import User -from kpi.constants import PERM_VIEW_ASSET, PERM_MANAGE_ASSET, PERM_FROM_KC_ONLY +from kpi.constants import PERM_FROM_KC_ONLY, PERM_MANAGE_ASSET, PERM_VIEW_ASSET from kpi.utils.permissions import is_user_anonymous @@ -302,3 +302,5 @@ def perm_parse(perm, obj=None): post_assign_perm = django.dispatch.Signal() post_remove_perm = django.dispatch.Signal() +post_assign_partial_perm = django.dispatch.Signal() +post_remove_partial_perms = django.dispatch.Signal() diff --git a/kpi/views/v2/asset_permission_assignment.py b/kpi/views/v2/asset_permission_assignment.py index 6ec0b69857..e9f8b756bb 100644 --- a/kpi/views/v2/asset_permission_assignment.py +++ b/kpi/views/v2/asset_permission_assignment.py @@ -1,7 +1,8 @@ # coding: utf-8 + from django.shortcuts import get_object_or_404 from django.utils.translation import gettext as t -from rest_framework import exceptions, renderers, status, viewsets +from rest_framework import exceptions, renderers, status from rest_framework.decorators import action from rest_framework.mixins import ( CreateModelMixin, @@ -12,11 +13,8 @@ from rest_framework.response import Response from rest_framework_extensions.mixins import NestedViewSetMixin -from kpi.constants import ( - CLONE_ARG_NAME, - PERM_MANAGE_ASSET, - PERM_VIEW_ASSET, -) +from kobo.apps.audit_log.base_views import AuditLoggedViewSet +from kpi.constants import CLONE_ARG_NAME, PERM_MANAGE_ASSET, PERM_VIEW_ASSET from kpi.models.asset import Asset from kpi.models.object_permission import ObjectPermission from kpi.permissions import AssetPermissionAssignmentPermission @@ -24,20 +22,18 @@ AssetBulkInsertPermissionSerializer, AssetPermissionAssignmentSerializer, ) -from kpi.utils.object_permission import ( - get_user_permission_assignments_queryset, -) +from kpi.utils.object_permission import get_user_permission_assignments_queryset from kpi.utils.viewset_mixins import AssetNestedObjectViewsetMixin class AssetPermissionAssignmentViewSet( + AuditLoggedViewSet, AssetNestedObjectViewsetMixin, NestedViewSetMixin, CreateModelMixin, RetrieveModelMixin, DestroyModelMixin, ListModelMixin, - viewsets.GenericViewSet, ): """ ## Permission assignments of an asset @@ -167,6 +163,8 @@ class AssetPermissionAssignmentViewSet( serializer_class = AssetPermissionAssignmentSerializer permission_classes = (AssetPermissionAssignmentPermission,) pagination_class = None + log_type = 'project-history' + logged_fields = ['asset.id'] # filter_backends = Just kidding! Look at this instead: # kpi.utils.object_permission.get_user_permission_assignments_queryset @@ -183,6 +181,7 @@ def bulk_assignments(self, request, *args, **kwargs): :param request: :return: JSON """ + request._request.updated_data = {'asset.id': self.asset.id} serializer = AssetBulkInsertPermissionSerializer( data={'assignments': request.data}, context=self.get_serializer_context(), @@ -240,7 +239,9 @@ def destroy(self, request, *args, **kwargs): {'detail': t("Owner's permissions cannot be deleted")}, status=status.HTTP_409_CONFLICT, ) - + # we don't call perform_destroy, so manually attach the relevant + # information to the request + request._request.initial_data = {'asset.id': self.asset.id} codename = object_permission.permission.codename self.asset.remove_perm(user, codename) return Response(status=status.HTTP_204_NO_CONTENT) @@ -266,5 +267,5 @@ def get_queryset(self): self.asset, self.request.user ) - def perform_create(self, serializer): + def perform_create_override(self, serializer): serializer.save(asset=self.asset)