Merge pull request 'Always prefer shared inbox' (#881) from Oneric/akkoma:shared-inbox into develop

Reviewed-on: #881

lib/pleroma/web/activity_pub/publisher.ex

@@ -47,7 +47,9 @@ def is_representable?(%Activity{} = activity) do
   * `actor`: the actor which is signing the message
   * `id`: the ActivityStreams URI of the message
   """
-  def publish_one(%{inbox: inbox, json: json, actor: %User{} = actor, id: id} = params) do
+  def publish_one(
+        %{"inbox" => inbox, "json" => json, "actor" => %User{} = actor, "id" => id} = params
+      ) do
     Logger.debug("Federating #{id} to #{inbox}")
     uri = %{path: path} = URI.parse(inbox)
     digest = "SHA-256=" <> (:crypto.hash(:sha256, json) |> Base.encode64())
@@ -74,24 +76,24 @@ def publish_one(%{inbox: inbox, json: json, actor: %User{} = actor, id: id} = pa
                {"digest", digest}
              ]
            ) do
-      if not Map.has_key?(params, :unreachable_since) || params[:unreachable_since] do
+      if not Map.has_key?(params, "unreachable_since") || params["unreachable_since"] do
         Instances.set_reachable(inbox)
       end
 
       result
     else
       {_post_result, response} ->
-        unless params[:unreachable_since], do: Instances.set_unreachable(inbox)
+        unless params["unreachable_since"], do: Instances.set_unreachable(inbox)
         {:error, response}
     end
   end
 
-  def publish_one(%{actor_id: actor_id} = params) do
+  def publish_one(%{"actor_id" => actor_id} = params) do
     actor = User.get_cached_by_id(actor_id)
 
     params
-    |> Map.delete(:actor_id)
-    |> Map.put(:actor, actor)
+    |> Map.delete("actor_id")
+    |> Map.put("actor", actor)
     |> publish_one()
   end
 
@@ -170,42 +172,8 @@ defp get_cc_ap_ids(ap_id, recipients) do
     |> Enum.map(& &1.ap_id)
   end
 
-  defp maybe_use_sharedinbox(%User{shared_inbox: nil, inbox: inbox}), do: inbox
-  defp maybe_use_sharedinbox(%User{shared_inbox: shared_inbox}), do: shared_inbox
-
-  @doc """
-  Determine a user inbox to use based on heuristics.  These heuristics
-  are based on an approximation of the ``sharedInbox`` rules in the
-  [ActivityPub specification][ap-sharedinbox].
-
-  Please do not edit this function (or its children) without reading
-  the spec, as editing the code is likely to introduce some breakage
-  without some familiarity.
-
-     [ap-sharedinbox]: https://www.w3.org/TR/activitypub/#shared-inbox-delivery
-  """
-  def determine_inbox(
-        %Activity{data: activity_data},
-        %User{inbox: inbox} = user
-      ) do
-    to = activity_data["to"] || []
-    cc = activity_data["cc"] || []
-    type = activity_data["type"]
-
-    cond do
-      type == "Delete" ->
-        maybe_use_sharedinbox(user)
-
-      Pleroma.Constants.as_public() in to || Pleroma.Constants.as_public() in cc ->
-        maybe_use_sharedinbox(user)
-
-      length(to) + length(cc) > 1 ->
-        maybe_use_sharedinbox(user)
-
-      true ->
-        inbox
-    end
-  end
+  defp try_sharedinbox(%User{shared_inbox: nil, inbox: inbox}), do: inbox
+  defp try_sharedinbox(%User{shared_inbox: shared_inbox}), do: shared_inbox
 
   @doc """
   Publishes an activity with BCC to all relevant peers.
@@ -237,11 +205,11 @@ def publish(%User{} = actor, %{data: %{"bcc" => bcc}} = activity)
           |> Jason.encode!()
 
         Pleroma.Web.Federator.Publisher.enqueue_one(__MODULE__, %{
-          inbox: inbox,
-          json: json,
-          actor_id: actor.id,
-          id: activity.data["id"],
-          unreachable_since: unreachable_since
+          "inbox" => inbox,
+          "json" => json,
+          "actor_id" => actor.id,
+          "id" => activity.data["id"],
+          "unreachable_since" => unreachable_since
         })
       end)
     end)
