diff --git a/admin/nodes/views.py b/admin/nodes/views.py index a97fec18597..9ec37e6f797 100644 --- a/admin/nodes/views.py +++ b/admin/nodes/views.py @@ -110,13 +110,16 @@ def post(self, request, *args, **kwargs): node.created = new_date node.save() + params = dict(node.log_params) + params.update({ + 'last_date': str(last_date), + 'new_date': str(new_date), + }) node.add_log( action=NodeLog.REGISTRATION_DATE_UPDATED, - auth=request, - params={ - 'last_date': str(last_date), - 'new_date': str(new_date) - }, + auth=None, + foreign_user=NodeLog.SUPPORT_USER_LABEL, + params=params, log_date=timezone.now(), should_hide=False, ) @@ -225,22 +228,18 @@ def post(self, request, *args, **kwargs): message=f'User {user.pk} removed from {node.__class__.__name__.lower()} {node.pk}.', action_flag=CONTRIBUTOR_REMOVED ) - # Log invisibly on the OSF. - self.add_contributor_removed_log(node, user) - return redirect(self.get_success_url()) + params = dict(node.log_params) + params['contributors'] = [user._id] + node.add_log( + action=NodeLog.CONTRIB_REMOVED, + auth=None, + foreign_user=NodeLog.SUPPORT_USER_LABEL, + params=params, + log_date=timezone.now(), + should_hide=False, + ) - def add_contributor_removed_log(self, node, user): - NodeLog( - action=NodeLog.CONTRIB_REMOVED, - user=None, - params={ - 'project': node.parent_id, - 'node': node.pk, - 'contributors': user.pk - }, - date=timezone.now(), - should_hide=True, - ).save() + return redirect(self.get_success_url()) class NodeUpdatePermissionsView(NodeMixin, View): @@ -266,6 +265,7 @@ def post(self, request, *args, **kwargs): new_permissions_to_add = data.get('new-permissions', []) new_permission_indexes_to_remove = [] + added_contributor_ids = [] for email, permission in zip(new_emails_to_add, new_permissions_to_add): contributor_user = OSFUser.objects.filter(emails__address=email.lower()).first() if not contributor_user: @@ -281,8 +281,10 @@ def post(self, request, *args, **kwargs): auth=request, user_id=contributor_user._id, permissions=permission, - notification_type=None + notification_type=None, + log=False, ) + added_contributor_ids.append(contributor_user._id) messages.success(self.request, f'User with email {email} was successfully added.') # should remove permissions of invalid emails because @@ -291,6 +293,19 @@ def post(self, request, *args, **kwargs): for permission_index in new_permission_indexes_to_remove: new_permissions_to_add.pop(permission_index) + # Log support-added contributors, if any + if added_contributor_ids: + params = resource.log_params + params['contributors'] = added_contributor_ids + resource.add_log( + action=NodeLog.CONTRIB_ADDED, + auth=None, + foreign_user=NodeLog.SUPPORT_USER_LABEL, + params=params, + log_date=timezone.now(), + should_hide=False, + ) + updated_permissions = data.get('updated-permissions', []) all_permissions = updated_permissions + new_permissions_to_add has_admin = list(filter(lambda permission: ADMIN in permission, all_permissions)) @@ -298,6 +313,7 @@ def post(self, request, *args, **kwargs): messages.error(self.request, 'Must be at least one admin on this node.') return redirect(self.get_success_url()) + permissions_changed = {} for contributor_permission in updated_permissions: guid, permission = contributor_permission.split('-') user = OSFUser.load(guid) @@ -307,7 +323,21 @@ def post(self, request, *args, **kwargs): resource.get_visible(user), request, save=True, - skip_permission=True + skip_permission=True, + log=False, + ) + permissions_changed[user._id] = permission + + if permissions_changed: + params = resource.log_params + params['contributors'] = permissions_changed + resource.add_log( + action=NodeLog.PERMISSIONS_UPDATED, + auth=None, + foreign_user=NodeLog.SUPPORT_USER_LABEL, + params=params, + log_date=timezone.now(), + should_hide=False, ) return redirect(self.get_success_url()) @@ -332,15 +362,14 @@ def post(self, request, *args, **kwargs): message=f'Node {node.pk} restored.', action_flag=NODE_RESTORED ) - NodeLog( + node.add_log( action=NodeLog.NODE_CREATED, - user=None, - params={ - 'project': node.parent_id, - }, - date=timezone.now(), - should_hide=True, - ).save() + auth=None, + foreign_user=NodeLog.SUPPORT_USER_LABEL, + params=dict(node.log_params), + log_date=timezone.now(), + should_hide=False, + ) else: node.is_deleted = True node.deleted = timezone.now() @@ -352,15 +381,14 @@ def post(self, request, *args, **kwargs): message=f'Node {node.pk} removed.', action_flag=NODE_REMOVED ) - NodeLog( + node.add_log( action=NodeLog.NODE_REMOVED, - user=None, - params={ - 'project': node.parent_id, - }, - date=timezone.now(), - should_hide=True, - ).save() + auth=None, + foreign_user=NodeLog.SUPPORT_USER_LABEL, + params=dict(node.log_params), + log_date=timezone.now(), + should_hide=False, + ) node.save() return redirect(self.get_success_url()) @@ -852,6 +880,15 @@ def post(self, request, *args, **kwargs): node.save() + node.add_log( + action=NodeLog.MADE_PRIVATE, + auth=None, + foreign_user=NodeLog.SUPPORT_USER_LABEL, + params=dict(node.log_params), + log_date=timezone.now(), + should_hide=False, + ) + return redirect(self.get_success_url()) @@ -863,9 +900,18 @@ class NodeMakePublic(NodeMixin, View): def post(self, request, *args, **kwargs): node = self.get_object() try: - node.set_privacy('public') + node.set_privacy('public', auth=None, log=False) except NodeStateError as e: messages.error(request, str(e)) + else: + node.add_log( + action=NodeLog.MADE_PUBLIC, + auth=None, + foreign_user=NodeLog.SUPPORT_USER_LABEL, + params=dict(node.log_params), + log_date=timezone.now(), + should_hide=False, + ) return redirect(self.get_success_url()) @@ -892,10 +938,26 @@ def _remove_file_from_schema_response_blocks(registration, removed_file_id): # delete file from registration and metadata and keep it for original project if guid and (file := guid.referent) and (file.target == node) and not isinstance(file, TrashedFile): + file_id = file._id + file_path = getattr(file, 'materialized_path', None) or getattr(file, 'path', None) or '' + copied_from_id = getattr(file, 'copied_from_id', None) or getattr(getattr(file, 'copied_from', None), '_id', None) with transaction.atomic(): file.delete() _update_schema_meta(file.target) - _remove_file_from_schema_response_blocks(node, [file._id, file.copied_from._id]) + _remove_file_from_schema_response_blocks(node, [file_id, copied_from_id]) + params = dict(node.log_params) + params.update({ + 'pathType': 'file', + 'path': file_path, + }) + node.add_log( + action=NodeLog.FILE_REMOVED, + auth=None, + foreign_user=NodeLog.SUPPORT_USER_LABEL, + params=params, + log_date=timezone.now(), + should_hide=False, + ) return redirect(self.get_success_url()) @@ -932,7 +994,18 @@ def post(self, request, *args, **kwargs): parent=osfstorage.get_root(), name=osfstorage.archive_folder_name ).first() - file.copy_under(archive_folder) + copied = file.copy_under(archive_folder) + copied_path = getattr(copied, 'materialized_path', None) or getattr(copied, 'path', None) or '' + params = dict(registration.log_params) + params['path'] = copied_path + registration.add_log( + action=NodeLog.FILE_ADDED, + auth=None, + foreign_user=NodeLog.SUPPORT_USER_LABEL, + params=params, + log_date=timezone.now(), + should_hide=False, + ) messages.success(request, 'The file was successfully added.') return redirect(self.get_success_url()) @@ -959,8 +1032,21 @@ def post(self, request, *args, **kwargs): if not registration_file.exists(): messages.error(request, 'The file with the provided guid is not part of the registration.') return redirect(self.get_success_url()) - + file_path = getattr(file, 'materialized_path', None) or getattr(file, 'path', None) or '' registration_file.delete() + params = dict(registration.log_params) + params.update({ + 'pathType': 'file', + 'path': file_path, + }) + registration.add_log( + action=NodeLog.FILE_REMOVED, + auth=None, + foreign_user=NodeLog.SUPPORT_USER_LABEL, + params=params, + log_date=timezone.now(), + should_hide=False, + ) messages.success(request, 'The file was successfully removed.') return redirect(self.get_success_url()) diff --git a/admin_tests/nodes/test_views.py b/admin_tests/nodes/test_views.py index a81d1102b7a..1f0afe18e5d 100644 --- a/admin_tests/nodes/test_views.py +++ b/admin_tests/nodes/test_views.py @@ -20,13 +20,18 @@ Embargo, SchemaResponse, DraftRegistration, + ) from admin.nodes.views import ( NodeAddOsfStorageFileView, NodeConfirmSpamView, NodeDeleteView, + NodeMakePrivate, + NodeMakePublic, NodeRemoveContributorView, NodeRemoveOsfStorageFileView, + NodeUpdatePermissionsView, + RegistrationUpdateDateView, NodeView, NodeReindexShare, NodeReindexElastic, @@ -74,6 +79,14 @@ def patch_messages(request): setattr(request, '_messages', messages) +def assert_support_attributed_log(log, admin_user=None): + """Support admin actions attribute logs to the support label, not the staff user.""" + assert log.user is None + assert log.foreign_user == NodeLog.SUPPORT_USER_LABEL + if admin_user is not None: + assert log.user_id is None + + class TestNodeView(AdminTestCase): def test_get_flagged_spam(self): @@ -173,7 +186,9 @@ class TestNodeDeleteView(AdminTestCase): def setUp(self): super().setUp() self.node = ProjectFactory() + self.admin_user = AuthUserFactory() self.request = RequestFactory().post('/fake_path') + self.request.user = self.admin_user self.plain_view = NodeDeleteView self.view = setup_log_view(self.plain_view(), self.request, guid=self.node._id) self.url = reverse('nodes:remove', kwargs={'guid': self.node._id}) @@ -187,18 +202,33 @@ def test_remove_node(self): assert self.node.is_deleted assert AdminLogEntry.objects.count() == count + 1 assert self.node.deleted == mock_now + log = self.node.logs.filter( + action=NodeLog.NODE_REMOVED, + foreign_user=NodeLog.SUPPORT_USER_LABEL, + ).latest('date') + assert_support_attributed_log(log, self.admin_user) def test_restore_node(self): self.view.post(self.request) self.node.refresh_from_db() assert self.node.is_deleted assert self.node.deleted is not None + remove_log = self.node.logs.filter( + action=NodeLog.NODE_REMOVED, + foreign_user=NodeLog.SUPPORT_USER_LABEL, + ).latest('date') + assert_support_attributed_log(remove_log, self.admin_user) count = AdminLogEntry.objects.count() self.view.post(self.request) self.node.reload() assert not self.node.is_deleted assert self.node.deleted is None assert AdminLogEntry.objects.count() == count + 1 + restore_log = self.node.logs.filter( + action=NodeLog.NODE_CREATED, + foreign_user=NodeLog.SUPPORT_USER_LABEL, + ).latest('date') + assert_support_attributed_log(restore_log, self.admin_user) def test_no_user_permissions_raises_error(self): user = AuthUserFactory() @@ -234,12 +264,14 @@ class TestRemoveContributor(AdminTestCase): def setUp(self): super().setUp() self.user = AuthUserFactory() + self.admin_user = AuthUserFactory() self.node = ProjectFactory(creator=self.user) self.user_2 = AuthUserFactory() self.node.add_contributor(self.user_2) self.node.save() self.view = NodeRemoveContributorView self.request = RequestFactory().post('/fake_path') + self.request.user = self.admin_user self.url = reverse('nodes:remove-user', kwargs={'guid': self.node._id, 'user_id': self.user.id}) def test_remove_contributor(self): @@ -269,10 +301,16 @@ def test_do_not_remove_last_admin(self): assert len(list(self.node.get_admin_contributors(self.node.contributors))) == 1 assert AdminLogEntry.objects.count() == count - def test_no_log(self): + def test_log(self): + assert not self.node.logs.filter(action=NodeLog.CONTRIB_REMOVED).exists() view = setup_log_view(self.view(), self.request, guid=self.node._id, user_id=self.user_2.id) view.post(self.request) - assert self.node.logs.latest().action != NodeLog.CONTRIB_REMOVED + log = self.node.logs.filter( + action=NodeLog.CONTRIB_REMOVED, + foreign_user=NodeLog.SUPPORT_USER_LABEL, + ).latest('date') + assert_support_attributed_log(log, self.admin_user) + assert self.user_2._id in log.params['contributors'] def test_no_user_permissions_raises_error(self): guid = self.node._id @@ -938,7 +976,9 @@ def setUp(self): self.registration_without_registered_from.registered_from = None self.registration_without_registered_from.save() + self.admin_user = AuthUserFactory() self.request = RequestFactory().get('/fake_path') + self.request.user = self.admin_user patch_messages(self.request) def test_no_guid_found(self): @@ -987,6 +1027,11 @@ def test_file_is_added_to_registration_osfstorage(self): assert registration_osfstorage.get_root().children.get( name=registration_osfstorage.archive_folder_name ).children.filter(name=file.name).exists() + log = self.registration_registered_from.logs.filter( + action=NodeLog.FILE_ADDED, + foreign_user=NodeLog.SUPPORT_USER_LABEL, + ).latest('date') + assert_support_attributed_log(log, self.admin_user) class TestOsfStorageRegistrationFileRemove(AdminTestCase): @@ -1012,7 +1057,9 @@ def setUp(self): self.project = ProjectFactory() self.registration_registered_from = RegistrationFactory(project=self.project) + self.admin_user = AuthUserFactory() self.request = RequestFactory().get('/fake_path') + self.request.user = self.admin_user patch_messages(self.request) def test_no_guid_found(self): @@ -1069,3 +1116,120 @@ def test_file_is_removed_from_registration_osfstorage(self): name=registration_osfstorage.archive_folder_name ).children.exists() assert not self.registration_registered_from.files.exists() + remove_log = self.registration_registered_from.logs.filter( + action=NodeLog.FILE_REMOVED, + foreign_user=NodeLog.SUPPORT_USER_LABEL, + ).latest('date') + assert_support_attributed_log(remove_log, self.admin_user) + + +class TestNodeMakePrivate(AdminTestCase): + def setUp(self): + super().setUp() + self.admin_user = AuthUserFactory() + self.node = ProjectFactory(is_public=True) + self.request = RequestFactory().post('/fake_path') + self.request.user = self.admin_user + + def test_make_private(self): + view = setup_log_view(NodeMakePrivate(), self.request, guid=self.node._id) + view.post(self.request) + self.node.refresh_from_db() + assert self.node.is_public is False + log = self.node.logs.filter( + action=NodeLog.MADE_PRIVATE, + foreign_user=NodeLog.SUPPORT_USER_LABEL, + ).latest('date') + assert_support_attributed_log(log, self.admin_user) + + +class TestNodeMakePublic(AdminTestCase): + def setUp(self): + super().setUp() + self.admin_user = AuthUserFactory() + self.node = ProjectFactory(is_public=False) + self.request = RequestFactory().post('/fake_path') + self.request.user = self.admin_user + + def test_make_public(self): + view = setup_log_view(NodeMakePublic(), self.request, guid=self.node._id) + view.post(self.request) + self.node.refresh_from_db() + assert self.node.is_public is True + log = self.node.logs.filter( + action=NodeLog.MADE_PUBLIC, + foreign_user=NodeLog.SUPPORT_USER_LABEL, + ).latest('date') + assert_support_attributed_log(log, self.admin_user) + + +class TestNodeUpdatePermissions(AdminTestCase): + def setUp(self): + super().setUp() + self.admin_user = AuthUserFactory() + self.node = ProjectFactory(creator=self.admin_user) + self.other_admin = AuthUserFactory() + self.node.add_contributor(self.other_admin, permissions=permissions.ADMIN) + self.node.save() + self.request = RequestFactory().post('/fake_path') + patch_messages(self.request) + self.request.user = self.admin_user + + def test_update_permissions(self): + # Form must include at least one admin permission or the view rejects the POST. + self.request.POST = { + 'updated-permissions': [ + f'{self.admin_user._id}-{permissions.ADMIN}', + f'{self.other_admin._id}-{permissions.READ}', + ], + } + view = setup_log_view(NodeUpdatePermissionsView(), self.request, guid=self.node._id) + view.post(self.request) + log = self.node.logs.filter( + action=NodeLog.PERMISSIONS_UPDATED, + foreign_user=NodeLog.SUPPORT_USER_LABEL, + ).latest('date') + assert_support_attributed_log(log, self.admin_user) + assert log.params['contributors'] == { + self.admin_user._id: permissions.ADMIN, + self.other_admin._id: permissions.READ, + } + + def test_add_contributor(self): + new_user = AuthUserFactory() + self.request.POST = { + 'updated-permissions': [f'{self.admin_user._id}-{permissions.ADMIN}'], + 'new-emails': [new_user.emails.first().address], + 'new-permissions': [permissions.READ], + } + view = setup_log_view(NodeUpdatePermissionsView(), self.request, guid=self.node._id) + view.post(self.request) + log = self.node.logs.filter( + action=NodeLog.CONTRIB_ADDED, + foreign_user=NodeLog.SUPPORT_USER_LABEL, + ).latest('date') + assert_support_attributed_log(log, self.admin_user) + assert log.params['contributors'] == [new_user._id] + + +class TestRegistrationUpdateDate(AdminTestCase): + def setUp(self): + super().setUp() + self.admin_user = AuthUserFactory() + self.registration = RegistrationFactory() + self.request = RequestFactory().post('/fake_path') + patch_messages(self.request) + self.request.user = self.admin_user + + def test_update_registration_date(self): + new_date = (timezone.now() - datetime.timedelta(days=30)).replace(microsecond=0) + self.request.POST = {'registered_date': new_date.strftime('%Y-%m-%d %H:%M:%S')} + view = setup_log_view(RegistrationUpdateDateView(), self.request, guid=self.registration._id) + view.post(self.request) + self.registration.refresh_from_db() + assert self.registration.registered_date == new_date + log = self.registration.logs.filter( + action=NodeLog.REGISTRATION_DATE_UPDATED, + foreign_user=NodeLog.SUPPORT_USER_LABEL, + ).latest('date') + assert_support_attributed_log(log, self.admin_user) diff --git a/admin_tests/preprints/test_views.py b/admin_tests/preprints/test_views.py index 5582c1b07eb..02519290733 100644 --- a/admin_tests/preprints/test_views.py +++ b/admin_tests/preprints/test_views.py @@ -542,7 +542,8 @@ def test_do_not_remove_last_admin(self): assert len(list(self.preprint.get_admin_contributors(self.preprint.contributors))) == 1 assert AdminLogEntry.objects.count() == count - def test_no_log(self): + def test_log(self): + assert not self.preprint.logs.filter(action=PreprintLog.CONTRIB_REMOVED).exists() view = setup_log_view( self.view(), self.request, @@ -550,7 +551,7 @@ def test_no_log(self): user_id=self.user_2.id ) view.post(self.request) - assert self.preprint.logs.latest().action != PreprintLog.CONTRIB_REMOVED + assert self.preprint.logs.filter(action=PreprintLog.CONTRIB_REMOVED).exists() @pytest.mark.urls('admin.base.urls') diff --git a/api/logs/serializers.py b/api/logs/serializers.py index 51567b991f3..7768074c6e0 100644 --- a/api/logs/serializers.py +++ b/api/logs/serializers.py @@ -203,11 +203,13 @@ class NodeLogSerializer(JSONAPISerializer): 'id', 'date', 'action', + 'foreign_user', ] id = ser.CharField(read_only=True, source='_id') date = VersionedDateTimeField(read_only=True) action = ser.CharField(read_only=True) + foreign_user = ser.CharField(read_only=True) params = ser.SerializerMethodField(read_only=True) links = LinksField({'self': 'get_absolute_url'}) diff --git a/osf/models/mixins.py b/osf/models/mixins.py index 692075ef4c6..d8c0b976565 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -1796,7 +1796,7 @@ def copy_unclaimed_records(self, resource): contributor.save() # TODO: optimize me - def update_contributor(self, user, permission, visible, auth, save=False, skip_permission=False): + def update_contributor(self, user, permission, visible, auth, save=False, skip_permission=False, log=True): """ TODO: this method should be updated as a replacement for the main loop of Node#manage_contributors. Right now there are redundancies, but to avoid major feature creep this will not be included as this time. @@ -1826,14 +1826,15 @@ def update_contributor(self, user, permission, visible, auth, save=False, skip_p permissions_changed = { user._id: permission } - params = self.log_params - params['contributors'] = permissions_changed - self.add_log( - action=self.log_class.PERMISSIONS_UPDATED, - params=params, - auth=auth, - save=False - ) + if log: + params = self.log_params + params['contributors'] = permissions_changed + self.add_log( + action=self.log_class.PERMISSIONS_UPDATED, + params=params, + auth=auth, + save=False + ) with transaction.atomic(): if [READ] in permissions_changed.values(): project_signals.write_permissions_revoked.send(self) diff --git a/osf/models/nodelog.py b/osf/models/nodelog.py index 6aa14f461e5..c40390ed6d4 100644 --- a/osf/models/nodelog.py +++ b/osf/models/nodelog.py @@ -16,6 +16,7 @@ class NodeLog(ObjectIDMixin, BaseModel): } DATE_FORMAT = '%m/%d/%Y %H:%M UTC' + SUPPORT_USER_LABEL = 'an OSF Support Team Member' # Log action constants -- NOTE: templates stored in log_templates.mako CREATED_FROM = 'created_from'