From e55876409b523d81bc19db876bc90f29ba80a47c 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 b451202b2..99358ddaf 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -1445,8 +1445,15 @@ defmodule Pleroma.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 2c1874051..1bdb3aa4d 100644 --- a/lib/pleroma/web/pleroma_api/controllers/pleroma_api_controller.ex +++ b/lib/pleroma/web/pleroma_api/controllers/pleroma_api_controller.ex @@ -61,7 +61,10 @@ defmodule Pleroma.Web.PleromaAPI.PleromaAPIController do 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 8df835b56..0f6ffb2b1 100644 --- a/test/tasks/user_test.exs +++ b/test/tasks/user_test.exs @@ -92,7 +92,7 @@ defmodule Mix.Tasks.Pleroma.UserTest 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 347c5be72..bff337d3e 100644 --- a/test/user_test.exs +++ b/test/user_test.exs @@ -1135,16 +1135,7 @@ defmodule Pleroma.UserTest 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) @@ -1164,8 +1155,7 @@ defmodule Pleroma.UserTest do 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 10d86ee45..36e1e7bd1 100644 --- a/test/web/activity_pub/transmogrifier_test.exs +++ b/test/web/activity_pub/transmogrifier_test.exs @@ -872,7 +872,8 @@ defmodule Pleroma.Web.ActivityPub.TransmogrifierTest 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")