From 8073af6e105ec583491191ded5f76a2700402224 Mon Sep 17 00:00:00 2001 From: lain Date: Sun, 3 Jun 2018 21:04:44 +0200 Subject: [PATCH] Better error handling for OstatusController. --- lib/pleroma/web/ostatus/ostatus_controller.ex | 99 ++++++++++++------- test/web/ostatus/ostatus_controller_test.exs | 40 ++++++++ 2 files changed, 105 insertions(+), 34 deletions(-) diff --git a/lib/pleroma/web/ostatus/ostatus_controller.ex b/lib/pleroma/web/ostatus/ostatus_controller.ex index de8567224..f346cc9af 100644 --- a/lib/pleroma/web/ostatus/ostatus_controller.ex +++ b/lib/pleroma/web/ostatus/ostatus_controller.ex @@ -9,36 +9,42 @@ defmodule Pleroma.Web.OStatus.OStatusController do alias Pleroma.Web.ActivityPub.ActivityPubController alias Pleroma.Web.ActivityPub.ActivityPub - def feed_redirect(conn, %{"nickname" => nickname}) do - user = User.get_cached_by_nickname(nickname) + action_fallback(:errors) - case get_format(conn) do - "html" -> Fallback.RedirectController.redirector(conn, nil) - "activity+json" -> ActivityPubController.call(conn, :user) - _ -> redirect(conn, external: OStatus.feed_path(user)) + def feed_redirect(conn, %{"nickname" => nickname}) do + with {_, %User{} = user} <- {:user, User.get_cached_by_nickname(nickname)} do + case get_format(conn) do + "html" -> Fallback.RedirectController.redirector(conn, nil) + "activity+json" -> ActivityPubController.call(conn, :user) + _ -> redirect(conn, external: OStatus.feed_path(user)) + end + else + {:user, nil} -> {:error, :not_found} end end def feed(conn, %{"nickname" => nickname} = params) do - user = User.get_cached_by_nickname(nickname) + with {_, %User{} = user} <- {:user, User.get_cached_by_nickname(nickname)} do + query_params = + Map.take(params, ["max_id"]) + |> Map.merge(%{"whole_db" => true, "actor_id" => user.ap_id}) - query_params = - Map.take(params, ["max_id"]) - |> Map.merge(%{"whole_db" => true, "actor_id" => user.ap_id}) + activities = + ActivityPub.fetch_public_activities(query_params) + |> Enum.reverse() - activities = - ActivityPub.fetch_public_activities(query_params) - |> Enum.reverse() + response = + user + |> FeedRepresenter.to_simple_form(activities, [user]) + |> :xmerl.export_simple(:xmerl_xml) + |> to_string - response = - user - |> FeedRepresenter.to_simple_form(activities, [user]) - |> :xmerl.export_simple(:xmerl_xml) - |> to_string - - conn - |> put_resp_content_type("application/atom+xml") - |> send_resp(200, response) + conn + |> put_resp_content_type("application/atom+xml") + |> send_resp(200, response) + else + {:user, nil} -> {:error, :not_found} + end end defp decode_or_retry(body) do @@ -73,7 +79,8 @@ def object(conn, %{"uuid" => uuid}) do ActivityPubController.call(conn, :object) else with id <- o_status_url(conn, :object, uuid), - %Activity{} = activity <- Activity.get_create_activity_by_object_ap_id(id), + {_, %Activity{} = activity} <- + {:activity, Activity.get_create_activity_by_object_ap_id(id)}, {_, true} <- {:public?, ActivityPub.is_public?(activity)}, %User{} = user <- User.get_cached_by_ap_id(activity.data["actor"]) do case get_format(conn) do @@ -82,16 +89,20 @@ def object(conn, %{"uuid" => uuid}) do end else {:public?, false} -> - conn - |> put_status(404) - |> json("Not found") + {:error, :not_found} + + {:activity, nil} -> + {:error, :not_found} + + e -> + e end end end def activity(conn, %{"uuid" => uuid}) do with id <- o_status_url(conn, :activity, uuid), - %Activity{} = activity <- Activity.get_by_ap_id(id), + {_, %Activity{} = activity} <- {:activity, Activity.get_by_ap_id(id)}, {_, true} <- {:public?, ActivityPub.is_public?(activity)}, %User{} = user <- User.get_cached_by_ap_id(activity.data["actor"]) do case get_format(conn) do @@ -100,14 +111,18 @@ def activity(conn, %{"uuid" => uuid}) do end else {:public?, false} -> - conn - |> put_status(404) - |> json("Not found") + {:error, :not_found} + + {:activity, nil} -> + {:error, :not_found} + + e -> + e end end def notice(conn, %{"id" => id}) do - with %Activity{} = activity <- Repo.get(Activity, id), + with {_, %Activity{} = activity} <- {:activity, Repo.get(Activity, id)}, {_, true} <- {:public?, ActivityPub.is_public?(activity)}, %User{} = user <- User.get_cached_by_ap_id(activity.data["actor"]) do case get_format(conn) do @@ -121,9 +136,13 @@ def notice(conn, %{"id" => id}) do end else {:public?, false} -> - conn - |> put_status(404) - |> json("Not found") + {:error, :not_found} + + {:activity, nil} -> + {:error, :not_found} + + e -> + e end end @@ -139,4 +158,16 @@ defp represent_activity(conn, activity, user) do |> put_resp_content_type("application/atom+xml") |> send_resp(200, response) end + + def errors(conn, {:error, :not_found}) do + conn + |> put_status(404) + |> text("Not found") + end + + def errors(conn, _) do + conn + |> put_status(500) + |> text("Something went wrong") + end end diff --git a/test/web/ostatus/ostatus_controller_test.exs b/test/web/ostatus/ostatus_controller_test.exs index faee4fc3e..d5adf3bf3 100644 --- a/test/web/ostatus/ostatus_controller_test.exs +++ b/test/web/ostatus/ostatus_controller_test.exs @@ -53,11 +53,21 @@ test "gets a feed", %{conn: conn} do conn = conn + |> put_req_header("content-type", "application/atom+xml") |> get("/users/#{user.nickname}/feed.atom") assert response(conn, 200) =~ note_activity.data["object"]["content"] end + test "returns 404 for a missing feed", %{conn: conn} do + conn = + conn + |> put_req_header("content-type", "application/atom+xml") + |> get("/users/nonexisting/feed.atom") + + assert response(conn, 404) + end + test "gets an object", %{conn: conn} do note_activity = insert(:note_activity) user = User.get_by_ap_id(note_activity.data["actor"]) @@ -90,6 +100,16 @@ test "404s on private objects", %{conn: conn} do assert response(conn, 404) end + test "404s on nonexisting objects", %{conn: conn} do + url = "/objects/123" + + conn = + conn + |> get(url) + + assert response(conn, 404) + end + test "gets an activity", %{conn: conn} do note_activity = insert(:note_activity) [_, uuid] = hd(Regex.scan(~r/.+\/([\w-]+)$/, note_activity.data["id"])) @@ -114,6 +134,16 @@ test "404s on private activities", %{conn: conn} do assert response(conn, 404) end + test "404s on nonexistent activities", %{conn: conn} do + url = "/activities/123" + + conn = + conn + |> get(url) + + assert response(conn, 404) + end + test "gets a notice", %{conn: conn} do note_activity = insert(:note_activity) url = "/notice/#{note_activity.id}" @@ -135,4 +165,14 @@ test "404s a private notice", %{conn: conn} do assert response(conn, 404) end + + test "404s a nonexisting notice", %{conn: conn} do + url = "/notice/123" + + conn = + conn + |> get(url) + + assert response(conn, 404) + end end