From 27ed260eed51798a20608b1c062d32bfc7b6cdc4 Mon Sep 17 00:00:00 2001 From: rinpatch Date: Fri, 12 Jul 2019 18:36:07 +0300 Subject: [PATCH 1/4] AP user view: Add a test for hiding totalItems in following/followers --- .../web/activity_pub/views/user_view_test.exs | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/web/activity_pub/views/user_view_test.exs b/test/web/activity_pub/views/user_view_test.exs index 969860c4c..86254117f 100644 --- a/test/web/activity_pub/views/user_view_test.exs +++ b/test/web/activity_pub/views/user_view_test.exs @@ -8,6 +8,7 @@ defmodule Pleroma.Web.ActivityPub.UserViewTest do alias Pleroma.User alias Pleroma.Web.ActivityPub.UserView + alias Pleroma.Web.CommonAPI test "Renders a user, including the public key" do user = insert(:user) @@ -82,4 +83,28 @@ test "instance users do not expose oAuth endpoints" do refute result["endpoints"]["oauthTokenEndpoint"] end end + + describe "followers" do + test "sets totalItems to zero when followers are hidden" do + user = insert(:user) + other_user = insert(:user) + {:ok, _other_user, user, _activity} = CommonAPI.follow(other_user, user) + assert %{"totalItems" => 1} = UserView.render("followers.json", %{user: user}) + info = Map.put(user.info, :hide_followers, true) + user = Map.put(user, :info, info) + assert %{"totalItems" => 0} = UserView.render("followers.json", %{user: user}) + end + end + + describe "following" do + test "sets totalItems to zero when follows are hidden" do + user = insert(:user) + other_user = insert(:user) + {:ok, user, _other_user, _activity} = CommonAPI.follow(user, other_user) + assert %{"totalItems" => 1} = UserView.render("following.json", %{user: user}) + info = Map.put(user.info, :hide_follows, true) + user = Map.put(user, :info, info) + assert %{"totalItems" => 0} = UserView.render("following.json", %{user: user}) + end + end end From 1f6ac7680d1ae07be7c7dfd81a8cec2ba52f1c82 Mon Sep 17 00:00:00 2001 From: rinpatch Date: Fri, 12 Jul 2019 19:41:05 +0300 Subject: [PATCH 2/4] ActivityPub User view: Following/Followers refactoring - Render the collection items if the user requesting == the user rendered - Do not render the first page if hide_{followers,follows} is set, just give the URI to it --- .../web/activity_pub/views/user_view.ex | 39 +++++++++++++------ .../activity_pub_controller_test.exs | 10 ++--- 2 files changed, 31 insertions(+), 18 deletions(-) diff --git a/lib/pleroma/web/activity_pub/views/user_view.ex b/lib/pleroma/web/activity_pub/views/user_view.ex index 327e0e05b..d9c1bcb2c 100644 --- a/lib/pleroma/web/activity_pub/views/user_view.ex +++ b/lib/pleroma/web/activity_pub/views/user_view.ex @@ -98,29 +98,31 @@ def render("user.json", %{user: user}) do |> Map.merge(Utils.make_json_ld_header()) end - def render("following.json", %{user: user, page: page}) do + def render("following.json", %{user: user, page: page} = opts) do + showing = (opts[:for] && opts[:for] == user) || !user.info.hide_follows query = User.get_friends_query(user) query = from(user in query, select: [:ap_id]) following = Repo.all(query) total = - if !user.info.hide_follows do + if showing do length(following) else 0 end - collection(following, "#{user.ap_id}/following", page, !user.info.hide_follows, total) + collection(following, "#{user.ap_id}/following", page, showing, total) |> Map.merge(Utils.make_json_ld_header()) end - def render("following.json", %{user: user}) do + def render("following.json", %{user: user} = opts) do + showing = (opts[:for] && opts[:for] == user) || !user.info.hide_follows query = User.get_friends_query(user) query = from(user in query, select: [:ap_id]) following = Repo.all(query) total = - if !user.info.hide_follows do + if showing do length(following) else 0 @@ -130,34 +132,43 @@ def render("following.json", %{user: user}) do "id" => "#{user.ap_id}/following", "type" => "OrderedCollection", "totalItems" => total, - "first" => collection(following, "#{user.ap_id}/following", 1, !user.info.hide_follows) + "first" => + if showing do + collection(following, "#{user.ap_id}/following", 1, !user.info.hide_follows) + else + "#{user.ap_id}/following?page=1" + end } |> Map.merge(Utils.make_json_ld_header()) end - def render("followers.json", %{user: user, page: page}) do + def render("followers.json", %{user: user, page: page} = opts) do + showing = (opts[:for] && opts[:for] == user) || !user.info.hide_followers + query = User.get_followers_query(user) query = from(user in query, select: [:ap_id]) followers = Repo.all(query) total = - if !user.info.hide_followers do + if showing do length(followers) else 0 end - collection(followers, "#{user.ap_id}/followers", page, !user.info.hide_followers, total) + collection(followers, "#{user.ap_id}/followers", page, showing, total) |> Map.merge(Utils.make_json_ld_header()) end - def render("followers.json", %{user: user}) do + def render("followers.json", %{user: user} = opts) do + showing = (opts[:for] && opts[:for] == user) || !user.info.hide_followers + query = User.get_followers_query(user) query = from(user in query, select: [:ap_id]) followers = Repo.all(query) total = - if !user.info.hide_followers do + if showing do length(followers) else 0 @@ -168,7 +179,11 @@ def render("followers.json", %{user: user}) do "type" => "OrderedCollection", "totalItems" => total, "first" => - collection(followers, "#{user.ap_id}/followers", 1, !user.info.hide_followers, total) + if showing do + collection(followers, "#{user.ap_id}/followers", 1, showing, total) + else + "#{user.ap_id}/followers?page=1" + end } |> Map.merge(Utils.make_json_ld_header()) end diff --git a/test/web/activity_pub/activity_pub_controller_test.exs b/test/web/activity_pub/activity_pub_controller_test.exs index 1f8eb9d71..cbc25bcdb 100644 --- a/test/web/activity_pub/activity_pub_controller_test.exs +++ b/test/web/activity_pub/activity_pub_controller_test.exs @@ -551,7 +551,7 @@ test "it returns the followers in a collection", %{conn: conn} do assert result["first"]["orderedItems"] == [user.ap_id] end - test "it returns returns empty if the user has 'hide_followers' set", %{conn: conn} do + test "it returns returns a uri if the user has 'hide_followers' set", %{conn: conn} do user = insert(:user) user_two = insert(:user, %{info: %{hide_followers: true}}) User.follow(user, user_two) @@ -561,8 +561,7 @@ test "it returns returns empty if the user has 'hide_followers' set", %{conn: co |> get("/users/#{user_two.nickname}/followers") |> json_response(200) - assert result["first"]["orderedItems"] == [] - assert result["totalItems"] == 0 + assert is_binary(result["first"]) end test "it works for more than 10 users", %{conn: conn} do @@ -606,7 +605,7 @@ test "it returns the following in a collection", %{conn: conn} do assert result["first"]["orderedItems"] == [user_two.ap_id] end - test "it returns returns empty if the user has 'hide_follows' set", %{conn: conn} do + test "it returns a uri if the user has 'hide_follows' set", %{conn: conn} do user = insert(:user, %{info: %{hide_follows: true}}) user_two = insert(:user) User.follow(user, user_two) @@ -616,8 +615,7 @@ test "it returns returns empty if the user has 'hide_follows' set", %{conn: conn |> get("/users/#{user.nickname}/following") |> json_response(200) - assert result["first"]["orderedItems"] == [] - assert result["totalItems"] == 0 + assert is_binary(result["first"]) end test "it works for more than 10 users", %{conn: conn} do From 97b79efbcd4ad829a575019f842e7dcd7548266a Mon Sep 17 00:00:00 2001 From: rinpatch Date: Fri, 12 Jul 2019 20:54:20 +0300 Subject: [PATCH 3/4] ActivityPub Controller: Actually pass for_user to following/followers views and give 403 errors when trying to request hidden follower pages when unauthenticated --- .../activity_pub/activity_pub_controller.ex | 51 +++++++++++++---- lib/pleroma/web/router.ex | 8 ++- .../activity_pub_controller_test.exs | 57 +++++++++++++++++++ 3 files changed, 102 insertions(+), 14 deletions(-) diff --git a/lib/pleroma/web/activity_pub/activity_pub_controller.ex b/lib/pleroma/web/activity_pub/activity_pub_controller.ex index cf5176201..e2af4ad1a 100644 --- a/lib/pleroma/web/activity_pub/activity_pub_controller.ex +++ b/lib/pleroma/web/activity_pub/activity_pub_controller.ex @@ -103,43 +103,57 @@ def activity(conn, %{"uuid" => uuid}) do end end - def following(conn, %{"nickname" => nickname, "page" => page}) do + def following(%{assigns: %{user: for_user}} = conn, %{"nickname" => nickname, "page" => page}) do with %User{} = user <- User.get_cached_by_nickname(nickname), - {:ok, user} <- User.ensure_keys_present(user) do + {user, for_user} <- ensure_user_keys_present_and_maybe_refresh_for_user(user, for_user), + {:show_follows, true} <- + {:show_follows, (for_user && for_user == user) || !user.info.hide_follows} do {page, _} = Integer.parse(page) conn |> put_resp_header("content-type", "application/activity+json") - |> json(UserView.render("following.json", %{user: user, page: page})) + |> json(UserView.render("following.json", %{user: user, page: page, for: for_user})) + else + {:show_follows, _} -> + conn + |> put_resp_header("content-type", "application/activity+json") + |> send_resp(403, "") end end - def following(conn, %{"nickname" => nickname}) do + def following(%{assigns: %{user: for_user}} = conn, %{"nickname" => nickname}) do with %User{} = user <- User.get_cached_by_nickname(nickname), - {:ok, user} <- User.ensure_keys_present(user) do + {user, for_user} <- ensure_user_keys_present_and_maybe_refresh_for_user(user, for_user) do conn |> put_resp_header("content-type", "application/activity+json") - |> json(UserView.render("following.json", %{user: user})) + |> json(UserView.render("following.json", %{user: user, for: for_user})) end end - def followers(conn, %{"nickname" => nickname, "page" => page}) do + def followers(%{assigns: %{user: for_user}} = conn, %{"nickname" => nickname, "page" => page}) do with %User{} = user <- User.get_cached_by_nickname(nickname), - {:ok, user} <- User.ensure_keys_present(user) do + {user, for_user} <- ensure_user_keys_present_and_maybe_refresh_for_user(user, for_user), + {:show_followers, true} <- + {:show_followers, (for_user && for_user == user) || !user.info.hide_followers} do {page, _} = Integer.parse(page) conn |> put_resp_header("content-type", "application/activity+json") - |> json(UserView.render("followers.json", %{user: user, page: page})) + |> json(UserView.render("followers.json", %{user: user, page: page, for: for_user})) + else + {:show_followers, _} -> + conn + |> put_resp_header("content-type", "application/activity+json") + |> send_resp(403, "") end end - def followers(conn, %{"nickname" => nickname}) do + def followers(%{assigns: %{user: for_user}} = conn, %{"nickname" => nickname}) do with %User{} = user <- User.get_cached_by_nickname(nickname), - {:ok, user} <- User.ensure_keys_present(user) do + {user, for_user} <- ensure_user_keys_present_and_maybe_refresh_for_user(user, for_user) do conn |> put_resp_header("content-type", "application/activity+json") - |> json(UserView.render("followers.json", %{user: user})) + |> json(UserView.render("followers.json", %{user: user, for: for_user})) end end @@ -325,4 +339,17 @@ defp set_requester_reachable(%Plug.Conn{} = conn, _) do conn end + + defp ensure_user_keys_present_and_maybe_refresh_for_user(user, for_user) do + {:ok, new_user} = User.ensure_keys_present(user) + + for_user = + if new_user != user and match?(%User{}, for_user) do + User.get_cached_by_nickname(for_user.nickname) + else + for_user + end + + {new_user, for_user} + end end diff --git a/lib/pleroma/web/router.ex b/lib/pleroma/web/router.ex index d53fa8a35..e03a3a2e5 100644 --- a/lib/pleroma/web/router.ex +++ b/lib/pleroma/web/router.ex @@ -623,8 +623,6 @@ defmodule Pleroma.Web.Router do # XXX: not really ostatus pipe_through(:ostatus) - get("/users/:nickname/followers", ActivityPubController, :followers) - get("/users/:nickname/following", ActivityPubController, :following) get("/users/:nickname/outbox", ActivityPubController, :outbox) get("/objects/:uuid/likes", ActivityPubController, :object_likes) end @@ -656,6 +654,12 @@ defmodule Pleroma.Web.Router do pipe_through(:oauth_write) post("/users/:nickname/outbox", ActivityPubController, :update_outbox) end + + scope [] do + pipe_through(:oauth_read_or_public) + get("/users/:nickname/followers", ActivityPubController, :followers) + get("/users/:nickname/following", ActivityPubController, :following) + end end scope "/relay", Pleroma.Web.ActivityPub do diff --git a/test/web/activity_pub/activity_pub_controller_test.exs b/test/web/activity_pub/activity_pub_controller_test.exs index cbc25bcdb..452172bb4 100644 --- a/test/web/activity_pub/activity_pub_controller_test.exs +++ b/test/web/activity_pub/activity_pub_controller_test.exs @@ -12,6 +12,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPubControllerTest do alias Pleroma.Web.ActivityPub.ObjectView alias Pleroma.Web.ActivityPub.UserView alias Pleroma.Web.ActivityPub.Utils + alias Pleroma.Web.CommonAPI setup_all do Tesla.Mock.mock_global(fn env -> apply(HttpRequestMock, :request, [env]) end) @@ -564,6 +565,34 @@ test "it returns returns a uri if the user has 'hide_followers' set", %{conn: co assert is_binary(result["first"]) end + test "it returns a 403 error on pages, if the user has 'hide_followers' set and the request is not authenticated", + %{conn: conn} do + user = insert(:user, %{info: %{hide_followers: true}}) + + result = + conn + |> get("/users/#{user.nickname}/followers?page=1") + + assert result.status == 403 + assert result.resp_body == "" + end + + test "it renders the page, if the user has 'hide_followers' set and the request is authenticated with the same user", + %{conn: conn} do + user = insert(:user, %{info: %{hide_followers: true}}) + other_user = insert(:user) + {:ok, _other_user, user, _activity} = CommonAPI.follow(other_user, user) + + result = + conn + |> assign(:user, user) + |> get("/users/#{user.nickname}/followers?page=1") + |> json_response(200) + + assert result["totalItems"] == 1 + assert result["orderedItems"] == [other_user.ap_id] + end + test "it works for more than 10 users", %{conn: conn} do user = insert(:user) @@ -618,6 +647,34 @@ test "it returns a uri if the user has 'hide_follows' set", %{conn: conn} do assert is_binary(result["first"]) end + test "it returns a 403 error on pages, if the user has 'hide_follows' set and the request is not authenticated", + %{conn: conn} do + user = insert(:user, %{info: %{hide_follows: true}}) + + result = + conn + |> get("/users/#{user.nickname}/following?page=1") + + assert result.status == 403 + assert result.resp_body == "" + end + + test "it renders the page, if the user has 'hide_follows' set and the request is authenticated with the same user", + %{conn: conn} do + user = insert(:user, %{info: %{hide_follows: true}}) + other_user = insert(:user) + {:ok, user, _other_user, _activity} = CommonAPI.follow(user, other_user) + + result = + conn + |> assign(:user, user) + |> get("/users/#{user.nickname}/following?page=1") + |> json_response(200) + + assert result["totalItems"] == 1 + assert result["orderedItems"] == [other_user.ap_id] + end + test "it works for more than 10 users", %{conn: conn} do user = insert(:user) From f40004e7463bafd07ba70046561dbf937bbf8646 Mon Sep 17 00:00:00 2001 From: rinpatch Date: Fri, 12 Jul 2019 21:49:16 +0300 Subject: [PATCH 4/4] Add changelog entries for follower/following collection behaviour changes --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 942733ab6..405b7680c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - **Breaking:** Configuration: A setting to explicitly disable the mailer was added, defaulting to true, if you are using a mailer add `config :pleroma, Pleroma.Emails.Mailer, enabled: true` to your config - Configuration: OpenGraph and TwitterCard providers enabled by default - Configuration: Filter.AnonymizeFilename added ability to retain file extension with custom text +- Federation: Return 403 errors when trying to request pages from a user's follower/following collections if they have `hide_followers`/`hide_follows` set - NodeInfo: Return `skipThreadContainment` in `metadata` for the `skip_thread_containment` option ### Fixed @@ -16,6 +17,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Mastodon API: Handling of search timeouts (`/api/v1/search` and `/api/v2/search`) - Mastodon API: Embedded relationships not being properly rendered in the Account entity of Status entity - Mastodon API: Add `account_id`, `type`, `offset`, and `limit` to search API (`/api/v1/search` and `/api/v2/search`) +- ActivityPub C2S: follower/following collection pages being inaccessible even when authentifucated if `hide_followers`/ `hide_follows` was set ### Added - MRF: Support for priming the mediaproxy cache (`Pleroma.Web.ActivityPub.MRF.MediaProxyWarmingPolicy`)