Fix about a million tests

lib/pleroma/signature.ex

@@ -8,32 +8,24 @@ defmodule Pleroma.Signature do
   alias Pleroma.User
   alias Pleroma.Web.ActivityPub.ActivityPub
   alias Pleroma.User.SigningKey
+  require Logger
 
   def key_id_to_actor_id(key_id) do
     # Given the key ID, first attempt to look it up in the signing keys table.
-    # If it's not found, then attempt to look it up via request to the remote instance.
     case SigningKey.key_id_to_ap_id(key_id) do
       nil ->
-        # this requires us to look up the url!
+        # hm, we SHOULD have gotten this in the pipeline before we hit here!
-        request_key_id_from_remote_instance(key_id)
+        Logger.error("Could not figure out who owns the key #{key_id}")
+        {:error, :key_owner_not_found}
 
       key ->
         {:ok, key}
     end
   end
 
-  def request_key_id_from_remote_instance(key_id) do
-    case SigningKey.fetch_remote_key(key_id) do
-      {:ok, key_id} ->
-        {:ok, key_id}
-
-      {:error, _} ->
-        {:error, "Key ID not found"}
-    end
-  end
-
   def fetch_public_key(conn) do
     with %{"keyId" => kid} <- HTTPSignatures.signature_for_conn(conn),
+         {:ok, %SigningKey{}} <- SigningKey.get_or_fetch_by_key_id(kid),
          {:ok, actor_id} <- key_id_to_actor_id(kid),
          {:ok, public_key} <- User.get_public_key_for_ap_id(actor_id) do
       {:ok, public_key}
@@ -45,8 +37,8 @@ def fetch_public_key(conn) do
 
   def refetch_public_key(conn) do
     with %{"keyId" => kid} <- HTTPSignatures.signature_for_conn(conn),
+         {:ok, %SigningKey{}} <- SigningKey.get_or_fetch_by_key_id(kid),
          {:ok, actor_id} <- key_id_to_actor_id(kid),
-         {:ok, _user} <- ActivityPub.make_user_from_ap_id(actor_id),
          {:ok, public_key} <- User.get_public_key_for_ap_id(actor_id) do
       {:ok, public_key}
     else

lib/pleroma/user/signing_key.ex

@@ -154,6 +154,21 @@ def private_key(%User{signing_key: %__MODULE__{private_key: private_key_pem}}) d
     {:ok, key}
   end
 
+  @spec get_or_fetch_by_key_id(String.t()) :: {:ok, __MODULE__} | {:error, String.t()}
+  @doc """
+  Given a key ID, return the signing key associated with that key.
+  Will either return the key if it exists locally, or fetch it from the remote instance.
+  """
+  def get_or_fetch_by_key_id(key_id) do
+    case key_id_to_user_id(key_id) do
+      nil ->
+        fetch_remote_key(key_id)
+
+      user_id ->
+        {:ok, Repo.get_by(__MODULE__, user_id: user_id)}
+    end
+  end
+
   @spec fetch_remote_key(String.t()) :: {:ok, __MODULE__} | {:error, String.t()}
   @doc """
   Fetch a remote key by key ID.
@@ -164,23 +179,33 @@ def private_key(%User{signing_key: %__MODULE__{private_key: private_key_pem}}) d
   So if we're rejected, we should probably just give up.
   """
   def fetch_remote_key(key_id) do
+    Logger.debug("Fetching remote key: #{key_id}")
     # we should probably sign this, just in case
     resp = Pleroma.Object.Fetcher.get_object(key_id)
 
-    case handle_signature_response(resp) do
+    case resp do
-      {:ok, ap_id, public_key_pem} ->
+      {:ok, _original_url, body} ->
-        # fetch the user
+        case handle_signature_response(resp) do
-        user = User.get_or_fetch_by_ap_id(ap_id)
+          {:ok, ap_id, public_key_pem} ->
-        # store the key
+            Logger.debug("Fetched remote key: #{ap_id}")
-        key = %__MODULE__{
+            # fetch the user
-          user_id: user.id,
+            {:ok, user} = User.get_or_fetch_by_ap_id(ap_id)
-          public_key: public_key_pem,
+            # store the key
-          key_id: key_id
+            key = %__MODULE__{
-        }
+              user_id: user.id,
+              public_key: public_key_pem,
+              key_id: key_id
+            }
 
