Merge branch 'cleanup/ostatus-user-upgrade' into 'develop'

Cleanup OStatus-era user upgrades and ap_enabled indicator

See merge request pleroma/pleroma!3880

changelog.d/3880.remove

@@ -0,0 +1 @@
+Cleanup OStatus-era user upgrades and ap_enabled indicator

lib/pleroma/user.ex

@@ -124,7 +124,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)
@@ -488,7 +487,6 @@ def remote_user_changeset(struct \\ %User{local: false}, params) do
         :nickname,
         :public_key,
         :avatar,
-        :ap_enabled,
         :banner,
         :is_locked,
         :last_refreshed_at,
@@ -1061,11 +1059,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
+    {:ok, follower, followed}
-      follow(follower, followed)
-    else
-      {:ok, follower, followed}
-    end
   end
 
   @doc "A mass follow for local users. Respects blocks in both directions but does not create activities."
@@ -1898,7 +1892,6 @@ def purge_user_changeset(user) do
       confirmation_token: nil,
       domain_blocks: [],
       is_active: false,
-      ap_enabled: false,
       is_moderator: false,
       is_admin: false,
       mascot: nil,
@@ -2151,10 +2144,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)

lib/pleroma/web/activity_pub/activity_pub.ex

@@ -1547,7 +1547,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"]),
       fields: fields,
       emoji: emojis,
@@ -1668,7 +1667,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),
          {:ok, data} <- user_data_from_user_object(data, additional) do
       {:ok, maybe_update_follow_information(data)}
@@ -1751,24 +1750,20 @@ def pinned_fetch_task(%{pinned_objects: pins}) do
   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
+    with {:ok, data} <- fetch_and_prepare_user_from_ap_id(ap_id, additional) do
-      Transmogrifier.upgrade_user_from_ap_id(ap_id)
+      {:ok, _pid} = Task.start(fn -> pinned_fetch_task(data) end)
-    else
-      with {:ok, data} <- fetch_and_prepare_user_from_ap_id(ap_id, additional) do
-        {:ok, _pid} = Task.start(fn -> pinned_fetch_task(data) end)
 
-        if user do
+      if user do
-          user
+        user
-          |> User.remote_user_changeset(data)
+        |> User.remote_user_changeset(data)
-          |> User.update_and_set_cache()
+        |> User.update_and_set_cache()
-        else
+      else
-          maybe_handle_clashing_nickname(data)
+        maybe_handle_clashing_nickname(data)
 
-          data
+        data
-          |> User.remote_user_changeset()
+        |> User.remote_user_changeset()
-          |> Repo.insert()
+        |> Repo.insert()
-          |> User.set_cache()
+        |> User.set_cache()
-        end
       end
     end
   end

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

lib/pleroma/web/activity_pub/publisher.ex

@@ -199,7 +199,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, public) end)
       |> Instances.filter_reachable()
@@ -241,7 +240,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)

lib/pleroma/web/activity_pub/transmogrifier.ex

@@ -20,7 +20,6 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do
   alias Pleroma.Web.ActivityPub.Utils
   alias Pleroma.Web.ActivityPub.Visibility
   alias Pleroma.Web.Federator
-  alias Pleroma.Workers.TransmogrifierWorker
 
   import Ecto.Query
 
@@ -946,47 +945,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
-      {:ok, _pid} = Task.start(fn -> ActivityPub.pinned_fetch_task(user) end)
-      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

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
@@ -80,7 +79,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)},
@@ -110,14 +109,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

lib/pleroma/workers/transmogrifier_worker.ex

@@ -1,18 +0,0 @@
-# Pleroma: A lightweight social networking server
-# Copyright © 2017-2022 Pleroma Authors <https://pleroma.social/>
-# 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
-
-  @impl Oban.Worker
-  def timeout(_job), do: :timer.seconds(5)
-end

priv/repo/migrations/20230504173400_remove_user_ap_enabled.exs

@@ -0,0 +1,13 @@
+# Pleroma: A lightweight social networking server
+# Copyright © 2017-2023 Pleroma Authors <https://pleroma.social/>
+# 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

test/pleroma/user_test.exs

@@ -1844,7 +1844,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,
         mascot: %{"a" => "b"},
@@ -1885,7 +1884,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,
              mascot: nil,
@@ -2473,8 +2471,7 @@ test "updates the counters normally on following/getting a follow when disabled"
         insert(:user,
           local: false,
           follower_address: "http://localhost:4001/users/masto_closed/followers",
-          following_address: "http://localhost:4001/users/masto_closed/following",
+          following_address: "http://localhost:4001/users/masto_closed/following"
-          ap_enabled: true
         )
 
       assert other_user.following_count == 0
@@ -2495,8 +2492,7 @@ test "synchronizes the counters with the remote instance for the followed when e
         insert(:user,
           local: false,
           follower_address: "http://localhost:4001/users/masto_closed/followers",
-          following_address: "http://localhost:4001/users/masto_closed/following",
+          following_address: "http://localhost:4001/users/masto_closed/following"
-          ap_enabled: true
         )
 
       assert other_user.following_count == 0
@@ -2517,8 +2513,7 @@ test "synchronizes the counters with the remote instance for the follower when e
         insert(:user,
           local: false,
           follower_address: "http://localhost:4001/users/masto_closed/followers",
-          following_address: "http://localhost:4001/users/masto_closed/following",
+          following_address: "http://localhost:4001/users/masto_closed/following"
-          ap_enabled: true
         )
 
       assert other_user.following_count == 0

test/pleroma/web/activity_pub/activity_pub_controller_test.exs

@@ -575,7 +575,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
         )

test/pleroma/web/activity_pub/activity_pub_test.exs

@@ -174,7 +174,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
 

test/pleroma/web/activity_pub/publisher_test.exs

@@ -276,8 +276,7 @@ test "publish to url with with different ports" do
       follower =
         insert(:user, %{
           local: false,
-          inbox: "https://domain.com/users/nick1/inbox",
+          inbox: "https://domain.com/users/nick1/inbox"
-          ap_enabled: true
         })
 
       actor = insert(:user, follower_address: follower.ap_id)
@@ -313,8 +312,7 @@ test "publish to url with with different ports" do
       follower =
         insert(:user, %{
           local: false,
-          inbox: "https://domain.com/users/nick1/inbox",
+          inbox: "https://domain.com/users/nick1/inbox"
-          ap_enabled: true
         })
 
       actor = insert(:user, follower_address: follower.ap_id)
@@ -348,8 +346,7 @@ test "publish to url with with different ports" do
       follower =
         insert(:user, %{
           local: false,
-          inbox: "https://domain.com/users/nick1/inbox",
+          inbox: "https://domain.com/users/nick1/inbox"
-          ap_enabled: true
         })
 
       actor = insert(:user, follower_address: follower.ap_id)
@@ -382,15 +379,13 @@ test "publish to url with with different ports" do
       fetcher =
         insert(:user,
           local: false,
-          inbox: "https://domain.com/users/nick1/inbox",
+          inbox: "https://domain.com/users/nick1/inbox"
-          ap_enabled: true
         )
 
       another_fetcher =
         insert(:user,
           local: false,
-          inbox: "https://domain2.com/users/nick1/inbox",
+          inbox: "https://domain2.com/users/nick1/inbox"
-          ap_enabled: true
         )
 
       actor = insert(:user)

test/pleroma/web/activity_pub/transmogrifier_test.exs

@@ -8,7 +8,6 @@ defmodule Pleroma.Web.ActivityPub.TransmogrifierTest do
 
   alias Pleroma.Activity
   alias Pleroma.Object
-  alias Pleroma.Tests.ObanHelpers
   alias Pleroma.User
   alias Pleroma.Web.ActivityPub.Transmogrifier
   alias Pleroma.Web.ActivityPub.Utils
@@ -353,69 +352,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 = %{

test/pleroma/web/common_api_test.exs

@@ -1339,7 +1339,7 @@ test "cancels 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)

test/pleroma/web/federator_test.exs

@@ -78,16 +78,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,
+        inbox: inbox1
-        ap_enabled: true
       })
 
       insert(:user, %{
         local: false,
         nickname: "nick2@domain2.com",
         ap_id: "https://domain2.com/users/nick2",
-        inbox: inbox2,
+        inbox: inbox2
-        ap_enabled: true
       })
 
       dt = NaiveDateTime.utc_now()

test/support/factory.ex

@@ -50,7 +50,6 @@ def user_factory(attrs \\ %{}) do
       last_refreshed_at: NaiveDateTime.utc_now(),
       notification_settings: %Pleroma.User.NotificationSetting{},
       multi_factor_authentication_settings: %Pleroma.MFA.Settings{},
-      ap_enabled: true,
       keys: pem
     }