Merge branch '1937-renaming' into 'develop'

ActivityPub: Don't rename a clashing nickname with the same ap id.

Closes #1937

See merge request pleroma/pleroma!2748

lib/pleroma/web/activity_pub/activity_pub.ex

@@ -1376,13 +1376,28 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do
     end
   end
 
-  def maybe_handle_clashing_nickname(nickname) do
-    with %User{} = old_user <- User.get_by_nickname(nickname) do
-      Logger.info("Found an old user for #{nickname}, ap id is #{old_user.ap_id}, renaming.")
+  def maybe_handle_clashing_nickname(data) do
+    nickname = data[:nickname]
+
+    with %User{} = old_user <- User.get_by_nickname(nickname),
+         {_, false} <- {:ap_id_comparison, data[:ap_id] == old_user.ap_id} do
+      Logger.info(
+        "Found an old user for #{nickname}, the old ap id is #{old_user.ap_id}, new one is #{
+          data[:ap_id]
+        }, renaming."
+      )
 
       old_user
       |> User.remote_user_changeset(%{nickname: "#{old_user.id}.#{old_user.nickname}"})
       |> User.update_and_set_cache()
+    else
+      {:ap_id_comparison, true} ->
+        Logger.info(
+          "Found an old user for #{nickname}, but the ap id #{data[:ap_id]} is the same as the new user. Race condition? Not changing anything."
+        )
+
+      _ ->
+        nil
     end
   end
 
@@ -1398,7 +1413,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do
           |> User.remote_user_changeset(data)
           |> User.update_and_set_cache()
         else
-          maybe_handle_clashing_nickname(data[:nickname])
+          maybe_handle_clashing_nickname(data)
 
           data
           |> User.remote_user_changeset()

test/web/activity_pub/activity_pub_test.exs

@@ -2056,4 +2056,46 @@ defmodule Pleroma.Web.ActivityPub.ActivityPubTest do
       assert [%{activity_id: ^id_create}] = Pleroma.ActivityExpiration |> Repo.all()
     end
   end
+
+  describe "handling of clashing nicknames" do
+    test "renames an existing user with a clashing nickname and a different ap id" do
+      orig_user =
+        insert(
+          :user,
+          local: false,
+          nickname: "admin@mastodon.example.org",
+          ap_id: "http://mastodon.example.org/users/harinezumigari"
+        )
+
+      %{
+        nickname: orig_user.nickname,
+        ap_id: orig_user.ap_id <> "part_2"
+      }
+      |> ActivityPub.maybe_handle_clashing_nickname()
+
+      user = User.get_by_id(orig_user.id)
+
+      assert user.nickname == "#{orig_user.id}.admin@mastodon.example.org"
+    end
+
+    test "does nothing with a clashing nickname and the same ap id" do
+      orig_user =
+        insert(
+          :user,
+          local: false,
+          nickname: "admin@mastodon.example.org",
+          ap_id: "http://mastodon.example.org/users/harinezumigari"
+        )
+
+      %{
+        nickname: orig_user.nickname,
+        ap_id: orig_user.ap_id
+      }
+      |> ActivityPub.maybe_handle_clashing_nickname()
+
+      user = User.get_by_id(orig_user.id)
+
+      assert user.nickname == orig_user.nickname
+    end
+  end
 end