-        Repo.insert(key)
+            Repo.insert(key, on_conflict: :replace_all, conflict_target: :key_id)
+
+          e ->
+            Logger.debug("Failed to fetch remote key: #{inspect(e)}")
+            {:error, "Could not fetch key"}
+        end
 
       _ ->
+        Logger.debug("Failed to fetch remote key: #{inspect(resp)}")
         {:error, "Could not fetch key"}
     end
   end
@@ -196,7 +221,7 @@ defp extract_key_details(%{"id" => ap_id, "publicKey" => public_key}) do
     end
   end
 
-  defp handle_signature_response({:ok, %{status: status, body: body}}) when status in 200..299 do
+  defp handle_signature_response({:ok, _original_url, body}) do
     case Jason.decode(body) do
       {:ok, %{"id" => _user_id, "publicKey" => _public_key} = body} ->
         extract_key_details(body)

lib/pleroma/web/activity_pub/views/user_view.ex

@@ -67,7 +67,12 @@ def render("user.json", %{user: %User{nickname: "internal." <> _} = user}),
     do: render("service.json", %{user: user}) |> Map.put("preferredUsername", user.nickname)
 
   def render("user.json", %{user: user}) do
-    {:ok, public_key} = User.SigningKey.public_key_pem(user)
+    public_key =
+      case User.SigningKey.public_key_pem(user) do
+        {:ok, public_key} -> public_key
+        _ -> nil
+      end
+
     user = User.sanitize_html(user)
 
     endpoints = render("endpoints.json", %{user: user})

lib/pleroma/web/plugs/ensure_user_public_key_plug.ex

@@ -14,7 +14,7 @@ def call(conn, _opts) do
     key_id = key_id_from_conn(conn)
 
     unless is_nil(key_id) do
-      SigningKey.fetch_remote_key(key_id)
+      User.SigningKey.fetch_remote_key(key_id)
       # now we SHOULD have the user that owns the key locally. maybe.
       # if we don't, we'll error out when we try to validate.
     end

priv/repo/migrations/20240625213637_create_signing_key_table.exs

@@ -10,6 +10,6 @@ def change do
       timestamps()
     end
 
-    create index(:signing_keys, [:key_id])
+    create unique_index(:signing_keys, [:key_id])
   end
 end

test/mix/tasks/pleroma/database_test.exs

@@ -353,7 +353,7 @@ test "with the --keep-threads option it keeps old threads with bookmarked posts"
 
     test "We don't have unexpected tables which may contain objects that are referenced by activities" do
       # We can delete orphaned activities. For that we look for the objects they reference in the 'objects', 'activities', and 'users' table.
-      # If someone adds another table with objects (idk, maybe with separate relations, or collections or w/e), then we need to make sure we 
+      # If someone adds another table with objects (idk, maybe with separate relations, or collections or w/e), then we need to make sure we
       # add logic for that in the 'prune_objects' task so that we don't wrongly delete their corresponding activities.
       # So when someone adds (or removes) a table, this test will fail.
       # Either the table contains objects which can be referenced from the activities table
@@ -401,6 +401,7 @@ test "We don't have unexpected tables which may contain objects that are referen
                ["rich_media_card"],
                ["scheduled_activities"],
                ["schema_migrations"],
+               ["signing_keys"],
                ["thread_mutes"],
                ["user_follows_hashtag"],
                ["user_frontend_setting_profiles"],

test/pleroma/signature_test.exs

@@ -35,25 +35,23 @@ defp make_fake_conn(key_id),
     do: %Plug.Conn{req_headers: %{"signature" => make_fake_signature(key_id <> "#main-key")}}
 
   describe "fetch_public_key/1" do
-    test "it returns key" do
+    test "it returns the key" do
       expected_result = {:ok, @rsa_public_key}
 
