Various HTTP signature code improvements

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

lib/federation/http_signatures/signature.ex

@@ -10,10 +10,9 @@ defmodule Mobilizon.Federation.HTTPSignatures.Signature do
 
   @behaviour HTTPSignatures.Adapter
 
+  alias Mobilizon.Actors
   alias Mobilizon.Actors.Actor
-
   alias Mobilizon.Federation.ActivityPub.Actor, as: ActivityPubActor
-  alias Mobilizon.Service.ErrorReporting.Sentry
 
   require Logger
 
@@ -52,10 +51,23 @@ 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 | :actor_not_fetchable}
+          | {:error, :actor_not_found | :pem_decode_error}
   defp get_public_key_for_url(url) do
-    case ActivityPubActor.get_or_fetch_actor_by_url(url) do
-      {:ok, %Actor{keys: keys}} ->
+    case Actors.get_actor_by_url(url) do
+      {:ok, %Actor{} = actor} ->
+        get_actor_public_key(actor)
+
+      {:error, :actor_not_found} ->
+        Logger.info(
+          "Unable to get actor from URL from local database, returning empty keys to trigger refreshment"
+        )
+
+        {:ok, ""}
+    end
+  end
+
+  @spec get_actor_public_key(Actor.t()) :: {:ok, String.t()} | {:error, :pem_decode_error}
+  defp get_actor_public_key(%Actor{keys: keys}) do
     case prepare_public_key(keys) do
       {:ok, public_key} ->
         {:ok, public_key}
@@ -65,23 +77,11 @@ defmodule Mobilizon.Federation.HTTPSignatures.Signature do
 
         {:error, :pem_decode_error}
     end
-
-      {:error, 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
   end
 
   @spec fetch_public_key(Plug.Conn.t()) ::
           {:ok, String.t()}
-          | {:error,
-             :actor_fetch_error | :actor_not_fetchable | :pem_decode_error | :no_signature_in_conn}
+          | {:error, :actor_not_found | :pem_decode_error | :no_signature_in_conn}
   def fetch_public_key(conn) do
     case HTTPSignatures.signature_for_conn(conn) do
       %{"keyId" => kid} ->
@@ -100,8 +100,8 @@ defmodule Mobilizon.Federation.HTTPSignatures.Signature do
              :actor_is_local}
   def refetch_public_key(conn) do
     %{"keyId" => kid} = HTTPSignatures.signature_for_conn(conn)
-    actor_id = key_id_to_actor_url(kid)
-    Logger.debug("Refetching public key for #{actor_id}")
+    actor_url = key_id_to_actor_url(kid)
+    Logger.debug("Refetching public key for #{actor_url}")
 
     with {:ok, %Actor{} = actor} <-
            ActivityPubActor.make_actor_from_url(actor_url, ignore_sign_object_fetches: true) do
@@ -134,6 +134,8 @@ defmodule Mobilizon.Federation.HTTPSignatures.Signature do
 
   @spec generate_date_header(NaiveDateTime.t()) :: String.t()
   def generate_date_header(%NaiveDateTime{} = date) do
+    # We make sure the format is correct
+    # TODO: Remove Timex, as this is the only usage (with parsing)
     Timex.lformat!(date, "{WDshort}, {0D} {Mshort} {YYYY} {h24}:{m}:{s} GMT", "en")
   end
 

lib/web/plugs/http_signatures.ex

@@ -36,14 +36,7 @@ defmodule Mobilizon.Web.Plugs.HTTPSignatures do
           "(request-target)",
           String.downcase("#{conn.method}") <> " #{conn.request_path}"
         )
-
-      conn =
-        if conn.assigns[:digest] do
-          conn
-          |> put_req_header("digest", conn.assigns[:digest])
-        else
-          conn
-        end
+        |> maybe_put_digest_header()
 
       signature_valid = HTTPSignatures.validate_conn(conn)
       Logger.debug("Is signature valid ? #{inspect(signature_valid)}")
@@ -53,6 +46,11 @@ defmodule Mobilizon.Web.Plugs.HTTPSignatures do
     end
   end
 
