Fix sentry issues

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

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")
+        handle_existing_entity(url, entity, options)
-
-        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
 
       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 ->
+          {:error, err} ->
-            Logger.warn("Could not fetch by AP id")
+            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))

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

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),

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)}
 

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()
-      Actors.list_external_groups_for_stream()
+    |> Enum.filter(&Actors.needs_update?/1)
-      |> Stream.filter(&Actors.needs_update?/1)
+    |> Enum.each(&refresh_profile/1)
-      |> Stream.map(&refresh_profile/1)
-      |> Stream.run()
-    end)
   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"])

lib/federation/activity_pub/transmogrifier.ex

@@ -371,11 +371,13 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier do
     end
   end
 
-  def handle_incoming(%{
+  def handle_incoming(
-        "type" => "Update",
+        %{
-        "object" => %{"type" => object_type} = object,
+          "type" => "Update",
-        "actor" => _actor_id
+          "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
 

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

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
 

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()
+  @spec list_external_groups(non_neg_integer()) :: list(Actor.t())
-  def list_external_groups_for_stream do
+  def list_external_groups(limit \\ 100) when limit > 0 do
     external_groups_query()
-    |> Repo.stream()
+    |> limit(^limit)
+    |> Repo.all()
   end
 
   @doc """

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
+    if headers["signature"] && params["actor"] &&
-      Logger.error(
+         String.contains?(headers["signature"], params["actor"]) do
+      Logger.debug(
         "Signature validation error for: #{params["actor"]}, make sure you are forwarding the HTTP Host header!"
       )
 

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")