From 3115b64cfe220c1db61c71fc4cef51bdf167b9ab Mon Sep 17 00:00:00 2001 From: lain Date: Wed, 5 Jun 2019 14:10:46 +0200 Subject: [PATCH 1/5] Transmogrifier: Add tests for incoming follows to locked accounts. --- lib/pleroma/web/activity_pub/transmogrifier.ex | 7 +++--- .../transmogrifier/follow_handling_test.exs | 29 +++++++++++++++++++++- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index 66fa7c0b3..8aa44ec09 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -458,10 +458,11 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do {:ok, %User{} = follower} <- User.get_or_fetch_by_ap_id(follower), {:ok, activity} <- ActivityPub.follow(follower, followed, id, false) do with deny_follow_blocked <- Pleroma.Config.get([:user, :deny_follow_blocked]), - {:user_blocked, false} <- + {_, false} <- {:user_blocked, User.blocks?(followed, follower) && deny_follow_blocked}, - {:user_locked, false} <- {:user_locked, User.locked?(followed)}, - {:follow, {:ok, follower}} <- {:follow, User.follow(follower, followed)} do + {_, false} <- {:user_locked, User.locked?(followed)}, + {_, {:ok, follower}} <- {:follow, User.follow(follower, followed)}, + {_, {:ok, _}} <- {:follow_state_update, Utils.update_follow_state(activity, "accept")} do ActivityPub.accept(%{ to: [follower.ap_id], actor: followed, diff --git a/test/web/activity_pub/transmogrifier/follow_handling_test.exs b/test/web/activity_pub/transmogrifier/follow_handling_test.exs index 5ddf6cd52..857d65564 100644 --- a/test/web/activity_pub/transmogrifier/follow_handling_test.exs +++ b/test/web/activity_pub/transmogrifier/follow_handling_test.exs @@ -27,14 +27,41 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier.FollowHandlingTest do |> Poison.decode!() |> Map.put("object", user.ap_id) - {:ok, %Activity{data: data, local: false}} = Transmogrifier.handle_incoming(data) + {:ok, %Activity{data: data, local: false} = activity} = Transmogrifier.handle_incoming(data) assert data["actor"] == "http://mastodon.example.org/users/admin" assert data["type"] == "Follow" assert data["id"] == "http://mastodon.example.org/users/admin#follows/2" + + activity = Repo.get(Activity, activity.id) + assert activity.data["state"] == "accept" assert User.following?(User.get_cached_by_ap_id(data["actor"]), user) end + test "with locked accounts, it does not create a follow or an accept" do + user = insert(:user, info: %{locked: true}) + + data = + File.read!("test/fixtures/mastodon-follow-activity.json") + |> Poison.decode!() + |> Map.put("object", user.ap_id) + + {:ok, %Activity{data: data, local: false}} = Transmogrifier.handle_incoming(data) + + assert data["state"] == "pending" + + refute User.following?(User.get_cached_by_ap_id(data["actor"]), user) + + accepts = + from( + a in Activity, + where: fragment("?->>'type' = ?", a.data, "Accept") + ) + |> Repo.all() + + assert length(accepts) == 0 + end + test "it works for follow requests when you are already followed, creating a new accept activity" do # This is important because the remote might have the wrong idea about the # current follow status. This can lead to instance A thinking that x@A is From 076c9ae40e1b944839472d5d337d32335590ab0c Mon Sep 17 00:00:00 2001 From: lain Date: Wed, 5 Jun 2019 14:24:31 +0200 Subject: [PATCH 2/5] User: Remove superfluous `maybe_follow`. --- lib/pleroma/user.ex | 8 -------- lib/pleroma/web/activity_pub/transmogrifier.ex | 21 ++++++++------------- lib/pleroma/web/common_api/common_api.ex | 2 +- test/user_test.exs | 2 +- 4 files changed, 10 insertions(+), 23 deletions(-) diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index d873d7623..3d8fa0edb 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -324,14 +324,6 @@ defmodule Pleroma.User do end end - def maybe_follow(%User{} = follower, %User{info: _info} = followed) do - if not following?(follower, followed) do - follow(follower, followed) - else - {:ok, follower} - end - end - @doc "A mass follow for local users. Respects blocks in both directions but does not create activities." @spec follow_all(User.t(), list(User.t())) :: {atom(), User.t()} def follow_all(follower, followeds) do diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index 8aa44ec09..e8df112b9 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -509,19 +509,14 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do {:ok, follow_activity} <- get_follow_activity(follow_object, followed), {:ok, follow_activity} <- Utils.update_follow_state(follow_activity, "accept"), %User{local: true} = follower <- User.get_cached_by_ap_id(follow_activity.data["actor"]), - {:ok, activity} <- - ActivityPub.accept(%{ - to: follow_activity.data["to"], - type: "Accept", - actor: followed, - object: follow_activity.data["id"], - local: false - }) do - if not User.following?(follower, followed) do - {:ok, _follower} = User.follow(follower, followed) - end - - {:ok, activity} + {:ok, _follower} = User.follow(follower, followed) do + ActivityPub.accept(%{ + to: follow_activity.data["to"], + type: "Accept", + actor: followed, + object: follow_activity.data["id"], + local: false + }) else _e -> :error end diff --git a/lib/pleroma/web/common_api/common_api.ex b/lib/pleroma/web/common_api/common_api.ex index 2b3ae3de5..71e166128 100644 --- a/lib/pleroma/web/common_api/common_api.ex +++ b/lib/pleroma/web/common_api/common_api.ex @@ -35,7 +35,7 @@ defmodule Pleroma.Web.CommonAPI do end def accept_follow_request(follower, followed) do - with {:ok, follower} <- User.maybe_follow(follower, followed), + with {:ok, follower} <- User.follow(follower, followed), %Activity{} = follow_activity <- Utils.fetch_latest_follow(follower, followed), {:ok, follow_activity} <- Utils.update_follow_state(follow_activity, "accept"), {:ok, _activity} <- diff --git a/test/user_test.exs b/test/user_test.exs index d7473ef43..466ddbf74 100644 --- a/test/user_test.exs +++ b/test/user_test.exs @@ -75,7 +75,7 @@ defmodule Pleroma.UserTest do Pleroma.Web.TwitterAPI.TwitterAPI.follow(pending_follower, %{"user_id" => locked.id}) Pleroma.Web.TwitterAPI.TwitterAPI.follow(pending_follower, %{"user_id" => locked.id}) Pleroma.Web.TwitterAPI.TwitterAPI.follow(accepted_follower, %{"user_id" => locked.id}) - User.maybe_follow(accepted_follower, locked) + User.follow(accepted_follower, locked) assert {:ok, [activity]} = User.get_follow_requests(locked) assert activity From 827a51e777a917c0a0f949c95d33192fd60c1b60 Mon Sep 17 00:00:00 2001 From: lain Date: Wed, 5 Jun 2019 15:43:54 +0200 Subject: [PATCH 3/5] CommonAPI: Add test for accept_follow_request. --- test/web/common_api/common_api_test.exs | 43 +++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/test/web/common_api/common_api_test.exs b/test/web/common_api/common_api_test.exs index 26efe6140..7ff23b63d 100644 --- a/test/web/common_api/common_api_test.exs +++ b/test/web/common_api/common_api_test.exs @@ -7,6 +7,7 @@ defmodule Pleroma.Web.CommonAPITest do alias Pleroma.Activity alias Pleroma.Object alias Pleroma.User + alias Pleroma.Web.ActivityPub.ActivityPub alias Pleroma.Web.CommonAPI import Pleroma.Factory @@ -339,4 +340,46 @@ defmodule Pleroma.Web.CommonAPITest do assert User.showing_reblogs?(muter, muted) == true end end + + describe "accept_follow_request/2" do + test "after acceptance, it sets all existing pending follow request states to 'accept'" do + user = insert(:user, info: %{locked: true}) + follower = insert(:user) + follower_two = insert(:user) + + {:ok, follow_activity} = ActivityPub.follow(follower, user) + {:ok, follow_activity_two} = ActivityPub.follow(follower, user) + {:ok, follow_activity_three} = ActivityPub.follow(follower_two, user) + + assert follow_activity.data["state"] == "pending" + assert follow_activity_two.data["state"] == "pending" + assert follow_activity_three.data["state"] == "pending" + + {:ok, _follower} = CommonAPI.accept_follow_request(follower, user) + + assert Repo.get(Activity, follow_activity.id).data["state"] == "accept" + assert Repo.get(Activity, follow_activity_two.id).data["state"] == "accept" + assert Repo.get(Activity, follow_activity_three.id).data["state"] == "pending" + end + + test "after rejection, it sets all existing pending follow request states to 'reject'" do + user = insert(:user, info: %{locked: true}) + follower = insert(:user) + follower_two = insert(:user) + + {:ok, follow_activity} = ActivityPub.follow(follower, user) + {:ok, follow_activity_two} = ActivityPub.follow(follower, user) + {:ok, follow_activity_three} = ActivityPub.follow(follower_two, user) + + assert follow_activity.data["state"] == "pending" + assert follow_activity_two.data["state"] == "pending" + assert follow_activity_three.data["state"] == "pending" + + {:ok, _follower} = CommonAPI.reject_follow_request(follower, user) + + assert Repo.get(Activity, follow_activity.id).data["state"] == "reject" + assert Repo.get(Activity, follow_activity_two.id).data["state"] == "reject" + assert Repo.get(Activity, follow_activity_three.id).data["state"] == "pending" + end + end end From ad19bfc7feecddd5a0f049ece436c1415335efa3 Mon Sep 17 00:00:00 2001 From: lain Date: Wed, 5 Jun 2019 16:43:35 +0200 Subject: [PATCH 4/5] Utils: Split update_follow_state and update_follow_state_for_all. --- lib/pleroma/web/activity_pub/utils.ex | 4 +-- test/web/activity_pub/utils_test.exs | 48 +++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/lib/pleroma/web/activity_pub/utils.ex b/lib/pleroma/web/activity_pub/utils.ex index faae7e747..10ff572a2 100644 --- a/lib/pleroma/web/activity_pub/utils.ex +++ b/lib/pleroma/web/activity_pub/utils.ex @@ -376,8 +376,8 @@ defmodule Pleroma.Web.ActivityPub.Utils do @doc """ Updates a follow activity's state (for locked accounts). """ - def update_follow_state( - %Activity{data: %{"actor" => actor, "object" => object, "state" => "pending"}} = activity, + def update_follow_state_for_all( + %Activity{data: %{"actor" => actor, "object" => object}} = activity, state ) do try do diff --git a/test/web/activity_pub/utils_test.exs b/test/web/activity_pub/utils_test.exs index de741c64b..932d5f5e7 100644 --- a/test/web/activity_pub/utils_test.exs +++ b/test/web/activity_pub/utils_test.exs @@ -2,6 +2,7 @@ defmodule Pleroma.Web.ActivityPub.UtilsTest do use Pleroma.DataCase alias Pleroma.Activity alias Pleroma.Object + alias Pleroma.Repo alias Pleroma.User alias Pleroma.Web.ActivityPub.ActivityPub alias Pleroma.Web.ActivityPub.Utils @@ -247,4 +248,51 @@ defmodule Pleroma.Web.ActivityPub.UtilsTest do assert fetched_vote.id == vote.id end end + + describe "update_follow_state_for_all/2" do + test "updates the state of all Follow activities with the same actor and object" do + user = insert(:user, info: %{locked: true}) + follower = insert(:user) + + {:ok, follow_activity} = ActivityPub.follow(follower, user) + {:ok, follow_activity_two} = ActivityPub.follow(follower, user) + + data = + follow_activity_two.data + |> Map.put("state", "accept") + + cng = Ecto.Changeset.change(follow_activity_two, data: data) + + {:ok, follow_activity_two} = Repo.update(cng) + + {:ok, follow_activity_two} = + Utils.update_follow_state_for_all(follow_activity_two, "accept") + + assert Repo.get(Activity, follow_activity.id).data["state"] == "accept" + assert Repo.get(Activity, follow_activity_two.id).data["state"] == "accept" + end + end + + describe "update_follow_state/2" do + test "updates the state of the given follow activity" do + user = insert(:user, info: %{locked: true}) + follower = insert(:user) + + {:ok, follow_activity} = ActivityPub.follow(follower, user) + {:ok, follow_activity_two} = ActivityPub.follow(follower, user) + + data = + follow_activity_two.data + |> Map.put("state", "accept") + + cng = Ecto.Changeset.change(follow_activity_two, data: data) + + {:ok, follow_activity_two} = Repo.update(cng) + + {:ok, follow_activity_two} = Utils.update_follow_state(follow_activity_two, "reject") + + assert Repo.get(Activity, follow_activity.id).data["state"] == "pending" + assert Repo.get(Activity, follow_activity_two.id).data["state"] == "reject" + end + end end From e1370ba1316734f61d8326200df1cb0789a4686f Mon Sep 17 00:00:00 2001 From: lain Date: Wed, 5 Jun 2019 16:51:28 +0200 Subject: [PATCH 5/5] Utils: Use update_follow_state_for_all when appropriate. --- CHANGELOG.md | 1 + lib/pleroma/web/activity_pub/transmogrifier.ex | 11 ++++++----- lib/pleroma/web/common_api/common_api.ex | 4 ++-- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d1d87238c..376df2e57 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -90,6 +90,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Respond with a 404 Not implemented JSON error message when requested API is not implemented ### Fixed +- Follow requests don't get 'stuck' anymore. - Added an FTS index on objects. Running `vacuum analyze` and setting a larger `work_mem` is recommended. - Followers counter not being updated when a follower is blocked - Deactivated users being able to request an access token diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index e8df112b9..ff031a16e 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -462,7 +462,8 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do {:user_blocked, User.blocks?(followed, follower) && deny_follow_blocked}, {_, false} <- {:user_locked, User.locked?(followed)}, {_, {:ok, follower}} <- {:follow, User.follow(follower, followed)}, - {_, {:ok, _}} <- {:follow_state_update, Utils.update_follow_state(activity, "accept")} do + {_, {:ok, _}} <- + {:follow_state_update, Utils.update_follow_state_for_all(activity, "accept")} do ActivityPub.accept(%{ to: [follower.ap_id], actor: followed, @@ -471,7 +472,7 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do }) else {:user_blocked, true} -> - {:ok, _} = Utils.update_follow_state(activity, "reject") + {:ok, _} = Utils.update_follow_state_for_all(activity, "reject") ActivityPub.reject(%{ to: [follower.ap_id], @@ -481,7 +482,7 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do }) {:follow, {:error, _}} -> - {:ok, _} = Utils.update_follow_state(activity, "reject") + {:ok, _} = Utils.update_follow_state_for_all(activity, "reject") ActivityPub.reject(%{ to: [follower.ap_id], @@ -507,7 +508,7 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do with actor <- Containment.get_actor(data), {:ok, %User{} = followed} <- User.get_or_fetch_by_ap_id(actor), {:ok, follow_activity} <- get_follow_activity(follow_object, followed), - {:ok, follow_activity} <- Utils.update_follow_state(follow_activity, "accept"), + {:ok, follow_activity} <- Utils.update_follow_state_for_all(follow_activity, "accept"), %User{local: true} = follower <- User.get_cached_by_ap_id(follow_activity.data["actor"]), {:ok, _follower} = User.follow(follower, followed) do ActivityPub.accept(%{ @@ -528,7 +529,7 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do with actor <- Containment.get_actor(data), {:ok, %User{} = followed} <- User.get_or_fetch_by_ap_id(actor), {:ok, follow_activity} <- get_follow_activity(follow_object, followed), - {:ok, follow_activity} <- Utils.update_follow_state(follow_activity, "reject"), + {:ok, follow_activity} <- Utils.update_follow_state_for_all(follow_activity, "reject"), %User{local: true} = follower <- User.get_cached_by_ap_id(follow_activity.data["actor"]), {:ok, activity} <- ActivityPub.reject(%{ diff --git a/lib/pleroma/web/common_api/common_api.ex b/lib/pleroma/web/common_api/common_api.ex index 71e166128..f5193512e 100644 --- a/lib/pleroma/web/common_api/common_api.ex +++ b/lib/pleroma/web/common_api/common_api.ex @@ -37,7 +37,7 @@ defmodule Pleroma.Web.CommonAPI do def accept_follow_request(follower, followed) do with {:ok, follower} <- User.follow(follower, followed), %Activity{} = follow_activity <- Utils.fetch_latest_follow(follower, followed), - {:ok, follow_activity} <- Utils.update_follow_state(follow_activity, "accept"), + {:ok, follow_activity} <- Utils.update_follow_state_for_all(follow_activity, "accept"), {:ok, _activity} <- ActivityPub.accept(%{ to: [follower.ap_id], @@ -51,7 +51,7 @@ defmodule Pleroma.Web.CommonAPI do def reject_follow_request(follower, followed) do with %Activity{} = follow_activity <- Utils.fetch_latest_follow(follower, followed), - {:ok, follow_activity} <- Utils.update_follow_state(follow_activity, "reject"), + {:ok, follow_activity} <- Utils.update_follow_state_for_all(follow_activity, "reject"), {:ok, _activity} <- ActivityPub.reject(%{ to: [follower.ap_id],