User: generate private keys on user creation

This fixes a race condition bug where keys could be regenerated
post-federation, causing activities and HTTP signatures from an user to
be dropped due to key differences.

lib/pleroma/signature.ex

@@ -66,9 +66,8 @@ def refetch_public_key(conn) do
     end
   end
 
-  def sign(%User{} = user, headers) do
+  def sign(%User{keys: keys} = user, headers) do
-    with {:ok, %{keys: keys}} <- User.ensure_keys_present(user),
+    with {:ok, private_key, _} <- Keys.keys_from_pem(keys) do
-         {:ok, private_key, _} <- Keys.keys_from_pem(keys) do
       HTTPSignatures.sign(private_key, user.ap_id <> "#main-key", headers)
     end
   end

lib/pleroma/user.ex

@@ -681,9 +681,9 @@ def register_changeset_ldap(struct, params = %{password: password})
     |> validate_exclusion(:nickname, Config.get([User, :restricted_nicknames]))
     |> validate_format(:nickname, local_nickname_regex())
     |> put_ap_id()
-    |> put_keys()
     |> unique_constraint(:ap_id)
     |> put_following_and_follower_and_featured_address()
+    |> put_private_key()
   end
 
   def register_changeset(struct, params \\ %{}, opts \\ []) do
@@ -741,10 +741,10 @@ def register_changeset(struct, params \\ %{}, opts \\ []) do
     |> validate_length(:registration_reason, max: reason_limit)
     |> maybe_validate_required_email(opts[:external])
     |> put_password_hash
-    |> put_keys()
     |> put_ap_id()
     |> unique_constraint(:ap_id)
     |> put_following_and_follower_and_featured_address()
+    |> put_private_key()
   end
 
   def maybe_validate_required_email(changeset, true), do: changeset
@@ -757,11 +757,6 @@ def maybe_validate_required_email(changeset, _) do
     end
   end
 
-  def put_keys(changeset) do
-    {:ok, pem} = Keys.generate_rsa_pem()
-    put_change(changeset, :keys, pem)
-  end
-
   def put_ap_id(changeset) do
     ap_id = ap_id(%User{nickname: get_field(changeset, :nickname)})
     put_change(changeset, :ap_id, ap_id)
@@ -779,6 +774,11 @@ def put_following_and_follower_and_featured_address(changeset) do
     |> put_change(:featured_address, featured)
   end
 
+  defp put_private_key(changeset) do
+    {:ok, pem} = Keys.generate_rsa_pem()
+    put_change(changeset, :keys, pem)
+  end
+
   defp autofollow_users(user) do
     candidates = Config.get([:instance, :autofollowed_nicknames])
 
@@ -1955,6 +1955,7 @@ defp create_service_actor(uri, nickname) do
       follower_address: uri <> "/followers"
     }
     |> change
+    |> put_private_key()
     |> unique_constraint(:nickname)
     |> Repo.insert()
     |> set_cache()
@@ -2220,17 +2221,6 @@ def get_mascot(%{mascot: mascot}) when is_nil(mascot) do
     }
   end
 
