From be2da95c36c14ac42eee4009c6e3e803bafd3d2c Mon Sep 17 00:00:00 2001 From: Alex Gleason Date: Tue, 29 Jun 2021 21:45:38 -0500 Subject: [PATCH 1/7] Correctly purge a remote user --- lib/pleroma/user.ex | 16 ++++++++++------ test/pleroma/user_test.exs | 18 ++++++++++++++++++ 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index 9942617d8..aebb5da95 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -1713,6 +1713,12 @@ def purge_user_changeset(user) do }) end + def purge(%User{} = user) do + user + |> purge_user_changeset() + |> update_and_set_cache() + end + def delete(users) when is_list(users) do for user <- users, do: delete(user) end @@ -1726,9 +1732,9 @@ defp delete_and_invalidate_cache(%User{} = user) do Repo.delete(user) end - defp delete_or_deactivate(%User{local: false} = user), do: delete_and_invalidate_cache(user) + defp delete_or_purge(%User{local: false} = user), do: purge(user) - defp delete_or_deactivate(%User{local: true} = user) do + defp delete_or_purge(%User{local: true} = user) do status = account_status(user) case status do @@ -1739,9 +1745,7 @@ defp delete_or_deactivate(%User{local: true} = user) do delete_and_invalidate_cache(user) _ -> - user - |> purge_user_changeset() - |> update_and_set_cache() + purge(user) end end @@ -1769,7 +1773,7 @@ def perform(:delete, %User{} = user) do delete_outgoing_pending_follow_requests(user) - delete_or_deactivate(user) + delete_or_purge(user) end def perform(:set_activation_async, user, status), do: set_activation(user, status) diff --git a/test/pleroma/user_test.exs b/test/pleroma/user_test.exs index 6f5bcab57..529f837e8 100644 --- a/test/pleroma/user_test.exs +++ b/test/pleroma/user_test.exs @@ -1684,6 +1684,24 @@ test "delete/1 purges a user when they wouldn't be fully deleted" do } = user end + test "delete/1 purges a remote user" do + user = + insert(:user, %{ + name: "qqqqqqq", + avatar: %{"a" => "b"}, + banner: %{"a" => "b"}, + local: false + }) + + {:ok, job} = User.delete(user) + {:ok, _} = ObanHelpers.perform(job) + user = User.get_by_id(user.id) + + assert user.name == nil + assert user.avatar == %{} + assert user.banner == %{} + end + test "get_public_key_for_ap_id fetches a user that's not in the db" do assert {:ok, _key} = User.get_public_key_for_ap_id("http://mastodon.example.org/users/admin") end From c6d4133727ba623d4c96358e3c4de5f2194d07f8 Mon Sep 17 00:00:00 2001 From: Alex Gleason Date: Tue, 29 Jun 2021 22:30:48 -0500 Subject: [PATCH 2/7] Deletions: purge the user immediately --- lib/pleroma/user.ex | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index aebb5da95..406a7f5f9 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -1724,31 +1724,27 @@ def delete(users) when is_list(users) do end def delete(%User{} = user) do + purge(user) BackgroundWorker.enqueue("delete_user", %{"user_id" => user.id}) end - defp delete_and_invalidate_cache(%User{} = user) do + defp delete_from_db(%User{} = user) do invalidate_cache(user) Repo.delete(user) end - defp delete_or_purge(%User{local: false} = user), do: purge(user) - - defp delete_or_purge(%User{local: true} = user) do + defp maybe_delete_from_db(%User{local: true} = user) do status = account_status(user) - case status do - :confirmation_pending -> - delete_and_invalidate_cache(user) - - :approval_pending -> - delete_and_invalidate_cache(user) - - _ -> - purge(user) + if status in [:confirmation_pending, :approval_pending] do + delete_from_db(user) + else + {:ok, user} end end + defp maybe_delete_from_db(user), do: {:ok, user} + def perform(:force_password_reset, user), do: force_password_reset(user) @spec perform(atom(), User.t()) :: {:ok, User.t()} @@ -1770,10 +1766,9 @@ def perform(:delete, %User{} = user) do delete_user_activities(user) delete_notifications_from_user_activities(user) - delete_outgoing_pending_follow_requests(user) - delete_or_purge(user) + maybe_delete_from_db(user) end def perform(:set_activation_async, user, status), do: set_activation(user, status) From 01c2d2a29670d8b3a4acee06c5f91b52e371fd00 Mon Sep 17 00:00:00 2001 From: Alex Gleason Date: Tue, 29 Jun 2021 22:53:33 -0500 Subject: [PATCH 3/7] Also purge the user in User.perform/2 --- lib/pleroma/user.ex | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index 406a7f5f9..f3cf3c69b 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -1724,6 +1724,7 @@ def delete(users) when is_list(users) do end def delete(%User{} = user) do + # Purge the user immediately purge(user) BackgroundWorker.enqueue("delete_user", %{"user_id" => user.id}) end @@ -1749,6 +1750,9 @@ def perform(:force_password_reset, user), do: force_password_reset(user) @spec perform(atom(), User.t()) :: {:ok, User.t()} def perform(:delete, %User{} = user) do + # Purge the user again, in case perform/2 is called directly + purge(user) + # Remove all relationships user |> get_followers() From a7929c4d89a07a7f577e7cde5638bde8b1cb586a Mon Sep 17 00:00:00 2001 From: Alex Gleason Date: Tue, 29 Jun 2021 23:56:19 -0500 Subject: [PATCH 4/7] Deletions: preserve account status fields during purge, fix checks --- lib/pleroma/user.ex | 22 ++++++++++++---------- test/pleroma/user_test.exs | 4 ++-- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index f3cf3c69b..5d8b936aa 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -1692,9 +1692,7 @@ def purge_user_changeset(user) do follower_count: 0, following_count: 0, is_locked: false, - is_confirmed: true, password_reset_pending: false, - is_approved: true, registration_reason: nil, confirmation_token: nil, domain_blocks: [], @@ -1710,9 +1708,15 @@ def purge_user_changeset(user) do raw_fields: [], is_discoverable: false, also_known_as: [] + # id: preserved + # ap_id: preserved + # nickname: preserved }) end + # Purge doesn't delete the user from the database. + # It just nulls all its fields and deactivates it. + # See `User.purge_user_changeset/1` above. def purge(%User{} = user) do user |> purge_user_changeset() @@ -1729,20 +1733,18 @@ def delete(%User{} = user) do BackgroundWorker.enqueue("delete_user", %{"user_id" => user.id}) end + # *Actually* delete the user from the DB defp delete_from_db(%User{} = user) do invalidate_cache(user) Repo.delete(user) end - defp maybe_delete_from_db(%User{local: true} = user) do - status = account_status(user) + # If the user never finalized their account, it's safe to delete them. + defp maybe_delete_from_db(%User{local: true, is_confirmed: false} = user), + do: delete_from_db(user) - if status in [:confirmation_pending, :approval_pending] do - delete_from_db(user) - else - {:ok, user} - end - end + defp maybe_delete_from_db(%User{local: true, is_approved: false} = user), + do: delete_from_db(user) defp maybe_delete_from_db(user), do: {:ok, user} diff --git a/test/pleroma/user_test.exs b/test/pleroma/user_test.exs index 529f837e8..60bc58a48 100644 --- a/test/pleroma/user_test.exs +++ b/test/pleroma/user_test.exs @@ -1663,9 +1663,9 @@ test "delete/1 purges a user when they wouldn't be fully deleted" do follower_count: 0, following_count: 0, is_locked: false, - is_confirmed: true, + is_confirmed: false, password_reset_pending: false, - is_approved: true, + is_approved: false, registration_reason: nil, confirmation_token: nil, domain_blocks: [], From 43800d83f4fc3b251cdd93c28dab2df7297021b3 Mon Sep 17 00:00:00 2001 From: Alex Gleason Date: Wed, 30 Jun 2021 01:14:34 -0500 Subject: [PATCH 5/7] Deletions: allow deactivated users to be deleted --- lib/pleroma/web/activity_pub/activity_pub.ex | 9 ++++++--- .../object_validators/delete_validator.ex | 12 +++++++++++- .../activity_pub/object_validators/undo_validator.ex | 12 +++++++++++- test/pleroma/user_test.exs | 8 ++++---- 4 files changed, 32 insertions(+), 9 deletions(-) diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index 5b45e2ca1..787b5884f 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -52,15 +52,18 @@ defp get_recipients(data) do {recipients, to, cc} end - defp check_actor_is_active(nil), do: true + defp check_actor_can_insert(%{"type" => "Delete"}), do: true + defp check_actor_can_insert(%{"type" => "Undo"}), do: true - defp check_actor_is_active(actor) when is_binary(actor) do + defp check_actor_can_insert(%{"actor" => actor}) when is_binary(actor) do case User.get_cached_by_ap_id(actor) do %User{is_active: true} -> true _ -> false end end + defp check_actor_can_insert(_), do: true + defp check_remote_limit(%{"object" => %{"content" => content}}) when not is_nil(content) do limit = Config.get([:instance, :remote_limit]) String.length(content) <= limit @@ -116,7 +119,7 @@ def persist(object, meta) do def insert(map, local \\ true, fake \\ false, bypass_actor_check \\ false) when is_map(map) do with nil <- Activity.normalize(map), map <- lazy_put_activity_defaults(map, fake), - {_, true} <- {:actor_check, bypass_actor_check || check_actor_is_active(map["actor"])}, + {_, true} <- {:actor_check, bypass_actor_check || check_actor_can_insert(map)}, {_, true} <- {:remote_limit_pass, check_remote_limit(map)}, {:ok, map} <- MRF.filter(map), {recipients, _, _} = get_recipients(map), diff --git a/lib/pleroma/web/activity_pub/object_validators/delete_validator.ex b/lib/pleroma/web/activity_pub/object_validators/delete_validator.ex index fc1a79a72..750ea0f7f 100644 --- a/lib/pleroma/web/activity_pub/object_validators/delete_validator.ex +++ b/lib/pleroma/web/activity_pub/object_validators/delete_validator.ex @@ -7,6 +7,7 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidators.DeleteValidator do alias Pleroma.Activity alias Pleroma.EctoType.ActivityPub.ObjectValidators + alias Pleroma.User import Ecto.Changeset import Pleroma.Web.ActivityPub.ObjectValidators.CommonValidations @@ -57,7 +58,7 @@ def validate_data(cng) do cng |> validate_required([:id, :type, :actor, :to, :cc, :object]) |> validate_inclusion(:type, ["Delete"]) - |> validate_actor_presence() + |> validate_delete_actor(:actor) |> validate_modification_rights() |> validate_object_or_user_presence(allowed_types: @deletable_types) |> add_deleted_activity_id() @@ -72,4 +73,13 @@ def cast_and_validate(data) do |> cast_data |> validate_data end + + defp validate_delete_actor(cng, field_name) do + validate_change(cng, field_name, fn field_name, actor -> + case User.get_cached_by_ap_id(actor) do + %User{} -> [] + _ -> [{field_name, "can't find user"}] + end + end) + end end diff --git a/lib/pleroma/web/activity_pub/object_validators/undo_validator.ex b/lib/pleroma/web/activity_pub/object_validators/undo_validator.ex index 783a79ddb..ab29f9820 100644 --- a/lib/pleroma/web/activity_pub/object_validators/undo_validator.ex +++ b/lib/pleroma/web/activity_pub/object_validators/undo_validator.ex @@ -7,6 +7,7 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidators.UndoValidator do alias Pleroma.Activity alias Pleroma.EctoType.ActivityPub.ObjectValidators + alias Pleroma.User import Ecto.Changeset import Pleroma.Web.ActivityPub.ObjectValidators.CommonValidations @@ -42,7 +43,7 @@ def validate_data(data_cng) do data_cng |> validate_inclusion(:type, ["Undo"]) |> validate_required([:id, :type, :object, :actor, :to, :cc]) - |> validate_actor_presence() + |> validate_undo_actor(:actor) |> validate_object_presence() |> validate_undo_rights() end @@ -59,4 +60,13 @@ def validate_undo_rights(cng) do _ -> cng end end + + defp validate_undo_actor(cng, field_name) do + validate_change(cng, field_name, fn field_name, actor -> + case User.get_cached_by_ap_id(actor) do + %User{} -> [] + _ -> [{field_name, "can't find user"}] + end + end) + end end diff --git a/test/pleroma/user_test.exs b/test/pleroma/user_test.exs index 60bc58a48..181990e4b 100644 --- a/test/pleroma/user_test.exs +++ b/test/pleroma/user_test.exs @@ -1621,9 +1621,9 @@ test "delete/1 purges a user when they wouldn't be fully deleted" do follower_count: 9, following_count: 9001, is_locked: true, - is_confirmed: false, + is_confirmed: true, password_reset_pending: true, - is_approved: false, + is_approved: true, registration_reason: "ahhhhh", confirmation_token: "qqqq", domain_blocks: ["lain.com"], @@ -1663,9 +1663,9 @@ test "delete/1 purges a user when they wouldn't be fully deleted" do follower_count: 0, following_count: 0, is_locked: false, - is_confirmed: false, + is_confirmed: true, password_reset_pending: false, - is_approved: false, + is_approved: true, registration_reason: nil, confirmation_token: nil, domain_blocks: [], From beb1c98ab5e0848127a4490180364552f6fcdbf5 Mon Sep 17 00:00:00 2001 From: Alex Gleason Date: Wed, 30 Jun 2021 01:48:17 -0500 Subject: [PATCH 6/7] Deletions: don't purge keys so Delete/Undo activities can be signed --- lib/pleroma/user.ex | 2 -- test/pleroma/user_test.exs | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index 5d8b936aa..de3b8ca3b 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -1680,8 +1680,6 @@ def purge_user_changeset(user) do email: nil, name: nil, password_hash: nil, - keys: nil, - public_key: nil, avatar: %{}, tags: [], last_refreshed_at: nil, diff --git a/test/pleroma/user_test.exs b/test/pleroma/user_test.exs index 181990e4b..ec0aaa9eb 100644 --- a/test/pleroma/user_test.exs +++ b/test/pleroma/user_test.exs @@ -1651,8 +1651,8 @@ test "delete/1 purges a user when they wouldn't be fully deleted" do email: nil, name: nil, password_hash: nil, - keys: nil, - public_key: nil, + keys: "RSA begin buplic key", + public_key: "--PRIVATE KEYE--", avatar: %{}, tags: [], last_refreshed_at: nil, From 310ef6b70d9ca18d857f43677d857d09d91ffe0e Mon Sep 17 00:00:00 2001 From: Alex Gleason Date: Wed, 30 Jun 2021 12:25:20 -0500 Subject: [PATCH 7/7] Deletions: change User.purge/1 to defp, add CHANGELOG entry --- CHANGELOG.md | 2 ++ lib/pleroma/user.ex | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 52d92c6d2..330802b29 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ### Fixed - Don't crash so hard when email settings are invalid. - Checking activated Upload Filters for required commands. +- Remote users can no longer reappear after being deleted. +- Deactivated users may now be deleted. - Mix task `pleroma.database prune_objects` ### Removed diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index f5b12abad..62506f37a 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -1730,7 +1730,7 @@ def purge_user_changeset(user) do # Purge doesn't delete the user from the database. # It just nulls all its fields and deactivates it. # See `User.purge_user_changeset/1` above. - def purge(%User{} = user) do + defp purge(%User{} = user) do user |> purge_user_changeset() |> update_and_set_cache()