From 1f3eb0f470c41aa0c9757756e28b8ae9ef1a2366 Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Fri, 25 May 2018 03:14:49 +0000 Subject: [PATCH 01/14] testsuite: fix module name for CommonAPI.Test (was duplicated with CommonAPI.UtilsTest) --- test/web/common_api/common_api_test.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/web/common_api/common_api_test.exs b/test/web/common_api/common_api_test.exs index b597e6e0a..a5da271b3 100644 --- a/test/web/common_api/common_api_test.exs +++ b/test/web/common_api/common_api_test.exs @@ -1,4 +1,4 @@ -defmodule Pleroma.Web.CommonAPI.UtilsTest do +defmodule Pleroma.Web.CommonAPI.Test do use Pleroma.DataCase alias Pleroma.Web.CommonAPI From c0ca9f82b991d89524a8f0f770f4b7b08da59e2f Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Fri, 25 May 2018 04:15:42 +0000 Subject: [PATCH 02/14] mastodon api: properly track if an account is locked or not --- lib/pleroma/user.ex | 3 ++- lib/pleroma/web/activity_pub/activity_pub.ex | 4 +++- lib/pleroma/web/activity_pub/views/user_view.ex | 2 +- lib/pleroma/web/mastodon_api/views/account_view.ex | 2 +- 4 files changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index 690cc7cf3..2e57f2b43 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -67,7 +67,8 @@ defmodule Pleroma.User do %{ following_count: length(user.following) - oneself, note_count: user.info["note_count"] || 0, - follower_count: user.info["follower_count"] || 0 + follower_count: user.info["follower_count"] || 0, + locked: user.info["locked"] || false } end diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index 8485a8009..30211072b 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -464,6 +464,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do "url" => [%{"href" => data["image"]["url"]}] } + locked = data["manuallyApprovesFollowers"] || false data = Transmogrifier.maybe_fix_user_object(data) user_data = %{ @@ -471,7 +472,8 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do info: %{ "ap_enabled" => true, "source_data" => data, - "banner" => banner + "banner" => banner, + "locked" => locked }, avatar: avatar, nickname: "#{data["preferredUsername"]}@#{URI.parse(data["id"]).host}", diff --git a/lib/pleroma/web/activity_pub/views/user_view.ex b/lib/pleroma/web/activity_pub/views/user_view.ex index ffd76b529..f4b2e0610 100644 --- a/lib/pleroma/web/activity_pub/views/user_view.ex +++ b/lib/pleroma/web/activity_pub/views/user_view.ex @@ -26,7 +26,7 @@ defmodule Pleroma.Web.ActivityPub.UserView do "name" => user.name, "summary" => user.bio, "url" => user.ap_id, - "manuallyApprovesFollowers" => false, + "manuallyApprovesFollowers" => user.info["locked"] || false, "publicKey" => %{ "id" => "#{user.ap_id}#main-key", "owner" => user.ap_id, diff --git a/lib/pleroma/web/mastodon_api/views/account_view.ex b/lib/pleroma/web/mastodon_api/views/account_view.ex index f378bb36e..9db683f44 100644 --- a/lib/pleroma/web/mastodon_api/views/account_view.ex +++ b/lib/pleroma/web/mastodon_api/views/account_view.ex @@ -19,7 +19,7 @@ defmodule Pleroma.Web.MastodonAPI.AccountView do username: hd(String.split(user.nickname, "@")), acct: user.nickname, display_name: user.name || user.nickname, - locked: false, + locked: user_info.locked, created_at: Utils.to_masto_date(user.inserted_at), followers_count: user_info.follower_count, following_count: user_info.following_count, From 502ba33d01bc73cc40fc6734c086fa4b58a76634 Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Fri, 25 May 2018 06:09:01 +0000 Subject: [PATCH 03/14] activitypub: fix up accept/reject semantics for following fixes #175 --- .../web/activity_pub/transmogrifier.ex | 43 +++++++++++++++++++ .../mastodon_api/mastodon_api_controller.ex | 2 - 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index 803445011..eaa716cea 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -7,6 +7,7 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do alias Pleroma.Activity alias Pleroma.Repo alias Pleroma.Web.ActivityPub.ActivityPub + alias Pleroma.Web.ActivityPub.Utils import Ecto.Query @@ -145,6 +146,48 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do end end + defp get_follow_activity(follow_object) do + cond do + is_map(follow_object) -> + {:ok, follow_object} + + is_binary(follow_object) -> + object = get_obj_helper(follow_object) || ActivityPub.fetch_object_from_id(follow_object) + if object do + {:ok, object} + else + {:error, nil} + end + + true -> + {:error, nil} + end + end + + def handle_incoming( + %{"type" => "Accept", "object" => follow_object, "actor" => actor, "id" => id} = data + ) do + with %User{} = followed <- User.get_or_fetch_by_ap_id(actor), + {:ok, follow_activity} <- get_follow_activity(follow_object), + %User{local: true} = follower <- User.get_cached_by_ap_id(follow_activity["actor"]) do + User.follow(follower, followed) + + {:ok, data} + end + end + + def handle_incoming( + %{"type" => "Reject", "object" => follow_object, "actor" => actor, "id" => id} = data + ) do + with %User{} = followed <- User.get_or_fetch_by_ap_id(actor), + {:ok, follow_activity} <- get_follow_activity(follow_object), + %User{local: true} = follower <- User.get_cached_by_ap_id(follow_activity["actor"]), + {:ok, follow_activity} <- Utils.fetch_latest_follow(follower, followed), + {:ok, activity} <- ActivityPub.delete(follow_activity, false) do + {:ok, activity} + end + end + def handle_incoming( %{"type" => "Like", "object" => object_id, "actor" => actor, "id" => id} = _data ) do diff --git a/lib/pleroma/web/mastodon_api/mastodon_api_controller.ex b/lib/pleroma/web/mastodon_api/mastodon_api_controller.ex index 460942f1a..d50d2d9b5 100644 --- a/lib/pleroma/web/mastodon_api/mastodon_api_controller.ex +++ b/lib/pleroma/web/mastodon_api/mastodon_api_controller.ex @@ -429,7 +429,6 @@ defmodule Pleroma.Web.MastodonAPI.MastodonAPIController do def follow(%{assigns: %{user: follower}} = conn, %{"id" => id}) do with %User{} = followed <- Repo.get(User, id), - {:ok, follower} <- User.follow(follower, followed), {:ok, _activity} <- ActivityPub.follow(follower, followed) do render(conn, AccountView, "relationship.json", %{user: follower, target: followed}) else @@ -442,7 +441,6 @@ defmodule Pleroma.Web.MastodonAPI.MastodonAPIController do def follow(%{assigns: %{user: follower}} = conn, %{"uri" => uri}) do with %User{} = followed <- Repo.get_by(User, nickname: uri), - {:ok, follower} <- User.follow(follower, followed), {:ok, _activity} <- ActivityPub.follow(follower, followed) do render(conn, AccountView, "account.json", %{user: followed}) else From 62c95e8d4d1d5ee2161eaee34523509af78a555f Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Fri, 25 May 2018 08:03:34 +0000 Subject: [PATCH 04/14] run mix format --- lib/pleroma/web/activity_pub/transmogrifier.ex | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index eaa716cea..525b74135 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -153,6 +153,7 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do is_binary(follow_object) -> object = get_obj_helper(follow_object) || ActivityPub.fetch_object_from_id(follow_object) + if object do {:ok, object} else From c89b90222c50dcb6f08e8987709dd2bac961f1cb Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Fri, 25 May 2018 08:35:38 +0000 Subject: [PATCH 05/14] twitter api: also remove explicit User.follow here --- lib/pleroma/web/twitter_api/twitter_api.ex | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/pleroma/web/twitter_api/twitter_api.ex b/lib/pleroma/web/twitter_api/twitter_api.ex index 3ccdaed6f..903c99a8e 100644 --- a/lib/pleroma/web/twitter_api/twitter_api.ex +++ b/lib/pleroma/web/twitter_api/twitter_api.ex @@ -25,7 +25,6 @@ defmodule Pleroma.Web.TwitterAPI.TwitterAPI do def follow(%User{} = follower, params) do with {:ok, %User{} = followed} <- get_user(params), - {:ok, follower} <- User.follow(follower, followed), {:ok, activity} <- ActivityPub.follow(follower, followed) do {:ok, follower, followed, activity} else From e80d91c64a53a9d64e48a528f40987c5b36f2ca5 Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Fri, 25 May 2018 09:31:42 +0000 Subject: [PATCH 06/14] introduce User.maybe_direct_follow() and use it where we used to call User.follow() --- lib/pleroma/user.ex | 29 +++++++++++++++++++ .../mastodon_api/mastodon_api_controller.ex | 2 ++ lib/pleroma/web/twitter_api/twitter_api.ex | 1 + 3 files changed, 32 insertions(+) diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index 2e57f2b43..e4fb57308 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -168,6 +168,35 @@ defmodule Pleroma.User do end end + def maybe_direct_follow(%User{} = follower, %User{info: info} = followed) do + user_info = user_info(followed) + + should_direct_follow = + cond do + # if the account is locked, don't pre-create the relationship + user_info.locked == true -> + false + + # if the users are blocking each other, we shouldn't even be here, but check for it anyway + User.blocks?(follower, followed) == true or User.blocks?(followed, follower) == true -> + false + + # if OStatus, then there is no three-way handshake to follow + User.ap_enabled?(followed) != true -> + true + + # if there are no other reasons not to, just pre-create the relationship + true -> + true + end + + if should_direct_follow do + follow(follower, followed) + else + follower + end + end + def follow(%User{} = follower, %User{info: info} = followed) do ap_followers = followed.follower_address diff --git a/lib/pleroma/web/mastodon_api/mastodon_api_controller.ex b/lib/pleroma/web/mastodon_api/mastodon_api_controller.ex index d50d2d9b5..e12d3fb5b 100644 --- a/lib/pleroma/web/mastodon_api/mastodon_api_controller.ex +++ b/lib/pleroma/web/mastodon_api/mastodon_api_controller.ex @@ -429,6 +429,7 @@ defmodule Pleroma.Web.MastodonAPI.MastodonAPIController do def follow(%{assigns: %{user: follower}} = conn, %{"id" => id}) do with %User{} = followed <- Repo.get(User, id), + {:ok, follower} <- User.maybe_direct_follow(follower, followed), {:ok, _activity} <- ActivityPub.follow(follower, followed) do render(conn, AccountView, "relationship.json", %{user: follower, target: followed}) else @@ -441,6 +442,7 @@ defmodule Pleroma.Web.MastodonAPI.MastodonAPIController do def follow(%{assigns: %{user: follower}} = conn, %{"uri" => uri}) do with %User{} = followed <- Repo.get_by(User, nickname: uri), + {:ok, follower} <- User.maybe_direct_follow(follower, followed), {:ok, _activity} <- ActivityPub.follow(follower, followed) do render(conn, AccountView, "account.json", %{user: followed}) else diff --git a/lib/pleroma/web/twitter_api/twitter_api.ex b/lib/pleroma/web/twitter_api/twitter_api.ex index 903c99a8e..331efa90b 100644 --- a/lib/pleroma/web/twitter_api/twitter_api.ex +++ b/lib/pleroma/web/twitter_api/twitter_api.ex @@ -25,6 +25,7 @@ defmodule Pleroma.Web.TwitterAPI.TwitterAPI do def follow(%User{} = follower, params) do with {:ok, %User{} = followed} <- get_user(params), + {:ok, follower} <- User.maybe_direct_follow(follower, followed), {:ok, activity} <- ActivityPub.follow(follower, followed) do {:ok, follower, followed, activity} else From f35e6bf75bf81496a5c7192780d9db62ba6b42a9 Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Fri, 25 May 2018 09:38:07 +0000 Subject: [PATCH 07/14] activitypub transmogrifier: clean up accept/reject handling a bit --- lib/pleroma/web/activity_pub/transmogrifier.ex | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index 525b74135..519548788 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -171,7 +171,9 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do with %User{} = followed <- User.get_or_fetch_by_ap_id(actor), {:ok, follow_activity} <- get_follow_activity(follow_object), %User{local: true} = follower <- User.get_cached_by_ap_id(follow_activity["actor"]) do - User.follow(follower, followed) + if not User.following?(follower, followed) do + User.follow(follower, followed) + end {:ok, data} end @@ -182,10 +184,10 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do ) do with %User{} = followed <- User.get_or_fetch_by_ap_id(actor), {:ok, follow_activity} <- get_follow_activity(follow_object), - %User{local: true} = follower <- User.get_cached_by_ap_id(follow_activity["actor"]), - {:ok, follow_activity} <- Utils.fetch_latest_follow(follower, followed), - {:ok, activity} <- ActivityPub.delete(follow_activity, false) do - {:ok, activity} + %User{local: true} = follower <- User.get_cached_by_ap_id(follow_activity["actor"]) do + User.unfollow(follower, followed) + + {:ok, data} end end From 7cf3cf77cfffd1e1f6187d7a3485bf2a5d18ca00 Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Fri, 25 May 2018 12:51:04 +0000 Subject: [PATCH 08/14] activitypub transmogrifier: cleanups and tests for incoming accepts/rejects --- .../web/activity_pub/transmogrifier.ex | 16 +-- test/fixtures/mastodon-reject-activity.json | 34 ++++++ test/web/activity_pub/transmogrifier_test.exs | 109 ++++++++++++++++++ 3 files changed, 152 insertions(+), 7 deletions(-) create mode 100644 test/fixtures/mastodon-reject-activity.json diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index 519548788..41198d4e6 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -152,10 +152,10 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do {:ok, follow_object} is_binary(follow_object) -> - object = get_obj_helper(follow_object) || ActivityPub.fetch_object_from_id(follow_object) + object = Activity.get_by_ap_id(follow_object) if object do - {:ok, object} + {:ok, object.data} else {:error, nil} end @@ -170,12 +170,13 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do ) do with %User{} = followed <- User.get_or_fetch_by_ap_id(actor), {:ok, follow_activity} <- get_follow_activity(follow_object), - %User{local: true} = follower <- User.get_cached_by_ap_id(follow_activity["actor"]) do + %User{local: true} = follower <- User.get_cached_by_ap_id(follow_activity["actor"]), + {:ok, activity} <- ActivityPub.insert(data, true) do if not User.following?(follower, followed) do - User.follow(follower, followed) + {:ok, follower} = User.follow(follower, followed) end - {:ok, data} + {:ok, activity} end end @@ -184,10 +185,11 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do ) do with %User{} = followed <- User.get_or_fetch_by_ap_id(actor), {:ok, follow_activity} <- get_follow_activity(follow_object), - %User{local: true} = follower <- User.get_cached_by_ap_id(follow_activity["actor"]) do + %User{local: true} = follower <- User.get_cached_by_ap_id(follow_activity["actor"]), + {:ok, activity} <- ActivityPub.insert(data, true) do User.unfollow(follower, followed) - {:ok, data} + {:ok, activity} end end diff --git a/test/fixtures/mastodon-reject-activity.json b/test/fixtures/mastodon-reject-activity.json new file mode 100644 index 000000000..9559d6c73 --- /dev/null +++ b/test/fixtures/mastodon-reject-activity.json @@ -0,0 +1,34 @@ +{ + "type": "Reject", + "signature": { + "type": "RsaSignature2017", + "signatureValue": "rBzK4Kqhd4g7HDS8WE5oRbWQb2R+HF/6awbUuMWhgru/xCODT0SJWSri0qWqEO4fPcpoUyz2d25cw6o+iy9wiozQb3hQNnu69AR+H5Mytc06+g10KCHexbGhbAEAw/7IzmeXELHUbaqeduaDIbdt1zw4RkwLXdqgQcGXTJ6ND1wM3WMHXQCK1m0flasIXFoBxpliPAGiElV8s0+Ltuh562GvflG3kB3WO+j+NaR0ZfG5G9N88xMj9UQlCKit5gpAE5p6syUsCU2WGBHywTumv73i3OVTIFfq+P9AdMsRuzw1r7zoKEsthW4aOzLQDi01ZjvdBz8zH6JnjDU7SMN/Ig==", + "creator": "http://mastodon.example.org/users/admin#main-key", + "created": "2018-02-17T14:36:41Z" + }, + "object": { + "type": "Follow", + "object": "http://mastodon.example.org/users/admin", + "id": "http://localtesting.pleroma.lol/users/lain#follows/4", + "actor": "http://localtesting.pleroma.lol/users/lain" + }, + "nickname": "lain", + "id": "http://mastodon.example.org/users/admin#rejects/follows/4", + "actor": "http://mastodon.example.org/users/admin", + "@context": [ + "https://www.w3.org/ns/activitystreams", + "https://w3id.org/security/v1", + { + "toot": "http://joinmastodon.org/ns#", + "sensitive": "as:sensitive", + "ostatus": "http://ostatus.org#", + "movedTo": "as:movedTo", + "manuallyApprovesFollowers": "as:manuallyApprovesFollowers", + "inReplyToAtomUri": "ostatus:inReplyToAtomUri", + "conversation": "ostatus:conversation", + "atomUri": "ostatus:atomUri", + "Hashtag": "as:Hashtag", + "Emoji": "toot:Emoji" + } + ] +} diff --git a/test/web/activity_pub/transmogrifier_test.exs b/test/web/activity_pub/transmogrifier_test.exs index cf6b1d0b5..b51e02b08 100644 --- a/test/web/activity_pub/transmogrifier_test.exs +++ b/test/web/activity_pub/transmogrifier_test.exs @@ -2,6 +2,7 @@ defmodule Pleroma.Web.ActivityPub.TransmogrifierTest do use Pleroma.DataCase alias Pleroma.Web.ActivityPub.Transmogrifier alias Pleroma.Web.ActivityPub.Utils + alias Pleroma.Web.ActivityPub.ActivityPub alias Pleroma.Web.OStatus alias Pleroma.Activity alias Pleroma.User @@ -385,6 +386,114 @@ defmodule Pleroma.Web.ActivityPub.TransmogrifierTest do refute User.blocks?(blocker, user) end + + test "it works for incoming accepts which were pre-accepted" do + follower = insert(:user) + followed = insert(:user) + + {:ok, follower} = User.follow(follower, followed) + assert User.following?(follower, followed) == true + + accept_data = + File.read!("test/fixtures/mastodon-accept-activity.json") + |> Poison.decode!() + |> Map.put("actor", followed.ap_id) + + accept_data = + Map.put(accept_data, "object", Map.put(accept_data["object"], "actor", follower.ap_id)) + + {:ok, %Activity{data: _}} = Transmogrifier.handle_incoming(accept_data) + + follower = Repo.get(User, follower.id) + + assert User.following?(follower, followed) == true + end + + test "it works for incoming accepts which were orphaned" do + follower = insert(:user) + followed = insert(:user, %{info: %{"locked" => true}}) + + {:ok, follow_activity} = ActivityPub.follow(follower, followed) + + accept_data = + File.read!("test/fixtures/mastodon-accept-activity.json") + |> Poison.decode!() + |> Map.put("actor", followed.ap_id) + + accept_data = + Map.put(accept_data, "object", Map.put(accept_data["object"], "actor", follower.ap_id)) + + {:ok, %Activity{data: _}} = Transmogrifier.handle_incoming(accept_data) + + follower = Repo.get(User, follower.id) + + assert User.following?(follower, followed) == true + end + + test "it works for incoming accepts which are referenced by IRI only" do + follower = insert(:user) + followed = insert(:user, %{info: %{"locked" => true}}) + + {:ok, follow_activity} = ActivityPub.follow(follower, followed) + + accept_data = + File.read!("test/fixtures/mastodon-accept-activity.json") + |> Poison.decode!() + |> Map.put("actor", followed.ap_id) + |> Map.put("object", follow_activity.data["id"]) + + {:ok, %Activity{data: _}} = Transmogrifier.handle_incoming(accept_data) + + follower = Repo.get(User, follower.id) + + assert User.following?(follower, followed) == true + end + + test "it works for incoming rejects which are orphaned" do + follower = insert(:user) + followed = insert(:user, %{info: %{"locked" => true}}) + + {:ok, follower} = User.follow(follower, followed) + {:ok, follow_activity} = ActivityPub.follow(follower, followed) + + assert User.following?(follower, followed) == true + + reject_data = + File.read!("test/fixtures/mastodon-reject-activity.json") + |> Poison.decode!() + |> Map.put("actor", followed.ap_id) + + reject_data = + Map.put(reject_data, "object", Map.put(reject_data["object"], "actor", follower.ap_id)) + + {:ok, %Activity{data: _}} = Transmogrifier.handle_incoming(reject_data) + + follower = Repo.get(User, follower.id) + + assert User.following?(follower, followed) == false + end + + test "it works for incoming rejects which are referenced by IRI only" do + follower = insert(:user) + followed = insert(:user, %{info: %{"locked" => true}}) + + {:ok, follower} = User.follow(follower, followed) + {:ok, follow_activity} = ActivityPub.follow(follower, followed) + + assert User.following?(follower, followed) == true + + reject_data = + File.read!("test/fixtures/mastodon-reject-activity.json") + |> Poison.decode!() + |> Map.put("actor", followed.ap_id) + |> Map.put("object", follow_activity.data["id"]) + + {:ok, %Activity{data: _}} = Transmogrifier.handle_incoming(reject_data) + + follower = Repo.get(User, follower.id) + + assert User.following?(follower, followed) == false + end end describe "prepare outgoing" do From 7e873756e7e2c669f9dc460b7d6356fb7d25b9dd Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Sat, 26 May 2018 11:07:04 +0000 Subject: [PATCH 09/14] activitypub transmogrifier: use fetch_latest_follow to verify a follow object exists --- lib/pleroma/web/activity_pub/transmogrifier.ex | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index 41198d4e6..ff83dfd36 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -171,12 +171,16 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do with %User{} = followed <- User.get_or_fetch_by_ap_id(actor), {:ok, follow_activity} <- get_follow_activity(follow_object), %User{local: true} = follower <- User.get_cached_by_ap_id(follow_activity["actor"]), + follow_activity <- Utils.fetch_latest_follow(follower, followed), + false <- is_nil(follow_activity), {:ok, activity} <- ActivityPub.insert(data, true) do if not User.following?(follower, followed) do {:ok, follower} = User.follow(follower, followed) end {:ok, activity} + else + _e -> :error end end @@ -186,10 +190,14 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do with %User{} = followed <- User.get_or_fetch_by_ap_id(actor), {:ok, follow_activity} <- get_follow_activity(follow_object), %User{local: true} = follower <- User.get_cached_by_ap_id(follow_activity["actor"]), + follow_activity <- Utils.fetch_latest_follow(follower, followed), + false <- is_nil(follow_activity), {:ok, activity} <- ActivityPub.insert(data, true) do User.unfollow(follower, followed) {:ok, activity} + else + _e -> :error end end From 1db0dc30728a64cf5a9905bcc5df5af9ff3c677b Mon Sep 17 00:00:00 2001 From: William Pitcock Date: Sat, 26 May 2018 11:16:05 +0000 Subject: [PATCH 10/14] tests: add tests to verify the accept request is discarded if no follow activity could be found --- test/web/activity_pub/transmogrifier_test.exs | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test/web/activity_pub/transmogrifier_test.exs b/test/web/activity_pub/transmogrifier_test.exs index b51e02b08..e4cff898d 100644 --- a/test/web/activity_pub/transmogrifier_test.exs +++ b/test/web/activity_pub/transmogrifier_test.exs @@ -394,6 +394,8 @@ defmodule Pleroma.Web.ActivityPub.TransmogrifierTest do {:ok, follower} = User.follow(follower, followed) assert User.following?(follower, followed) == true + {:ok, follow_activity} = ActivityPub.follow(follower, followed) + accept_data = File.read!("test/fixtures/mastodon-accept-activity.json") |> Poison.decode!() @@ -449,6 +451,25 @@ defmodule Pleroma.Web.ActivityPub.TransmogrifierTest do assert User.following?(follower, followed) == true end + test "it fails for incoming accepts which cannot be correlated" do + follower = insert(:user) + followed = insert(:user, %{info: %{"locked" => true}}) + + accept_data = + File.read!("test/fixtures/mastodon-accept-activity.json") + |> Poison.decode!() + |> Map.put("actor", followed.ap_id) + + accept_data = + Map.put(accept_data, "object", Map.put(accept_data["object"], "actor", follower.ap_id)) + + :error = Transmogrifier.handle_incoming(accept_data) + + follower = Repo.get(User, follower.id) + + refute User.following?(follower, followed) == true + end + test "it works for incoming rejects which are orphaned" do follower = insert(:user) followed = insert(:user, %{info: %{"locked" => true}}) From dd9bb3789302f1f8e0e6cc61623b37251ff4ad4c Mon Sep 17 00:00:00 2001 From: lain Date: Sat, 26 May 2018 13:52:05 +0200 Subject: [PATCH 11/14] Rename id helper method. --- lib/pleroma/plugs/http_signature.ex | 2 +- lib/pleroma/web/activity_pub/transmogrifier.ex | 9 +-------- lib/pleroma/web/activity_pub/utils.ex | 13 +++++-------- lib/pleroma/web/http_signatures/http_signatures.ex | 4 ++-- 4 files changed, 9 insertions(+), 19 deletions(-) diff --git a/lib/pleroma/plugs/http_signature.ex b/lib/pleroma/plugs/http_signature.ex index 2d0e10cad..38bcd3a78 100644 --- a/lib/pleroma/plugs/http_signature.ex +++ b/lib/pleroma/plugs/http_signature.ex @@ -13,7 +13,7 @@ defmodule Pleroma.Web.Plugs.HTTPSignaturePlug do end def call(conn, _opts) do - user = Utils.normalize_actor(conn.params["actor"]) + user = Utils.get_ap_id(conn.params["actor"]) Logger.debug("Checking sig for #{user}") [signature | _] = get_req_header(conn, "signature") diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index ff83dfd36..690ca62ec 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -263,11 +263,7 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do def handle_incoming( %{"type" => "Delete", "object" => object_id, "actor" => actor, "id" => _id} = _data ) do - object_id = - case object_id do - %{"id" => id} -> id - id -> id - end + object_id = Utils.get_ap_id(object_id) with %User{} = _actor <- User.get_or_fetch_by_ap_id(actor), {:ok, object} <- @@ -365,9 +361,6 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do end end - # TODO - # Accept - def handle_incoming(_), do: :error def get_obj_helper(id) do diff --git a/lib/pleroma/web/activity_pub/utils.ex b/lib/pleroma/web/activity_pub/utils.ex index 831e13b7e..7362a3ccf 100644 --- a/lib/pleroma/web/activity_pub/utils.ex +++ b/lib/pleroma/web/activity_pub/utils.ex @@ -7,18 +7,15 @@ defmodule Pleroma.Web.ActivityPub.Utils do # Some implementations send the actor URI as the actor field, others send the entire actor object, # so figure out what the actor's URI is based on what we have. - def normalize_actor(actor) do - cond do - is_binary(actor) -> - actor - - is_map(actor) -> - actor["id"] + def get_ap_id(object) do + case object do + %{"id" => id} -> id + id -> id end end def normalize_params(params) do - Map.put(params, "actor", normalize_actor(params["actor"])) + Map.put(params, "actor", get_ap_id(params["actor"])) end def make_json_ld_header do diff --git a/lib/pleroma/web/http_signatures/http_signatures.ex b/lib/pleroma/web/http_signatures/http_signatures.ex index 4e0adbc1d..5e42a871b 100644 --- a/lib/pleroma/web/http_signatures/http_signatures.ex +++ b/lib/pleroma/web/http_signatures/http_signatures.ex @@ -32,14 +32,14 @@ defmodule Pleroma.Web.HTTPSignatures do def validate_conn(conn) do # TODO: How to get the right key and see if it is actually valid for that request. # For now, fetch the key for the actor. - with actor_id <- Utils.normalize_actor(conn.params["actor"]), + with actor_id <- Utils.get_ap_id(conn.params["actor"]), {:ok, public_key} <- User.get_public_key_for_ap_id(actor_id) do if validate_conn(conn, public_key) do true else Logger.debug("Could not validate, re-fetching user and trying one more time") # Fetch user anew and try one more time - with actor_id <- Utils.normalize_actor(conn.params["actor"]), + with actor_id <- Utils.get_ap_id(conn.params["actor"]), {:ok, _user} <- ActivityPub.make_user_from_ap_id(actor_id), {:ok, public_key} <- User.get_public_key_for_ap_id(actor_id) do validate_conn(conn, public_key) From 3839a11ef51a7602bd4c0b5c5d1318bb9cedd213 Mon Sep 17 00:00:00 2001 From: lain Date: Sat, 26 May 2018 14:07:46 +0200 Subject: [PATCH 12/14] Don't treat remote accepts/rejects as local. Also, use specialized functions to get safe data. --- lib/pleroma/web/activity_pub/activity_pub.ex | 11 +++++++ .../web/activity_pub/transmogrifier.ex | 4 +-- test/web/activity_pub/transmogrifier_test.exs | 33 ++++++++++++++++--- 3 files changed, 42 insertions(+), 6 deletions(-) diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index 30211072b..1a1bfbffd 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -95,6 +95,17 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do end end + def reject(%{to: to, actor: actor, object: object} = params) do + # only accept false as false value + local = !(params[:local] == false) + + with data <- %{"to" => to, "type" => "Reject", "actor" => actor, "object" => object}, + {:ok, activity} <- insert(data, local), + :ok <- maybe_federate(activity) do + {:ok, activity} + end + end + def update(%{to: to, cc: cc, actor: actor, object: object} = params) do # only accept false as false value local = !(params[:local] == false) diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index 690ca62ec..b2224514c 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -173,7 +173,7 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do %User{local: true} = follower <- User.get_cached_by_ap_id(follow_activity["actor"]), follow_activity <- Utils.fetch_latest_follow(follower, followed), false <- is_nil(follow_activity), - {:ok, activity} <- ActivityPub.insert(data, true) do + {:ok, activity} <- ActivityPub.accept(%{to: follow_activity.data["to"], type: "Accept", actor: followed.ap_id, object: follow_activity.data["id"], local: false}) do if not User.following?(follower, followed) do {:ok, follower} = User.follow(follower, followed) end @@ -192,7 +192,7 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do %User{local: true} = follower <- User.get_cached_by_ap_id(follow_activity["actor"]), follow_activity <- Utils.fetch_latest_follow(follower, followed), false <- is_nil(follow_activity), - {:ok, activity} <- ActivityPub.insert(data, true) do + {:ok, activity} <- ActivityPub.accept(%{to: follow_activity.data["to"], type: "Accept", actor: followed.ap_id, object: follow_activity.data["id"], local: false}) do User.unfollow(follower, followed) {:ok, activity} diff --git a/test/web/activity_pub/transmogrifier_test.exs b/test/web/activity_pub/transmogrifier_test.exs index e4cff898d..761d9d992 100644 --- a/test/web/activity_pub/transmogrifier_test.exs +++ b/test/web/activity_pub/transmogrifier_test.exs @@ -404,7 +404,10 @@ defmodule Pleroma.Web.ActivityPub.TransmogrifierTest do accept_data = Map.put(accept_data, "object", Map.put(accept_data["object"], "actor", follower.ap_id)) - {:ok, %Activity{data: _}} = Transmogrifier.handle_incoming(accept_data) + {:ok, activity} = Transmogrifier.handle_incoming(accept_data) + refute activity.local + + assert activity.data["object"] == follow_activity.data["id"] follower = Repo.get(User, follower.id) @@ -425,7 +428,8 @@ defmodule Pleroma.Web.ActivityPub.TransmogrifierTest do accept_data = Map.put(accept_data, "object", Map.put(accept_data["object"], "actor", follower.ap_id)) - {:ok, %Activity{data: _}} = Transmogrifier.handle_incoming(accept_data) + {:ok, activity} = Transmogrifier.handle_incoming(accept_data) + assert activity.data["object"] == follow_activity.data["id"] follower = Repo.get(User, follower.id) @@ -444,7 +448,8 @@ defmodule Pleroma.Web.ActivityPub.TransmogrifierTest do |> Map.put("actor", followed.ap_id) |> Map.put("object", follow_activity.data["id"]) - {:ok, %Activity{data: _}} = Transmogrifier.handle_incoming(accept_data) + {:ok, activity} = Transmogrifier.handle_incoming(accept_data) + assert activity.data["object"] == follow_activity.data["id"] follower = Repo.get(User, follower.id) @@ -470,6 +475,25 @@ defmodule Pleroma.Web.ActivityPub.TransmogrifierTest do refute User.following?(follower, followed) == true end + test "it fails for incoming rejects which cannot be correlated" do + follower = insert(:user) + followed = insert(:user, %{info: %{"locked" => true}}) + + accept_data = + File.read!("test/fixtures/mastodon-reject-activity.json") + |> Poison.decode!() + |> Map.put("actor", followed.ap_id) + + accept_data = + Map.put(accept_data, "object", Map.put(accept_data["object"], "actor", follower.ap_id)) + + :error = Transmogrifier.handle_incoming(accept_data) + + follower = Repo.get(User, follower.id) + + refute User.following?(follower, followed) == true + end + test "it works for incoming rejects which are orphaned" do follower = insert(:user) followed = insert(:user, %{info: %{"locked" => true}}) @@ -487,7 +511,8 @@ defmodule Pleroma.Web.ActivityPub.TransmogrifierTest do reject_data = Map.put(reject_data, "object", Map.put(reject_data["object"], "actor", follower.ap_id)) - {:ok, %Activity{data: _}} = Transmogrifier.handle_incoming(reject_data) + {:ok, activity} = Transmogrifier.handle_incoming(reject_data) + refute activity.local follower = Repo.get(User, follower.id) From bfce29866fea3ec7ab91a6b1f20a845248fa0130 Mon Sep 17 00:00:00 2001 From: lain Date: Sat, 26 May 2018 15:07:21 +0200 Subject: [PATCH 13/14] Make Mastodon follow hack more explicit. --- .../web/activity_pub/transmogrifier.ex | 44 ++++++++++--------- test/web/activity_pub/transmogrifier_test.exs | 12 ++--- 2 files changed, 30 insertions(+), 26 deletions(-) diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index b2224514c..fee0b5859 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -146,21 +146,27 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do end end - defp get_follow_activity(follow_object) do - cond do - is_map(follow_object) -> - {:ok, follow_object} + defp mastodon_follow_hack(%{"id" => id, "actor" => follower_id}, followed) do + with true <- id =~ "follows", + %User{local: true} = follower <- User.get_cached_by_ap_id(follower_id), + %Activity{} = activity <- Utils.fetch_latest_follow(follower, followed) do + {:ok, activity} + else + _ -> {:error, nil} + end + end - is_binary(follow_object) -> - object = Activity.get_by_ap_id(follow_object) + defp mastodon_follow_hack(_), do: {:error, nil} - if object do - {:ok, object.data} - else - {:error, nil} - end - - true -> + defp get_follow_activity(follow_object, followed) do + with object_id when not is_nil(object_id) <- Utils.get_ap_id(follow_object), + {_, %Activity{} = activity} <- {:activity, Activity.get_by_ap_id(object_id)} do + {:ok, activity} + else + # Can't find the activity. This might a Mastodon 2.3 "Accept" + {:activity, nil} -> + mastodon_follow_hack(follow_object, followed) + _ -> {:error, nil} end end @@ -169,10 +175,8 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do %{"type" => "Accept", "object" => follow_object, "actor" => actor, "id" => id} = data ) do with %User{} = followed <- User.get_or_fetch_by_ap_id(actor), - {:ok, follow_activity} <- get_follow_activity(follow_object), - %User{local: true} = follower <- User.get_cached_by_ap_id(follow_activity["actor"]), - follow_activity <- Utils.fetch_latest_follow(follower, followed), - false <- is_nil(follow_activity), + {:ok, follow_activity} <- get_follow_activity(follow_object, followed), + %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.ap_id, object: follow_activity.data["id"], local: false}) do if not User.following?(follower, followed) do {:ok, follower} = User.follow(follower, followed) @@ -188,10 +192,8 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do %{"type" => "Reject", "object" => follow_object, "actor" => actor, "id" => id} = data ) do with %User{} = followed <- User.get_or_fetch_by_ap_id(actor), - {:ok, follow_activity} <- get_follow_activity(follow_object), - %User{local: true} = follower <- User.get_cached_by_ap_id(follow_activity["actor"]), - follow_activity <- Utils.fetch_latest_follow(follower, followed), - false <- is_nil(follow_activity), + {:ok, follow_activity} <- get_follow_activity(follow_object, followed), + %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.ap_id, object: follow_activity.data["id"], local: false}) do User.unfollow(follower, followed) diff --git a/test/web/activity_pub/transmogrifier_test.exs b/test/web/activity_pub/transmogrifier_test.exs index 761d9d992..43395eef1 100644 --- a/test/web/activity_pub/transmogrifier_test.exs +++ b/test/web/activity_pub/transmogrifier_test.exs @@ -8,8 +8,6 @@ defmodule Pleroma.Web.ActivityPub.TransmogrifierTest do alias Pleroma.User alias Pleroma.Repo alias Pleroma.Web.Websub.WebsubClientSubscription - alias Pleroma.Web.Websub.WebsubServerSubscription - import Ecto.Query import Pleroma.Factory alias Pleroma.Web.CommonAPI @@ -284,7 +282,7 @@ defmodule Pleroma.Web.ActivityPub.TransmogrifierTest do |> Map.put("object", object) |> Map.put("actor", activity.data["actor"]) - {:ok, %Activity{data: data, local: false}} = Transmogrifier.handle_incoming(data) + {:ok, %Activity{local: false}} = Transmogrifier.handle_incoming(data) refute Repo.get(Activity, activity.id) end @@ -401,8 +399,12 @@ defmodule Pleroma.Web.ActivityPub.TransmogrifierTest do |> Poison.decode!() |> Map.put("actor", followed.ap_id) + object = accept_data["object"] + |> Map.put("actor", follower.ap_id) + |> Map.put("id", follow_activity.data["id"]) + accept_data = - Map.put(accept_data, "object", Map.put(accept_data["object"], "actor", follower.ap_id)) + Map.put(accept_data, "object", object) {:ok, activity} = Transmogrifier.handle_incoming(accept_data) refute activity.local @@ -499,7 +501,7 @@ defmodule Pleroma.Web.ActivityPub.TransmogrifierTest do followed = insert(:user, %{info: %{"locked" => true}}) {:ok, follower} = User.follow(follower, followed) - {:ok, follow_activity} = ActivityPub.follow(follower, followed) + {:ok, _follow_activity} = ActivityPub.follow(follower, followed) assert User.following?(follower, followed) == true From 0a6c897c9488b26273a5fbb2de1ae1bfa0c96675 Mon Sep 17 00:00:00 2001 From: lain Date: Sat, 26 May 2018 15:11:50 +0200 Subject: [PATCH 14/14] Formatting. --- .../web/activity_pub/transmogrifier.ex | 19 +++++++++++++++++-- test/web/activity_pub/transmogrifier_test.exs | 10 +++++----- .../mastodon_api_controller_test.exs | 2 +- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index fee0b5859..62667daa2 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -166,6 +166,7 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do # Can't find the activity. This might a Mastodon 2.3 "Accept" {:activity, nil} -> mastodon_follow_hack(follow_object, followed) + _ -> {:error, nil} end @@ -177,7 +178,14 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do with %User{} = followed <- User.get_or_fetch_by_ap_id(actor), {:ok, follow_activity} <- get_follow_activity(follow_object, followed), %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.ap_id, object: follow_activity.data["id"], local: false}) do + {:ok, activity} <- + ActivityPub.accept(%{ + to: follow_activity.data["to"], + type: "Accept", + actor: followed.ap_id, + object: follow_activity.data["id"], + local: false + }) do if not User.following?(follower, followed) do {:ok, follower} = User.follow(follower, followed) end @@ -194,7 +202,14 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do with %User{} = followed <- User.get_or_fetch_by_ap_id(actor), {:ok, follow_activity} <- get_follow_activity(follow_object, followed), %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.ap_id, object: follow_activity.data["id"], local: false}) do + {:ok, activity} <- + ActivityPub.accept(%{ + to: follow_activity.data["to"], + type: "Accept", + actor: followed.ap_id, + object: follow_activity.data["id"], + local: false + }) do User.unfollow(follower, followed) {:ok, activity} diff --git a/test/web/activity_pub/transmogrifier_test.exs b/test/web/activity_pub/transmogrifier_test.exs index 43395eef1..384844095 100644 --- a/test/web/activity_pub/transmogrifier_test.exs +++ b/test/web/activity_pub/transmogrifier_test.exs @@ -399,12 +399,12 @@ defmodule Pleroma.Web.ActivityPub.TransmogrifierTest do |> Poison.decode!() |> Map.put("actor", followed.ap_id) - object = accept_data["object"] - |> Map.put("actor", follower.ap_id) - |> Map.put("id", follow_activity.data["id"]) + object = + accept_data["object"] + |> Map.put("actor", follower.ap_id) + |> Map.put("id", follow_activity.data["id"]) - accept_data = - Map.put(accept_data, "object", object) + accept_data = Map.put(accept_data, "object", object) {:ok, activity} = Transmogrifier.handle_incoming(accept_data) refute activity.local diff --git a/test/web/mastodon_api/mastodon_api_controller_test.exs b/test/web/mastodon_api/mastodon_api_controller_test.exs index 0f091b986..936d27182 100644 --- a/test/web/mastodon_api/mastodon_api_controller_test.exs +++ b/test/web/mastodon_api/mastodon_api_controller_test.exs @@ -298,7 +298,7 @@ defmodule Pleroma.Web.MastodonAPI.MastodonAPIControllerTest do test "list timeline", %{conn: conn} do user = insert(:user) other_user = insert(:user) - {:ok, activity_one} = TwitterAPI.create_status(user, %{"status" => "Marisa is cute."}) + {:ok, _activity_one} = TwitterAPI.create_status(user, %{"status" => "Marisa is cute."}) {:ok, activity_two} = TwitterAPI.create_status(other_user, %{"status" => "Marisa is cute."}) {:ok, list} = Pleroma.List.create("name", user) {:ok, list} = Pleroma.List.follow(list, other_user)