From c17681ae1e4df46a6626d9d7be58bbfefeefe537 Mon Sep 17 00:00:00 2001 From: "Haelwenn (lanodan) Monnier" Date: Fri, 5 May 2023 10:53:09 +0200 Subject: [PATCH] Purge obsolete ap_enabled indicator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It was used to migrate OStatus connections to ActivityPub if possible, but support for OStatus was long since dropped, all new actors always AP and if anything wasn't migrated before, their instance is already marked as unreachable anyway. The associated logic was also buggy in several ways and deleted users got set to ap_enabled=false also causing some issues. This patch is a pretty direct port of the original Pleroma MR; follow-up commits will further fix and clean up remaining issues. Changes made (other than trivial merge conflict resolutions): - converted CHANGELOG format - adapted migration id for Akkoma’s timeline - removed ap_enabled from additional tests Ported-from: https://git.pleroma.social/pleroma/pleroma/-/merge_requests/3880 --- CHANGELOG.md | 7 ++ lib/pleroma/user.ex | 13 +--- lib/pleroma/web/activity_pub/activity_pub.ex | 45 ++++++------- .../object_validators/add_remove_validator.ex | 3 +- lib/pleroma/web/activity_pub/publisher.ex | 2 - .../web/activity_pub/transmogrifier.ex | 42 ------------ lib/pleroma/web/federator.ex | 13 +--- lib/pleroma/workers/transmogrifier_worker.ex | 15 ----- .../20241213000000_remove_user_ap_enabled.exs | 13 ++++ test/pleroma/user_test.exs | 11 +--- .../activity_pub_controller_test.exs | 1 - .../web/activity_pub/activity_pub_test.exs | 1 - .../web/activity_pub/publisher_test.exs | 18 ++---- .../web/activity_pub/transmogrifier_test.exs | 64 ------------------- test/pleroma/web/common_api_test.exs | 2 +- test/pleroma/web/federator_test.exs | 6 +- test/support/factory.ex | 3 +- 17 files changed, 57 insertions(+), 202 deletions(-) delete mode 100644 lib/pleroma/workers/transmogrifier_worker.ex create mode 100644 priv/repo/migrations/20241213000000_remove_user_ap_enabled.exs diff --git a/CHANGELOG.md b/CHANGELOG.md index 42ed630fb..b0448b0d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ## Unreleased +## Added + +## Fixed + +## Changed +- Dropped obsolete `ap_enabled` indicator from user table and associated buggy logic + ## 2025.01.01 Hotfix: Federation could break if a null value found its way into `should_federate?\1` diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index dfeab0410..3042d86f2 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -127,7 +127,6 @@ defmodule Pleroma.User do field(:domain_blocks, {:array, :string}, default: []) field(:is_active, :boolean, default: true) field(:no_rich_text, :boolean, default: false) - field(:ap_enabled, :boolean, default: false) field(:is_moderator, :boolean, default: false) field(:is_admin, :boolean, default: false) field(:show_role, :boolean, default: true) @@ -473,7 +472,6 @@ def remote_user_changeset(struct \\ %User{local: false}, params) do :shared_inbox, :nickname, :avatar, - :ap_enabled, :banner, :background, :is_locked, @@ -1006,11 +1004,7 @@ def maybe_direct_follow(%User{} = follower, %User{local: true} = followed) do end def maybe_direct_follow(%User{} = follower, %User{} = followed) do - if not ap_enabled?(followed) do - follow(follower, followed) - else - {:ok, follower, followed} - end + {:ok, follower, followed} end @doc "A mass follow for local users. Respects blocks in both directions but does not create activities." @@ -1826,7 +1820,6 @@ def purge_user_changeset(user) do confirmation_token: nil, domain_blocks: [], is_active: false, - ap_enabled: false, is_moderator: false, is_admin: false, mastofe_settings: nil, @@ -2073,10 +2066,6 @@ def get_public_key_for_ap_id(ap_id) do end end - def ap_enabled?(%User{local: true}), do: true - def ap_enabled?(%User{ap_enabled: ap_enabled}), do: ap_enabled - def ap_enabled?(_), do: false - @doc "Gets or fetch a user by uri or nickname." @spec get_or_fetch(String.t()) :: {:ok, User.t()} | {:error, String.t()} def get_or_fetch("http://" <> _host = uri), do: get_or_fetch_by_ap_id(uri) diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index 9b28e64d9..494a27421 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -1626,7 +1626,6 @@ defp object_to_user_data(data, additional) do %{ ap_id: data["id"], uri: get_actor_url(data["url"]), - ap_enabled: true, banner: normalize_image(data["image"]), background: normalize_image(data["backgroundUrl"]), fields: fields, @@ -1743,7 +1742,7 @@ def user_data_from_user_object(data, additional \\ []) do end end - def fetch_and_prepare_user_from_ap_id(ap_id, additional \\ []) do + defp fetch_and_prepare_user_from_ap_id(ap_id, additional) do with {:ok, data} <- Fetcher.fetch_and_contain_remote_object_from_id(ap_id), {:valid, {:ok, _, _}} <- {:valid, UserValidator.validate(data, [])}, {:ok, data} <- user_data_from_user_object(data, additional) do @@ -1857,31 +1856,27 @@ def enqueue_pin_fetches(_), do: nil def make_user_from_ap_id(ap_id, additional \\ []) do user = User.get_cached_by_ap_id(ap_id) - if user && !User.ap_enabled?(user) do - Transmogrifier.upgrade_user_from_ap_id(ap_id) - else - with {:ok, data} <- fetch_and_prepare_user_from_ap_id(ap_id, additional) do - user = - if data.ap_id != ap_id do - User.get_cached_by_ap_id(data.ap_id) - else - user - end - - if user do - user - |> User.remote_user_changeset(data) - |> User.update_and_set_cache() - |> tap(fn _ -> enqueue_pin_fetches(data) end) + with {:ok, data} <- fetch_and_prepare_user_from_ap_id(ap_id, additional) do + user = + if data.ap_id != ap_id do + User.get_cached_by_ap_id(data.ap_id) else - maybe_handle_clashing_nickname(data) - - data - |> User.remote_user_changeset() - |> Repo.insert() - |> User.set_cache() - |> tap(fn _ -> enqueue_pin_fetches(data) end) + user end + + if user do + user + |> User.remote_user_changeset(data) + |> User.update_and_set_cache() + |> tap(fn _ -> enqueue_pin_fetches(data) end) + else + maybe_handle_clashing_nickname(data) + + data + |> User.remote_user_changeset() + |> Repo.insert() + |> User.set_cache() + |> tap(fn _ -> enqueue_pin_fetches(data) end) end end end diff --git a/lib/pleroma/web/activity_pub/object_validators/add_remove_validator.ex b/lib/pleroma/web/activity_pub/object_validators/add_remove_validator.ex index fc482c9c0..b2fa35831 100644 --- a/lib/pleroma/web/activity_pub/object_validators/add_remove_validator.ex +++ b/lib/pleroma/web/activity_pub/object_validators/add_remove_validator.ex @@ -73,6 +73,7 @@ defp maybe_refetch_user(%User{featured_address: address} = user) when is_binary( end defp maybe_refetch_user(%User{ap_id: ap_id}) do - Pleroma.Web.ActivityPub.Transmogrifier.upgrade_user_from_ap_id(ap_id) + # Maybe it could use User.get_or_fetch_by_ap_id to avoid refreshing too often + User.fetch_by_ap_id(ap_id) end end diff --git a/lib/pleroma/web/activity_pub/publisher.ex b/lib/pleroma/web/activity_pub/publisher.ex index 07f430805..056de24bc 100644 --- a/lib/pleroma/web/activity_pub/publisher.ex +++ b/lib/pleroma/web/activity_pub/publisher.ex @@ -219,7 +219,6 @@ def publish(%User{} = actor, %{data: %{"bcc" => bcc}} = activity) inboxes = recipients - |> Enum.filter(&User.ap_enabled?/1) |> Enum.map(fn actor -> actor.inbox end) |> Enum.filter(fn inbox -> should_federate?(inbox) end) |> Instances.filter_reachable() @@ -261,7 +260,6 @@ def publish(%User{} = actor, %Activity{} = activity) do json = Jason.encode!(data) recipients(actor, activity) - |> Enum.filter(fn user -> User.ap_enabled?(user) end) |> Enum.map(fn %User{} = user -> determine_inbox(activity, user) end) diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index 5c4db39b9..251ba6540 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -21,7 +21,6 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do alias Pleroma.Web.ActivityPub.Visibility alias Pleroma.Web.ActivityPub.ObjectValidators.CommonFixes alias Pleroma.Web.Federator - alias Pleroma.Workers.TransmogrifierWorker import Ecto.Query @@ -1007,47 +1006,6 @@ defp strip_internal_tags(%{"tag" => tags} = object) do defp strip_internal_tags(object), do: object - def perform(:user_upgrade, user) do - # we pass a fake user so that the followers collection is stripped away - old_follower_address = User.ap_followers(%User{nickname: user.nickname}) - - from( - a in Activity, - where: ^old_follower_address in a.recipients, - update: [ - set: [ - recipients: - fragment( - "array_replace(?,?,?)", - a.recipients, - ^old_follower_address, - ^user.follower_address - ) - ] - ] - ) - |> Repo.update_all([]) - end - - def upgrade_user_from_ap_id(ap_id) do - with %User{local: false} = user <- User.get_cached_by_ap_id(ap_id), - {:ok, data} <- ActivityPub.fetch_and_prepare_user_from_ap_id(ap_id), - {:ok, user} <- update_user(user, data) do - ActivityPub.enqueue_pin_fetches(user) - TransmogrifierWorker.enqueue("user_upgrade", %{"user_id" => user.id}) - {:ok, user} - else - %User{} = user -> {:ok, user} - e -> e - end - end - - defp update_user(user, data) do - user - |> User.remote_user_changeset(data) - |> User.update_and_set_cache() - end - def maybe_fix_user_url(%{"url" => url} = data) when is_map(url) do Map.put(data, "url", url["href"]) end diff --git a/lib/pleroma/web/federator.ex b/lib/pleroma/web/federator.ex index 3a00424c6..aaf89412f 100644 --- a/lib/pleroma/web/federator.ex +++ b/lib/pleroma/web/federator.ex @@ -6,7 +6,6 @@ defmodule Pleroma.Web.Federator do alias Pleroma.Activity alias Pleroma.Object.Containment alias Pleroma.User - alias Pleroma.Web.ActivityPub.ActivityPub alias Pleroma.Web.ActivityPub.Transmogrifier alias Pleroma.Web.ActivityPub.Utils alias Pleroma.Web.Federator.Publisher @@ -92,7 +91,7 @@ def perform(:incoming_ap_doc, params) do # NOTE: we use the actor ID to do the containment, this is fine because an # actor shouldn't be acting on objects outside their own AP server. - with {_, {:ok, _user}} <- {:actor, ap_enabled_actor(actor)}, + with {_, {:ok, _user}} <- {:actor, User.get_or_fetch_by_ap_id(actor)}, nil <- Activity.normalize(params["id"]), {_, :ok} <- {:correct_origin?, Containment.contain_origin_from_id(actor, params)}, @@ -122,14 +121,4 @@ def perform(:incoming_ap_doc, params) do {:error, e} end end - - def ap_enabled_actor(id) do - user = User.get_cached_by_ap_id(id) - - if User.ap_enabled?(user) do - {:ok, user} - else - ActivityPub.make_user_from_ap_id(id) - end - end end diff --git a/lib/pleroma/workers/transmogrifier_worker.ex b/lib/pleroma/workers/transmogrifier_worker.ex deleted file mode 100644 index b39c1ea62..000000000 --- a/lib/pleroma/workers/transmogrifier_worker.ex +++ /dev/null @@ -1,15 +0,0 @@ -# Pleroma: A lightweight social networking server -# Copyright © 2017-2021 Pleroma Authors -# SPDX-License-Identifier: AGPL-3.0-only - -defmodule Pleroma.Workers.TransmogrifierWorker do - alias Pleroma.User - - use Pleroma.Workers.WorkerHelper, queue: "transmogrifier" - - @impl Oban.Worker - def perform(%Job{args: %{"op" => "user_upgrade", "user_id" => user_id}}) do - user = User.get_cached_by_id(user_id) - Pleroma.Web.ActivityPub.Transmogrifier.perform(:user_upgrade, user) - end -end diff --git a/priv/repo/migrations/20241213000000_remove_user_ap_enabled.exs b/priv/repo/migrations/20241213000000_remove_user_ap_enabled.exs new file mode 100644 index 000000000..0aea41324 --- /dev/null +++ b/priv/repo/migrations/20241213000000_remove_user_ap_enabled.exs @@ -0,0 +1,13 @@ +# Pleroma: A lightweight social networking server +# Copyright © 2017-2023 Pleroma Authors +# SPDX-License-Identifier: AGPL-3.0-only + +defmodule Pleroma.Repo.Migrations.RemoveUserApEnabled do + use Ecto.Migration + + def change do + alter table(:users) do + remove(:ap_enabled, :boolean, default: false, null: false) + end + end +end diff --git a/test/pleroma/user_test.exs b/test/pleroma/user_test.exs index ac886aaf9..0d5c9faec 100644 --- a/test/pleroma/user_test.exs +++ b/test/pleroma/user_test.exs @@ -1694,7 +1694,6 @@ test "delete/1 purges a user when they wouldn't be fully deleted" do confirmation_token: "qqqq", domain_blocks: ["lain.com"], is_active: false, - ap_enabled: true, is_moderator: true, is_admin: true, mastofe_settings: %{"a" => "b"}, @@ -1734,7 +1733,6 @@ test "delete/1 purges a user when they wouldn't be fully deleted" do confirmation_token: nil, domain_blocks: [], is_active: false, - ap_enabled: false, is_moderator: false, is_admin: false, mastofe_settings: nil, @@ -2217,8 +2215,7 @@ test "updates the counters normally on following/getting a follow when disabled" insert(:user, local: false, follower_address: "http://remote.org/users/masto_closed/followers", - following_address: "http://remote.org/users/masto_closed/following", - ap_enabled: true + following_address: "http://remote.org/users/masto_closed/following" ) assert other_user.following_count == 0 @@ -2239,8 +2236,7 @@ test "synchronizes the counters with the remote instance for the followed when e insert(:user, local: false, follower_address: "http://remote.org/users/masto_closed/followers", - following_address: "http://remote.org/users/masto_closed/following", - ap_enabled: true + following_address: "http://remote.org/users/masto_closed/following" ) assert other_user.following_count == 0 @@ -2261,8 +2257,7 @@ test "synchronizes the counters with the remote instance for the follower when e insert(:user, local: false, follower_address: "http://remote.org/users/masto_closed/followers", - following_address: "http://remote.org/users/masto_closed/following", - ap_enabled: true + following_address: "http://remote.org/users/masto_closed/following" ) assert other_user.following_count == 0 diff --git a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs index 5b3697244..4cc7f93f5 100644 --- a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs @@ -579,7 +579,6 @@ test "it inserts an incoming activity into the database" <> user = insert(:user, ap_id: "https://mastodon.example.org/users/raymoo", - ap_enabled: true, local: false, last_refreshed_at: nil ) diff --git a/test/pleroma/web/activity_pub/activity_pub_test.exs b/test/pleroma/web/activity_pub/activity_pub_test.exs index c8f93f84d..7990b7ef5 100644 --- a/test/pleroma/web/activity_pub/activity_pub_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_test.exs @@ -178,7 +178,6 @@ test "it returns a user" do {:ok, user} = ActivityPub.make_user_from_ap_id(user_id) assert user.ap_id == user_id assert user.nickname == "admin@mastodon.example.org" - assert user.ap_enabled assert user.follower_address == "http://mastodon.example.org/users/admin/followers" end diff --git a/test/pleroma/web/activity_pub/publisher_test.exs b/test/pleroma/web/activity_pub/publisher_test.exs index 5896568b8..b90422d61 100644 --- a/test/pleroma/web/activity_pub/publisher_test.exs +++ b/test/pleroma/web/activity_pub/publisher_test.exs @@ -306,15 +306,13 @@ test "publish to url with with different ports" do follower = insert(:user, %{ local: false, - inbox: "https://domain.com/users/nick1/inbox", - ap_enabled: true + inbox: "https://domain.com/users/nick1/inbox" }) another_follower = insert(:user, %{ local: false, - inbox: "https://rejected.com/users/nick2/inbox", - ap_enabled: true + inbox: "https://rejected.com/users/nick2/inbox" }) actor = @@ -386,8 +384,7 @@ test "publish to url with with different ports" do follower = insert(:user, %{ local: false, - inbox: "https://domain.com/users/nick1/inbox", - ap_enabled: true + inbox: "https://domain.com/users/nick1/inbox" }) actor = @@ -425,8 +422,7 @@ test "publish to url with with different ports" do follower = insert(:user, %{ local: false, - inbox: "https://domain.com/users/nick1/inbox", - ap_enabled: true + inbox: "https://domain.com/users/nick1/inbox" }) actor = insert(:user, follower_address: follower.ap_id) @@ -461,15 +457,13 @@ test "publish to url with with different ports" do fetcher = insert(:user, local: false, - inbox: "https://domain.com/users/nick1/inbox", - ap_enabled: true + inbox: "https://domain.com/users/nick1/inbox" ) another_fetcher = insert(:user, local: false, - inbox: "https://domain2.com/users/nick1/inbox", - ap_enabled: true + inbox: "https://domain2.com/users/nick1/inbox" ) actor = insert(:user) diff --git a/test/pleroma/web/activity_pub/transmogrifier_test.exs b/test/pleroma/web/activity_pub/transmogrifier_test.exs index 1be69317c..94df25100 100644 --- a/test/pleroma/web/activity_pub/transmogrifier_test.exs +++ b/test/pleroma/web/activity_pub/transmogrifier_test.exs @@ -8,7 +8,6 @@ defmodule Pleroma.Web.ActivityPub.TransmogrifierTest do @moduletag :mocked alias Pleroma.Activity alias Pleroma.Object - alias Pleroma.Tests.ObanHelpers alias Pleroma.User alias Pleroma.Web.ActivityPub.Transmogrifier alias Pleroma.Web.ActivityPub.Utils @@ -348,69 +347,6 @@ test "Updates of Notes are handled" do end end - describe "user upgrade" do - test "it upgrades a user to activitypub" do - user = - insert(:user, %{ - nickname: "rye@niu.moe", - local: false, - ap_id: "https://niu.moe/users/rye", - follower_address: User.ap_followers(%User{nickname: "rye@niu.moe"}) - }) - - user_two = insert(:user) - Pleroma.FollowingRelationship.follow(user_two, user, :follow_accept) - - {:ok, activity} = CommonAPI.post(user, %{status: "test"}) - {:ok, unrelated_activity} = CommonAPI.post(user_two, %{status: "test"}) - assert "http://localhost:4001/users/rye@niu.moe/followers" in activity.recipients - - user = User.get_cached_by_id(user.id) - assert user.note_count == 1 - - {:ok, user} = Transmogrifier.upgrade_user_from_ap_id("https://niu.moe/users/rye") - ObanHelpers.perform_all() - - assert user.ap_enabled - assert user.note_count == 1 - assert user.follower_address == "https://niu.moe/users/rye/followers" - assert user.following_address == "https://niu.moe/users/rye/following" - - user = User.get_cached_by_id(user.id) - assert user.note_count == 1 - - activity = Activity.get_by_id(activity.id) - assert user.follower_address in activity.recipients - - assert %{ - "url" => [ - %{ - "href" => - "https://cdn.niu.moe/accounts/avatars/000/033/323/original/fd7f8ae0b3ffedc9.jpeg" - } - ] - } = user.avatar - - assert %{ - "url" => [ - %{ - "href" => - "https://cdn.niu.moe/accounts/headers/000/033/323/original/850b3448fa5fd477.png" - } - ] - } = user.banner - - refute "..." in activity.recipients - - unrelated_activity = Activity.get_by_id(unrelated_activity.id) - refute user.follower_address in unrelated_activity.recipients - - user_two = User.get_cached_by_id(user_two.id) - assert User.following?(user_two, user) - refute "..." in User.following(user_two) - end - end - describe "actor rewriting" do test "it fixes the actor URL property to be a proper URI" do data = %{ diff --git a/test/pleroma/web/common_api_test.exs b/test/pleroma/web/common_api_test.exs index 379cf85b8..b32334389 100644 --- a/test/pleroma/web/common_api_test.exs +++ b/test/pleroma/web/common_api_test.exs @@ -1129,7 +1129,7 @@ test "removes a pending follow for a local user" do test "cancels a pending follow for a remote user" do follower = insert(:user) - followed = insert(:user, is_locked: true, local: false, ap_enabled: true) + followed = insert(:user, is_locked: true, local: false) assert {:ok, follower, followed, %{id: _activity_id, data: %{"state" => "pending"}}} = CommonAPI.follow(follower, followed) diff --git a/test/pleroma/web/federator_test.exs b/test/pleroma/web/federator_test.exs index d3cc239cf..c9a13632a 100644 --- a/test/pleroma/web/federator_test.exs +++ b/test/pleroma/web/federator_test.exs @@ -79,16 +79,14 @@ test "it federates only to reachable instances via AP" do local: false, nickname: "nick1@domain.com", ap_id: "https://domain.com/users/nick1", - inbox: inbox1, - ap_enabled: true + inbox: inbox1 }) insert(:user, %{ local: false, nickname: "nick2@domain2.com", ap_id: "https://domain2.com/users/nick2", - inbox: inbox2, - ap_enabled: true + inbox: inbox2 }) dt = NaiveDateTime.utc_now() diff --git a/test/support/factory.ex b/test/support/factory.ex index 54e5f91b7..7797fc9b8 100644 --- a/test/support/factory.ex +++ b/test/support/factory.ex @@ -62,8 +62,7 @@ def user_factory(attrs \\ %{}) do last_digest_emailed_at: NaiveDateTime.utc_now(), last_refreshed_at: NaiveDateTime.utc_now(), notification_settings: %Pleroma.User.NotificationSetting{}, - multi_factor_authentication_settings: %Pleroma.MFA.Settings{}, - ap_enabled: true + multi_factor_authentication_settings: %Pleroma.MFA.Settings{} } urls =