From a80444041cd5ab00573eeda3301fc3c0b7a03abe Mon Sep 17 00:00:00 2001 From: Oneric Date: Fri, 14 Mar 2025 01:03:39 +0100 Subject: [PATCH] federation: always prefer the shared inbox MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In theory a pedantic reading of the spec indeed suggests DMs must only be delivered to personal inboxes. However, in practice the normative force of real-world implementations disagrees. Mastodon, Iceshrimp.NET and GtS (the latter notably has a config option to never use sharedInboxes) all unconditionally prefer sharedInbox for everything without ill effect. This saves on duplicate deliveries on the sending and processing on the receiving end. (Typically the receiving side ends up rejecting all but the first copy as duplicates) Furthermore current determine_inbox logic also actually needs up forcing personal inboxes for follower-only posts, unless they additionally explicitly address at least one specific actor. This is even much wasteful and directly contradicts the explicit intent of the spec. There’s one part where the use of sharedInbox falls apart, namely spec-compliant bcc and bto addressing. AP spec requires bcc/bto fields to be stripped before delivery and then implicitly reconstructed by the receiver based on the addressed personal inbox. In practice however, this addressing mode is almost unused. Neither of the three implementations brought up above supports it and while *oma does use bcc for list addressing, it does not use it in a spec-compliant way and even copies same-host recipients into cc before delivery. Messages with bcc addressing are handled in another function clause, always force personal inboxes for every recipient and not affected by this commit. In theory it would be beneficial to use sharedInbox there too for all but bcc recipients. But in practice list addressing has been broken for quite some time already and is not actually exposed in any frontend, as discussed in https://akkoma.dev/AkkomaGang/akkoma/issues/812. Therefore any changes here have virtually no effect anyway and all code concerning it may just be outright removed. --- lib/pleroma/web/activity_pub/publisher.ex | 40 +--------- .../web/activity_pub/publisher_test.exs | 77 ------------------- 2 files changed, 3 insertions(+), 114 deletions(-) diff --git a/lib/pleroma/web/activity_pub/publisher.ex b/lib/pleroma/web/activity_pub/publisher.ex index 00c1a1b96..8d9d07874 100644 --- a/lib/pleroma/web/activity_pub/publisher.ex +++ b/lib/pleroma/web/activity_pub/publisher.ex @@ -172,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. @@ -263,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) diff --git a/test/pleroma/web/activity_pub/publisher_test.exs b/test/pleroma/web/activity_pub/publisher_test.exs index 675efae3f..40e635467 100644 --- a/test/pleroma/web/activity_pub/publisher_test.exs +++ b/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"