diff --git a/api/funkwhale_api/federation/serializers.py b/api/funkwhale_api/federation/serializers.py index 7d71e38da824753b7953f8b3bcf105e68686df02..473585fe7f56d1d519bf2e721bc982da4b31fbb5 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 5dfef61d31c270f1a71643d8d5a167b7689e7478..56834d55f472a5ce9e3d49ee6e96486bf59bd673 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 0000000000000000000000000000000000000000..190c1127d79231191c313410f28e95a2eac34bc5 --- /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 1309b2f2b3893121d3c21f2d9c75870ea36895e2..b78f7f220af0b02438d87740e3ab3c1d2d7981be 100644 --- a/front/src/views/content/remote/Card.vue +++ b/front/src/views/content/remote/Card.vue @@ -80,35 +80,41 @@ </div> </div> </div> - <div v-if="displayFollow" class="ui bottom attached buttons"> + <div v-if="displayFollow" :class="['ui', 'bottom', {two: library.follow}, 'attached', 'buttons']"> <button v-if="!library.follow" @click="follow()" :class="['ui', 'green', {'loading': isLoadingFollow}, 'button']"> <translate translate-context="Content/Library/Card.Button.Label/Verb">Follow</translate> </button> - <button - v-else-if="!library.follow.approved" - class="ui disabled button"><i class="hourglass icon"></i> - <translate translate-context="Content/Library/Card.Paragraph">Follow request pending approval</translate> - </button> - <button - v-else-if="!library.follow.approved" - class="ui disabled button"><i class="check icon"></i> - <translate translate-context="Content/Library/Card.Paragraph">Following</translate> - </button> - <dangerous-button - v-else-if="library.follow.approved" - color="" - :class="['ui', 'button']" - :action="unfollow"> - <translate translate-context="*/Library/Button.Label/Verb">Unfollow</translate> - <p slot="modal-header"><translate translate-context="Popup/Library/Title">Unfollow this library?</translate></p> - <div slot="modal-content"> - <p><translate translate-context="Popup/Library/Paragraph">By unfollowing this library, you loose access to its content.</translate></p> - </div> - <div slot="modal-confirm"><translate translate-context="*/Library/Button.Label/Verb">Unfollow</translate></div> - </dangerous-button> + <template v-else-if="!library.follow.approved"> + <button + class="ui disabled button"><i class="hourglass icon"></i> + <translate translate-context="Content/Library/Card.Paragraph">Follow request pending approval</translate> + </button> + <button + @click="unfollow" + class="ui button"> + <translate translate-context="Content/Library/Card.Paragraph">Cancel follow request</translate> + </button> + </template> + <template v-else-if="library.follow.approved"> + <button + class="ui disabled button"><i class="check icon"></i> + <translate translate-context="Content/Library/Card.Paragraph">Following</translate> + </button> + <dangerous-button + color="" + :class="['ui', 'button']" + :action="unfollow"> + <translate translate-context="*/Library/Button.Label/Verb">Unfollow</translate> + <p slot="modal-header"><translate translate-context="Popup/Library/Title">Unfollow this library?</translate></p> + <div slot="modal-content"> + <p><translate translate-context="Popup/Library/Paragraph">By unfollowing this library, you loose access to its content.</translate></p> + </div> + <div slot="modal-confirm"><translate translate-context="*/Library/Button.Label/Verb">Unfollow</translate></div> + </dangerous-button> + </template> </div> </div> </template> @@ -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