From f6024252ae8601d41bea943bb3cae5c656416eb9 Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Fri, 2 Oct 2020 22:18:02 +0300 Subject: [PATCH] [#3053] No auth check in StaticFEController, even on non-federating instances. Adjusted tests. --- lib/pleroma/web/feed/tag_controller.ex | 4 +- lib/pleroma/web/feed/user_controller.ex | 4 +- lib/pleroma/web/router.ex | 11 +- .../web/static_fe/static_fe_controller.ex | 176 +++++++++--------- test/support/conn_case.ex | 22 --- .../activity_pub_controller_test.exs | 19 ++ test/web/feed/user_controller_test.exs | 12 +- test/web/ostatus/ostatus_controller_test.exs | 24 +-- .../static_fe/static_fe_controller_test.exs | 24 ++- 9 files changed, 162 insertions(+), 134 deletions(-) diff --git a/lib/pleroma/web/feed/tag_controller.ex b/lib/pleroma/web/feed/tag_controller.ex index 93a8294b7..c348b32c2 100644 --- a/lib/pleroma/web/feed/tag_controller.ex +++ b/lib/pleroma/web/feed/tag_controller.ex @@ -10,14 +10,14 @@ defmodule Pleroma.Web.Feed.TagController do alias Pleroma.Web.Feed.FeedView def feed(conn, params) do - unless Pleroma.Config.restrict_unauthenticated_access?(:activities, :local) do + unless Config.restrict_unauthenticated_access?(:activities, :local) do render_feed(conn, params) else render_error(conn, :not_found, "Not found") end end - def render_feed(conn, %{"tag" => raw_tag} = params) do + defp render_feed(conn, %{"tag" => raw_tag} = params) do {format, tag} = parse_tag(raw_tag) activities = diff --git a/lib/pleroma/web/feed/user_controller.ex b/lib/pleroma/web/feed/user_controller.ex index 09ecdedb4..5fbcd82d7 100644 --- a/lib/pleroma/web/feed/user_controller.ex +++ b/lib/pleroma/web/feed/user_controller.ex @@ -40,11 +40,11 @@ def feed(conn, params) do end end - def render_feed(conn, %{"nickname" => nickname} = params) do + defp render_feed(conn, %{"nickname" => nickname} = params) do format = get_format(conn) format = - if format in ["rss", "atom"] do + if format in ["atom", "rss"] do format else "atom" diff --git a/lib/pleroma/web/router.ex b/lib/pleroma/web/router.ex index 42a9db21d..e0e92549f 100644 --- a/lib/pleroma/web/router.ex +++ b/lib/pleroma/web/router.ex @@ -561,12 +561,17 @@ defmodule Pleroma.Web.Router do plug(Pleroma.Plugs.StaticFEPlug) end + pipeline :ostatus_no_html do + plug(:accepts, ["xml", "rss", "atom", "activity+json", "json"]) + end + pipeline :oembed do plug(:accepts, ["json", "xml"]) end scope "/", Pleroma.Web do - pipe_through([:ostatus, :http_signature]) + # Note: no authentication plugs, all endpoints below should only yield public objects + pipe_through(:ostatus) get("/objects/:uuid", OStatus.OStatusController, :object) get("/activities/:uuid", OStatus.OStatusController, :activity) @@ -579,6 +584,10 @@ defmodule Pleroma.Web.Router do get("/users/:nickname/feed", Feed.UserController, :feed, as: :user_feed) get("/users/:nickname", Feed.UserController, :feed_redirect, as: :user_feed) + end + + scope "/", Pleroma.Web do + pipe_through(:ostatus_no_html) get("/tags/:tag", Feed.TagController, :feed, as: :tag_feed) end diff --git a/lib/pleroma/web/static_fe/static_fe_controller.ex b/lib/pleroma/web/static_fe/static_fe_controller.ex index a7a891b13..b1c62f5b0 100644 --- a/lib/pleroma/web/static_fe/static_fe_controller.ex +++ b/lib/pleroma/web/static_fe/static_fe_controller.ex @@ -17,12 +17,95 @@ defmodule Pleroma.Web.StaticFE.StaticFEController do plug(:put_view, Pleroma.Web.StaticFE.StaticFEView) plug(:assign_id) - plug(Pleroma.Plugs.EnsureAuthenticatedPlug, - unless_func: &Pleroma.Web.FederatingPlug.federating?/1 - ) - @page_keys ["max_id", "min_id", "limit", "since_id", "order"] + @doc "Renders requested local public activity" + def show(%{assigns: %{notice_id: notice_id}} = conn, _params) do + with %Activity{local: true} = activity <- + Activity.get_by_id_with_object(notice_id), + true <- Visibility.is_public?(activity.object), + %User{} = user <- User.get_by_ap_id(activity.object.data["actor"]) do + meta = Metadata.build_tags(%{activity_id: notice_id, object: activity.object, user: user}) + + timeline = + activity.object.data["context"] + |> ActivityPub.fetch_activities_for_context(%{}) + |> Enum.reverse() + |> Enum.map(&represent(&1, &1.object.id == activity.object.id)) + + render(conn, "conversation.html", %{activities: timeline, meta: meta}) + else + %Activity{object: %Object{data: data}} -> + conn + |> put_status(:found) + |> redirect(external: data["url"] || data["external_url"] || data["id"]) + + _ -> + not_found(conn, "Post not found.") + end + end + + @doc "Renders public activities of requested user" + def show(%{assigns: %{username_or_id: username_or_id}} = conn, params) do + case User.get_cached_by_nickname_or_id(username_or_id) do + %User{} = user -> + meta = Metadata.build_tags(%{user: user}) + + params = + params + |> Map.take(@page_keys) + |> Map.new(fn {k, v} -> {String.to_existing_atom(k), v} end) + + timeline = + user + |> ActivityPub.fetch_user_activities(_reading_user = nil, params) + |> Enum.map(&represent/1) + + prev_page_id = + (params["min_id"] || params["max_id"]) && + List.first(timeline) && List.first(timeline).id + + next_page_id = List.last(timeline) && List.last(timeline).id + + render(conn, "profile.html", %{ + user: User.sanitize_html(user), + timeline: timeline, + prev_page_id: prev_page_id, + next_page_id: next_page_id, + meta: meta + }) + + _ -> + not_found(conn, "User not found.") + end + end + + def show(%{assigns: %{object_id: _}} = conn, _params) do + url = Helpers.url(conn) <> conn.request_path + + case Activity.get_create_by_object_ap_id_with_object(url) do + %Activity{} = activity -> + to = Helpers.o_status_path(Pleroma.Web.Endpoint, :notice, activity) + redirect(conn, to: to) + + _ -> + not_found(conn, "Post not found.") + end + end + + def show(%{assigns: %{activity_id: _}} = conn, _params) do + url = Helpers.url(conn) <> conn.request_path + + case Activity.get_by_ap_id(url) do + %Activity{} = activity -> + to = Helpers.o_status_path(Pleroma.Web.Endpoint, :notice, activity) + redirect(conn, to: to) + + _ -> + not_found(conn, "Post not found.") + end + end + defp get_title(%Object{data: %{"name" => name}}) when is_binary(name), do: name @@ -81,91 +164,6 @@ defp represent(%Activity{object: %Object{data: data}} = activity, selected) do } end - def show(%{assigns: %{notice_id: notice_id}} = conn, _params) do - with %Activity{local: true} = activity <- - Activity.get_by_id_with_object(notice_id), - true <- Visibility.is_public?(activity.object), - %User{} = user <- User.get_by_ap_id(activity.object.data["actor"]) do - meta = Metadata.build_tags(%{activity_id: notice_id, object: activity.object, user: user}) - - timeline = - activity.object.data["context"] - |> ActivityPub.fetch_activities_for_context(%{}) - |> Enum.reverse() - |> Enum.map(&represent(&1, &1.object.id == activity.object.id)) - - render(conn, "conversation.html", %{activities: timeline, meta: meta}) - else - %Activity{object: %Object{data: data}} -> - conn - |> put_status(:found) - |> redirect(external: data["url"] || data["external_url"] || data["id"]) - - _ -> - not_found(conn, "Post not found.") - end - end - - def show(%{assigns: %{username_or_id: username_or_id}} = conn, params) do - case User.get_cached_by_nickname_or_id(username_or_id) do - %User{} = user -> - meta = Metadata.build_tags(%{user: user}) - - params = - params - |> Map.take(@page_keys) - |> Map.new(fn {k, v} -> {String.to_existing_atom(k), v} end) - - timeline = - user - |> ActivityPub.fetch_user_activities(nil, params) - |> Enum.map(&represent/1) - - prev_page_id = - (params["min_id"] || params["max_id"]) && - List.first(timeline) && List.first(timeline).id - - next_page_id = List.last(timeline) && List.last(timeline).id - - render(conn, "profile.html", %{ - user: User.sanitize_html(user), - timeline: timeline, - prev_page_id: prev_page_id, - next_page_id: next_page_id, - meta: meta - }) - - _ -> - not_found(conn, "User not found.") - end - end - - def show(%{assigns: %{object_id: _}} = conn, _params) do - url = Helpers.url(conn) <> conn.request_path - - case Activity.get_create_by_object_ap_id_with_object(url) do - %Activity{} = activity -> - to = Helpers.o_status_path(Pleroma.Web.Endpoint, :notice, activity) - redirect(conn, to: to) - - _ -> - not_found(conn, "Post not found.") - end - end - - def show(%{assigns: %{activity_id: _}} = conn, _params) do - url = Helpers.url(conn) <> conn.request_path - - case Activity.get_by_ap_id(url) do - %Activity{} = activity -> - to = Helpers.o_status_path(Pleroma.Web.Endpoint, :notice, activity) - redirect(conn, to: to) - - _ -> - not_found(conn, "Post not found.") - end - end - defp assign_id(%{path_info: ["notice", notice_id]} = conn, _opts), do: assign(conn, :notice_id, notice_id) diff --git a/test/support/conn_case.ex b/test/support/conn_case.ex index 7ef681258..7b28d70e7 100644 --- a/test/support/conn_case.ex +++ b/test/support/conn_case.ex @@ -111,28 +111,6 @@ defp json_response_and_validate_schema( defp json_response_and_validate_schema(conn, _status) do flunk("Response schema not found for #{conn.method} #{conn.request_path} #{conn.status}") end - - defp ensure_federating_or_authenticated(conn, url, user) do - initial_setting = Config.get([:instance, :federating]) - on_exit(fn -> Config.put([:instance, :federating], initial_setting) end) - - Config.put([:instance, :federating], false) - - conn - |> get(url) - |> response(403) - - conn - |> assign(:user, user) - |> get(url) - |> response(200) - - Config.put([:instance, :federating], true) - - conn - |> get(url) - |> response(200) - end end end diff --git a/test/web/activity_pub/activity_pub_controller_test.exs b/test/web/activity_pub/activity_pub_controller_test.exs index 0517571f2..ab57b6523 100644 --- a/test/web/activity_pub/activity_pub_controller_test.exs +++ b/test/web/activity_pub/activity_pub_controller_test.exs @@ -33,6 +33,25 @@ defmodule Pleroma.Web.ActivityPub.ActivityPubControllerTest do setup do: clear_config([:instance, :federating], true) + defp ensure_federating_or_authenticated(conn, url, user) do + Config.put([:instance, :federating], false) + + conn + |> get(url) + |> response(403) + + conn + |> assign(:user, user) + |> get(url) + |> response(200) + + Config.put([:instance, :federating], true) + + conn + |> get(url) + |> response(200) + end + describe "/relay" do setup do: clear_config([:instance, :allow_relay]) diff --git a/test/web/feed/user_controller_test.exs b/test/web/feed/user_controller_test.exs index 9a5610baa..7383e82cc 100644 --- a/test/web/feed/user_controller_test.exs +++ b/test/web/feed/user_controller_test.exs @@ -13,7 +13,7 @@ defmodule Pleroma.Web.Feed.UserControllerTest do alias Pleroma.User alias Pleroma.Web.CommonAPI - setup do: clear_config([:instance, :federating], true) + setup do: clear_config([:static_fe, :enabled], false) describe "feed" do setup do: clear_config([:feed]) @@ -192,6 +192,16 @@ test "returns 404 when the user is remote", %{conn: conn} do |> get(user_feed_path(conn, :feed, user.nickname)) |> response(404) end + + test "does not require authentication on non-federating instances", %{conn: conn} do + clear_config([:instance, :federating], false) + user = insert(:user) + + conn + |> put_req_header("accept", "application/rss+xml") + |> get("/users/#{user.nickname}/feed.rss") + |> response(200) + end end # Note: see ActivityPubControllerTest for JSON format tests diff --git a/test/web/ostatus/ostatus_controller_test.exs b/test/web/ostatus/ostatus_controller_test.exs index ee498f4b5..65b2c22db 100644 --- a/test/web/ostatus/ostatus_controller_test.exs +++ b/test/web/ostatus/ostatus_controller_test.exs @@ -7,7 +7,6 @@ defmodule Pleroma.Web.OStatus.OStatusControllerTest do import Pleroma.Factory - alias Pleroma.Config alias Pleroma.Object alias Pleroma.User alias Pleroma.Web.ActivityPub.ActivityPub @@ -21,7 +20,7 @@ defmodule Pleroma.Web.OStatus.OStatusControllerTest do :ok end - setup do: clear_config([:instance, :federating], true) + setup do: clear_config([:static_fe, :enabled], false) describe "Mastodon compatibility routes" do setup %{conn: conn} do @@ -215,15 +214,16 @@ test "404s a non-existing notice", %{conn: conn} do assert response(conn, 404) end - test "it requires authentication if instance is NOT federating", %{ + test "does not require authentication on non-federating instances", %{ conn: conn } do - user = insert(:user) + clear_config([:instance, :federating], false) note_activity = insert(:note_activity) - conn = put_req_header(conn, "accept", "text/html") - - ensure_federating_or_authenticated(conn, "/notice/#{note_activity.id}", user) + conn + |> put_req_header("accept", "text/html") + |> get("/notice/#{note_activity.id}") + |> response(200) end end @@ -325,14 +325,16 @@ test "404s when attachment isn't audio or video", %{conn: conn} do |> response(404) end - test "it requires authentication if instance is NOT federating", %{ + test "does not require authentication on non-federating instances", %{ conn: conn, note_activity: note_activity } do - user = insert(:user) - conn = put_req_header(conn, "accept", "text/html") + clear_config([:instance, :federating], false) - ensure_federating_or_authenticated(conn, "/notice/#{note_activity.id}/embed_player", user) + conn + |> put_req_header("accept", "text/html") + |> get("/notice/#{note_activity.id}/embed_player") + |> response(200) end end end diff --git a/test/web/static_fe/static_fe_controller_test.exs b/test/web/static_fe/static_fe_controller_test.exs index 1598bf675..bab0b0a7b 100644 --- a/test/web/static_fe/static_fe_controller_test.exs +++ b/test/web/static_fe/static_fe_controller_test.exs @@ -2,14 +2,12 @@ defmodule Pleroma.Web.StaticFE.StaticFEControllerTest do use Pleroma.Web.ConnCase alias Pleroma.Activity - alias Pleroma.Config alias Pleroma.Web.ActivityPub.Transmogrifier alias Pleroma.Web.CommonAPI import Pleroma.Factory setup_all do: clear_config([:static_fe, :enabled], true) - setup do: clear_config([:instance, :federating], true) setup %{conn: conn} do conn = put_req_header(conn, "accept", "text/html") @@ -70,8 +68,15 @@ test "pagination, page 2", %{conn: conn, user: user} do refute html =~ ">test29<" end - test "it requires authentication if instance is NOT federating", %{conn: conn, user: user} do - ensure_federating_or_authenticated(conn, "/users/#{user.nickname}", user) + test "does not require authentication on non-federating instances", %{ + conn: conn, + user: user + } do + clear_config([:instance, :federating], false) + + conn = get(conn, "/users/#{user.nickname}") + + assert html_response(conn, 200) =~ user.nickname end end @@ -183,10 +188,17 @@ test "302 for remote cached status", %{conn: conn, user: user} do assert html_response(conn, 302) =~ "redirected" end - test "it requires authentication if instance is NOT federating", %{conn: conn, user: user} do + test "does not require authentication on non-federating instances", %{ + conn: conn, + user: user + } do + clear_config([:instance, :federating], false) + {:ok, activity} = CommonAPI.post(user, %{status: "testing a thing!"}) - ensure_federating_or_authenticated(conn, "/notice/#{activity.id}", user) + conn = get(conn, "/notice/#{activity.id}") + + assert html_response(conn, 200) =~ "testing a thing!" end end end