From 33190613a251d8ae222a0d0ab767d2b5707509ad Mon Sep 17 00:00:00 2001
From: Eliot Berriot <contact@eliotberriot.com>
Date: Wed, 6 Mar 2019 15:33:39 +0100
Subject: [PATCH] Fix #737: delivering of local activities causing unintended
side effects, such as rollbacking changes
---
api/funkwhale_api/federation/activity.py | 111 ++++++++++++-----------
api/funkwhale_api/federation/tasks.py | 5 +-
api/tests/federation/test_activity.py | 10 ++
api/tests/federation/test_tasks.py | 10 +-
changes/changelog.d/737.bugfix | 1 +
5 files changed, 79 insertions(+), 58 deletions(-)
create mode 100644 changes/changelog.d/737.bugfix
diff --git a/api/funkwhale_api/federation/activity.py b/api/funkwhale_api/federation/activity.py
index b9f8ffd692..2436044d7e 100644
--- a/api/funkwhale_api/federation/activity.py
+++ b/api/funkwhale_api/federation/activity.py
@@ -173,69 +173,76 @@ class Router:
class InboxRouter(Router):
+ def get_matching_handlers(self, payload):
+ return [
+ handler for route, handler in self.routes if match_route(route, payload)
+ ]
+
@transaction.atomic
- def dispatch(self, payload, context):
+ def dispatch(self, payload, context, call_handlers=True):
"""
Receives an Activity payload and some context and trigger our
- business logic
+ business logic.
+
+ call_handlers should be False when are delivering a local activity, because
+ we want only want to bind activities to their recipients, not reapply the changes.
"""
from . import api_serializers
from . import models
- for route, handler in self.routes:
- if match_route(route, payload):
+ handlers = self.get_matching_handlers(payload)
+ for handler in handlers:
+ if call_handlers:
r = handler(payload, context=context)
- activity_obj = context.get("activity")
- if activity_obj and r:
- # handler returned additional data we can use
- # to update the activity target
- for key, value in r.items():
- setattr(activity_obj, key, value)
-
- update_fields = []
- for k in r.keys():
- if k in ["object", "target", "related_object"]:
- update_fields += [
- "{}_id".format(k),
- "{}_content_type".format(k),
- ]
- else:
- update_fields.append(k)
- activity_obj.save(update_fields=update_fields)
-
- if payload["type"] not in BROADCAST_TO_USER_ACTIVITIES:
- return
-
- inbox_items = context.get(
- "inbox_items", models.InboxItem.objects.none()
- )
- inbox_items = (
- inbox_items.select_related()
- .select_related("actor__user")
- .prefetch_related(
- "activity__object",
- "activity__target",
- "activity__related_object",
- )
+ else:
+ r = None
+ activity_obj = context.get("activity")
+ if activity_obj and r:
+ # handler returned additional data we can use
+ # to update the activity target
+ for key, value in r.items():
+ setattr(activity_obj, key, value)
+
+ update_fields = []
+ for k in r.keys():
+ if k in ["object", "target", "related_object"]:
+ update_fields += [
+ "{}_id".format(k),
+ "{}_content_type".format(k),
+ ]
+ else:
+ update_fields.append(k)
+ activity_obj.save(update_fields=update_fields)
+
+ if payload["type"] not in BROADCAST_TO_USER_ACTIVITIES:
+ return
+
+ inbox_items = context.get("inbox_items", models.InboxItem.objects.none())
+ inbox_items = (
+ inbox_items.select_related()
+ .select_related("actor__user")
+ .prefetch_related(
+ "activity__object", "activity__target", "activity__related_object"
)
+ )
- for ii in inbox_items:
- user = ii.actor.get_user()
- if not user:
- continue
- group = "user.{}.inbox".format(user.pk)
- channels.group_send(
- group,
- {
- "type": "event.send",
- "text": "",
- "data": {
- "type": "inbox.item_added",
- "item": api_serializers.InboxItemSerializer(ii).data,
- },
+ for ii in inbox_items:
+ user = ii.actor.get_user()
+ if not user:
+ continue
+ group = "user.{}.inbox".format(user.pk)
+ channels.group_send(
+ group,
+ {
+ "type": "event.send",
+ "text": "",
+ "data": {
+ "type": "inbox.item_added",
+ "item": api_serializers.InboxItemSerializer(ii).data,
},
- )
- return
+ },
+ )
+ return
ACTOR_KEY_ROTATION_LOCK_CACHE_KEY = "federation:actor-key-rotation-lock:{}"
diff --git a/api/funkwhale_api/federation/tasks.py b/api/funkwhale_api/federation/tasks.py
index f7d8913b76..e7aafc2e85 100644
--- a/api/funkwhale_api/federation/tasks.py
+++ b/api/funkwhale_api/federation/tasks.py
@@ -71,7 +71,7 @@ def get_files(storage, *parts):
@celery.app.task(name="federation.dispatch_inbox")
@celery.require_instance(models.Activity.objects.select_related(), "activity")
-def dispatch_inbox(activity):
+def dispatch_inbox(activity, call_handlers=True):
"""
Given an activity instance, triggers our internal delivery logic (follow
creation, etc.)
@@ -84,6 +84,7 @@ def dispatch_inbox(activity):
"actor": activity.actor,
"inbox_items": activity.inbox_items.filter(is_read=False),
},
+ call_handlers=call_handlers,
)
@@ -96,7 +97,7 @@ def dispatch_outbox(activity):
inbox_items = activity.inbox_items.filter(is_read=False).select_related()
if inbox_items.exists():
- dispatch_inbox.delay(activity_id=activity.pk)
+ dispatch_inbox.delay(activity_id=activity.pk, call_handlers=False)
if not preferences.get("federation__enabled"):
# federation is disabled, we only deliver to local recipients
diff --git a/api/tests/federation/test_activity.py b/api/tests/federation/test_activity.py
index fa83ed1f4d..a1eedeb493 100644
--- a/api/tests/federation/test_activity.py
+++ b/api/tests/federation/test_activity.py
@@ -190,6 +190,16 @@ def test_inbox_routing(factories, mocker):
assert a.target == target
+def test_inbox_routing_no_handler(factories, mocker):
+ router = activity.InboxRouter()
+ a = factories["federation.Activity"](type="Follow")
+ handler = mocker.Mock()
+ router.connect({"type": "Follow"}, handler)
+
+ router.dispatch({"type": "Follow"}, context={"activity": a}, call_handlers=False)
+ handler.assert_not_called()
+
+
def test_inbox_routing_send_to_channel(factories, mocker):
group_send = mocker.patch("funkwhale_api.common.channels.group_send")
a = factories["federation.Activity"](type="Follow")
diff --git a/api/tests/federation/test_tasks.py b/api/tests/federation/test_tasks.py
index f3216eed77..21aa181f87 100644
--- a/api/tests/federation/test_tasks.py
+++ b/api/tests/federation/test_tasks.py
@@ -74,10 +74,12 @@ def test_handle_in(factories, mocker, now, queryset_equal_list):
a = factories["federation.Activity"](payload={"hello": "world"})
ii1 = factories["federation.InboxItem"](activity=a, actor=r1)
ii2 = factories["federation.InboxItem"](activity=a, actor=r2)
- tasks.dispatch_inbox(activity_id=a.pk)
+ tasks.dispatch_inbox(activity_id=a.pk, call_handlers=False)
mocked_dispatch.assert_called_once_with(
- a.payload, context={"actor": a.actor, "activity": a, "inbox_items": [ii1, ii2]}
+ a.payload,
+ context={"actor": a.actor, "activity": a, "inbox_items": [ii1, ii2]},
+ call_handlers=False,
)
@@ -90,7 +92,7 @@ def test_dispatch_outbox(factories, mocker):
factories["federation.InboxItem"](activity=activity)
delivery = factories["federation.Delivery"](activity=activity)
tasks.dispatch_outbox(activity_id=activity.pk)
- mocked_inbox.assert_called_once_with(activity_id=activity.pk)
+ mocked_inbox.assert_called_once_with(activity_id=activity.pk, call_handlers=False)
mocked_deliver_to_remote.assert_called_once_with(delivery_id=delivery.pk)
@@ -104,7 +106,7 @@ def test_dispatch_outbox_disabled_federation(factories, mocker, preferences):
factories["federation.InboxItem"](activity=activity)
factories["federation.Delivery"](activity=activity)
tasks.dispatch_outbox(activity_id=activity.pk)
- mocked_inbox.assert_called_once_with(activity_id=activity.pk)
+ mocked_inbox.assert_called_once_with(activity_id=activity.pk, call_handlers=False)
mocked_deliver_to_remote.assert_not_called()
diff --git a/changes/changelog.d/737.bugfix b/changes/changelog.d/737.bugfix
new file mode 100644
index 0000000000..c65050a6ee
--- /dev/null
+++ b/changes/changelog.d/737.bugfix
@@ -0,0 +1 @@
+Fixed delivering of local activities causing unintended side effects, such as rollbacking changes (#737)
--
GitLab