From 0efa8aa0b9567f42b1af63e2b93a9c51e9a0fb11 Mon Sep 17 00:00:00 2001
From: lain <lain@soykaf.club>
Date: Fri, 5 Jun 2020 12:26:07 +0200
Subject: [PATCH] Transmogrifier: For follows, create notifications last.

As the notification type changes depending on the follow state,
the notification should not be created and streamed out before the
state settles. For this reason, the notification creation has been
delayed until it's clear if the user has been followed or not.

This is a bit hacky but it will be properly rewritten using the
pipeline soon.
---
 lib/pleroma/web/activity_pub/activity_pub.ex   | 12 +++++++-----
 lib/pleroma/web/activity_pub/transmogrifier.ex |  5 +++--
 .../transmogrifier/follow_handling_test.exs    | 18 ++++++++++++++++++
 3 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex
index 568db2348..4f7043c92 100644
--- a/lib/pleroma/web/activity_pub/activity_pub.ex
+++ b/lib/pleroma/web/activity_pub/activity_pub.ex
@@ -363,19 +363,21 @@ def update(%{to: to, cc: cc, actor: actor, object: object} = params) do
     end
   end
 
-  @spec follow(User.t(), User.t(), String.t() | nil, boolean()) ::
+  @spec follow(User.t(), User.t(), String.t() | nil, boolean(), keyword()) ::
           {:ok, Activity.t()} | {:error, any()}
-  def follow(follower, followed, activity_id \\ nil, local \\ true) do
+  def follow(follower, followed, activity_id \\ nil, local \\ true, opts \\ []) do
     with {:ok, result} <-
-           Repo.transaction(fn -> do_follow(follower, followed, activity_id, local) end) do
+           Repo.transaction(fn -> do_follow(follower, followed, activity_id, local, opts) end) do
       result
     end
   end
 
-  defp do_follow(follower, followed, activity_id, local) do
+  defp do_follow(follower, followed, activity_id, local, opts) do
+    skip_notify_and_stream = Keyword.get(opts, :skip_notify_and_stream, false)
+
     with data <- make_follow_data(follower, followed, activity_id),
          {:ok, activity} <- insert(data, local),
-         _ <- notify_and_stream(activity),
+         _ <- skip_notify_and_stream || notify_and_stream(activity),
          :ok <- maybe_federate(activity) do
       {:ok, activity}
     else
diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex
index b2461de2b..50f3216f3 100644
--- a/lib/pleroma/web/activity_pub/transmogrifier.ex
+++ b/lib/pleroma/web/activity_pub/transmogrifier.ex
@@ -533,12 +533,12 @@ def handle_incoming(
            User.get_cached_by_ap_id(Containment.get_actor(%{"actor" => followed})),
          {:ok, %User{} = follower} <-
            User.get_or_fetch_by_ap_id(Containment.get_actor(%{"actor" => follower})),
-         {:ok, activity} <- ActivityPub.follow(follower, followed, id, false) do
+         {:ok, activity} <-
+           ActivityPub.follow(follower, followed, id, false, skip_notify_and_stream: true) do
       with deny_follow_blocked <- Pleroma.Config.get([:user, :deny_follow_blocked]),
            {_, false} <- {:user_blocked, User.blocks?(followed, follower) && deny_follow_blocked},
            {_, false} <- {:user_locked, User.locked?(followed)},
            {_, {:ok, follower}} <- {:follow, User.follow(follower, followed)},
-           _ <- Notification.update_notification_type(followed, activity),
            {_, {:ok, _}} <-
              {:follow_state_update, Utils.update_follow_state_for_all(activity, "accept")},
            {:ok, _relationship} <-
@@ -577,6 +577,7 @@ def handle_incoming(
           :noop
       end
 
+      ActivityPub.notify_and_stream(activity)
       {:ok, activity}
     else
       _e ->
diff --git a/test/web/activity_pub/transmogrifier/follow_handling_test.exs b/test/web/activity_pub/transmogrifier/follow_handling_test.exs
index 6b003b51c..06c39eed6 100644
--- a/test/web/activity_pub/transmogrifier/follow_handling_test.exs
+++ b/test/web/activity_pub/transmogrifier/follow_handling_test.exs
@@ -13,6 +13,7 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier.FollowHandlingTest do
 
   import Pleroma.Factory
   import Ecto.Query
+  import Mock
 
   setup_all do
     Tesla.Mock.mock_global(fn env -> apply(HttpRequestMock, :request, [env]) end)
@@ -151,6 +152,23 @@ test "it rejects incoming follow requests from blocked users when deny_follow_bl
       assert activity.data["state"] == "reject"
     end
 
+    test "it rejects incoming follow requests if the following errors for some reason" do
+      user = insert(:user)
+
+      data =
+        File.read!("test/fixtures/mastodon-follow-activity.json")
+        |> Poison.decode!()
+        |> Map.put("object", user.ap_id)
+
+      with_mock Pleroma.User, [:passthrough], follow: fn _, _ -> {:error, :testing} end do
+        {:ok, %Activity{data: %{"id" => id}}} = Transmogrifier.handle_incoming(data)
+
+        %Activity{} = activity = Activity.get_by_ap_id(id)
+
+        assert activity.data["state"] == "reject"
+      end
+    end
+
     test "it works for incoming follow requests from hubzilla" do
       user = insert(:user)