From 32b8386edeec3e9b24123c3ccc81a22f1edd5a1c Mon Sep 17 00:00:00 2001 From: lain Date: Thu, 30 Apr 2020 21:23:18 +0200 Subject: [PATCH] DeleteValidator: Don't federate local deletions of remote objects. Closes #1497 --- .../web/activity_pub/object_validator.ex | 8 +- .../object_validators/delete_validator.ex | 20 ++++- lib/pleroma/web/activity_pub/pipeline.ex | 4 +- .../activity_pub/object_validator_test.exs | 17 +++- test/web/common_api/common_api_test.exs | 80 +++++++++++++++++++ 5 files changed, 119 insertions(+), 10 deletions(-) diff --git a/lib/pleroma/web/activity_pub/object_validator.ex b/lib/pleroma/web/activity_pub/object_validator.ex index 32f606917..479f922f5 100644 --- a/lib/pleroma/web/activity_pub/object_validator.ex +++ b/lib/pleroma/web/activity_pub/object_validator.ex @@ -19,11 +19,11 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidator do def validate(object, meta) def validate(%{"type" => "Delete"} = object, meta) do - with {:ok, object} <- - object - |> DeleteValidator.cast_and_validate() - |> Ecto.Changeset.apply_action(:insert) do + with cng <- DeleteValidator.cast_and_validate(object), + do_not_federate <- DeleteValidator.do_not_federate?(cng), + {:ok, object} <- Ecto.Changeset.apply_action(cng, :insert) do object = stringify_keys(object) + meta = Keyword.put(meta, :do_not_federate, do_not_federate) {:ok, object, meta} end end 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 951cc1414..a2eff7b69 100644 --- a/lib/pleroma/web/activity_pub/object_validators/delete_validator.ex +++ b/lib/pleroma/web/activity_pub/object_validators/delete_validator.ex @@ -6,6 +6,7 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidators.DeleteValidator do use Ecto.Schema alias Pleroma.Activity + alias Pleroma.User alias Pleroma.Web.ActivityPub.ObjectValidators.Types import Ecto.Changeset @@ -45,12 +46,17 @@ def validate_data(cng) do cng |> validate_required([:id, :type, :actor, :to, :cc, :object]) |> validate_inclusion(:type, ["Delete"]) - |> validate_same_domain() + |> validate_actor_presence() + |> validate_deletion_rights() |> validate_object_or_user_presence() |> add_deleted_activity_id() end - def validate_same_domain(cng) do + def do_not_federate?(cng) do + !same_domain?(cng) + end + + defp same_domain?(cng) do actor_domain = cng |> get_field(:actor) @@ -63,11 +69,17 @@ def validate_same_domain(cng) do |> URI.parse() |> (& &1.host).() - if object_domain != actor_domain do + object_domain == actor_domain + end + + def validate_deletion_rights(cng) do + actor = User.get_cached_by_ap_id(get_field(cng, :actor)) + + if User.superuser?(actor) || same_domain?(cng) do cng - |> add_error(:actor, "is not allowed to delete object") else cng + |> add_error(:actor, "is not allowed to delete object") end end diff --git a/lib/pleroma/web/activity_pub/pipeline.ex b/lib/pleroma/web/activity_pub/pipeline.ex index 7ccee54c9..017e39abb 100644 --- a/lib/pleroma/web/activity_pub/pipeline.ex +++ b/lib/pleroma/web/activity_pub/pipeline.ex @@ -29,7 +29,9 @@ def common_pipeline(object, meta) do defp maybe_federate(activity, meta) do with {:ok, local} <- Keyword.fetch(meta, :local) do - if local do + do_not_federate = meta[:do_not_federate] + + if !do_not_federate && local do Federator.publish(activity) {:ok, :federated} else diff --git a/test/web/activity_pub/object_validator_test.exs b/test/web/activity_pub/object_validator_test.exs index 1d3646487..412db09ff 100644 --- a/test/web/activity_pub/object_validator_test.exs +++ b/test/web/activity_pub/object_validator_test.exs @@ -52,9 +52,11 @@ test "it's invalid if the object doesn't exist", %{valid_post_delete: valid_post test "it's invalid if the actor of the object and the actor of delete are from different domains", %{valid_post_delete: valid_post_delete} do + valid_user = insert(:user) + valid_other_actor = valid_post_delete - |> Map.put("actor", valid_post_delete["actor"] <> "1") + |> Map.put("actor", valid_user.ap_id) assert match?({:ok, _, _}, ObjectValidator.validate(valid_other_actor, [])) @@ -66,6 +68,19 @@ test "it's invalid if the actor of the object and the actor of delete are from d assert {:actor, {"is not allowed to delete object", []}} in cng.errors end + + test "it's valid if the actor of the object is a local superuser", + %{valid_post_delete: valid_post_delete} do + user = + insert(:user, local: true, is_moderator: true, ap_id: "https://gensokyo.2hu/users/raymoo") + + valid_other_actor = + valid_post_delete + |> Map.put("actor", user.ap_id) + + {:ok, _, meta} = ObjectValidator.validate(valid_other_actor, []) + assert meta[:do_not_federate] + end end describe "likes" do diff --git a/test/web/common_api/common_api_test.exs b/test/web/common_api/common_api_test.exs index 1758662b0..32d91ce02 100644 --- a/test/web/common_api/common_api_test.exs +++ b/test/web/common_api/common_api_test.exs @@ -9,11 +9,13 @@ defmodule Pleroma.Web.CommonAPITest do alias Pleroma.Object alias Pleroma.User alias Pleroma.Web.ActivityPub.ActivityPub + alias Pleroma.Web.ActivityPub.Transmogrifier alias Pleroma.Web.ActivityPub.Visibility alias Pleroma.Web.AdminAPI.AccountView alias Pleroma.Web.CommonAPI import Pleroma.Factory + import Mock require Pleroma.Constants @@ -21,6 +23,84 @@ defmodule Pleroma.Web.CommonAPITest do setup do: clear_config([:instance, :limit]) setup do: clear_config([:instance, :max_pinned_statuses]) + describe "deletion" do + test "it allows users to delete their posts" do + user = insert(:user) + + {:ok, post} = CommonAPI.post(user, %{"status" => "namu amida butsu"}) + + with_mock Pleroma.Web.Federator, + publish: fn _ -> nil end do + assert {:ok, delete} = CommonAPI.delete(post.id, user) + assert delete.local + assert called(Pleroma.Web.Federator.publish(delete)) + end + + refute Activity.get_by_id(post.id) + end + + test "it does not allow a user to delete their posts" do + user = insert(:user) + other_user = insert(:user) + + {:ok, post} = CommonAPI.post(user, %{"status" => "namu amida butsu"}) + + assert {:error, "Could not delete"} = CommonAPI.delete(post.id, other_user) + assert Activity.get_by_id(post.id) + end + + test "it allows moderators to delete other user's posts" do + user = insert(:user) + moderator = insert(:user, is_moderator: true) + + {:ok, post} = CommonAPI.post(user, %{"status" => "namu amida butsu"}) + + assert {:ok, delete} = CommonAPI.delete(post.id, moderator) + assert delete.local + + refute Activity.get_by_id(post.id) + end + + test "it allows admins to delete other user's posts" do + user = insert(:user) + moderator = insert(:user, is_admin: true) + + {:ok, post} = CommonAPI.post(user, %{"status" => "namu amida butsu"}) + + assert {:ok, delete} = CommonAPI.delete(post.id, moderator) + assert delete.local + + refute Activity.get_by_id(post.id) + end + + test "superusers deleting non-local posts won't federate the delete" do + # This is the user of the ingested activity + _user = + insert(:user, + local: false, + ap_id: "http://mastodon.example.org/users/admin", + last_refreshed_at: NaiveDateTime.utc_now() + ) + + moderator = insert(:user, is_admin: true) + + data = + File.read!("test/fixtures/mastodon-post-activity.json") + |> Jason.decode!() + + {:ok, post} = Transmogrifier.handle_incoming(data) + + with_mock Pleroma.Web.Federator, + publish: fn _ -> nil end do + assert {:ok, delete} = CommonAPI.delete(post.id, moderator) + assert delete.local + refute called(Pleroma.Web.Federator.publish(:_)) + end + + refute Activity.get_by_id(post.id) + end + end + test "favoriting race condition" do user = insert(:user) users_serial = insert_list(10, :user)