Merge branch '830-resend-accept' into 'develop'

Fix #830: Better handling of follow/accept messages to avoid and recover from...

Closes #830

See merge request funkwhale/funkwhale!765

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 {

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)

changes/changelog.d/830.enhancement

@@ -0,0 +1 @@
+Better handling of follow/accept messages to avoid and recover from desync between instances (#830)

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
+      <template v-else-if="!library.follow.approved">
-        v-else-if="!library.follow.approved"
+        <button
-        class="ui disabled button"><i class="hourglass icon"></i>
+          class="ui disabled button"><i class="hourglass icon"></i>
-        <translate translate-context="Content/Library/Card.Paragraph">Follow request pending approval</translate>
+          <translate translate-context="Content/Library/Card.Paragraph">Follow request pending approval</translate>
-      </button>
+        </button>
-      <button
+        <button
-        v-else-if="!library.follow.approved"
+          @click="unfollow"
-        class="ui disabled button"><i class="check icon"></i>
+          class="ui button">
-        <translate translate-context="Content/Library/Card.Paragraph">Following</translate>
+          <translate translate-context="Content/Library/Card.Paragraph">Cancel follow request</translate>
-      </button>
+        </button>
-      <dangerous-button
+      </template>
-        v-else-if="library.follow.approved"
+      <template v-else-if="library.follow.approved">
-        color=""
+        <button
-        :class="['ui', 'button']"
+          class="ui disabled button"><i class="check icon"></i>
-        :action="unfollow">
+          <translate translate-context="Content/Library/Card.Paragraph">Following</translate>
-        <translate translate-context="*/Library/Button.Label/Verb">Unfollow</translate>
+        </button>
-        <p slot="modal-header"><translate translate-context="Popup/Library/Title">Unfollow this library?</translate></p>
+        <dangerous-button
-        <div slot="modal-content">
+          color=""
-          <p><translate translate-context="Popup/Library/Paragraph">By unfollowing this library, you loose access to its content.</translate></p>
+          :class="['ui', 'button']"
-        </div>
+          :action="unfollow">
-        <div slot="modal-confirm"><translate translate-context="*/Library/Button.Label/Verb">Unfollow</translate></div>
+          <translate translate-context="*/Library/Button.Label/Verb">Unfollow</translate>
-      </dangerous-button>
+          <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