@@ -261,7 +229,7 @@ def publish(%User{} = actor, %Activity{} = activity) do
 
     recipients(actor, activity)
     |> Enum.map(fn %User{} = user ->
-      determine_inbox(activity, user)
+      try_sharedinbox(user)
     end)
     |> Enum.uniq()
     |> Enum.filter(fn inbox -> should_federate?(inbox) end)
@@ -270,11 +238,11 @@ def publish(%User{} = actor, %Activity{} = activity) do
       Pleroma.Web.Federator.Publisher.enqueue_one(
         __MODULE__,
         %{
-          inbox: inbox,
-          json: json,
-          actor_id: actor.id,
-          id: activity.data["id"],
-          unreachable_since: unreachable_since
+          "inbox" => inbox,
+          "json" => json,
+          "actor_id" => actor.id,
+          "id" => activity.data["id"],
+          "unreachable_since" => unreachable_since
         }
       )
     end)

lib/pleroma/workers/publisher_worker.ex

@@ -30,7 +30,6 @@ def perform(%Job{
   end
 
   def perform(%Job{args: %{"op" => "publish_one", "module" => module_name, "params" => params}}) do
-    params = Map.new(params, fn {k, v} -> {String.to_atom(k), v} end)
-    Federator.perform(:publish_one, String.to_atom(module_name), params)
+    Federator.perform(:publish_one, String.to_existing_atom(module_name), params)
   end
 end

test/pleroma/web/activity_pub/publisher_test.exs

@@ -11,7 +11,6 @@ defmodule Pleroma.Web.ActivityPub.PublisherTest do
   import Tesla.Mock
   import Mock
 
-  alias Pleroma.Activity
   alias Pleroma.Instances
   alias Pleroma.Object
   alias Pleroma.Web.ActivityPub.Publisher
@@ -51,82 +50,6 @@ test "it returns links" do
     end
   end
 
-  describe "determine_inbox/2" do
-    test "it returns sharedInbox for messages involving as:Public in to" do
-      user = insert(:user, %{shared_inbox: "http://example.com/inbox"})
-
-      activity = %Activity{
-        data: %{"to" => [@as_public], "cc" => [user.follower_address]}
-      }
-
-      assert Publisher.determine_inbox(activity, user) == "http://example.com/inbox"
-    end
-
-    test "it returns sharedInbox for messages involving as:Public in cc" do
-      user = insert(:user, %{shared_inbox: "http://example.com/inbox"})
-
-      activity = %Activity{
-        data: %{"cc" => [@as_public], "to" => [user.follower_address]}
-      }
-
-      assert Publisher.determine_inbox(activity, user) == "http://example.com/inbox"
-    end
-
-    test "it returns sharedInbox for messages involving multiple recipients in to" do
-      user = insert(:user, %{shared_inbox: "http://example.com/inbox"})
-      user_two = insert(:user)
-      user_three = insert(:user)
-
-      activity = %Activity{
-        data: %{"cc" => [], "to" => [user.ap_id, user_two.ap_id, user_three.ap_id]}
-      }
-
-      assert Publisher.determine_inbox(activity, user) == "http://example.com/inbox"
-    end
-
-    test "it returns sharedInbox for messages involving multiple recipients in cc" do
-      user = insert(:user, %{shared_inbox: "http://example.com/inbox"})
-      user_two = insert(:user)
-      user_three = insert(:user)
-
-      activity = %Activity{
-        data: %{"to" => [], "cc" => [user.ap_id, user_two.ap_id, user_three.ap_id]}
-      }
-
-      assert Publisher.determine_inbox(activity, user) == "http://example.com/inbox"
-    end
-
-    test "it returns sharedInbox for messages involving multiple recipients in total" do
-      user =
-        insert(:user, %{
-          shared_inbox: "http://example.com/inbox",
-          inbox: "http://example.com/personal-inbox"
-        })
-
-      user_two = insert(:user)
-
-      activity = %Activity{
-        data: %{"to" => [user_two.ap_id], "cc" => [user.ap_id]}
-      }
-
-      assert Publisher.determine_inbox(activity, user) == "http://example.com/inbox"
-    end
-
-    test "it returns inbox for messages involving single recipients in total" do
-      user =
-        insert(:user, %{
-          shared_inbox: "http://example.com/inbox",
-          inbox: "http://example.com/personal-inbox"
-        })
-
-      activity = %Activity{
-        data: %{"to" => [user.ap_id], "cc" => []}
-      }
-
-      assert Publisher.determine_inbox(activity, user) == "http://example.com/personal-inbox"
-    end
-  end
-
   describe "publish_one/1" do
     test "publish to url with with different ports" do
       inbox80 = "http://42.site/users/nick1/inbox"
@@ -146,20 +69,20 @@ test "publish to url with with different ports" do
 
       assert {:ok, %{body: "port 42"}} =
                Publisher.publish_one(%{
-                 inbox: inbox42,
-                 json: "{}",
-                 actor: actor,
-                 id: 1,
-                 unreachable_since: true
+                 "inbox" => inbox42,
+                 "json" => "{}",
+                 "actor" => actor,
+                 "id" => 1,
+                 "unreachable_since" => true
                })
 
       assert {:ok, %{body: "port 80"}} =
                Publisher.publish_one(%{
-                 inbox: inbox80,
-                 json: "{}",
-                 actor: actor,
-                 id: 1,
-                 unreachable_since: true
+                 "inbox" => inbox80,
+                 "json" => "{}",
+                 "actor" => actor,
+                 "id" => 1,
+                 "unreachable_since" => true
                })
     end
 
@@ -173,7 +96,14 @@ test "publish to url with with different ports" do
 
       inbox = "http://200.site/users/nick1/inbox"
 
-      assert {:ok, _} = Publisher.publish_one(%{inbox: inbox, json: "{}", actor: actor, id: 1})
+      assert {:ok, _} =
+               Publisher.publish_one(%{
+                 "inbox" => inbox,
+                 "json" => "{}",
+                 "actor" => actor,
+                 "id" => 1
+               })
+
       assert called(Instances.set_reachable(inbox))
     end
 
@@ -189,11 +119,11 @@ test "publish to url with with different ports" do
 
       assert {:ok, _} =
                Publisher.publish_one(%{
-                 inbox: inbox,
-                 json: "{}",
-                 actor: actor,
-                 id: 1,
-                 unreachable_since: NaiveDateTime.utc_now()
+                 "inbox" => inbox,
+                 "json" => "{}",
+                 "actor" => actor,
+                 "id" => 1,
+                 "unreachable_since" => NaiveDateTime.utc_now()
                })
 
       assert called(Instances.set_reachable(inbox))
@@ -211,11 +141,11 @@ test "publish to url with with different ports" do
 
       assert {:ok, _} =
                Publisher.publish_one(%{
-                 inbox: inbox,
-                 json: "{}",
-                 actor: actor,
-                 id: 1,
-                 unreachable_since: nil
+                 "inbox" => inbox,
+                 "json" => "{}",
+                 "actor" => actor,
+                 "id" => 1,
+                 "unreachable_since" => nil
                })
 
       refute called(Instances.set_reachable(inbox))
@@ -231,7 +161,13 @@ test "publish to url with with different ports" do
 
       inbox = "http://404.site/users/nick1/inbox"
 
-      assert {:error, _} = Publisher.publish_one(%{inbox: inbox, json: "{}", actor: actor, id: 1})
+      assert {:error, _} =
+               Publisher.publish_one(%{
+                 "inbox" => inbox,
+                 "json" => "{}",
+                 "actor" => actor,
+                 "id" => 1
+               })
 
       assert called(Instances.set_unreachable(inbox))
     end
@@ -248,7 +184,12 @@ test "publish to url with with different ports" do
 
       assert capture_log(fn ->
                assert {:error, _} =
-                        Publisher.publish_one(%{inbox: inbox, json: "{}", actor: actor, id: 1})
+                        Publisher.publish_one(%{
+                          "inbox" => inbox,
+                          "json" => "{}",
+                          "actor" => actor,
+                          "id" => 1
+                        })
              end) =~ "connrefused"
 
       assert called(Instances.set_unreachable(inbox))
@@ -264,7 +205,13 @@ test "publish to url with with different ports" do
 
       inbox = "http://200.site/users/nick1/inbox"
 
-      assert {:ok, _} = Publisher.publish_one(%{inbox: inbox, json: "{}", actor: actor, id: 1})
+      assert {:ok, _} =
+               Publisher.publish_one(%{
+                 "inbox" => inbox,
+                 "json" => "{}",
+                 "actor" => actor,
+                 "id" => 1
+               })
 
       refute called(Instances.set_unreachable(inbox))
     end
@@ -282,11 +229,11 @@ test "publish to url with with different ports" do
       assert capture_log(fn ->
                assert {:error, _} =
                         Publisher.publish_one(%{
-                          inbox: inbox,
-                          json: "{}",
-                          actor: actor,
-                          id: 1,
-                          unreachable_since: NaiveDateTime.utc_now()
+                          "inbox" => inbox,
+                          "json" => "{}",
+                          "actor" => actor,
+                          "id" => 1,
+                          "unreachable_since" => NaiveDateTime.utc_now()
                         })
              end) =~ "connrefused"
 
@@ -344,33 +291,33 @@ test "publish to url with with different ports" do
 
       assert not called(
                Pleroma.Web.Federator.Publisher.enqueue_one(Publisher, %{
-                 inbox: "https://domain.com/users/nick1/inbox",
-                 actor_id: actor.id,
-                 id: note_activity.data["id"]
+                 "inbox" => "https://domain.com/users/nick1/inbox",
+                 "actor_id" => actor.id,
+                 "id" => note_activity.data["id"]
                })
              )
 
       assert not called(
                Pleroma.Web.Federator.Publisher.enqueue_one(Publisher, %{
-                 inbox: "https://domain.com/users/nick1/inbox",
-                 actor_id: actor.id,
-                 id: public_note_activity.data["id"]
+                 "inbox" => "https://domain.com/users/nick1/inbox",
+                 "actor_id" => actor.id,
+                 "id" => public_note_activity.data["id"]
                })
              )
 
       assert not called(
                Pleroma.Web.Federator.Publisher.enqueue_one(Publisher, %{
-                 inbox: "https://rejected.com/users/nick2/inbox",
-                 actor_id: actor.id,
-                 id: note_activity.data["id"]
+                 "inbox" => "https://rejected.com/users/nick2/inbox",
+                 "actor_id" => actor.id,
+                 "id" => note_activity.data["id"]
                })
              )
 
       assert not called(
                Pleroma.Web.Federator.Publisher.enqueue_one(Publisher, %{
-                 inbox: "https://rejected.com/users/nick2/inbox",
-                 actor_id: actor.id,
-                 id: public_note_activity.data["id"]
+                 "inbox" => "https://rejected.com/users/nick2/inbox",
+                 "actor_id" => actor.id,
+                 "id" => public_note_activity.data["id"]
                })
              )
     end
@@ -406,9 +353,9 @@ test "publish to url with with different ports" do
 
       assert called(
                Pleroma.Web.Federator.Publisher.enqueue_one(Publisher, %{
-                 inbox: "https://domain.com/users/nick1/inbox",
-                 actor_id: actor.id,
-                 id: note_activity.data["id"]
+                 "inbox" => "https://domain.com/users/nick1/inbox",
+                 "actor_id" => actor.id,
+                 "id" => note_activity.data["id"]
                })
              )
     end
@@ -441,9 +388,9 @@ test "publish to url with with different ports" do
 
       assert called(
                Pleroma.Web.Federator.Publisher.enqueue_one(Publisher, %{
-                 inbox: "https://domain.com/users/nick1/inbox",
-                 actor_id: actor.id,
-                 id: note_activity.data["id"]
+                 "inbox" => "https://domain.com/users/nick1/inbox",
+                 "actor_id" => actor.id,
+                 "id" => note_activity.data["id"]
                })
              )
     end
@@ -493,17 +440,17 @@ test "publish to url with with different ports" do
 
       assert called(
                Pleroma.Web.Federator.Publisher.enqueue_one(Publisher, %{
-                 inbox: "https://domain.com/users/nick1/inbox",
-                 actor_id: actor.id,
-                 id: delete.data["id"]
+                 "inbox" => "https://domain.com/users/nick1/inbox",
+                 "actor_id" => actor.id,
+                 "id" => delete.data["id"]
                })
              )
 
       assert called(
                Pleroma.Web.Federator.Publisher.enqueue_one(Publisher, %{
-                 inbox: "https://domain2.com/users/nick1/inbox",
-                 actor_id: actor.id,
-                 id: delete.data["id"]
+                 "inbox" => "https://domain2.com/users/nick1/inbox",
+                 "actor_id" => actor.id,
+                 "id" => delete.data["id"]
                })
              )
     end