+  defp maybe_put_digest_header(%Plug.Conn{assigns: %{digest: digest}} = conn),
+    do: put_req_header(conn, "digest", digest)
+
+  defp maybe_put_digest_header(%Plug.Conn{} = conn), do: conn
+
   @spec date_valid?(Plug.Conn.t()) :: boolean()
   defp date_valid?(conn) do
     date = conn |> get_req_header("date") |> List.first()

lib/web/plugs/mapped_signature_to_identity.ex

@@ -11,7 +11,6 @@ defmodule Mobilizon.Web.Plugs.MappedSignatureToIdentity do
   import Plug.Conn
 
   alias Mobilizon.Actors.Actor
-
   alias Mobilizon.Federation.ActivityPub.Actor, as: ActivityPubActor
   alias Mobilizon.Federation.ActivityPub.Utils
   alias Mobilizon.Federation.HTTPSignatures.Signature
@@ -32,16 +31,20 @@ defmodule Mobilizon.Web.Plugs.MappedSignatureToIdentity do
     end
   end
 
-  @spec actor_from_key_id(Plug.Conn.t()) :: Actor.t() | nil
+  @spec actor_from_key_id(Plug.Conn.t()) ::
+          {:ok, Actor.t()} | {:error, :actor_not_found | :no_key_in_conn}
   defp actor_from_key_id(conn) do
     Logger.debug("Determining actor from connection signature")
 
-    with key_actor_id when is_binary(key_actor_id) <- key_id_from_conn(conn),
-         {:ok, %Actor{} = actor} <- ActivityPubActor.get_or_fetch_actor_by_url(key_actor_id) do
-      actor
-    else
-      _ ->
-        nil
+    case key_id_from_conn(conn) do
+      key_actor_id when is_binary(key_actor_id) ->
+        # We don't need to call refreshment here since
+        # the Mobilizon.Federation.HTTPSignatures.Signature plug
+        # should already have refreshed the actor if needed
+        ActivityPubActor.make_actor_from_url(key_actor_id, ignore_sign_object_fetches: true)
+
+      nil ->
+        {:error, :no_key_in_conn}
     end
   end
 
@@ -51,7 +54,7 @@ defmodule Mobilizon.Web.Plugs.MappedSignatureToIdentity do
   # if this has payload make sure it is signed by the same actor that made it
   def call(%{assigns: %{valid_signature: true}, params: %{"actor" => actor}} = conn, _opts) do
     with actor_id when actor_id != nil <- Utils.get_url(actor),
-         {:actor, %Actor{} = actor} <- {:actor, actor_from_key_id(conn)},
+         {:ok, %Actor{} = actor} <- actor_from_key_id(conn),
          {:actor_match, true} <- {:actor_match, actor.url == actor_id} do
       Logger.debug("Mapped identity to #{actor.url} from actor param")
       assign(conn, :actor, actor)
@@ -61,8 +64,12 @@ defmodule Mobilizon.Web.Plugs.MappedSignatureToIdentity do
         Logger.debug("key_id=#{key_id_from_conn(conn)}, actor=#{actor}")
         assign(conn, :valid_signature, false)
 
+      {:error, :no_key_in_conn} ->
+        Logger.debug("There was no key in conn")
+        conn
+
       # TODO: remove me once testsuite uses mapped capabilities instead of what we do now
-      {:actor, nil} ->
+      {:error, :actor_not_found} ->
         Logger.debug("Failed to map identity from signature (lookup failure)")
         Logger.debug("key_id=#{key_id_from_conn(conn)}, actor=#{actor}")
         conn
@@ -72,11 +79,15 @@ defmodule Mobilizon.Web.Plugs.MappedSignatureToIdentity do
   # no payload, probably a signed fetch
   def call(%{assigns: %{valid_signature: true}} = conn, _opts) do
     case actor_from_key_id(conn) do
-      %Actor{} = actor ->
+      {:ok, %Actor{} = actor} ->
         Logger.debug("Mapped identity to #{actor.url} from signed fetch")
         assign(conn, :actor, actor)
 
-      _ ->
+      {:error, :no_key_in_conn} ->
+        Logger.debug("There was no key in conn")
+        assign(conn, :valid_signature, false)
+
+      {:error, :actor_not_found} ->
         Logger.debug("Failed to map identity from signature (no payload actor mismatch)")
         Logger.debug("key_id=#{key_id_from_conn(conn)}")
         assign(conn, :valid_signature, false)