From 5df2ac676b50c2304d0d8765e2c379471d7731f5 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sat, 17 Apr 2021 12:39:16 -0700 Subject: [PATCH 1/5] Fixes error on deletion requests for unknown users --- bookwyrm/activitypub/verbs.py | 21 ++++++---- .../tests/views/inbox/test_inbox_delete.py | 40 ++++++++++++++++++- 2 files changed, 52 insertions(+), 9 deletions(-) diff --git a/bookwyrm/activitypub/verbs.py b/bookwyrm/activitypub/verbs.py index 442e3858..c2cbfea3 100644 --- a/bookwyrm/activitypub/verbs.py +++ b/bookwyrm/activitypub/verbs.py @@ -16,11 +16,10 @@ class Verb(ActivityObject): def action(self): """ usually we just want to update and save """ - obj = self.object - # it may return None if the object is invalid in an expected way + # self.object may return None if the object is invalid in an expected way # ie, Question type - if obj: - obj.to_model() + if self.object: + self.object.to_model() @dataclass(init=False) @@ -43,7 +42,16 @@ class Delete(Verb): def action(self): """ find and delete the activity object """ - obj = self.object.to_model(save=False, allow_create=False) + if not self.object: + return + + if isinstance(self.object, str): + # Deleted users are passed as strings. Not wild about this fix + model = apps.get_model("bookwyrm.User") + obj = model.find_existing_by_remote_id(self.object) + else: + obj = self.object.to_model(save=False, allow_create=False) + if obj: obj.delete() # if we can't find it, we don't need to delete it because we don't have it @@ -58,8 +66,7 @@ class Update(Verb): def action(self): """ update a model instance from the dataclass """ - obj = self.object - if obj: + if self.object: self.object.to_model(allow_create=False) diff --git a/bookwyrm/tests/views/inbox/test_inbox_delete.py b/bookwyrm/tests/views/inbox/test_inbox_delete.py index 65a75426..51c59015 100644 --- a/bookwyrm/tests/views/inbox/test_inbox_delete.py +++ b/bookwyrm/tests/views/inbox/test_inbox_delete.py @@ -49,7 +49,7 @@ class InboxActivities(TestCase): } models.SiteSettings.objects.create() - def test_handle_delete_status(self): + def test_delete_status(self): """ remove a status """ self.assertFalse(self.status.deleted) activity = { @@ -70,7 +70,7 @@ class InboxActivities(TestCase): self.assertTrue(status.deleted) self.assertIsInstance(status.deleted_date, datetime) - def test_handle_delete_status_notifications(self): + def test_delete_status_notifications(self): """ remove a status with related notifications """ models.Notification.objects.create( related_status=self.status, @@ -104,3 +104,39 @@ class InboxActivities(TestCase): # notifications should be truly deleted self.assertEqual(models.Notification.objects.count(), 1) self.assertEqual(models.Notification.objects.get(), notif) + + def test_delete_user(self): + """ delete a user """ + self.assertTrue( + models.User.objects.get(username="rat@example.com").is_active + ) + activity = { + '@context': 'https://www.w3.org/ns/activitystreams', + 'id': 'https://example.com/users/test-user#delete', + 'type': 'Delete', + 'actor': 'https://example.com/users/test-user', + 'to': ['https://www.w3.org/ns/activitystreams#Public'], + 'object': 'https://example.com/users/test-user', + } + + views.inbox.activity_task(activity) + self.assertFalse( + models.User.objects.get(username="rat@example.com").is_active + ) + + + def test_delete_user_unknown(self): + """ don't worry about it if we don't know the user """ + self.assertEqual(models.User.objects.filter(is_active=True).count(), 2) + activity = { + '@context': 'https://www.w3.org/ns/activitystreams', + 'id': 'https://example.com/users/test-user#delete', + 'type': 'Delete', + 'actor': 'https://example.com/users/test-user', + 'to': ['https://www.w3.org/ns/activitystreams#Public'], + 'object': 'https://example.com/users/test-user', + } + + # nothing happens. + views.inbox.activity_task(activity) + self.assertEqual(models.User.objects.filter(is_active=True).count(), 2) From 5b7f2007e8f30c080d25f378cd0062fe04e67114 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sat, 17 Apr 2021 12:51:36 -0700 Subject: [PATCH 2/5] Broadcast user deletions --- bookwyrm/models/activitypub_mixin.py | 2 +- bookwyrm/models/user.py | 6 ++++++ bookwyrm/tests/views/inbox/test_inbox_delete.py | 2 +- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/bookwyrm/models/activitypub_mixin.py b/bookwyrm/models/activitypub_mixin.py index c6ba249c..efdf9a47 100644 --- a/bookwyrm/models/activitypub_mixin.py +++ b/bookwyrm/models/activitypub_mixin.py @@ -241,7 +241,7 @@ class ObjectMixin(ActivitypubMixin): return # is this a deletion? - if hasattr(self, "deleted") and self.deleted: + if (hasattr(self, "deleted") and self.deleted) or hasattr(self, "is_active") and not self.is_active: activity = self.to_delete_activity(user) else: activity = self.to_update_activity(user) diff --git a/bookwyrm/models/user.py b/bookwyrm/models/user.py index 15ceb19b..d0a8a34f 100644 --- a/bookwyrm/models/user.py +++ b/bookwyrm/models/user.py @@ -283,6 +283,12 @@ class User(OrderedCollectionPageMixin, AbstractUser): editable=False, ).save(broadcast=False) + def delete(self, *args, **kwargs): + """ deactivate rather than delete a user """ + self.is_active = False + # skip the logic in this class's save() + super().save(*args, **kwargs) + @property def local_path(self): """ this model doesn't inherit bookwyrm model, so here we are """ diff --git a/bookwyrm/tests/views/inbox/test_inbox_delete.py b/bookwyrm/tests/views/inbox/test_inbox_delete.py index 51c59015..88ef288e 100644 --- a/bookwyrm/tests/views/inbox/test_inbox_delete.py +++ b/bookwyrm/tests/views/inbox/test_inbox_delete.py @@ -116,7 +116,7 @@ class InboxActivities(TestCase): 'type': 'Delete', 'actor': 'https://example.com/users/test-user', 'to': ['https://www.w3.org/ns/activitystreams#Public'], - 'object': 'https://example.com/users/test-user', + 'object': self.remote_user.remote_id, } views.inbox.activity_task(activity) From ebdbdc8790ff12417548ec5dea09b92f46610ad8 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sat, 17 Apr 2021 13:19:23 -0700 Subject: [PATCH 3/5] Broadcast deletions --- bookwyrm/models/activitypub_mixin.py | 6 +++- bookwyrm/models/user.py | 6 ++++ bookwyrm/tests/models/test_user_model.py | 15 +++++++++ .../tests/views/inbox/test_inbox_delete.py | 33 ++++++++----------- 4 files changed, 40 insertions(+), 20 deletions(-) diff --git a/bookwyrm/models/activitypub_mixin.py b/bookwyrm/models/activitypub_mixin.py index efdf9a47..26ad7115 100644 --- a/bookwyrm/models/activitypub_mixin.py +++ b/bookwyrm/models/activitypub_mixin.py @@ -241,7 +241,11 @@ class ObjectMixin(ActivitypubMixin): return # is this a deletion? - if (hasattr(self, "deleted") and self.deleted) or hasattr(self, "is_active") and not self.is_active: + if ( + (hasattr(self, "deleted") and self.deleted) + or hasattr(self, "is_active") + and not self.is_active + ): activity = self.to_delete_activity(user) else: activity = self.to_update_activity(user) diff --git a/bookwyrm/models/user.py b/bookwyrm/models/user.py index d0a8a34f..b26b3b00 100644 --- a/bookwyrm/models/user.py +++ b/bookwyrm/models/user.py @@ -289,6 +289,12 @@ class User(OrderedCollectionPageMixin, AbstractUser): # skip the logic in this class's save() super().save(*args, **kwargs) + def to_activity(self, *args, **kwargs): + """ return a remote_id only if the user is inactive """ + if not self.is_active: + return self.remote_id + return super().to_activity_dataclass(*args, **kwargs) + @property def local_path(self): """ this model doesn't inherit bookwyrm model, so here we are """ diff --git a/bookwyrm/tests/models/test_user_model.py b/bookwyrm/tests/models/test_user_model.py index bd5255ce..883ef669 100644 --- a/bookwyrm/tests/models/test_user_model.py +++ b/bookwyrm/tests/models/test_user_model.py @@ -1,4 +1,5 @@ """ testing models """ +import json from unittest.mock import patch from django.test import TestCase import responses @@ -152,3 +153,17 @@ class User(TestCase): self.assertEqual(server.server_name, DOMAIN) self.assertIsNone(server.application_type) self.assertIsNone(server.application_version) + + def test_delete_user(self): + """ deactivate a user """ + self.assertTrue(self.user.is_active) + with patch( + "bookwyrm.models.activitypub_mixin.broadcast_task.delay" + ) as broadcast_mock: + self.user.delete() + + self.assertEqual(broadcast_mock.call_count, 1) + activity = json.loads(broadcast_mock.call_args[0][1]) + self.assertEqual(activity["type"], "Delete") + self.assertEqual(activity["object"], self.user.remote_id) + self.assertFalse(self.user.is_active) diff --git a/bookwyrm/tests/views/inbox/test_inbox_delete.py b/bookwyrm/tests/views/inbox/test_inbox_delete.py index 88ef288e..03598b88 100644 --- a/bookwyrm/tests/views/inbox/test_inbox_delete.py +++ b/bookwyrm/tests/views/inbox/test_inbox_delete.py @@ -107,34 +107,29 @@ class InboxActivities(TestCase): def test_delete_user(self): """ delete a user """ - self.assertTrue( - models.User.objects.get(username="rat@example.com").is_active - ) + self.assertTrue(models.User.objects.get(username="rat@example.com").is_active) activity = { - '@context': 'https://www.w3.org/ns/activitystreams', - 'id': 'https://example.com/users/test-user#delete', - 'type': 'Delete', - 'actor': 'https://example.com/users/test-user', - 'to': ['https://www.w3.org/ns/activitystreams#Public'], - 'object': self.remote_user.remote_id, + "@context": "https://www.w3.org/ns/activitystreams", + "id": "https://example.com/users/test-user#delete", + "type": "Delete", + "actor": "https://example.com/users/test-user", + "to": ["https://www.w3.org/ns/activitystreams#Public"], + "object": self.remote_user.remote_id, } views.inbox.activity_task(activity) - self.assertFalse( - models.User.objects.get(username="rat@example.com").is_active - ) - + self.assertFalse(models.User.objects.get(username="rat@example.com").is_active) def test_delete_user_unknown(self): """ don't worry about it if we don't know the user """ self.assertEqual(models.User.objects.filter(is_active=True).count(), 2) activity = { - '@context': 'https://www.w3.org/ns/activitystreams', - 'id': 'https://example.com/users/test-user#delete', - 'type': 'Delete', - 'actor': 'https://example.com/users/test-user', - 'to': ['https://www.w3.org/ns/activitystreams#Public'], - 'object': 'https://example.com/users/test-user', + "@context": "https://www.w3.org/ns/activitystreams", + "id": "https://example.com/users/test-user#delete", + "type": "Delete", + "actor": "https://example.com/users/test-user", + "to": ["https://www.w3.org/ns/activitystreams#Public"], + "object": "https://example.com/users/test-user", } # nothing happens. From 44528eaa09057d1d53e1177e03b563b2cb135799 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sat, 17 Apr 2021 13:31:37 -0700 Subject: [PATCH 4/5] Don't double-override to_activity --- bookwyrm/models/user.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/bookwyrm/models/user.py b/bookwyrm/models/user.py index b26b3b00..9585a4c4 100644 --- a/bookwyrm/models/user.py +++ b/bookwyrm/models/user.py @@ -205,6 +205,9 @@ class User(OrderedCollectionPageMixin, AbstractUser): def to_activity(self, **kwargs): """override default AP serializer to add context object idk if this is the best way to go about this""" + if not self.is_active: + return self.remote_id + activity_object = super().to_activity(**kwargs) activity_object["@context"] = [ "https://www.w3.org/ns/activitystreams", @@ -289,12 +292,6 @@ class User(OrderedCollectionPageMixin, AbstractUser): # skip the logic in this class's save() super().save(*args, **kwargs) - def to_activity(self, *args, **kwargs): - """ return a remote_id only if the user is inactive """ - if not self.is_active: - return self.remote_id - return super().to_activity_dataclass(*args, **kwargs) - @property def local_path(self): """ this model doesn't inherit bookwyrm model, so here we are """ From 8421a2e83258b20642c3e3dab6dec5253bb71366 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sat, 17 Apr 2021 15:14:23 -0700 Subject: [PATCH 5/5] Clarifies logic in if statement --- bookwyrm/models/activitypub_mixin.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/bookwyrm/models/activitypub_mixin.py b/bookwyrm/models/activitypub_mixin.py index 26ad7115..ce6245b2 100644 --- a/bookwyrm/models/activitypub_mixin.py +++ b/bookwyrm/models/activitypub_mixin.py @@ -241,10 +241,8 @@ class ObjectMixin(ActivitypubMixin): return # is this a deletion? - if ( - (hasattr(self, "deleted") and self.deleted) - or hasattr(self, "is_active") - and not self.is_active + if (hasattr(self, "deleted") and self.deleted) or ( + hasattr(self, "is_active") and not self.is_active ): activity = self.to_delete_activity(user) else: