From 1bfff235f30a7471494064101d4f9ba2e201867f Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Mon, 29 Nov 2021 10:29:26 +0100 Subject: [PATCH] Don't sign fetches to instance actor when refreshing their keys Closes #963 Signed-off-by: Thomas Citharel --- lib/federation/activity_pub/actor.ex | 15 ++++++++------- lib/federation/activity_pub/transmogrifier.ex | 3 ++- lib/federation/http_signatures/signature.ex | 3 ++- test/federation/activity_pub/actor_test.exs | 10 +++++----- 4 files changed, 17 insertions(+), 14 deletions(-) diff --git a/lib/federation/activity_pub/actor.ex b/lib/federation/activity_pub/actor.ex index 578628cf3..30f141a9a 100644 --- a/lib/federation/activity_pub/actor.ex +++ b/lib/federation/activity_pub/actor.ex @@ -14,37 +14,38 @@ defmodule Mobilizon.Federation.ActivityPub.Actor do @doc """ Getting an actor from url, eventually creating it if we don't have it locally or if it needs an update """ - @spec get_or_fetch_actor_by_url(url :: String.t(), preload :: boolean()) :: + @spec get_or_fetch_actor_by_url(url :: String.t(), options :: Keyword.t()) :: {:ok, Actor.t()} | {:error, make_actor_errors} | {:error, :no_internal_relay_actor} | {:error, :url_nil} - def get_or_fetch_actor_by_url(url, preload \\ false) - - def get_or_fetch_actor_by_url(nil, _preload), do: {:error, :url_nil} + def get_or_fetch_actor_by_url(url, options \\ []) def get_or_fetch_actor_by_url("https://www.w3.org/ns/activitystreams#Public", _preload) do %Actor{url: url} = Relay.get_actor() get_or_fetch_actor_by_url(url) end - def get_or_fetch_actor_by_url(url, preload) do + def get_or_fetch_actor_by_url(url, options) when is_binary(url) and is_list(options) do Logger.debug("Getting or fetching actor by URL #{url}") + preload = Keyword.get(options, :preload, false) case Actors.get_actor_by_url(url, preload) do {:ok, %Actor{} = cached_actor} -> if Actors.needs_update?(cached_actor) do - __MODULE__.make_actor_from_url(url, preload: preload) + __MODULE__.make_actor_from_url(url, options) else {:ok, cached_actor} end {:error, :actor_not_found} -> # For tests, see https://github.com/jjh42/mock#not-supported---mocking-internal-function-calls and Mobilizon.Federation.ActivityPubTest - __MODULE__.make_actor_from_url(url, preload: preload) + __MODULE__.make_actor_from_url(url, options) end end + def get_or_fetch_actor_by_url(nil, _preload), do: {:error, :url_nil} + @type make_actor_errors :: Fetcher.fetch_actor_errors() | :actor_is_local @doc """ diff --git a/lib/federation/activity_pub/transmogrifier.ex b/lib/federation/activity_pub/transmogrifier.ex index 0d7f61e8b..40c612c83 100644 --- a/lib/federation/activity_pub/transmogrifier.ex +++ b/lib/federation/activity_pub/transmogrifier.ex @@ -210,7 +210,8 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier do def handle_incoming( %{"type" => "Follow", "object" => followed, "actor" => follower, "id" => id} = _data ) do - with {:ok, %Actor{} = followed} <- ActivityPubActor.get_or_fetch_actor_by_url(followed, true), + with {:ok, %Actor{} = followed} <- + ActivityPubActor.get_or_fetch_actor_by_url(followed, preload: true), {:ok, %Actor{} = follower} <- ActivityPubActor.get_or_fetch_actor_by_url(follower), {:ok, activity, object} <- Actions.Follow.follow(follower, followed, id, false) do diff --git a/lib/federation/http_signatures/signature.ex b/lib/federation/http_signatures/signature.ex index ccb53203b..0427a63e9 100644 --- a/lib/federation/http_signatures/signature.ex +++ b/lib/federation/http_signatures/signature.ex @@ -52,7 +52,8 @@ defmodule Mobilizon.Federation.HTTPSignatures.Signature do {:ok, String.t()} | {:error, :actor_not_found | :pem_decode_error} defp get_public_key_for_url(url) do - with {:ok, %Actor{} = actor} <- ActivityPubActor.get_or_fetch_actor_by_url(url) do + with {:ok, %Actor{} = actor} <- + ActivityPubActor.get_or_fetch_actor_by_url(url, ignore_sign_object_fetches: true) do get_actor_public_key(actor) end end diff --git a/test/federation/activity_pub/actor_test.exs b/test/federation/activity_pub/actor_test.exs index c18a44b54..5298f0e84 100644 --- a/test/federation/activity_pub/actor_test.exs +++ b/test/federation/activity_pub/actor_test.exs @@ -126,7 +126,7 @@ defmodule Mobilizon.Federation.ActivityPub.ActorTest do needs_update?: fn _ -> false end ]}, {ActivityPubActor, [:passthrough], - make_actor_from_url: fn @actor_url, preload: false -> + make_actor_from_url: fn @actor_url, [] -> {:ok, %Actor{ preferred_username: "tcit", @@ -138,7 +138,7 @@ defmodule Mobilizon.Federation.ActivityPub.ActorTest do ActivityPubActor.get_or_fetch_actor_by_url(@actor_url) assert_called(Actors.needs_update?(:_)) - refute called(ActivityPubActor.make_actor_from_url(@actor_url, preload: false)) + refute called(ActivityPubActor.make_actor_from_url(@actor_url, [])) end # Fetch doesn't use cache if Actors.needs_update? returns true @@ -155,7 +155,7 @@ defmodule Mobilizon.Federation.ActivityPub.ActorTest do needs_update?: fn _ -> true end ]}, {ActivityPubActor, [:passthrough], - make_actor_from_url: fn @actor_url, preload: false -> + make_actor_from_url: fn @actor_url, [] -> {:ok, %Actor{ preferred_username: "tcit", @@ -169,7 +169,7 @@ defmodule Mobilizon.Federation.ActivityPub.ActorTest do assert_called(ActivityPubActor.get_or_fetch_actor_by_url(@actor_url)) assert_called(Actors.get_actor_by_url(@actor_url, false)) assert_called(Actors.needs_update?(:_)) - assert_called(ActivityPubActor.make_actor_from_url(@actor_url, preload: false)) + assert_called(ActivityPubActor.make_actor_from_url(@actor_url, [])) end end @@ -182,7 +182,7 @@ defmodule Mobilizon.Federation.ActivityPub.ActorTest do assert match?( {:error, :actor_deleted}, - ActivityPubActor.make_actor_from_url(@actor_url, preload: false) + ActivityPubActor.make_actor_from_url(@actor_url, []) ) end