From 61889e00fc4a77e92ed7af3b6a270d10d5b2f34b Mon Sep 17 00:00:00 2001 From: rinpatch Date: Wed, 29 Apr 2020 14:26:31 +0300 Subject: [PATCH] Deactivate local users on deletion instead of deleting the record Prevents the possibility of re-registration, which allowed to read DMs of the deleted account. Also includes a migration that tries to find any already deleted accounts and insert skeletons for them. Closes pleroma/pleroma#1687 --- lib/pleroma/user.ex | 11 ++++- .../controllers/pleroma_api_controller.ex | 5 ++- ...338_insert_skeletons_for_deleted_users.exs | 45 +++++++++++++++++++ test/tasks/user_test.exs | 2 +- test/user_test.exs | 14 +----- test/web/activity_pub/transmogrifier_test.exs | 3 +- 6 files changed, 63 insertions(+), 17 deletions(-) create mode 100644 priv/repo/migrations/20200428221338_insert_skeletons_for_deleted_users.exs diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index 0115abed5..0e5121694 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -1416,8 +1416,15 @@ def perform(:delete, %User{} = user) do end) delete_user_activities(user) - invalidate_cache(user) - Repo.delete(user) + + if user.local do + user + |> change(%{deactivated: true, email: nil}) + |> update_and_set_cache() + else + invalidate_cache(user) + Repo.delete(user) + end end def perform(:deactivate_async, user, status), do: deactivate(user, status) diff --git a/lib/pleroma/web/pleroma_api/controllers/pleroma_api_controller.ex b/lib/pleroma/web/pleroma_api/controllers/pleroma_api_controller.ex index dae7f0f2f..41677d04d 100644 --- a/lib/pleroma/web/pleroma_api/controllers/pleroma_api_controller.ex +++ b/lib/pleroma/web/pleroma_api/controllers/pleroma_api_controller.ex @@ -53,7 +53,10 @@ def emoji_reactions_by(%{assigns: %{user: user}} = conn, %{"id" => activity_id} else users = Enum.map(user_ap_ids, &User.get_cached_by_ap_id/1) - |> Enum.filter(& &1) + |> Enum.filter(fn + %{deactivated: false} -> true + _ -> false + end) %{ name: emoji, diff --git a/priv/repo/migrations/20200428221338_insert_skeletons_for_deleted_users.exs b/priv/repo/migrations/20200428221338_insert_skeletons_for_deleted_users.exs new file mode 100644 index 000000000..11d9a70ba --- /dev/null +++ b/priv/repo/migrations/20200428221338_insert_skeletons_for_deleted_users.exs @@ -0,0 +1,45 @@ +defmodule Pleroma.Repo.Migrations.InsertSkeletonsForDeletedUsers do + use Ecto.Migration + + alias Pleroma.User + alias Pleroma.Repo + + import Ecto.Query + + def change do + Application.ensure_all_started(:flake_id) + + local_ap_id = + User.Query.build(%{local: true}) + |> select([u], u.ap_id) + |> limit(1) + |> Repo.one() + + unless local_ap_id == nil do + # Hack to get instance base url because getting it from Phoenix + # would require starting the whole application + instance_uri = + local_ap_id + |> URI.parse() + |> Map.put(:query, nil) + |> Map.put(:path, nil) + |> URI.to_string() + + {:ok, %{rows: ap_ids}} = + Ecto.Adapters.SQL.query( + Repo, + "select distinct unnest(nonexistent_locals.recipients) from activities, lateral (select array_agg(recipient) as recipients from unnest(activities.recipients) as recipient where recipient similar to '#{ + instance_uri + }/users/[A-Za-z0-9]*' and not(recipient in (select ap_id from users where local = true))) nonexistent_locals;", + [], + timeout: :infinity + ) + + ap_ids + |> Enum.each(fn [ap_id] -> + Ecto.Changeset.change(%User{}, deactivated: true, ap_id: ap_id) + |> Repo.insert() + end) + end + end +end diff --git a/test/tasks/user_test.exs b/test/tasks/user_test.exs index b45f37263..22030a423 100644 --- a/test/tasks/user_test.exs +++ b/test/tasks/user_test.exs @@ -92,7 +92,7 @@ test "user is deleted" do assert_received {:mix_shell, :info, [message]} assert message =~ " deleted" - refute User.get_by_nickname(user.nickname) + assert %{deactivated: true} = User.get_by_nickname(user.nickname) end test "no user to delete" do diff --git a/test/user_test.exs b/test/user_test.exs index f3d044a80..555bbb92f 100644 --- a/test/user_test.exs +++ b/test/user_test.exs @@ -1127,16 +1127,7 @@ test ".delete_user_activities deletes all create activities", %{user: user} do refute Activity.get_by_id(activity.id) end - test "it deletes deactivated user" do - {:ok, user} = insert(:user, deactivated: true) |> User.set_cache() - - {:ok, job} = User.delete(user) - {:ok, _user} = ObanHelpers.perform(job) - - refute User.get_by_id(user.id) - end - - test "it deletes a user, all follow relationships and all activities", %{user: user} do + test "it deactivates a user, all follow relationships and all activities", %{user: user} do follower = insert(:user) {:ok, follower} = User.follow(follower, user) @@ -1156,8 +1147,7 @@ test "it deletes a user, all follow relationships and all activities", %{user: u follower = User.get_cached_by_id(follower.id) refute User.following?(follower, user) - refute User.get_by_id(user.id) - assert {:ok, nil} == Cachex.get(:user_cache, "ap_id:#{user.ap_id}") + assert %{deactivated: true} = User.get_by_id(user.id) user_activities = user.ap_id diff --git a/test/web/activity_pub/transmogrifier_test.exs b/test/web/activity_pub/transmogrifier_test.exs index efbca82f6..2baf9ce03 100644 --- a/test/web/activity_pub/transmogrifier_test.exs +++ b/test/web/activity_pub/transmogrifier_test.exs @@ -870,7 +870,8 @@ test "it fails for incoming deletes with spoofed origin" do @tag capture_log: true test "it works for incoming user deletes" do - %{ap_id: ap_id} = insert(:user, ap_id: "http://mastodon.example.org/users/admin") + %{ap_id: ap_id} = + insert(:user, ap_id: "http://mastodon.example.org/users/admin", local: false) data = File.read!("test/fixtures/mastodon-delete-user.json")