-  def ensure_keys_present(%{keys: keys} = user) when not is_nil(keys), do: {:ok, user}
-
-  def ensure_keys_present(%User{} = user) do
-    with {:ok, pem} <- Keys.generate_rsa_pem() do
-      user
-      |> cast(%{keys: pem}, [:keys])
-      |> validate_required([:keys])
-      |> update_and_set_cache()
-    end
-  end
-
   def get_ap_ids_by_nicknames(nicknames) do
     from(u in User,
       where: u.nickname in ^nicknames,

lib/pleroma/web/activity_pub/activity_pub_controller.ex

@@ -66,8 +66,7 @@ defp relay_active?(conn, _) do
   end
 
   def user(conn, %{"nickname" => nickname}) do
-    with %User{local: true} = user <- User.get_cached_by_nickname(nickname),
+    with %User{local: true} = user <- User.get_cached_by_nickname(nickname) do
-         {:ok, user} <- User.ensure_keys_present(user) do
       conn
       |> put_resp_content_type("application/activity+json")
       |> put_view(UserView)
@@ -174,7 +173,6 @@ def relay_following(conn, _params) do
 
   def following(%{assigns: %{user: for_user}} = conn, %{"nickname" => nickname, "page" => page}) do
     with %User{} = user <- User.get_cached_by_nickname(nickname),
-         {user, for_user} <- ensure_user_keys_present_and_maybe_refresh_for_user(user, for_user),
          {:show_follows, true} <-
            {:show_follows, (for_user && for_user == user) || !user.hide_follows} do
       {page, _} = Integer.parse(page)
@@ -192,8 +190,7 @@ def following(%{assigns: %{user: for_user}} = conn, %{"nickname" => nickname, "p
   end
 
   def following(%{assigns: %{user: for_user}} = conn, %{"nickname" => nickname}) do
-    with %User{} = user <- User.get_cached_by_nickname(nickname),
+    with %User{} = user <- User.get_cached_by_nickname(nickname) do
-         {user, for_user} <- ensure_user_keys_present_and_maybe_refresh_for_user(user, for_user) do
       conn
       |> put_resp_content_type("application/activity+json")
       |> put_view(UserView)
@@ -213,7 +210,6 @@ def relay_followers(conn, _params) do
 
   def followers(%{assigns: %{user: for_user}} = conn, %{"nickname" => nickname, "page" => page}) do
     with %User{} = user <- User.get_cached_by_nickname(nickname),
-         {user, for_user} <- ensure_user_keys_present_and_maybe_refresh_for_user(user, for_user),
          {:show_followers, true} <-
            {:show_followers, (for_user && for_user == user) || !user.hide_followers} do
       {page, _} = Integer.parse(page)
@@ -231,8 +227,7 @@ def followers(%{assigns: %{user: for_user}} = conn, %{"nickname" => nickname, "p
   end
 
   def followers(%{assigns: %{user: for_user}} = conn, %{"nickname" => nickname}) do
-    with %User{} = user <- User.get_cached_by_nickname(nickname),
+    with %User{} = user <- User.get_cached_by_nickname(nickname) do
-         {user, for_user} <- ensure_user_keys_present_and_maybe_refresh_for_user(user, for_user) do
       conn
       |> put_resp_content_type("application/activity+json")
       |> put_view(UserView)
@@ -245,8 +240,7 @@ def outbox(
         %{"nickname" => nickname, "page" => page?} = params
       )
       when page? in [true, "true"] do
-    with %User{} = user <- User.get_cached_by_nickname(nickname),
+    with %User{} = user <- User.get_cached_by_nickname(nickname) do
-         {:ok, user} <- User.ensure_keys_present(user) do
       # "include_poll_votes" is a hack because postgres generates inefficient
       # queries when filtering by 'Answer', poll votes will be hidden by the
       # visibility filter in this case anyway
@@ -270,8 +264,7 @@ def outbox(
   end
 
   def outbox(conn, %{"nickname" => nickname}) do
-    with %User{} = user <- User.get_cached_by_nickname(nickname),
+    with %User{} = user <- User.get_cached_by_nickname(nickname) do
-         {:ok, user} <- User.ensure_keys_present(user) do
       conn
       |> put_resp_content_type("application/activity+json")
       |> put_view(UserView)
@@ -328,14 +321,10 @@ defp post_inbox_relayed_create(conn, params) do
   end
 
   defp represent_service_actor(%User{} = user, conn) do
-    with {:ok, user} <- User.ensure_keys_present(user) do
     conn
     |> put_resp_content_type("application/activity+json")
     |> put_view(UserView)
     |> render("user.json", %{user: user})
-    else
-      nil -> {:error, :not_found}
-    end
   end
 
   defp represent_service_actor(nil, _), do: {:error, :not_found}
@@ -388,13 +377,11 @@ def read_inbox(
   def read_inbox(%{assigns: %{user: %User{nickname: nickname} = user}} = conn, %{
         "nickname" => nickname
       }) do
-    with {:ok, user} <- User.ensure_keys_present(user) do
     conn
     |> put_resp_content_type("application/activity+json")
     |> put_view(UserView)
     |> render("activity_collection.json", %{iri: "#{user.ap_id}/inbox"})
   end
-  end
 
   def read_inbox(%{assigns: %{user: %User{nickname: as_nickname}}} = conn, %{
         "nickname" => nickname
@@ -530,19 +517,6 @@ defp set_requester_reachable(%Plug.Conn{} = conn, _) do
     conn
   end
 
-  defp ensure_user_keys_present_and_maybe_refresh_for_user(user, for_user) do
-    {:ok, new_user} = User.ensure_keys_present(user)
-
-    for_user =
-      if new_user != user and match?(%User{}, for_user) do
-        User.get_cached_by_nickname(for_user.nickname)
-      else
-        for_user
-      end
-
-    {new_user, for_user}
-  end
-
   def upload_media(%{assigns: %{user: %User{} = user}} = conn, %{"file" => file} = data) do
     with {:ok, object} <-
            ActivityPub.upload(

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

@@ -34,7 +34,6 @@ def render("endpoints.json", %{user: %User{local: true} = _user}) do
   def render("endpoints.json", _), do: %{}
 
   def render("service.json", %{user: user}) do
-    {:ok, user} = User.ensure_keys_present(user)
     {:ok, _, public_key} = Keys.keys_from_pem(user.keys)
     public_key = :public_key.pem_entry_encode(:SubjectPublicKeyInfo, public_key)
     public_key = :public_key.pem_encode([public_key])
@@ -71,7 +70,6 @@ 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, user} = User.ensure_keys_present(user)
     {:ok, _, public_key} = Keys.keys_from_pem(user.keys)
     public_key = :public_key.pem_entry_encode(:SubjectPublicKeyInfo, public_key)
     public_key = :public_key.pem_encode([public_key])

lib/pleroma/web/federator.ex

@@ -69,11 +69,9 @@ def perform(:publish_one, module, params) do
   def perform(:publish, activity) do
     Logger.debug(fn -> "Running publish for #{activity.data["id"]}" end)
 
-    with %User{} = actor <- User.get_cached_by_ap_id(activity.data["actor"]),
+    %User{} = actor = User.get_cached_by_ap_id(activity.data["actor"])
-         {:ok, actor} <- User.ensure_keys_present(actor) do
     Publisher.publish(actor, activity)
   end
-  end
 
   def perform(:incoming_ap_doc, params) do
     Logger.debug("Handling incoming AP activity")

lib/pleroma/web/web_finger.ex

@@ -69,8 +69,6 @@ defp gather_aliases(%User{} = user) do
   end
 
   def represent_user(user, "JSON") do
-    {:ok, user} = User.ensure_keys_present(user)
-
     %{
       "subject" => "acct:#{user.nickname}@#{domain()}",
       "aliases" => gather_aliases(user),
@@ -79,8 +77,6 @@ def represent_user(user, "JSON") do
   end
 
   def represent_user(user, "XML") do
-    {:ok, user} = User.ensure_keys_present(user)
-
     aliases =
       user
       |> gather_aliases()

test/pleroma/user_test.exs

@@ -620,15 +620,15 @@ test "it blocks blacklisted email domains" do
       assert changeset.valid?
     end
 
-    test "it sets the password_hash, ap_id and PEM key" do
+    test "it sets the password_hash, ap_id, private key and followers collection address" do
       changeset = User.register_changeset(%User{}, @full_user_data)
 
       assert changeset.valid?
 
       assert is_binary(changeset.changes[:password_hash])
+      assert is_binary(changeset.changes[:keys])
       assert changeset.changes[:ap_id] == User.ap_id(%User{nickname: @full_user_data.nickname})
       assert is_binary(changeset.changes[:keys])
-
       assert changeset.changes.follower_address == "#{changeset.changes.ap_id}/followers"
     end
 
@@ -2130,21 +2130,6 @@ test "Only includes users with no read notifications" do
     end
   end
 
-  describe "ensure_keys_present" do
-    test "it creates keys for a user and stores them in info" do
-      user = insert(:user)
-      refute is_binary(user.keys)
-      {:ok, user} = User.ensure_keys_present(user)
-      assert is_binary(user.keys)
-    end
-
-    test "it doesn't create keys if there already are some" do
-      user = insert(:user, keys: "xxx")
-      {:ok, user} = User.ensure_keys_present(user)
-      assert user.keys == "xxx"
-    end
-  end
-
   describe "get_ap_ids_by_nicknames" do
     test "it returns a list of AP ids for a given set of nicknames" do
       user = insert(:user)

test/pleroma/web/activity_pub/views/user_view_test.exs

@@ -12,7 +12,6 @@ defmodule Pleroma.Web.ActivityPub.UserViewTest do
 
   test "Renders a user, including the public key" do
     user = insert(:user)
-    {:ok, user} = User.ensure_keys_present(user)
 
     result = UserView.render("user.json", %{user: user})
 
@@ -55,7 +54,6 @@ test "Renders with emoji tags" do
 
   test "Does not add an avatar image if the user hasn't set one" do
     user = insert(:user)
-    {:ok, user} = User.ensure_keys_present(user)
 
     result = UserView.render("user.json", %{user: user})
     refute result["icon"]
@@ -67,8 +65,6 @@ test "Does not add an avatar image if the user hasn't set one" do
         banner: %{"url" => [%{"href" => "https://somebanner"}]}
       )
 
-    {:ok, user} = User.ensure_keys_present(user)
-
     result = UserView.render("user.json", %{user: user})
     assert result["icon"]["url"] == "https://someurl"
     assert result["image"]["url"] == "https://somebanner"
@@ -89,7 +85,6 @@ test "renders AKAs" do
   describe "endpoints" do
     test "local users have a usable endpoints structure" do
       user = insert(:user)
-      {:ok, user} = User.ensure_keys_present(user)
 
       result = UserView.render("user.json", %{user: user})
 
@@ -105,7 +100,6 @@ test "local users have a usable endpoints structure" do
 
     test "remote users have an empty endpoints structure" do
       user = insert(:user, local: false)
-      {:ok, user} = User.ensure_keys_present(user)
 
       result = UserView.render("user.json", %{user: user})
 
@@ -115,7 +109,6 @@ test "remote users have an empty endpoints structure" do
 
     test "instance users do not expose oAuth endpoints" do
       user = insert(:user, nickname: nil, local: true)
-      {:ok, user} = User.ensure_keys_present(user)
 
       result = UserView.render("user.json", %{user: user})
 

test/support/factory.ex

@@ -7,6 +7,7 @@ defmodule Pleroma.Factory do
 
   require Pleroma.Constants
 
+  alias Pleroma.Keys
   alias Pleroma.Object
   alias Pleroma.User
 
@@ -28,6 +29,8 @@ def conversation_factory do
   end
 
   def user_factory(attrs \\ %{}) do
+    {:ok, pem} = Keys.generate_rsa_pem()
+
     user = %User{
       name: sequence(:name, &"Test テスト User #{&1}"),
       email: sequence(:email, &"user#{&1}@example.com"),
@@ -39,7 +42,8 @@ def user_factory(attrs \\ %{}) do
       last_refreshed_at: NaiveDateTime.utc_now(),
       notification_settings: %Pleroma.User.NotificationSetting{},
       multi_factor_authentication_settings: %Pleroma.MFA.Settings{},
-      ap_enabled: true
+      ap_enabled: true,
+      keys: pem
     }
 
     urls =