From a3bb2e5474ee068bf375b24df8906e51654c9699 Mon Sep 17 00:00:00 2001 From: lain Date: Tue, 5 May 2020 16:42:34 +0200 Subject: [PATCH] Undoing: Move undoing announcements to the pipeline everywhere. --- lib/pleroma/user.ex | 10 +--- lib/pleroma/web/activity_pub/activity_pub.ex | 28 ----------- lib/pleroma/web/activity_pub/side_effects.ex | 8 ++++ .../web/activity_pub/transmogrifier.ex | 21 +-------- lib/pleroma/web/common_api/common_api.ex | 9 ++-- .../controllers/status_controller.ex | 6 +-- test/notification_test.exs | 2 +- test/web/activity_pub/activity_pub_test.exs | 46 ------------------- test/web/activity_pub/side_effects_test.exs | 26 ++++++++++- .../transmogrifier/undo_handling_test.exs | 5 +- 10 files changed, 45 insertions(+), 116 deletions(-) diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index 0136ba119..aa675a521 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -1554,7 +1554,7 @@ defp delete_activity(%{data: %{"type" => "Create"}} = activity) do |> ActivityPub.delete() end - defp delete_activity(%{data: %{"type" => "Like"}} = activity) do + defp delete_activity(%{data: %{"type" => type}} = activity) when type in ["Like", "Announce"] do actor = activity.actor |> get_cached_by_ap_id() @@ -1564,14 +1564,6 @@ defp delete_activity(%{data: %{"type" => "Like"}} = activity) do Pipeline.common_pipeline(undo, local: true) end - defp delete_activity(%{data: %{"type" => "Announce"}} = activity) do - object = Object.normalize(activity) - - activity.actor - |> get_cached_by_ap_id() - |> ActivityPub.unannounce(object) - end - defp delete_activity(_activity), do: "Doing nothing" def html_filter_policy(%User{no_rich_text: true}) do diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index c94af3b5f..be3d72c82 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -402,34 +402,6 @@ defp do_announce(user, object, activity_id, local, public) do end end - @spec unannounce(User.t(), Object.t(), String.t() | nil, boolean()) :: - {:ok, Activity.t(), Object.t()} | {:ok, Object.t()} | {:error, any()} - def unannounce( - %User{} = actor, - %Object{} = object, - activity_id \\ nil, - local \\ true - ) do - with {:ok, result} <- - Repo.transaction(fn -> do_unannounce(actor, object, activity_id, local) end) do - result - end - end - - defp do_unannounce(actor, object, activity_id, local) do - with %Activity{} = announce_activity <- get_existing_announce(actor.ap_id, object), - unannounce_data <- make_unannounce_data(actor, announce_activity, activity_id), - {:ok, unannounce_activity} <- insert(unannounce_data, local), - :ok <- maybe_federate(unannounce_activity), - {:ok, _activity} <- Repo.delete(announce_activity), - {:ok, object} <- remove_announce_from_object(announce_activity, object) do - {:ok, unannounce_activity, object} - else - nil -> {:ok, object} - {:error, error} -> Repo.rollback(error) - end - end - @spec follow(User.t(), User.t(), String.t() | nil, boolean()) :: {:ok, Activity.t()} | {:error, any()} def follow(follower, followed, activity_id \\ nil, local \\ true) do diff --git a/lib/pleroma/web/activity_pub/side_effects.ex b/lib/pleroma/web/activity_pub/side_effects.ex index d58df9394..146d30ac1 100644 --- a/lib/pleroma/web/activity_pub/side_effects.ex +++ b/lib/pleroma/web/activity_pub/side_effects.ex @@ -53,5 +53,13 @@ def handle_undoing(%{data: %{"type" => "EmojiReact"}} = object) do end end + def handle_undoing(%{data: %{"type" => "Announce"}} = object) do + with %Object{} = liked_object <- Object.get_by_ap_id(object.data["object"]), + {:ok, _} <- Utils.remove_announce_from_object(object, liked_object), + {:ok, _} <- Repo.delete(object) do + :ok + end + end + def handle_undoing(object), do: {:error, ["don't know how to handle", object]} end diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index 94849b5f5..afa171448 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -768,25 +768,6 @@ def handle_incoming( end end - def handle_incoming( - %{ - "type" => "Undo", - "object" => %{"type" => "Announce", "object" => object_id}, - "actor" => _actor, - "id" => id - } = data, - _options - ) do - with actor <- Containment.get_actor(data), - {:ok, %User{} = actor} <- User.get_or_fetch_by_ap_id(actor), - {:ok, object} <- get_obj_helper(object_id), - {:ok, activity, _} <- ActivityPub.unannounce(actor, object, id, false) do - {:ok, activity} - else - _e -> :error - end - end - def handle_incoming( %{ "type" => "Undo", @@ -847,7 +828,7 @@ def handle_incoming( } = data, _options ) - when type in ["Like", "EmojiReact"] do + when type in ["Like", "EmojiReact", "Announce"] do with {:ok, activity, _} <- Pipeline.common_pipeline(data, local: false) do {:ok, activity} end diff --git a/lib/pleroma/web/common_api/common_api.ex b/lib/pleroma/web/common_api/common_api.ex index 067ac875e..fc8246871 100644 --- a/lib/pleroma/web/common_api/common_api.ex +++ b/lib/pleroma/web/common_api/common_api.ex @@ -107,9 +107,12 @@ def repeat(id, user, params \\ %{}) do def unrepeat(id, user) do with {_, %Activity{data: %{"type" => "Create"}} = activity} <- - {:find_activity, Activity.get_by_id(id)} do - object = Object.normalize(activity) - ActivityPub.unannounce(user, object) + {:find_activity, Activity.get_by_id(id)}, + %Object{} = note <- Object.normalize(activity, false), + %Activity{} = announce <- Utils.get_existing_announce(user.ap_id, note), + {:ok, undo, _} <- Builder.undo(user, announce), + {:ok, activity, _} <- Pipeline.common_pipeline(undo, local: true) do + {:ok, activity} else {:find_activity, _} -> {:error, :not_found} _ -> {:error, dgettext("errors", "Could not unrepeat")} diff --git a/lib/pleroma/web/mastodon_api/controllers/status_controller.ex b/lib/pleroma/web/mastodon_api/controllers/status_controller.ex index 2a5eac9d9..12e3ba15e 100644 --- a/lib/pleroma/web/mastodon_api/controllers/status_controller.ex +++ b/lib/pleroma/web/mastodon_api/controllers/status_controller.ex @@ -206,9 +206,9 @@ def reblog(%{assigns: %{user: user}} = conn, %{"id" => ap_id_or_id} = params) do end @doc "POST /api/v1/statuses/:id/unreblog" - def unreblog(%{assigns: %{user: user}} = conn, %{"id" => ap_id_or_id}) do - with {:ok, _unannounce, %{data: %{"id" => id}}} <- CommonAPI.unrepeat(ap_id_or_id, user), - %Activity{} = activity <- Activity.get_create_by_object_ap_id_with_object(id) do + def unreblog(%{assigns: %{user: user}} = conn, %{"id" => activity_id}) do + with {:ok, _unannounce} <- CommonAPI.unrepeat(activity_id, user), + %Activity{} = activity <- Activity.get_by_id(activity_id) do try_render(conn, "show.json", %{activity: activity, for: user, as: :activity}) end end diff --git a/test/notification_test.exs b/test/notification_test.exs index 7d5b82993..09714f4c5 100644 --- a/test/notification_test.exs +++ b/test/notification_test.exs @@ -758,7 +758,7 @@ test "repeating an activity results in 1 notification, then 0 if the activity is assert length(Notification.for_user(user)) == 1 - {:ok, _, _} = CommonAPI.unrepeat(activity.id, other_user) + {:ok, _} = CommonAPI.unrepeat(activity.id, other_user) assert Enum.empty?(Notification.for_user(user)) end diff --git a/test/web/activity_pub/activity_pub_test.exs b/test/web/activity_pub/activity_pub_test.exs index cb2d41f0b..2c3d354f2 100644 --- a/test/web/activity_pub/activity_pub_test.exs +++ b/test/web/activity_pub/activity_pub_test.exs @@ -1008,52 +1008,6 @@ test "does not add an announce activity to the db if the announcer is not the au end end - describe "unannouncing an object" do - test "unannouncing a previously announced object" do - note_activity = insert(:note_activity) - object = Object.normalize(note_activity) - user = insert(:user) - - # Unannouncing an object that is not announced does nothing - {:ok, object} = ActivityPub.unannounce(user, object) - refute object.data["announcement_count"] - - {:ok, announce_activity, object} = ActivityPub.announce(user, object) - assert object.data["announcement_count"] == 1 - - {:ok, unannounce_activity, object} = ActivityPub.unannounce(user, object) - assert object.data["announcement_count"] == 0 - - assert unannounce_activity.data["to"] == [ - User.ap_followers(user), - object.data["actor"] - ] - - assert unannounce_activity.data["type"] == "Undo" - assert unannounce_activity.data["object"] == announce_activity.data - assert unannounce_activity.data["actor"] == user.ap_id - assert unannounce_activity.data["context"] == announce_activity.data["context"] - - assert Activity.get_by_id(announce_activity.id) == nil - end - - test "reverts unannouncing on error" do - note_activity = insert(:note_activity) - object = Object.normalize(note_activity) - user = insert(:user) - - {:ok, _announce_activity, object} = ActivityPub.announce(user, object) - assert object.data["announcement_count"] == 1 - - with_mock(Utils, [:passthrough], maybe_federate: fn _ -> {:error, :reverted} end) do - assert {:error, :reverted} = ActivityPub.unannounce(user, object) - end - - object = Object.get_by_ap_id(object.data["id"]) - assert object.data["announcement_count"] == 1 - end - end - describe "uploading files" do test "copies the file to the configured folder" do file = %Plug.Upload{ diff --git a/test/web/activity_pub/side_effects_test.exs b/test/web/activity_pub/side_effects_test.exs index abcfdfa2f..00241320b 100644 --- a/test/web/activity_pub/side_effects_test.exs +++ b/test/web/activity_pub/side_effects_test.exs @@ -22,8 +22,8 @@ defmodule Pleroma.Web.ActivityPub.SideEffectsTest do user = insert(:user) {:ok, post} = CommonAPI.post(poster, %{"status" => "hey"}) {:ok, like} = CommonAPI.favorite(user, post.id) - {:ok, reaction, _} = CommonAPI.react_with_emoji(post.id, user, "👍") + {:ok, announce, _} = CommonAPI.repeat(post.id, user) {:ok, undo_data, _meta} = Builder.undo(user, like) {:ok, like_undo, _meta} = ActivityPub.persist(undo_data, local: true) @@ -31,15 +31,37 @@ defmodule Pleroma.Web.ActivityPub.SideEffectsTest do {:ok, undo_data, _meta} = Builder.undo(user, reaction) {:ok, reaction_undo, _meta} = ActivityPub.persist(undo_data, local: true) + {:ok, undo_data, _meta} = Builder.undo(user, announce) + {:ok, announce_undo, _meta} = ActivityPub.persist(undo_data, local: true) + %{ like_undo: like_undo, post: post, like: like, reaction_undo: reaction_undo, - reaction: reaction + reaction: reaction, + announce_undo: announce_undo, + announce: announce } end + test "an announce undo removes the announce from the object", %{ + announce_undo: announce_undo, + post: post + } do + {:ok, _announce_undo, _} = SideEffects.handle(announce_undo) + + object = Object.get_by_ap_id(post.data["object"]) + + assert object.data["announcement_count"] == 0 + assert object.data["announcements"] == [] + end + + test "deletes the original announce", %{announce_undo: announce_undo, announce: announce} do + {:ok, _announce_undo, _} = SideEffects.handle(announce_undo) + refute Activity.get_by_id(announce.id) + end + test "a reaction undo removes the reaction from the object", %{ reaction_undo: reaction_undo, post: post diff --git a/test/web/activity_pub/transmogrifier/undo_handling_test.exs b/test/web/activity_pub/transmogrifier/undo_handling_test.exs index bf2a6bc5b..281cf5b0d 100644 --- a/test/web/activity_pub/transmogrifier/undo_handling_test.exs +++ b/test/web/activity_pub/transmogrifier/undo_handling_test.exs @@ -125,11 +125,8 @@ test "it works for incoming unannounces with an existing notice" do {:ok, %Activity{data: data, local: false}} = Transmogrifier.handle_incoming(data) assert data["type"] == "Undo" - assert object_data = data["object"] - assert object_data["type"] == "Announce" - assert object_data["object"] == activity.data["object"] - assert object_data["id"] == + assert data["object"] == "http://mastodon.example.org/users/admin/statuses/99542391527669785/activity" end