From 67b537f380f472fad481128733b62b8713596b9b Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Wed, 21 Apr 2021 18:57:23 +0200 Subject: [PATCH] Fix sentry issues Signed-off-by: Thomas Citharel --- lib/federation/activity_pub/activity_pub.ex | 93 +++++++++++-------- lib/federation/activity_pub/federator.ex | 2 +- lib/federation/activity_pub/fetcher.ex | 18 +++- lib/federation/activity_pub/preloader.ex | 1 + lib/federation/activity_pub/refresher.ex | 41 ++++++-- lib/federation/activity_pub/transmogrifier.ex | 50 ++++++++-- lib/federation/activity_pub/utils.ex | 41 +++++++- lib/federation/http_signatures/signature.ex | 19 ++-- lib/mobilizon/actors/actors.ex | 21 ++++- .../controllers/activity_pub_controller.ex | 21 ++++- lib/web/views/error_view.ex | 4 + 11 files changed, 229 insertions(+), 82 deletions(-) diff --git a/lib/federation/activity_pub/activity_pub.ex b/lib/federation/activity_pub/activity_pub.ex index 824b9c3c5..08cd8e825 100644 --- a/lib/federation/activity_pub/activity_pub.ex +++ b/lib/federation/activity_pub/activity_pub.ex @@ -79,7 +79,6 @@ defmodule Mobilizon.Federation.ActivityPub do {:ok, struct()} | {:error, any()} def fetch_object_from_url(url, options \\ []) do Logger.info("Fetching object from url #{url}") - force_fetch = Keyword.get(options, :force, false) with {:not_http, true} <- {:not_http, String.starts_with?(url, "http")}, {:existing, nil} <- @@ -99,39 +98,7 @@ defmodule Mobilizon.Federation.ActivityPub do Preloader.maybe_preload(entity) else {:existing, entity} -> - Logger.debug("Entity is already existing") - - res = - if force_fetch and not are_same_origin?(url, Endpoint.url()) do - Logger.debug("Entity is external and we want a force fetch") - - case Fetcher.fetch_and_update(url, options) do - {:ok, _activity, entity} -> - {:ok, entity} - - {:error, "Gone"} -> - {:error, "Gone", entity} - - {:error, "Not found"} -> - {:error, "Not found", entity} - end - else - {:ok, entity} - end - - Logger.debug("Going to preload an existing entity") - - case res do - {:ok, entity} -> - Preloader.maybe_preload(entity) - - {:error, status, entity} -> - {:ok, entity} = Preloader.maybe_preload(entity) - {:error, status, entity} - - err -> - err - end + handle_existing_entity(url, entity, options) e -> Logger.warn("Something failed while fetching url #{inspect(e)}") @@ -139,6 +106,54 @@ defmodule Mobilizon.Federation.ActivityPub do end end + @spec handle_existing_entity(String.t(), struct(), Keyword.t()) :: + {:ok, struct()} + | {:ok, struct()} + | {:error, String.t(), struct()} + | {:error, String.t()} + defp handle_existing_entity(url, entity, options) do + Logger.debug("Entity is already existing") + Logger.debug("Going to preload an existing entity") + + case refresh_entity(url, entity, options) do + {:ok, entity} -> + Preloader.maybe_preload(entity) + + {:error, status, entity} -> + {:ok, entity} = Preloader.maybe_preload(entity) + {:error, status, entity} + + err -> + err + end + end + + @spec refresh_entity(String.t(), struct(), Keyword.t()) :: + {:ok, struct()} | {:error, String.t(), struct()} | {:error, String.t()} + defp refresh_entity(url, entity, options) do + force_fetch = Keyword.get(options, :force, false) + + if force_fetch and not are_same_origin?(url, Endpoint.url()) do + Logger.debug("Entity is external and we want a force fetch") + + case Fetcher.fetch_and_update(url, options) do + {:ok, _activity, entity} -> + {:ok, entity} + + {:error, "Gone"} -> + {:error, "Gone", entity} + + {:error, "Not found"} -> + {:error, "Not found", entity} + + {:error, "Object origin check failed"} -> + {:error, "Object origin check failed"} + end + else + {:ok, entity} + end + end + @doc """ Getting an actor from url, eventually creating it if we don't have it locally or if it needs an update """ @@ -165,8 +180,8 @@ defmodule Mobilizon.Federation.ActivityPub do {:ok, %Actor{} = actor} -> {:ok, actor} - err -> - Logger.warn("Could not fetch by AP id") + {:error, err} -> + Logger.debug("Could not fetch by AP id") Logger.debug(inspect(err)) {:error, "Could not fetch by AP id"} end @@ -624,10 +639,6 @@ defmodule Mobilizon.Federation.ActivityPub do {:error, e} -> Logger.warn("Failed to make actor from url") {:error, e} - - e -> - Logger.warn("Failed to make actor from url") - {:error, e} end end end @@ -784,7 +795,7 @@ defmodule Mobilizon.Federation.ActivityPub do end # Fetching a remote actor's information through its AP ID - @spec fetch_and_prepare_actor_from_url(String.t()) :: {:ok, struct()} | {:error, atom()} | any() + @spec fetch_and_prepare_actor_from_url(String.t()) :: {:ok, map()} | {:error, atom()} | any() defp fetch_and_prepare_actor_from_url(url) do Logger.debug("Fetching and preparing actor from url") Logger.debug(inspect(url)) diff --git a/lib/federation/activity_pub/federator.ex b/lib/federation/activity_pub/federator.ex index d9027084f..8d0e7a7f9 100644 --- a/lib/federation/activity_pub/federator.ex +++ b/lib/federation/activity_pub/federator.ex @@ -61,7 +61,7 @@ defmodule Mobilizon.Federation.ActivityPub.Federator do e -> # Just drop those for now - Logger.error("Unhandled activity") + Logger.debug("Unhandled activity") Logger.debug(inspect(e)) Logger.debug(Jason.encode!(params)) end diff --git a/lib/federation/activity_pub/fetcher.ex b/lib/federation/activity_pub/fetcher.ex index 65971b6c7..7808c9710 100644 --- a/lib/federation/activity_pub/fetcher.ex +++ b/lib/federation/activity_pub/fetcher.ex @@ -30,11 +30,11 @@ defmodule Mobilizon.Federation.ActivityPub.Fetcher do {:ok, data} else {:ok, %Tesla.Env{status: 410}} -> - Logger.warn("Resource at #{url} is 410 Gone") + Logger.debug("Resource at #{url} is 410 Gone") {:error, "Gone"} {:ok, %Tesla.Env{status: 404}} -> - Logger.warn("Resource at #{url} is 404 Gone") + Logger.debug("Resource at #{url} is 404 Gone") {:error, "Not found"} {:ok, %Tesla.Env{} = res} -> @@ -75,7 +75,7 @@ defmodule Mobilizon.Federation.ActivityPub.Fetcher do @spec fetch_and_update(String.t(), Keyword.t()) :: {:ok, map(), struct()} def fetch_and_update(url, options \\ []) do with {:ok, data} when is_map(data) <- fetch(url, options), - {:origin_check, true} <- {:origin_check, origin_check?(url, data)}, + {:origin_check, true} <- {:origin_check, origin_check(url, data)}, params <- %{ "type" => "Update", "to" => data["to"], @@ -87,7 +87,6 @@ defmodule Mobilizon.Federation.ActivityPub.Fetcher do Transmogrifier.handle_incoming(params) else {:origin_check, false} -> - Logger.warn("Object origin check failed") {:error, "Object origin check failed"} {:error, err} -> @@ -95,6 +94,17 @@ defmodule Mobilizon.Federation.ActivityPub.Fetcher do end end + @spec origin_check(String.t(), map()) :: boolean() + defp origin_check(url, data) do + if origin_check?(url, data) do + true + else + Sentry.capture_message("Object origin check failed", extra: %{url: url, data: data}) + Logger.debug("Object origin check failed") + false + end + end + @spec address_invalid(String.t()) :: false | {:error, :invalid_url} defp address_invalid(address) do with %URI{host: host, scheme: scheme} <- URI.parse(address), diff --git a/lib/federation/activity_pub/preloader.ex b/lib/federation/activity_pub/preloader.ex index 79db7a65a..bf8325acc 100644 --- a/lib/federation/activity_pub/preloader.ex +++ b/lib/federation/activity_pub/preloader.ex @@ -12,6 +12,7 @@ defmodule Mobilizon.Federation.ActivityPub.Preloader do alias Mobilizon.Resources.Resource alias Mobilizon.Tombstone + @spec maybe_preload(struct()) :: {:ok, struct()} | {:error, struct()} def maybe_preload(%Event{url: url}), do: {:ok, Events.get_public_event_by_url_with_preload!(url)} diff --git a/lib/federation/activity_pub/refresher.ex b/lib/federation/activity_pub/refresher.ex index 6f485ba4b..e4ff02be5 100644 --- a/lib/federation/activity_pub/refresher.ex +++ b/lib/federation/activity_pub/refresher.ex @@ -7,7 +7,6 @@ defmodule Mobilizon.Federation.ActivityPub.Refresher do alias Mobilizon.Actors.Actor alias Mobilizon.Federation.ActivityPub alias Mobilizon.Federation.ActivityPub.{Fetcher, Relay, Transmogrifier, Utils} - alias Mobilizon.Storage.Repo require Logger @doc """ @@ -60,9 +59,23 @@ defmodule Mobilizon.Federation.ActivityPub.Refresher do :ok <- fetch_collection(events_url, on_behalf_of) do :ok else + {:error, err} -> + Logger.error("Error while refreshing a group") + + Sentry.capture_message("Error while refreshing a group", + extra: %{group_url: group_url} + ) + + Logger.debug(inspect(err)) + err -> Logger.error("Error while refreshing a group") - Logger.error(inspect(err)) + + Sentry.capture_message("Error while refreshing a group", + extra: %{group_url: group_url} + ) + + Logger.debug(inspect(err)) end end @@ -96,14 +109,11 @@ defmodule Mobilizon.Federation.ActivityPub.Refresher do end end - @spec refresh_all_external_groups :: any() + @spec refresh_all_external_groups :: :ok def refresh_all_external_groups do - Repo.transaction(fn -> - Actors.list_external_groups_for_stream() - |> Stream.filter(&Actors.needs_update?/1) - |> Stream.map(&refresh_profile/1) - |> Stream.run() - end) + Actors.list_external_groups() + |> Enum.filter(&Actors.needs_update?/1) + |> Enum.each(&refresh_profile/1) end defp process_collection(%{"type" => type, "orderedItems" => items}, _on_behalf_of) @@ -122,6 +132,14 @@ defmodule Mobilizon.Federation.ActivityPub.Refresher do :ok end + # Lemmy uses an OrderedCollection with the items property + defp process_collection(%{"type" => type, "items" => items} = collection, on_behalf_of) + when type in ["OrderedCollection", "OrderedCollectionPage"] do + collection + |> Map.put("orderedItems", items) + |> process_collection(on_behalf_of) + end + defp process_collection(%{"type" => "OrderedCollection", "first" => first}, on_behalf_of) when is_map(first), do: process_collection(first, on_behalf_of) @@ -150,6 +168,11 @@ defmodule Mobilizon.Federation.ActivityPub.Refresher do Transmogrifier.handle_incoming(data) end + # If we're handling an announce activity + defp handling_element(%{"type" => "Announce"} = data) do + handling_element(get_in(data, ["object"])) + end + # If we're handling directly an object defp handling_element(data) when is_map(data) do object = get_in(data, ["object"]) diff --git a/lib/federation/activity_pub/transmogrifier.ex b/lib/federation/activity_pub/transmogrifier.ex index 620c2e747..1e5ab2fb1 100644 --- a/lib/federation/activity_pub/transmogrifier.ex +++ b/lib/federation/activity_pub/transmogrifier.ex @@ -371,11 +371,13 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier do end end - def handle_incoming(%{ - "type" => "Update", - "object" => %{"type" => object_type} = object, - "actor" => _actor_id - }) + def handle_incoming( + %{ + "type" => "Update", + "object" => %{"type" => object_type} = object, + "actor" => _actor_id + } = params + ) when object_type in ["Person", "Group", "Application", "Service", "Organization"] do with {:ok, %Actor{suspended: false} = old_actor} <- ActivityPub.get_or_fetch_actor_by_url(object["id"]), @@ -386,7 +388,11 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier do {:ok, activity, new_actor} else e -> - Logger.error(inspect(e)) + Sentry.capture_message("Error while handling an Update activity", + extra: %{params: params} + ) + + Logger.debug(inspect(e)) :error end end @@ -572,7 +578,8 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier do %{"type" => "Delete", "object" => object, "actor" => _actor, "id" => _id} = data ) do with actor_url <- Utils.get_actor(data), - {:ok, %Actor{} = actor} <- ActivityPub.get_or_fetch_actor_by_url(actor_url), + {:actor, {:ok, %Actor{} = actor}} <- + {:actor, ActivityPub.get_or_fetch_actor_by_url(actor_url)}, object_id <- Utils.get_url(object), {:ok, object} <- is_group_object_gone(object_id), {:origin_check, true} <- @@ -586,8 +593,25 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier do Logger.warn("Object origin check failed") :error + {:actor, {:error, "Could not fetch by AP id"}} -> + {:error, :unknown_actor} + + {:error, e} -> + Logger.debug(inspect(e)) + + # Sentry.capture_message("Error while handling a Delete activity", + # extra: %{data: data} + # ) + + :error + e -> Logger.error(inspect(e)) + + # Sentry.capture_message("Error while handling a Delete activity", + # extra: %{data: data} + # ) + :error end end @@ -610,7 +634,12 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier do {:ok, activity, new_resource} else e -> - Logger.error(inspect(e)) + Logger.debug(inspect(e)) + + Sentry.capture_message("Error while handling an Move activity", + extra: %{data: data} + ) + :error end end @@ -741,6 +770,11 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier do def handle_incoming(object) do Logger.info("Handing something with type #{object["type"]} not supported") Logger.debug(inspect(object)) + + Sentry.capture_message("Handing something with type #{object["type"]} not supported", + extra: %{object: object} + ) + {:error, :not_supported} end diff --git a/lib/federation/activity_pub/utils.ex b/lib/federation/activity_pub/utils.ex index b21a5bdcc..d1cb0a3ed 100644 --- a/lib/federation/activity_pub/utils.ex +++ b/lib/federation/activity_pub/utils.ex @@ -259,7 +259,8 @@ defmodule Mobilizon.Federation.ActivityPub.Utils do are_same_origin?(id, actor) end - def origin_check?(_id, %{"type" => type} = _params) when type in ["Actor", "Group"], do: true + def origin_check?(_id, %{"type" => type} = _params) when type in ["Actor", "Person", "Group"], + do: true def origin_check?(_id, %{"actor" => nil} = _args), do: false @@ -701,4 +702,42 @@ defmodule Mobilizon.Federation.ActivityPub.Utils do true end end + + @spec label_in_collection?(any(), any()) :: boolean() + defp label_in_collection?(url, coll) when is_binary(coll), do: url == coll + defp label_in_collection?(url, coll) when is_list(coll), do: url in coll + defp label_in_collection?(_, _), do: false + + @spec label_in_message?(String.t(), map()) :: boolean() + def label_in_message?(label, params), + do: + [params["to"], params["cc"], params["bto"], params["bcc"]] + |> Enum.any?(&label_in_collection?(label, &1)) + + @spec unaddressed_message?(map()) :: boolean() + def unaddressed_message?(params), + do: + [params["to"], params["cc"], params["bto"], params["bcc"]] + |> Enum.all?(&is_nil(&1)) + + @spec recipient_in_message(Actor.t(), Actor.t(), map()) :: boolean() + def recipient_in_message(%Actor{url: url} = _recipient, %Actor{} = _actor, params), + do: label_in_message?(url, params) || unaddressed_message?(params) + + defp extract_list(target) when is_binary(target), do: [target] + defp extract_list(lst) when is_list(lst), do: lst + defp extract_list(_), do: [] + + def maybe_splice_recipient(url, params) do + need_splice? = + !label_in_collection?(url, params["to"]) && + !label_in_collection?(url, params["cc"]) + + if need_splice? do + cc_list = extract_list(params["cc"]) + Map.put(params, "cc", [url | cc_list]) + else + params + end + end end diff --git a/lib/federation/http_signatures/signature.ex b/lib/federation/http_signatures/signature.ex index bab2b14d7..787b4ccb4 100644 --- a/lib/federation/http_signatures/signature.ex +++ b/lib/federation/http_signatures/signature.ex @@ -50,7 +50,8 @@ defmodule Mobilizon.Federation.HTTPSignatures.Signature do # Gets a public key for a given ActivityPub actor ID (url). @spec get_public_key_for_url(String.t()) :: - {:ok, String.t()} | {:error, :actor_fetch_error | :pem_decode_error} + {:ok, String.t()} + | {:error, :actor_fetch_error | :pem_decode_error | :actor_not_fetchable} defp get_public_key_for_url(url) do with {:ok, %Actor{keys: keys}} <- ActivityPub.get_or_fetch_actor_by_url(url), {:ok, public_key} <- prepare_public_key(keys) do @@ -61,8 +62,16 @@ defmodule Mobilizon.Federation.HTTPSignatures.Signature do {:error, :pem_decode_error} - _ -> + {:error, "Could not fetch by AP id"} -> + {:error, :actor_not_fetchable} + + err -> + Sentry.capture_message("Unable to fetch actor, so no keys for you", + extra: %{url: url} + ) + Logger.error("Unable to fetch actor, so no keys for you") + Logger.error(inspect(err)) {:error, :actor_fetch_error} end @@ -74,9 +83,6 @@ defmodule Mobilizon.Federation.HTTPSignatures.Signature do :ok <- Logger.debug("Fetching public key for #{actor_id}"), {:ok, public_key} <- get_public_key_for_url(actor_id) do {:ok, public_key} - else - e -> - {:error, e} end end @@ -87,9 +93,6 @@ defmodule Mobilizon.Federation.HTTPSignatures.Signature do {:ok, _actor} <- ActivityPub.make_actor_from_url(actor_id), {:ok, public_key} <- get_public_key_for_url(actor_id) do {:ok, public_key} - else - e -> - {:error, e} end end diff --git a/lib/mobilizon/actors/actors.ex b/lib/mobilizon/actors/actors.ex index aabdf21ef..4ae932e27 100644 --- a/lib/mobilizon/actors/actors.ex +++ b/lib/mobilizon/actors/actors.ex @@ -374,12 +374,22 @@ defmodule Mobilizon.Actors do {:error, remove, error, _} when remove in [:remove_banner, :remove_avatar] -> Logger.error("Error while deleting actor's banner or avatar") - Logger.error(inspect(error, pretty: true)) + + Sentry.capture_message("Error while deleting actor's banner or avatar", + extra: %{err: error} + ) + + Logger.debug(inspect(error, pretty: true)) {:error, error} err -> Logger.error("Unknown error while deleting actor") - Logger.error(inspect(err, pretty: true)) + + Sentry.capture_message("Error while deleting actor's banner or avatar", + extra: %{err: err} + ) + + Logger.debug(inspect(err, pretty: true)) {:error, err} end end @@ -652,10 +662,11 @@ defmodule Mobilizon.Actors do @doc """ Lists the groups. """ - @spec list_groups_for_stream :: Enum.t() - def list_external_groups_for_stream do + @spec list_external_groups(non_neg_integer()) :: list(Actor.t()) + def list_external_groups(limit \\ 100) when limit > 0 do external_groups_query() - |> Repo.stream() + |> limit(^limit) + |> Repo.all() end @doc """ diff --git a/lib/web/controllers/activity_pub_controller.ex b/lib/web/controllers/activity_pub_controller.ex index 0e35181c4..909369976 100644 --- a/lib/web/controllers/activity_pub_controller.ex +++ b/lib/web/controllers/activity_pub_controller.ex @@ -10,7 +10,7 @@ defmodule Mobilizon.Web.ActivityPubController do alias Mobilizon.Actors.{Actor, Member} alias Mobilizon.Federation.ActivityPub - alias Mobilizon.Federation.ActivityPub.Federator + alias Mobilizon.Federation.ActivityPub.{Federator, Utils} alias Mobilizon.Web.ActivityPub.ActorView alias Mobilizon.Web.Cache @@ -105,7 +105,17 @@ defmodule Mobilizon.Web.ActivityPubController do actor_collection(conn, "outbox", args) end - # TODO: Ensure that this inbox is a recipient of the message + def inbox(%{assigns: %{valid_signature: true}} = conn, %{"name" => preferred_username} = params) do + with %Actor{url: recipient_url} = recipient <- + Actors.get_local_actor_by_name(preferred_username), + {:ok, %Actor{} = actor} <- ActivityPub.get_or_fetch_actor_by_url(params["actor"]), + true <- Utils.recipient_in_message(recipient, actor, params), + params <- Utils.maybe_splice_recipient(recipient_url, params) do + Federator.enqueue(:incoming_ap_doc, params) + json(conn, "ok") + end + end + def inbox(%{assigns: %{valid_signature: true}} = conn, params) do Logger.debug("Got something with valid signature inside inbox") Federator.enqueue(:incoming_ap_doc, params) @@ -114,7 +124,7 @@ defmodule Mobilizon.Web.ActivityPubController do # only accept relayed Creates def inbox(conn, %{"type" => "Create"} = params) do - Logger.info( + Logger.debug( "Signature missing or not from author, relayed Create message, fetching object from source" ) @@ -126,8 +136,9 @@ defmodule Mobilizon.Web.ActivityPubController do def inbox(conn, params) do headers = Enum.into(conn.req_headers, %{}) - if String.contains?(headers["signature"], params["actor"]) do - Logger.error( + if headers["signature"] && params["actor"] && + String.contains?(headers["signature"], params["actor"]) do + Logger.debug( "Signature validation error for: #{params["actor"]}, make sure you are forwarding the HTTP Host header!" ) diff --git a/lib/web/views/error_view.ex b/lib/web/views/error_view.ex index 7eac0ca4d..4cbc238a2 100644 --- a/lib/web/views/error_view.ex +++ b/lib/web/views/error_view.ex @@ -47,6 +47,10 @@ defmodule Mobilizon.Web.ErrorView do %{msg: "Not acceptable"} end + def render("500.json", assigns) do + render("500.html", assigns) + end + def render("500.html", assigns) do Mobilizon.Config.instance_config() |> Keyword.get(:default_language, "en")