-      user = insert(:user, public_key: @public_key)
+      user =
+        insert(:user)
+        |> with_signing_key(public_key: @public_key)
 
       assert Signature.fetch_public_key(make_fake_conn(user.ap_id)) == expected_result
     end
 
-    test "it returns error when not found user" do
-      assert capture_log(fn ->
-               assert Signature.fetch_public_key(make_fake_conn("https://test-ap-id")) ==
-                        {:error, :error}
-             end) =~ "[error] Could not decode user"
-    end
-
     test "it returns error if public key is nil" do
-      user = insert(:user, public_key: nil)
+      # this actually needs the URL to be valid
+      user = insert(:user)
+      key_id = user.ap_id <> "#main-key"
+      Tesla.Mock.mock(fn %{url: ^key_id} -> {:ok, %{status: 404}} end)
 
-      assert Signature.fetch_public_key(make_fake_conn(user.ap_id)) == {:error, :error}
+      assert {:error, _} = Signature.fetch_public_key(make_fake_conn(user.ap_id))
     end
   end
 
@@ -63,12 +61,6 @@ test "it returns key" do
 
       assert Signature.refetch_public_key(make_fake_conn(ap_id)) == {:ok, @rsa_public_key}
     end
-
-    test "it returns error when not found user" do
-      assert capture_log(fn ->
-               {:error, _} = Signature.refetch_public_key(make_fake_conn("https://test-ap_id"))
-             end) =~ "[error] Could not decode user"
-    end
   end
 
   defp split_signature(sig) do
@@ -104,9 +96,9 @@ defp assert_part_equal(part_a, part_b) do
     test "it returns signature headers" do
       user =
         insert(:user, %{
-          ap_id: "https://mastodon.social/users/lambadalambda",
+          ap_id: "https://mastodon.social/users/lambadalambda"
-          keys: @private_key
         })
+        |> with_signing_key(private_key: @private_key)
 
       headers = %{
         host: "test.test",
@@ -121,50 +113,15 @@ test "it returns signature headers" do
         "keyId=\"https://mastodon.social/users/lambadalambda#main-key\",algorithm=\"rsa-sha256\",headers=\"content-length host\",signature=\"sibUOoqsFfTDerquAkyprxzDjmJm6erYc42W5w1IyyxusWngSinq5ILTjaBxFvfarvc7ci1xAi+5gkBwtshRMWm7S+Uqix24Yg5EYafXRun9P25XVnYBEIH4XQ+wlnnzNIXQkU3PU9e6D8aajDZVp3hPJNeYt1gIPOA81bROI8/glzb1SAwQVGRbqUHHHKcwR8keiR/W2h7BwG3pVRy4JgnIZRSW7fQogKedDg02gzRXwUDFDk0pr2p3q6bUWHUXNV8cZIzlMK+v9NlyFbVYBTHctAR26GIAN6Hz0eV0mAQAePHDY1mXppbA8Gpp6hqaMuYfwifcXmcc+QFm4e+n3A==\""
       )
     end
-
-    test "it returns error" do
-      user = insert(:user, %{ap_id: "https://mastodon.social/users/lambadalambda", keys: ""})
-
-      assert Signature.sign(
-               user,
-               %{host: "test.test", "content-length": "100"}
-             ) == {:error, []}
-    end
   end
 
   describe "key_id_to_actor_id/1" do
-    test "it properly deduces the actor id for misskey" do
+    test "it reverses the key id to actor id" do
-      assert Signature.key_id_to_actor_id("https://example.com/users/1234/publickey") ==
+      user =
-               {:ok, "https://example.com/users/1234"}
+        insert(:user)
-    end
+        |> with_signing_key()
 
