From 2b94ae3b3911d78ee603163f7c8aa256ed65643f Mon Sep 17 00:00:00 2001 From: Maxim Filippov Date: Thu, 15 Aug 2019 01:35:29 +0300 Subject: [PATCH] Do not check if actor is active when deleting a user --- CHANGELOG.md | 1 + lib/mix/tasks/pleroma/user.ex | 2 +- lib/pleroma/user.ex | 34 +++++++++---------- lib/pleroma/web/activity_pub/activity_pub.ex | 20 +++++------ .../web/activity_pub/transmogrifier.ex | 2 +- .../web/admin_api/admin_api_controller.ex | 4 +-- .../web/ostatus/handlers/delete_handler.ex | 2 +- test/user_test.exs | 3 +- 8 files changed, 34 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dccc36965..a7b776beb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ### Security - OStatus: eliminate the possibility of a protocol downgrade attack. - OStatus: prevent following locked accounts, bypassing the approval process. +– ActivityPub: Do not check if actor is active when deleting a user ### Changed - **Breaking:** Configuration: A setting to explicitly disable the mailer was added, defaulting to true, if you are using a mailer add `config :pleroma, Pleroma.Emails.Mailer, enabled: true` to your config diff --git a/lib/mix/tasks/pleroma/user.ex b/lib/mix/tasks/pleroma/user.ex index f33d01429..a3f8bc945 100644 --- a/lib/mix/tasks/pleroma/user.ex +++ b/lib/mix/tasks/pleroma/user.ex @@ -176,7 +176,7 @@ defmodule Mix.Tasks.Pleroma.User do start_pleroma() with %User{local: true} = user <- User.get_cached_by_nickname(nickname) do - User.perform(:delete, user, nil) + User.perform(:delete, user) shell_info("User #{nickname} deleted.") else _ -> diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index 14057a0e4..7d18f099e 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -1029,26 +1029,13 @@ defmodule Pleroma.User do |> update_and_set_cache() end - @spec perform(atom(), User.t()) :: {:ok, User.t()} - def perform(:fetch_initial_posts, %User{} = user) do - pages = Pleroma.Config.get!([:fetch_initial_posts, :pages]) - - Enum.each( - # Insert all the posts in reverse order, so they're in the right order on the timeline - Enum.reverse(Utils.fetch_ordered_collection(user.info.source_data["outbox"], pages)), - &Pleroma.Web.Federator.incoming_ap_doc/1 - ) - - {:ok, user} - end - @spec delete(User.t()) :: :ok - def delete(%User{} = user, actor \\ nil), - do: PleromaJobQueue.enqueue(:background, __MODULE__, [:delete, user, actor]) + def delete(%User{} = user), + do: PleromaJobQueue.enqueue(:background, __MODULE__, [:delete, user]) @spec perform(atom(), User.t()) :: {:ok, User.t()} - def perform(:delete, %User{} = user, actor) do - {:ok, _user} = ActivityPub.delete(user, actor: actor) + def perform(:delete, %User{} = user) do + {:ok, _user} = ActivityPub.delete(user) # Remove all relationships {:ok, followers} = User.get_followers(user) @@ -1070,6 +1057,19 @@ defmodule Pleroma.User do Repo.delete(user) end + @spec perform(atom(), User.t()) :: {:ok, User.t()} + def perform(:fetch_initial_posts, %User{} = user) do + pages = Pleroma.Config.get!([:fetch_initial_posts, :pages]) + + Enum.each( + # Insert all the posts in reverse order, so they're in the right order on the timeline + Enum.reverse(Utils.fetch_ordered_collection(user.info.source_data["outbox"], pages)), + &Pleroma.Web.Federator.incoming_ap_doc/1 + ) + + {:ok, user} + end + def perform(:deactivate_async, user, status), do: deactivate(user, status) @spec perform(atom(), User.t(), list()) :: list() | {:error, any()} diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index 8f669acb9..da873b7b0 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -61,7 +61,9 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do {recipients, to, cc} end - defp check_actor_is_active(actor) do + defp check_actor_is_active(true, _), do: :ok + + defp check_actor_is_active(false, actor) do if not is_nil(actor) do with user <- User.get_cached_by_ap_id(actor), false <- user.info.deactivated do @@ -119,10 +121,10 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do def increase_poll_votes_if_vote(_create_data), do: :noop - def insert(map, local \\ true, fake \\ false) when is_map(map) 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), - :ok <- check_actor_is_active(map["actor"]), + :ok <- check_actor_is_active(bypass_actor_check, map["actor"]), {_, true} <- {:remote_limit_error, check_remote_limit(map)}, {:ok, map} <- MRF.filter(map), {recipients, _, _} = get_recipients(map), @@ -403,22 +405,20 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do end end - def delete(data, opts \\ %{actor: nil, local: true}) - - def delete(%User{ap_id: ap_id, follower_address: follower_address} = user, opts) do + def delete(%User{ap_id: ap_id, follower_address: follower_address} = user) do with data <- %{ "to" => [follower_address], "type" => "Delete", - "actor" => opts[:actor] || ap_id, + "actor" => ap_id, "object" => %{"type" => "Person", "id" => ap_id} }, - {:ok, activity} <- insert(data, true, true), + {:ok, activity} <- insert(data, true, true, true), :ok <- maybe_federate(activity) do {:ok, user} end end - def delete(%Object{data: %{"id" => id, "actor" => actor}} = object, opts) do + def delete(%Object{data: %{"id" => id, "actor" => actor}} = object, local \\ true) do user = User.get_cached_by_ap_id(actor) to = (object.data["to"] || []) ++ (object.data["cc"] || []) @@ -430,7 +430,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do "to" => to, "deleted_activity_id" => activity && activity.id }, - {:ok, activity} <- insert(data, opts[:local], false), + {:ok, activity} <- insert(data, local, false), stream_out_participations(object, user), _ <- decrease_replies_count_if_reply(object), # Changing note count prior to enqueuing federation task in order to avoid diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index b34ef73c0..5403b71d8 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -649,7 +649,7 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do {:ok, %User{} = actor} <- User.get_or_fetch_by_ap_id(actor), {:ok, object} <- get_obj_helper(object_id), :ok <- Containment.contain_origin(actor.ap_id, object.data), - {:ok, activity} <- ActivityPub.delete(object, local: false) do + {:ok, activity} <- ActivityPub.delete(object, false) do {:ok, activity} else nil -> diff --git a/lib/pleroma/web/admin_api/admin_api_controller.ex b/lib/pleroma/web/admin_api/admin_api_controller.ex index 63c9a7d7f..2d3d0adc4 100644 --- a/lib/pleroma/web/admin_api/admin_api_controller.ex +++ b/lib/pleroma/web/admin_api/admin_api_controller.ex @@ -25,9 +25,9 @@ defmodule Pleroma.Web.AdminAPI.AdminAPIController do action_fallback(:errors) - def user_delete(%{assigns: %{user: admin}} = conn, %{"nickname" => nickname}) do + def user_delete(conn, %{"nickname" => nickname}) do User.get_cached_by_nickname(nickname) - |> User.delete(admin.ap_id) + |> User.delete() conn |> json(nickname) diff --git a/lib/pleroma/web/ostatus/handlers/delete_handler.ex b/lib/pleroma/web/ostatus/handlers/delete_handler.ex index ac2dc115c..b2f9f3946 100644 --- a/lib/pleroma/web/ostatus/handlers/delete_handler.ex +++ b/lib/pleroma/web/ostatus/handlers/delete_handler.ex @@ -11,7 +11,7 @@ defmodule Pleroma.Web.OStatus.DeleteHandler do def handle_delete(entry, _doc \\ nil) do with id <- XML.string_from_xpath("//id", entry), %Object{} = object <- Object.normalize(id), - {:ok, delete} <- ActivityPub.delete(object, local: false) do + {:ok, delete} <- ActivityPub.delete(object, false) do delete end end diff --git a/test/user_test.exs b/test/user_test.exs index e2da8d84b..755c6005d 100644 --- a/test/user_test.exs +++ b/test/user_test.exs @@ -999,10 +999,9 @@ defmodule Pleroma.UserTest do end test "it deletes deactivated user" do - admin = insert(:user, %{info: %{is_admin: true}}) {:ok, user} = insert(:user, info: %{deactivated: true}) |> User.set_cache() - assert {:ok, _} = User.delete(user, admin.ap_id) + assert {:ok, _} = User.delete(user) refute User.get_by_id(user.id) end