From c077ad0b3305e74f5b8d1b9bf38d4f480d76c1a6 Mon Sep 17 00:00:00 2001 From: rinpatch Date: Sat, 11 Apr 2020 21:44:52 +0300 Subject: [PATCH 1/2] Remove User.upgrade_changeset in favor of remote_user_creation The two changesets had the same purpose, yet some changes were updated in one, but not the other (`uri`, for example). Also makes `Transmogrifier.upgrade_user_from_ap_id` be called from `ActivityPub.make_user_from_ap_id` only when the user is actually not AP enabled yet. I did not bother rewriting tests that used `User.insert_or_update` to use the changeset instead because they seemed to just test the implementation, rather than behavior. --- lib/pleroma/user.ex | 60 ++--------------- lib/pleroma/web/activity_pub/activity_pub.ex | 15 ++++- .../web/activity_pub/transmogrifier.ex | 14 ++-- test/user_test.exs | 64 ++----------------- .../web/activity_pub/views/user_view_test.exs | 2 +- 5 files changed, 31 insertions(+), 124 deletions(-) diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index 71c8c3a4e..fab405233 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -339,18 +339,20 @@ defp truncate_if_exists(params, key, max_length) do end end - def remote_user_creation(params) do + def remote_user_changeset(struct \\ %User{local: false}, params) do bio_limit = Pleroma.Config.get([:instance, :user_bio_length], 5000) name_limit = Pleroma.Config.get([:instance, :user_name_length], 100) params = params + |> Map.put(:name, blank?(params[:name]) || params[:nickname]) + |> Map.put_new(:last_refreshed_at, NaiveDateTime.utc_now()) |> truncate_if_exists(:name, name_limit) |> truncate_if_exists(:bio, bio_limit) |> truncate_fields_param() changeset = - %User{local: false} + struct |> cast( params, [ @@ -375,7 +377,8 @@ def remote_user_creation(params) do :discoverable, :invisible, :actor_type, - :also_known_as + :also_known_as, + :last_refreshed_at ] ) |> validate_required([:name, :ap_id]) @@ -488,49 +491,6 @@ defp put_upload(value, type) do end end - def upgrade_changeset(struct, params \\ %{}, remote? \\ false) do - bio_limit = Pleroma.Config.get([:instance, :user_bio_length], 5000) - name_limit = Pleroma.Config.get([:instance, :user_name_length], 100) - - params = Map.put(params, :last_refreshed_at, NaiveDateTime.utc_now()) - - params = if remote?, do: truncate_fields_param(params), else: params - - struct - |> cast( - params, - [ - :bio, - :name, - :follower_address, - :following_address, - :avatar, - :last_refreshed_at, - :ap_enabled, - :source_data, - :banner, - :locked, - :magic_key, - :follower_count, - :following_count, - :hide_follows, - :fields, - :hide_followers, - :allow_following_move, - :discoverable, - :hide_followers_count, - :hide_follows_count, - :actor_type, - :also_known_as - ] - ) - |> unique_constraint(:nickname) - |> validate_format(:nickname, local_nickname_regex()) - |> validate_length(:bio, max: bio_limit) - |> validate_length(:name, max: name_limit) - |> validate_fields(remote?) - end - def update_as_admin_changeset(struct, params) do struct |> update_changeset(params) @@ -1642,14 +1602,6 @@ def get_public_key_for_ap_id(ap_id) do defp blank?(""), do: nil defp blank?(n), do: n - def insert_or_update_user(data) do - data - |> Map.put(:name, blank?(data[:name]) || data[:nickname]) - |> remote_user_creation() - |> Repo.insert(on_conflict: {:replace_all_except, [:id]}, conflict_target: :nickname) - |> set_cache() - end - def ap_enabled?(%User{local: true}), do: true def ap_enabled?(%User{ap_enabled: ap_enabled}), do: ap_enabled def ap_enabled?(_), do: false diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index 86b105b7f..2602b966b 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -1551,11 +1551,22 @@ def fetch_and_prepare_user_from_ap_id(ap_id) do end def make_user_from_ap_id(ap_id) do - if _user = User.get_cached_by_ap_id(ap_id) do + user = User.get_cached_by_ap_id(ap_id) + + if user && !User.ap_enabled?(user) do Transmogrifier.upgrade_user_from_ap_id(ap_id) else with {:ok, data} <- fetch_and_prepare_user_from_ap_id(ap_id) do - User.insert_or_update_user(data) + if user do + user + |> User.remote_user_changeset(data) + |> User.update_and_set_cache() + else + data + |> User.remote_user_changeset() + |> Repo.insert() + |> User.set_cache() + end else e -> {:error, e} end diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index f9951cc5d..18fd56bed 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -710,7 +710,7 @@ def handle_incoming( {:ok, new_user_data} = ActivityPub.user_data_from_user_object(object) actor - |> User.upgrade_changeset(new_user_data, true) + |> User.remote_user_changeset(new_user_data) |> User.update_and_set_cache() ActivityPub.update(%{ @@ -1253,12 +1253,8 @@ def perform(:user_upgrade, user) do def upgrade_user_from_ap_id(ap_id) do with %User{local: false} = user <- User.get_cached_by_ap_id(ap_id), {:ok, data} <- ActivityPub.fetch_and_prepare_user_from_ap_id(ap_id), - already_ap <- User.ap_enabled?(user), - {:ok, user} <- upgrade_user(user, data) do - if not already_ap do - TransmogrifierWorker.enqueue("user_upgrade", %{"user_id" => user.id}) - end - + {:ok, user} <- update_user(user, data) do + TransmogrifierWorker.enqueue("user_upgrade", %{"user_id" => user.id}) {:ok, user} else %User{} = user -> {:ok, user} @@ -1266,9 +1262,9 @@ def upgrade_user_from_ap_id(ap_id) do end end - defp upgrade_user(user, data) do + defp update_user(user, data) do user - |> User.upgrade_changeset(data, true) + |> User.remote_user_changeset(data) |> User.update_and_set_cache() end diff --git a/test/user_test.exs b/test/user_test.exs index d39787f35..5c24955c2 100644 --- a/test/user_test.exs +++ b/test/user_test.exs @@ -609,7 +609,7 @@ test "returns an ap_followers link for a user" do ) <> "/followers" end - describe "remote user creation changeset" do + describe "remote user changeset" do @valid_remote %{ bio: "hello", name: "Someone", @@ -621,28 +621,28 @@ test "returns an ap_followers link for a user" do setup do: clear_config([:instance, :user_name_length]) test "it confirms validity" do - cs = User.remote_user_creation(@valid_remote) + cs = User.remote_user_changeset(@valid_remote) assert cs.valid? end test "it sets the follower_adress" do - cs = User.remote_user_creation(@valid_remote) + cs = User.remote_user_changeset(@valid_remote) # remote users get a fake local follower address assert cs.changes.follower_address == User.ap_followers(%User{nickname: @valid_remote[:nickname]}) end test "it enforces the fqn format for nicknames" do - cs = User.remote_user_creation(%{@valid_remote | nickname: "bla"}) + cs = User.remote_user_changeset(%{@valid_remote | nickname: "bla"}) assert Ecto.Changeset.get_field(cs, :local) == false assert cs.changes.avatar refute cs.valid? end test "it has required fields" do - [:name, :ap_id] + [:ap_id] |> Enum.each(fn field -> - cs = User.remote_user_creation(Map.delete(@valid_remote, field)) + cs = User.remote_user_changeset(Map.delete(@valid_remote, field)) refute cs.valid? end) end @@ -1198,58 +1198,6 @@ test "get_public_key_for_ap_id fetches a user that's not in the db" do assert {:ok, _key} = User.get_public_key_for_ap_id("http://mastodon.example.org/users/admin") end - describe "insert or update a user from given data" do - test "with normal data" do - user = insert(:user, %{nickname: "nick@name.de"}) - data = %{ap_id: user.ap_id <> "xxx", name: user.name, nickname: user.nickname} - - assert {:ok, %User{}} = User.insert_or_update_user(data) - end - - test "with overly long fields" do - current_max_length = Pleroma.Config.get([:instance, :account_field_value_length], 255) - user = insert(:user, nickname: "nickname@supergood.domain") - - data = %{ - ap_id: user.ap_id, - name: user.name, - nickname: user.nickname, - fields: [ - %{"name" => "myfield", "value" => String.duplicate("h", current_max_length + 1)} - ] - } - - assert {:ok, %User{}} = User.insert_or_update_user(data) - end - - test "with an overly long bio" do - current_max_length = Pleroma.Config.get([:instance, :user_bio_length], 5000) - user = insert(:user, nickname: "nickname@supergood.domain") - - data = %{ - ap_id: user.ap_id, - name: user.name, - nickname: user.nickname, - bio: String.duplicate("h", current_max_length + 1) - } - - assert {:ok, %User{}} = User.insert_or_update_user(data) - end - - test "with an overly long display name" do - current_max_length = Pleroma.Config.get([:instance, :user_name_length], 100) - user = insert(:user, nickname: "nickname@supergood.domain") - - data = %{ - ap_id: user.ap_id, - name: String.duplicate("h", current_max_length + 1), - nickname: user.nickname - } - - assert {:ok, %User{}} = User.insert_or_update_user(data) - end - end - describe "per-user rich-text filtering" do test "html_filter_policy returns default policies, when rich-text is enabled" do user = insert(:user) diff --git a/test/web/activity_pub/views/user_view_test.exs b/test/web/activity_pub/views/user_view_test.exs index ecb2dc386..514fd97b8 100644 --- a/test/web/activity_pub/views/user_view_test.exs +++ b/test/web/activity_pub/views/user_view_test.exs @@ -29,7 +29,7 @@ test "Renders profile fields" do {:ok, user} = insert(:user) - |> User.upgrade_changeset(%{fields: fields}) + |> User.update_changeset(%{fields: fields}) |> User.update_and_set_cache() assert %{ From 77ee64b9930bf6b439f87112fa35e302f5125aa2 Mon Sep 17 00:00:00 2001 From: rinpatch Date: Thu, 16 Apr 2020 17:54:57 +0300 Subject: [PATCH 2/2] user: remove blank? --- lib/pleroma/user.ex | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index fab405233..753b0c686 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -343,9 +343,15 @@ def remote_user_changeset(struct \\ %User{local: false}, params) do bio_limit = Pleroma.Config.get([:instance, :user_bio_length], 5000) name_limit = Pleroma.Config.get([:instance, :user_name_length], 100) + name = + case params[:name] do + name when is_binary(name) and byte_size(name) > 0 -> name + _ -> params[:nickname] + end + params = params - |> Map.put(:name, blank?(params[:name]) || params[:nickname]) + |> Map.put(:name, name) |> Map.put_new(:last_refreshed_at, NaiveDateTime.utc_now()) |> truncate_if_exists(:name, name_limit) |> truncate_if_exists(:bio, bio_limit) @@ -1599,9 +1605,6 @@ def get_public_key_for_ap_id(ap_id) do end end - defp blank?(""), do: nil - defp blank?(n), do: n - def ap_enabled?(%User{local: true}), do: true def ap_enabled?(%User{ap_enabled: ap_enabled}), do: ap_enabled def ap_enabled?(_), do: false