-    test "it properly deduces the actor id for mastodon and pleroma" do
+      assert Signature.key_id_to_actor_id(user.signing_key.key_id) == {:ok, user.ap_id}
-      assert Signature.key_id_to_actor_id("https://example.com/users/1234#main-key") ==
-               {:ok, "https://example.com/users/1234"}
-    end
-
-    test "it deduces the actor id for gotoSocial" do
-      assert Signature.key_id_to_actor_id("https://example.com/users/1234/main-key") ==
-               {:ok, "https://example.com/users/1234"}
-    end
-
-    test "it deduces the actor ID for streams" do
-      assert Signature.key_id_to_actor_id("https://example.com/users/1234?operation=getkey") ==
-               {:ok, "https://example.com/users/1234"}
-    end
-
-    test "it deduces the actor ID for bridgy" do
-      assert Signature.key_id_to_actor_id("https://example.com/1234#key") ==
-               {:ok, "https://example.com/1234"}
-    end
-
-    test "it calls webfinger for 'acct:' accounts" do
-      with_mock(Pleroma.Web.WebFinger,
-        finger: fn _ -> {:ok, %{"ap_id" => "https://gensokyo.2hu/users/raymoo"}} end
-      ) do
-        assert Signature.key_id_to_actor_id("acct:raymoo@gensokyo.2hu") ==
-                 {:ok, "https://gensokyo.2hu/users/raymoo"}
-      end
     end
   end
 

test/pleroma/user_search_test.exs

@@ -259,7 +259,7 @@ test "works with URIs" do
         |> Map.put(:multi_factor_authentication_settings, nil)
         |> Map.put(:notification_settings, nil)
 
-      assert user == expected
+        assert_user_match(user, expected)
     end
 
     test "excludes a blocked users from search result" do

test/pleroma/web/activity_pub/activity_pub_controller_test.exs

@@ -645,7 +645,7 @@ test "accept follow activity", %{conn: conn} do
       assert "ok" ==
                conn
                |> assign(:valid_signature, true)
-               |> put_req_header("signature", "keyId=\"#{followed_relay.ap_id}/main-key\"")
+               |> put_req_header("signature", "keyId=\"#{followed_relay.ap_id}#main-key\"")
                |> put_req_header("content-type", "application/activity+json")
                |> post("/inbox", accept)
                |> json_response(200)
@@ -682,6 +682,7 @@ test "accepts Add/Remove activities", %{conn: conn} do
         |> String.replace("{{nickname}}", "lain")
 
       actor = "https://example.com/users/lain"
+      key_id = "#{actor}/main-key"
 
       insert(:user,
         ap_id: actor,
@@ -709,6 +710,16 @@ test "accepts Add/Remove activities", %{conn: conn} do
             headers: [{"content-type", "application/activity+json"}]
           }
 
