From 4a412d36a993f97c69b57ecd4d75372769185caf Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Tue, 14 May 2019 10:59:49 +0200 Subject: [PATCH] Fix #830: Better handling of follow/accept messages to avoid and recover from desync between instances --- api/funkwhale_api/federation/serializers.py | 23 ++++++++- api/tests/federation/test_routes.py | 38 +++++++++++++++ changes/changelog.d/830.enhancement | 1 + front/src/views/content/remote/Card.vue | 53 ++++++++++++--------- 4 files changed, 90 insertions(+), 25 deletions(-) create mode 100644 changes/changelog.d/830.enhancement diff --git a/api/funkwhale_api/federation/serializers.py b/api/funkwhale_api/federation/serializers.py index 7d71e38da..473585fe7 100644 --- a/api/funkwhale_api/federation/serializers.py +++ b/api/funkwhale_api/federation/serializers.py @@ -1,6 +1,7 @@ import logging import mimetypes import urllib.parse +import uuid from django.core.exceptions import ObjectDoesNotExist from django.core.paginator import Paginator @@ -297,11 +298,29 @@ class FollowSerializer(serializers.Serializer): follow_class = models.Follow defaults = kwargs defaults["fid"] = self.validated_data["id"] - return follow_class.objects.update_or_create( + approved = kwargs.pop("approved", None) + follow, created = follow_class.objects.update_or_create( actor=self.validated_data["actor"], target=self.validated_data["object"], defaults=defaults, - )[0] + ) + if not created: + # We likely received a new follow when we had an existing one in database + # this can happen when two instances are out of sync, e.g because some + # messages are not delivered properly. In this case, we don't change + # the follow approved status and return the follow as is. + # We set a new UUID to ensure the follow urls are updated properly + # cf #830 + follow.uuid = uuid.uuid4() + follow.save(update_fields=["uuid"]) + return follow + + # it's a brand new follow, we use the approved value stored earlier + if approved != follow.approved: + follow.approved = approved + follow.save(update_fields=["approved"]) + + return follow def to_representation(self, instance): return { diff --git a/api/tests/federation/test_routes.py b/api/tests/federation/test_routes.py index 5dfef61d3..56834d55f 100644 --- a/api/tests/federation/test_routes.py +++ b/api/tests/federation/test_routes.py @@ -117,6 +117,44 @@ def test_inbox_follow_library_manual_approve(factories, mocker): mocked_outbox_dispatch.assert_not_called() +def test_inbox_follow_library_already_approved(factories, mocker): + """Cf #830, out of sync follows""" + mocked_outbox_dispatch = mocker.patch( + "funkwhale_api.federation.activity.OutboxRouter.dispatch" + ) + + local_actor = factories["users.User"]().create_actor() + remote_actor = factories["federation.Actor"]() + library = factories["music.Library"](actor=local_actor, privacy_level="me") + ii = factories["federation.InboxItem"](actor=local_actor) + existing_follow = factories["federation.LibraryFollow"]( + target=library, actor=remote_actor, approved=True + ) + payload = { + "type": "Follow", + "id": "https://test.follow", + "actor": remote_actor.fid, + "object": library.fid, + } + + result = routes.inbox_follow( + payload, + context={"actor": remote_actor, "inbox_items": [ii], "raise_exception": True}, + ) + follow = library.received_follows.latest("id") + + assert result["object"] == library + assert result["related_object"] == follow + + assert follow.fid == payload["id"] + assert follow.actor == remote_actor + assert follow.approved is True + assert follow.uuid != existing_follow.uuid + mocked_outbox_dispatch.assert_called_once_with( + {"type": "Accept"}, context={"follow": follow} + ) + + def test_outbox_accept(factories, mocker): remote_actor = factories["federation.Actor"]() follow = factories["federation.LibraryFollow"](actor=remote_actor) diff --git a/changes/changelog.d/830.enhancement b/changes/changelog.d/830.enhancement new file mode 100644 index 000000000..190c1127d --- /dev/null +++ b/changes/changelog.d/830.enhancement @@ -0,0 +1 @@ +Better handling of follow/accept messages to avoid and recover from desync between instances (#830) diff --git a/front/src/views/content/remote/Card.vue b/front/src/views/content/remote/Card.vue index 1309b2f2b..b78f7f220 100644 --- a/front/src/views/content/remote/Card.vue +++ b/front/src/views/content/remote/Card.vue @@ -80,35 +80,41 @@ -
+
- - - - Unfollow -

Unfollow this library?

-
-

By unfollowing this library, you loose access to its content.

-
-
Unfollow
- + +
@@ -199,6 +205,7 @@ export default { this.isLoadingFollow = true axios.delete(`federation/follows/library/${this.library.follow.uuid}/`).then((response) => { self.$emit('deleted') + self.library.follow = null self.isLoadingFollow = false }, error => { self.isLoadingFollow = false