From 2548ba926aaca6be25ae5e26cbe0c55271edfedd Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sat, 13 Mar 2021 07:15:30 -0800 Subject: [PATCH 1/4] Fixes error when receiving Undo for unknown boost --- bookwyrm/activitypub/verbs.py | 3 +++ bookwyrm/tests/views/test_inbox.py | 17 +++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/bookwyrm/activitypub/verbs.py b/bookwyrm/activitypub/verbs.py index cd7a757b..1788dda7 100644 --- a/bookwyrm/activitypub/verbs.py +++ b/bookwyrm/activitypub/verbs.py @@ -70,6 +70,9 @@ class Undo(Verb): if self.object.type == "Follow": model = apps.get_model("bookwyrm.UserFollows") obj = self.object.to_model(model=model, save=False, allow_create=False) + if not obj: + # if we don't have the object, we can't undo it. happens a lot with boosts + return obj.delete() diff --git a/bookwyrm/tests/views/test_inbox.py b/bookwyrm/tests/views/test_inbox.py index cab50892..d43018db 100644 --- a/bookwyrm/tests/views/test_inbox.py +++ b/bookwyrm/tests/views/test_inbox.py @@ -563,6 +563,23 @@ class Inbox(TestCase): } views.inbox.activity_task(activity) + def test_handle_unboost_unknown_boost(self): + """ undo a boost """ + activity = { + "type": "Undo", + "actor": "hi", + "id": "bleh", + "to": ["https://www.w3.org/ns/activitystreams#public"], + "cc": ["https://example.com/user/mouse/followers"], + "object": { + "type": "Announce", + "id": "http://fake.com/unknown/boost", + "actor": self.remote_user.remote_id, + "object": self.status.remote_id, + }, + } + views.inbox.activity_task(activity) + def test_handle_add_book_to_shelf(self): """ shelving a book """ work = models.Work.objects.create(title="work title") From 1f4b3e958639c951a63c2335e4a828ca7d28ed01 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sat, 13 Mar 2021 07:38:19 -0800 Subject: [PATCH 2/4] Adds tests for status recipients property --- bookwyrm/tests/models/test_status_model.py | 121 +++++++++++++++------ 1 file changed, 87 insertions(+), 34 deletions(-) diff --git a/bookwyrm/tests/models/test_status_model.py b/bookwyrm/tests/models/test_status_model.py index 21982c20..ddc92d36 100644 --- a/bookwyrm/tests/models/test_status_model.py +++ b/bookwyrm/tests/models/test_status_model.py @@ -12,15 +12,26 @@ from django.utils import timezone from bookwyrm import models, settings +# pylint: disable=too-many-public-methods @patch("bookwyrm.models.Status.broadcast") class Status(TestCase): """ lotta types of statuses """ def setUp(self): """ useful things for creating a status """ - self.user = models.User.objects.create_user( + self.local_user = models.User.objects.create_user( "mouse", "mouse@mouse.mouse", "mouseword", local=True, localname="mouse" ) + with patch("bookwyrm.models.user.set_remote_server.delay"): + self.remote_user = models.User.objects.create_user( + "rat", + "rat@rat.com", + "ratword", + local=False, + remote_id="https://example.com/users/rat", + inbox="https://example.com/users/rat/inbox", + outbox="https://example.com/users/rat/outbox", + ) self.book = models.Edition.objects.create(title="Test Edition") image_file = pathlib.Path(__file__).parent.joinpath( @@ -34,22 +45,22 @@ class Status(TestCase): def test_status_generated_fields(self, _): """ setting remote id """ - status = models.Status.objects.create(content="bleh", user=self.user) + status = models.Status.objects.create(content="bleh", user=self.local_user) expected_id = "https://%s/user/mouse/status/%d" % (settings.DOMAIN, status.id) self.assertEqual(status.remote_id, expected_id) self.assertEqual(status.privacy, "public") def test_replies(self, _): """ get a list of replies """ - parent = models.Status.objects.create(content="hi", user=self.user) + parent = models.Status.objects.create(content="hi", user=self.local_user) child = models.Status.objects.create( - content="hello", reply_parent=parent, user=self.user + content="hello", reply_parent=parent, user=self.local_user ) models.Review.objects.create( - content="hey", reply_parent=parent, user=self.user, book=self.book + content="hey", reply_parent=parent, user=self.local_user, book=self.book ) models.Status.objects.create( - content="hi hello", reply_parent=child, user=self.user + content="hi hello", reply_parent=child, user=self.local_user ) replies = models.Status.replies(parent) @@ -75,15 +86,15 @@ class Status(TestCase): def test_to_replies(self, _): """ activitypub replies collection """ - parent = models.Status.objects.create(content="hi", user=self.user) + parent = models.Status.objects.create(content="hi", user=self.local_user) child = models.Status.objects.create( - content="hello", reply_parent=parent, user=self.user + content="hello", reply_parent=parent, user=self.local_user ) models.Review.objects.create( - content="hey", reply_parent=parent, user=self.user, book=self.book + content="hey", reply_parent=parent, user=self.local_user, book=self.book ) models.Status.objects.create( - content="hi hello", reply_parent=child, user=self.user + content="hi hello", reply_parent=child, user=self.local_user ) replies = parent.to_replies() @@ -92,7 +103,9 @@ class Status(TestCase): def test_status_to_activity(self, _): """ subclass of the base model version with a "pure" serializer """ - status = models.Status.objects.create(content="test content", user=self.user) + status = models.Status.objects.create( + content="test content", user=self.local_user + ) activity = status.to_activity() self.assertEqual(activity["id"], status.remote_id) self.assertEqual(activity["type"], "Note") @@ -103,7 +116,7 @@ class Status(TestCase): """ subclass of the base model version with a "pure" serializer """ status = models.Status.objects.create( content="test content", - user=self.user, + user=self.local_user, deleted=True, deleted_date=timezone.now(), ) @@ -114,7 +127,9 @@ class Status(TestCase): def test_status_to_pure_activity(self, _): """ subclass of the base model version with a "pure" serializer """ - status = models.Status.objects.create(content="test content", user=self.user) + status = models.Status.objects.create( + content="test content", user=self.local_user + ) activity = status.to_activity(pure=True) self.assertEqual(activity["id"], status.remote_id) self.assertEqual(activity["type"], "Note") @@ -125,10 +140,10 @@ class Status(TestCase): def test_generated_note_to_activity(self, _): """ subclass of the base model version with a "pure" serializer """ status = models.GeneratedNote.objects.create( - content="test content", user=self.user + content="test content", user=self.local_user ) status.mention_books.set([self.book]) - status.mention_users.set([self.user]) + status.mention_users.set([self.local_user]) activity = status.to_activity() self.assertEqual(activity["id"], status.remote_id) self.assertEqual(activity["type"], "GeneratedNote") @@ -139,10 +154,10 @@ class Status(TestCase): def test_generated_note_to_pure_activity(self, _): """ subclass of the base model version with a "pure" serializer """ status = models.GeneratedNote.objects.create( - content="test content", user=self.user + content="test content", user=self.local_user ) status.mention_books.set([self.book]) - status.mention_users.set([self.user]) + status.mention_users.set([self.local_user]) activity = status.to_activity(pure=True) self.assertEqual(activity["id"], status.remote_id) self.assertEqual( @@ -163,7 +178,7 @@ class Status(TestCase): def test_comment_to_activity(self, _): """ subclass of the base model version with a "pure" serializer """ status = models.Comment.objects.create( - content="test content", user=self.user, book=self.book + content="test content", user=self.local_user, book=self.book ) activity = status.to_activity() self.assertEqual(activity["id"], status.remote_id) @@ -174,7 +189,7 @@ class Status(TestCase): def test_comment_to_pure_activity(self, _): """ subclass of the base model version with a "pure" serializer """ status = models.Comment.objects.create( - content="test content", user=self.user, book=self.book + content="test content", user=self.local_user, book=self.book ) activity = status.to_activity(pure=True) self.assertEqual(activity["id"], status.remote_id) @@ -196,7 +211,7 @@ class Status(TestCase): status = models.Quotation.objects.create( quote="a sickening sense", content="test content", - user=self.user, + user=self.local_user, book=self.book, ) activity = status.to_activity() @@ -211,7 +226,7 @@ class Status(TestCase): status = models.Quotation.objects.create( quote="a sickening sense", content="test content", - user=self.user, + user=self.local_user, book=self.book, ) activity = status.to_activity(pure=True) @@ -235,7 +250,7 @@ class Status(TestCase): name="Review name", content="test content", rating=3, - user=self.user, + user=self.local_user, book=self.book, ) activity = status.to_activity() @@ -252,7 +267,7 @@ class Status(TestCase): name="Review name", content="test content", rating=3, - user=self.user, + user=self.local_user, book=self.book, ) activity = status.to_activity(pure=True) @@ -275,30 +290,34 @@ class Status(TestCase): def fav_broadcast_mock(_, activity, user): """ ok """ - self.assertEqual(user.remote_id, self.user.remote_id) + self.assertEqual(user.remote_id, self.local_user.remote_id) self.assertEqual(activity["type"], "Like") models.Favorite.broadcast = fav_broadcast_mock - status = models.Status.objects.create(content="test content", user=self.user) - fav = models.Favorite.objects.create(status=status, user=self.user) + status = models.Status.objects.create( + content="test content", user=self.local_user + ) + fav = models.Favorite.objects.create(status=status, user=self.local_user) # can't fav a status twice with self.assertRaises(IntegrityError): - models.Favorite.objects.create(status=status, user=self.user) + models.Favorite.objects.create(status=status, user=self.local_user) activity = fav.to_activity() self.assertEqual(activity["type"], "Like") - self.assertEqual(activity["actor"], self.user.remote_id) + self.assertEqual(activity["actor"], self.local_user.remote_id) self.assertEqual(activity["object"], status.remote_id) models.Favorite.broadcast = real_broadcast def test_boost(self, _): """ boosting, this one's a bit fussy """ - status = models.Status.objects.create(content="test content", user=self.user) - boost = models.Boost.objects.create(boosted_status=status, user=self.user) + status = models.Status.objects.create( + content="test content", user=self.local_user + ) + boost = models.Boost.objects.create(boosted_status=status, user=self.local_user) activity = boost.to_activity() - self.assertEqual(activity["actor"], self.user.remote_id) + self.assertEqual(activity["actor"], self.local_user.remote_id) self.assertEqual(activity["object"], status.remote_id) self.assertEqual(activity["type"], "Announce") self.assertEqual(activity, boost.to_activity(pure=True)) @@ -306,18 +325,20 @@ class Status(TestCase): def test_notification(self, _): """ a simple model """ notification = models.Notification.objects.create( - user=self.user, notification_type="FAVORITE" + user=self.local_user, notification_type="FAVORITE" ) self.assertFalse(notification.read) with self.assertRaises(IntegrityError): models.Notification.objects.create( - user=self.user, notification_type="GLORB" + user=self.local_user, notification_type="GLORB" ) def test_create_broadcast(self, broadcast_mock): """ should send out two verions of a status on create """ - models.Comment.objects.create(content="hi", user=self.user, book=self.book) + models.Comment.objects.create( + content="hi", user=self.local_user, book=self.book + ) self.assertEqual(broadcast_mock.call_count, 2) pure_call = broadcast_mock.call_args_list[0] bw_call = broadcast_mock.call_args_list[1] @@ -332,3 +353,35 @@ class Status(TestCase): args = bw_call[0][0] self.assertEqual(args["type"], "Create") self.assertEqual(args["object"]["type"], "Comment") + + def test_recipients_with_mentions(self, _): + """ get recipients to broadcast a status """ + status = models.GeneratedNote.objects.create( + content="test content", user=self.local_user + ) + status.mention_users.add(self.remote_user) + + self.assertEqual(status.recipients, [self.remote_user]) + + def test_recipients_with_reply_parent(self, _): + """ get recipients to broadcast a status """ + parent_status = models.GeneratedNote.objects.create( + content="test content", user=self.remote_user + ) + status = models.GeneratedNote.objects.create( + content="test content", user=self.local_user, reply_parent=parent_status + ) + + self.assertEqual(status.recipients, [self.remote_user]) + + def test_recipients_with_reply_parent_and_mentions(self, _): + """ get recipients to broadcast a status """ + parent_status = models.GeneratedNote.objects.create( + content="test content", user=self.remote_user + ) + status = models.GeneratedNote.objects.create( + content="test content", user=self.local_user, reply_parent=parent_status + ) + status.mention_users.set([self.remote_user]) + + self.assertEqual(status.recipients, [self.remote_user]) From 919b166241ee1b9b2193265d4a07f75e27096982 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sat, 13 Mar 2021 08:13:20 -0800 Subject: [PATCH 3/4] Catch error in serializing unknown boosts --- bookwyrm/activitypub/base_activity.py | 5 ++--- bookwyrm/connectors/abstract_connector.py | 2 +- bookwyrm/models/status.py | 9 +++++++-- bookwyrm/tests/models/test_status_model.py | 16 +++++++++++++++- 4 files changed, 25 insertions(+), 7 deletions(-) diff --git a/bookwyrm/activitypub/base_activity.py b/bookwyrm/activitypub/base_activity.py index 315ff58c..170bdfb9 100644 --- a/bookwyrm/activitypub/base_activity.py +++ b/bookwyrm/activitypub/base_activity.py @@ -258,10 +258,9 @@ def resolve_remote_id(remote_id, model=None, refresh=False, save=True): # load the data and create the object try: data = get_data(remote_id) - except (ConnectorException, ConnectionError): + except ConnectorException: raise ActivitySerializerError( - "Could not connect to host for remote_id in %s model: %s" - % (model.__name__, remote_id) + "Could not connect to host for remote_id in: %s" % (remote_id) ) # determine the model implicitly, if not provided if not model: diff --git a/bookwyrm/connectors/abstract_connector.py b/bookwyrm/connectors/abstract_connector.py index 9f31b337..3ebedf65 100644 --- a/bookwyrm/connectors/abstract_connector.py +++ b/bookwyrm/connectors/abstract_connector.py @@ -244,7 +244,7 @@ def get_data(url): "User-Agent": settings.USER_AGENT, }, ) - except (RequestError, SSLError) as e: + except (RequestError, SSLError, ConnectionError) as e: logger.exception(e) raise ConnectorException() diff --git a/bookwyrm/models/status.py b/bookwyrm/models/status.py index 80f2b593..c5e69936 100644 --- a/bookwyrm/models/status.py +++ b/bookwyrm/models/status.py @@ -115,13 +115,18 @@ class Status(OrderedCollectionPageMixin, BookWyrmModel): def ignore_activity(cls, activity): """ keep notes if they are replies to existing statuses """ if activity.type == "Announce": - # keep it if the booster or the boosted are local - boosted = activitypub.resolve_remote_id(activity.object, save=False) + try: + boosted = activitypub.resolve_remote_id(activity.object, save=False) + except activitypub.ActivitySerializerError: + # if we can't load the status, definitely ignore it + return True + # keep the boost if we would keep the status return cls.ignore_activity(boosted.to_activity_dataclass()) # keep if it if it's a custom type if activity.type != "Note": return False + # keep it if it's a reply to an existing status if cls.objects.filter(remote_id=activity.inReplyTo).exists(): return False diff --git a/bookwyrm/tests/models/test_status_model.py b/bookwyrm/tests/models/test_status_model.py index ddc92d36..b2ee69b8 100644 --- a/bookwyrm/tests/models/test_status_model.py +++ b/bookwyrm/tests/models/test_status_model.py @@ -8,8 +8,9 @@ from django.core.files.base import ContentFile from django.db import IntegrityError from django.test import TestCase from django.utils import timezone +import responses -from bookwyrm import models, settings +from bookwyrm import activitypub, models, settings # pylint: disable=too-many-public-methods @@ -385,3 +386,16 @@ class Status(TestCase): status.mention_users.set([self.remote_user]) self.assertEqual(status.recipients, [self.remote_user]) + + @responses.activate + def test_ignore_activity_boost(self, _): + """ don't bother with most remote statuses """ + activity = activitypub.Announce( + id="http://www.faraway.com/boost/12", + actor=self.remote_user.remote_id, + object="http://fish.com/nothing", + ) + + responses.add(responses.GET, "http://fish.com/nothing", status=404) + + self.assertTrue(models.Status.ignore_activity(activity)) From 3edfcb76318e6f09e6d14a8dfdc623ac5e6a8bfe Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sat, 13 Mar 2021 08:15:58 -0800 Subject: [PATCH 4/4] Trying to fix broadcast connectionerror exceptions --- bookwyrm/models/activitypub_mixin.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bookwyrm/models/activitypub_mixin.py b/bookwyrm/models/activitypub_mixin.py index 4ced78c2..691cad00 100644 --- a/bookwyrm/models/activitypub_mixin.py +++ b/bookwyrm/models/activitypub_mixin.py @@ -456,8 +456,8 @@ def broadcast_task(sender_id, activity, recipients): for recipient in recipients: try: sign_and_send(sender, activity, recipient) - except (HTTPError, SSLError, ConnectionError) as e: - logger.exception(e) + except (HTTPError, SSLError, ConnectionError): + pass def sign_and_send(sender, data, destination):