+          %{
+            method: :get,
+            url: ^key_id
+          } ->
+            %Tesla.Env{
+              status: 200,
+              body: user,
+              headers: [{"content-type", "application/activity+json"}]
+            }
+
         %{method: :get, url: "https://example.com/users/lain/collections/featured"} ->
           %Tesla.Env{
             status: 200,
@@ -782,6 +793,7 @@ test "mastodon pin/unpin", %{conn: conn} do
         |> String.replace("{{nickname}}", "lain")
 
       actor = "https://example.com/users/lain"
+      key_id = "#{actor}/main-key"
 
       sender =
         insert(:user,
@@ -811,6 +823,15 @@ test "mastodon pin/unpin", %{conn: conn} do
             headers: [{"content-type", "application/activity+json"}]
           }
 
+          %{
+            method: :get,
+            url: ^key_id
+          } ->
+            %Tesla.Env{
+              status: 200,
+              body: user,
+              headers: [{"content-type", "application/activity+json"}]
+            }
         %{method: :get, url: "https://example.com/users/lain/collections/featured"} ->
           %Tesla.Env{
             status: 200,
@@ -907,6 +928,7 @@ test "it inserts an incoming activity into the database", %{conn: conn, data: da
 
     test "it accepts messages with to as string instead of array", %{conn: conn, data: data} do
       user = insert(:user)
+      |> with_signing_key()
 
       data =
         data
@@ -952,6 +974,7 @@ test "it accepts messages with cc as string instead of array", %{conn: conn, dat
 
     test "it accepts messages with bcc as string instead of array", %{conn: conn, data: data} do
       user = insert(:user)
+      |> with_signing_key()
 
       data =
         data
@@ -1273,7 +1296,7 @@ test "forwarded report from mastodon", %{conn: conn} do
 
       conn
       |> assign(:valid_signature, true)
-      |> put_req_header("signature", "keyId=\"#{remote_actor}/main-key\"")
+      |> put_req_header("signature", "keyId=\"#{remote_actor}#main-key\"")
       |> put_req_header("content-type", "application/activity+json")
       |> post("/users/#{reported_user.nickname}/inbox", data)
       |> json_response(200)

test/pleroma/web/activity_pub/publisher_test.exs

@@ -141,6 +141,7 @@ test "publish to url with with different ports" do
       end)
 
       actor = insert(:user)
+      |> with_signing_key()
 
       assert {:ok, %{body: "port 42"}} =
                Publisher.publish_one(%{
@@ -166,6 +167,7 @@ test "publish to url with with different ports" do
                    [:passthrough],
                    [] do
       actor = insert(:user)
+      |> with_signing_key()
       inbox = "http://200.site/users/nick1/inbox"
 
       assert {:ok, _} = Publisher.publish_one(%{inbox: inbox, json: "{}", actor: actor, id: 1})
@@ -177,6 +179,7 @@ test "publish to url with with different ports" do
                    [:passthrough],
                    [] do
       actor = insert(:user)
+      |> with_signing_key()
       inbox = "http://200.site/users/nick1/inbox"
 
       assert {:ok, _} =
@@ -196,6 +199,7 @@ test "publish to url with with different ports" do
                    [:passthrough],
                    [] do
       actor = insert(:user)
+      |> with_signing_key()
       inbox = "http://200.site/users/nick1/inbox"
 
       assert {:ok, _} =
@@ -215,6 +219,7 @@ test "publish to url with with different ports" do
                    [:passthrough],
                    [] do
       actor = insert(:user)
+      |> with_signing_key()
       inbox = "http://404.site/users/nick1/inbox"
 
       assert {:error, _} = Publisher.publish_one(%{inbox: inbox, json: "{}", actor: actor, id: 1})
@@ -227,6 +232,7 @@ test "publish to url with with different ports" do
                    [:passthrough],
                    [] do
       actor = insert(:user)
+      |> with_signing_key()
       inbox = "http://connrefused.site/users/nick1/inbox"
 
       assert capture_log(fn ->
@@ -242,6 +248,7 @@ test "publish to url with with different ports" do
                    [:passthrough],
                    [] do
       actor = insert(:user)
+      |> with_signing_key()
       inbox = "http://200.site/users/nick1/inbox"
 
       assert {:ok, _} = Publisher.publish_one(%{inbox: inbox, json: "{}", actor: actor, id: 1})
@@ -254,6 +261,7 @@ test "publish to url with with different ports" do
                    [:passthrough],
                    [] do
       actor = insert(:user)
+      |> with_signing_key()
       inbox = "http://connrefused.site/users/nick1/inbox"
 
       assert capture_log(fn ->
@@ -295,6 +303,7 @@ test "publish to url with with different ports" do
         })
 
       actor = insert(:user, follower_address: follower.ap_id)
+      |> with_signing_key()
 
       {:ok, follower, actor} = Pleroma.User.follow(follower, actor)
       {:ok, _another_follower, actor} = Pleroma.User.follow(another_follower, actor)
@@ -366,6 +375,7 @@ test "publish to url with with different ports" do
         })
 
       actor = insert(:user, follower_address: follower.ap_id)
+      |> with_signing_key()
 
       {:ok, follower, actor} = Pleroma.User.follow(follower, actor)
       actor = refresh_record(actor)

test/pleroma/web/plugs/http_signature_plug_test.exs

@@ -17,8 +17,8 @@ defmodule Pleroma.Web.Plugs.HTTPSignaturePlugTest do
   setup do
     user =
       :user
-      |> insert(%{ ap_id: "http://mastodon.example.org/users/admin" })
+      |> insert(%{ap_id: "http://mastodon.example.org/users/admin"})
-      |> with_signing_key(%{ key_id: "http://mastodon.example.org/users/admin#main-key" })
+      |> with_signing_key(%{key_id: "http://mastodon.example.org/users/admin#main-key"})
 
     {:ok, %{user: user}}
   end

test/pleroma/web/plugs/mapped_signature_to_identity_plug_test.exs

@@ -14,7 +14,11 @@ defmodule Pleroma.Web.Plugs.MappedSignatureToIdentityPlugTest do
 
   setup do
     mock(fn env -> apply(HttpRequestMock, :request, [env]) end)
-    user = insert(:user)
+
+    user =
+      insert(:user)
+      |> with_signing_key()
+
     {:ok, %{user: user}}
   end
 

test/support/factory.ex

@@ -96,9 +96,9 @@ def user_factory(attrs \\ %{}) do
   end
 
   def with_signing_key(%User{} = user, attrs \\ %{}) do
-
+    signing_key =
-    signing_key = build(:signing_key, %{user: user, key_id: "#{user.ap_id}#main-key"})
+      build(:signing_key, %{user: user, key_id: "#{user.ap_id}#main-key"})
-    |> merge_attributes(attrs)
+      |> merge_attributes(attrs)
 
     insert(signing_key)
     %{user | signing_key: signing_key}
@@ -111,8 +111,8 @@ def signing_key_factory(attrs \\ %{}) do
 
     %Pleroma.User.SigningKey{
       user_id: user.id,
-      public_key: public_key,
+      public_key: attrs[:public_key] || public_key,
-      private_key: pem,
+      private_key: attrs[:private_key] || pem,
       key_id: attrs[:key_id]
     }
   end

test/support/helpers.ex

@@ -65,6 +65,7 @@ defmacro __using__(_opts) do
           clear_config: 1,
           clear_config: 2
         ]
+      import Pleroma.Test.MatchingHelpers
 
       def time_travel(entity, seconds) do
         new_time = NaiveDateTime.add(entity.inserted_at, seconds)

test/support/http_request_mock.ex

@@ -424,6 +424,15 @@ def get("http://mastodon.example.org/users/admin", _, _, _) do
      }}
   end
 
