From dfae61c25c7ee2bb8add38b2cbaa8391f03c9550 Mon Sep 17 00:00:00 2001
From: Maxim Filippov <colixer@gmail.com>
Date: Fri, 9 Aug 2019 23:05:28 +0300
Subject: [PATCH 1/4] Fix deactivated user deletion

---
 lib/mix/tasks/pleroma/user.ex                 |  2 +-
 lib/pleroma/user.ex                           | 34 +++++++++----------
 lib/pleroma/web/activity_pub/activity_pub.ex  | 10 +++---
 .../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                            |  8 +++++
 7 files changed, 36 insertions(+), 26 deletions(-)

diff --git a/lib/mix/tasks/pleroma/user.ex b/lib/mix/tasks/pleroma/user.ex
index a3f8bc945..f33d01429 100644
--- a/lib/mix/tasks/pleroma/user.ex
+++ b/lib/mix/tasks/pleroma/user.ex
@@ -176,7 +176,7 @@ def run(["rm", nickname]) do
     start_pleroma()
 
     with %User{local: true} = user <- User.get_cached_by_nickname(nickname) do
-      User.perform(:delete, user)
+      User.perform(:delete, user, nil)
       shell_info("User #{nickname} deleted.")
     else
       _ ->
diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex
index 7d18f099e..14057a0e4 100644
--- a/lib/pleroma/user.ex
+++ b/lib/pleroma/user.ex
@@ -1029,13 +1029,26 @@ def update_notification_settings(%User{} = user, settings \\ %{}) 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),
-    do: PleromaJobQueue.enqueue(:background, __MODULE__, [:delete, user])
+  def delete(%User{} = user, actor \\ nil),
+    do: PleromaJobQueue.enqueue(:background, __MODULE__, [:delete, user, actor])
 
   @spec perform(atom(), User.t()) :: {:ok, User.t()}
-  def perform(:delete, %User{} = user) do
-    {:ok, _user} = ActivityPub.delete(user)
+  def perform(:delete, %User{} = user, actor) do
+    {:ok, _user} = ActivityPub.delete(user, actor: actor)
 
     # Remove all relationships
     {:ok, followers} = User.get_followers(user)
@@ -1057,19 +1070,6 @@ def perform(:delete, %User{} = 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 1a279a7df..8f669acb9 100644
--- a/lib/pleroma/web/activity_pub/activity_pub.ex
+++ b/lib/pleroma/web/activity_pub/activity_pub.ex
@@ -403,11 +403,13 @@ def unfollow(follower, followed, activity_id \\ nil, local \\ true) do
     end
   end
 
-  def delete(%User{ap_id: ap_id, follower_address: follower_address} = user) do
+  def delete(data, opts \\ %{actor: nil, local: true})
+
+  def delete(%User{ap_id: ap_id, follower_address: follower_address} = user, opts) do
     with data <- %{
            "to" => [follower_address],
            "type" => "Delete",
-           "actor" => ap_id,
+           "actor" => opts[:actor] || ap_id,
            "object" => %{"type" => "Person", "id" => ap_id}
          },
          {:ok, activity} <- insert(data, true, true),
@@ -416,7 +418,7 @@ def delete(%User{ap_id: ap_id, follower_address: follower_address} = user) do
     end
   end
 
-  def delete(%Object{data: %{"id" => id, "actor" => actor}} = object, local \\ true) do
+  def delete(%Object{data: %{"id" => id, "actor" => actor}} = object, opts) do
     user = User.get_cached_by_ap_id(actor)
     to = (object.data["to"] || []) ++ (object.data["cc"] || [])
 
@@ -428,7 +430,7 @@ def delete(%Object{data: %{"id" => id, "actor" => actor}} = object, local \\ tru
            "to" => to,
            "deleted_activity_id" => activity && activity.id
          },
-         {:ok, activity} <- insert(data, local, false),
+         {:ok, activity} <- insert(data, opts[: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 5403b71d8..b34ef73c0 100644
--- a/lib/pleroma/web/activity_pub/transmogrifier.ex
+++ b/lib/pleroma/web/activity_pub/transmogrifier.ex
@@ -649,7 +649,7 @@ def handle_incoming(
          {: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, false) do
+         {:ok, activity} <- ActivityPub.delete(object, local: 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 2d3d0adc4..63c9a7d7f 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(conn, %{"nickname" => nickname}) do
+  def user_delete(%{assigns: %{user: admin}} = conn, %{"nickname" => nickname}) do
     User.get_cached_by_nickname(nickname)
-    |> User.delete()
+    |> User.delete(admin.ap_id)
 
     conn
     |> json(nickname)
diff --git a/lib/pleroma/web/ostatus/handlers/delete_handler.ex b/lib/pleroma/web/ostatus/handlers/delete_handler.ex
index b2f9f3946..ac2dc115c 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, false) do
+         {:ok, delete} <- ActivityPub.delete(object, local: false) do
       delete
     end
   end
diff --git a/test/user_test.exs b/test/user_test.exs
index 8440d456d..e2da8d84b 100644
--- a/test/user_test.exs
+++ b/test/user_test.exs
@@ -998,6 +998,14 @@ 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
+      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)
+      refute User.get_by_id(user.id)
+    end
+
     test "it deletes a user, all follow relationships and all activities", %{user: user} do
       follower = insert(:user)
       {:ok, follower} = User.follow(follower, user)

From 2b94ae3b3911d78ee603163f7c8aa256ed65643f Mon Sep 17 00:00:00 2001
From: Maxim Filippov <colixer@gmail.com>
Date: Thu, 15 Aug 2019 01:35:29 +0300
Subject: [PATCH 2/4] 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 @@ def run(["rm", nickname]) 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 @@ def update_notification_settings(%User{} = user, settings \\ %{}) 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 @@ def perform(:delete, %User{} = user, actor) 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 @@ defp get_recipients(data) 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 @@ def increase_poll_votes_if_vote(%{
 
   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 @@ def unfollow(follower, followed, activity_id \\ nil, local \\ true) 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 @@ def delete(%Object{data: %{"id" => id, "actor" => actor}} = object, opts) 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 @@ def handle_incoming(
          {: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 @@ test ".delete_user_activities deletes all create activities", %{user: user} 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
 

From b27fafe161241c954b713281bebd6ffe1e990884 Mon Sep 17 00:00:00 2001
From: Maxim Filippov <colixer@gmail.com>
Date: Thu, 15 Aug 2019 01:42:43 +0300
Subject: [PATCH 3/4] Fix CHANGELOG entry

---
 CHANGELOG.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index a7b776beb..161f88176 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,7 +7,6 @@ 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
@@ -39,6 +38,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 - Invalid SemVer version generation, when the current branch does not have commits ahead of tag/checked out on a tag
 - Pleroma.Upload base_url was not automatically whitelisted by MediaProxy. Now your custom CDN or file hosting will be accessed directly as expected.
 - Report email not being sent to admins when the reporter is a remote user
+- ActivityPub: Deactivated user deletion
 
 ### Added
 - MRF: Support for priming the mediaproxy cache (`Pleroma.Web.ActivityPub.MRF.MediaProxyWarmingPolicy`)

From 5171aa5b4d6be5ba911039c52fa356da068b4b4f Mon Sep 17 00:00:00 2001
From: Maxim Filippov <colixer@gmail.com>
Date: Mon, 19 Aug 2019 20:36:25 +0300
Subject: [PATCH 4/4] Refactor check_actor_is_active

---
 lib/pleroma/web/activity_pub/activity_pub.ex | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex
index 16e0c3880..2e8cbe13d 100644
--- a/lib/pleroma/web/activity_pub/activity_pub.ex
+++ b/lib/pleroma/web/activity_pub/activity_pub.ex
@@ -61,18 +61,16 @@ defp get_recipients(data) do
     {recipients, to, cc}
   end
 
-  defp check_actor_is_active(true, _), do: :ok
-
-  defp check_actor_is_active(false, actor) do
+  defp check_actor_is_active(actor) do
     if not is_nil(actor) do
       with user <- User.get_cached_by_ap_id(actor),
            false <- user.info.deactivated do
-        :ok
+        true
       else
-        _e -> :reject
+        _e -> false
       end
     else
-      :ok
+      true
     end
   end
 
@@ -124,7 +122,7 @@ def increase_poll_votes_if_vote(_create_data), do: :noop
   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(bypass_actor_check, map["actor"]),
+         true <- bypass_actor_check || check_actor_is_active(map["actor"]),
          {_, true} <- {:remote_limit_error, check_remote_limit(map)},
          {:ok, map} <- MRF.filter(map),
          {recipients, _, _} = get_recipients(map),