+  def get("http://mastodon.example.org/users/admin/main-key", _, _, _) do
+    {:ok,
+     %Tesla.Env{
+       status: 200,
+       body: File.read!("test/fixtures/tesla_mock/admin@mastdon.example.org.json"),
+       headers: activitypub_object_headers()
+     }}
+  end
+
   def get(
         "http://mastodon.example.org/users/admin/statuses/99512778738411822/replies?min_id=99512778738411824&page=true",
         _,
@@ -958,6 +967,15 @@ def get("https://mastodon.social/users/lambadalambda", _, _, _) do
      }}
   end
 
+  def get("https://mastodon.social/users/lambadalambda#main-key", _, _, _) do
+    {:ok,
+     %Tesla.Env{
+       status: 200,
+       body: File.read!("test/fixtures/lambadalambda.json"),
+       headers: activitypub_object_headers()
+     }}
+  end
+
   def get("https://mastodon.social/users/lambadalambda/collections/featured", _, _, _) do
     {:ok,
      %Tesla.Env{
@@ -1403,6 +1421,15 @@ def get("https://relay.mastodon.host/actor", _, _, _) do
      }}
   end
 
+  def get("https://relay.mastodon.host/actor#main-key", _, _, _) do
+    {:ok,
+     %Tesla.Env{
+       status: 200,
+       body: File.read!("test/fixtures/relay/relay.json"),
+       headers: activitypub_object_headers()
+     }}
+  end
+
   def get("http://localhost:4001/", _, "", [{"accept", "text/html"}]) do
     {:ok, %Tesla.Env{status: 200, body: File.read!("test/fixtures/tesla_mock/7369654.html")}}
   end

test/support/matching_helpers.ex

@@ -0,0 +1,9 @@
+defmodule Pleroma.Test.MatchingHelpers do
+  import ExUnit.Assertions
+  @assoc_fields [
+    :signing_key
+  ]
+  def assert_user_match(actor1, actor2) do
+    assert Ecto.reset_fields(actor1, @assoc_fields) == Ecto.reset_fields(actor2, @assoc_fields)
+  end
+end