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.
This commit is contained in:
rinpatch 2020-04-11 21:44:52 +03:00
parent fc4f92c5de
commit c077ad0b33
5 changed files with 31 additions and 124 deletions

View file

@ -339,18 +339,20 @@ defmodule Pleroma.User do
end end
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) bio_limit = Pleroma.Config.get([:instance, :user_bio_length], 5000)
name_limit = Pleroma.Config.get([:instance, :user_name_length], 100) name_limit = Pleroma.Config.get([:instance, :user_name_length], 100)
params = params =
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(:name, name_limit)
|> truncate_if_exists(:bio, bio_limit) |> truncate_if_exists(:bio, bio_limit)
|> truncate_fields_param() |> truncate_fields_param()
changeset = changeset =
%User{local: false} struct
|> cast( |> cast(
params, params,
[ [
@ -375,7 +377,8 @@ defmodule Pleroma.User do
:discoverable, :discoverable,
:invisible, :invisible,
:actor_type, :actor_type,
:also_known_as :also_known_as,
:last_refreshed_at
] ]
) )
|> validate_required([:name, :ap_id]) |> validate_required([:name, :ap_id])
@ -488,49 +491,6 @@ defmodule Pleroma.User do
end end
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 def update_as_admin_changeset(struct, params) do
struct struct
|> update_changeset(params) |> update_changeset(params)
@ -1642,14 +1602,6 @@ defmodule Pleroma.User do
defp blank?(""), do: nil defp blank?(""), do: nil
defp blank?(n), do: n 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{local: true}), do: true
def ap_enabled?(%User{ap_enabled: ap_enabled}), do: ap_enabled def ap_enabled?(%User{ap_enabled: ap_enabled}), do: ap_enabled
def ap_enabled?(_), do: false def ap_enabled?(_), do: false

View file

@ -1551,11 +1551,22 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do
end end
def make_user_from_ap_id(ap_id) do 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) Transmogrifier.upgrade_user_from_ap_id(ap_id)
else else
with {:ok, data} <- fetch_and_prepare_user_from_ap_id(ap_id) do 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 else
e -> {:error, e} e -> {:error, e}
end end

View file

@ -710,7 +710,7 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do
{:ok, new_user_data} = ActivityPub.user_data_from_user_object(object) {:ok, new_user_data} = ActivityPub.user_data_from_user_object(object)
actor actor
|> User.upgrade_changeset(new_user_data, true) |> User.remote_user_changeset(new_user_data)
|> User.update_and_set_cache() |> User.update_and_set_cache()
ActivityPub.update(%{ ActivityPub.update(%{
@ -1253,12 +1253,8 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do
def upgrade_user_from_ap_id(ap_id) do def upgrade_user_from_ap_id(ap_id) do
with %User{local: false} = user <- User.get_cached_by_ap_id(ap_id), 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), {:ok, data} <- ActivityPub.fetch_and_prepare_user_from_ap_id(ap_id),
already_ap <- User.ap_enabled?(user), {:ok, user} <- update_user(user, data) do
{:ok, user} <- upgrade_user(user, data) do TransmogrifierWorker.enqueue("user_upgrade", %{"user_id" => user.id})
if not already_ap do
TransmogrifierWorker.enqueue("user_upgrade", %{"user_id" => user.id})
end
{:ok, user} {:ok, user}
else else
%User{} = user -> {:ok, user} %User{} = user -> {:ok, user}
@ -1266,9 +1262,9 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do
end end
end end
defp upgrade_user(user, data) do defp update_user(user, data) do
user user
|> User.upgrade_changeset(data, true) |> User.remote_user_changeset(data)
|> User.update_and_set_cache() |> User.update_and_set_cache()
end end

View file

@ -609,7 +609,7 @@ defmodule Pleroma.UserTest do
) <> "/followers" ) <> "/followers"
end end
describe "remote user creation changeset" do describe "remote user changeset" do
@valid_remote %{ @valid_remote %{
bio: "hello", bio: "hello",
name: "Someone", name: "Someone",
@ -621,28 +621,28 @@ defmodule Pleroma.UserTest do
setup do: clear_config([:instance, :user_name_length]) setup do: clear_config([:instance, :user_name_length])
test "it confirms validity" do test "it confirms validity" do
cs = User.remote_user_creation(@valid_remote) cs = User.remote_user_changeset(@valid_remote)
assert cs.valid? assert cs.valid?
end end
test "it sets the follower_adress" do 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 # remote users get a fake local follower address
assert cs.changes.follower_address == assert cs.changes.follower_address ==
User.ap_followers(%User{nickname: @valid_remote[:nickname]}) User.ap_followers(%User{nickname: @valid_remote[:nickname]})
end end
test "it enforces the fqn format for nicknames" do 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 Ecto.Changeset.get_field(cs, :local) == false
assert cs.changes.avatar assert cs.changes.avatar
refute cs.valid? refute cs.valid?
end end
test "it has required fields" do test "it has required fields" do
[:name, :ap_id] [:ap_id]
|> Enum.each(fn field -> |> 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? refute cs.valid?
end) end)
end end
@ -1198,58 +1198,6 @@ defmodule Pleroma.UserTest do
assert {:ok, _key} = User.get_public_key_for_ap_id("http://mastodon.example.org/users/admin") assert {:ok, _key} = User.get_public_key_for_ap_id("http://mastodon.example.org/users/admin")
end 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 describe "per-user rich-text filtering" do
test "html_filter_policy returns default policies, when rich-text is enabled" do test "html_filter_policy returns default policies, when rich-text is enabled" do
user = insert(:user) user = insert(:user)

View file

@ -29,7 +29,7 @@ defmodule Pleroma.Web.ActivityPub.UserViewTest do
{:ok, user} = {:ok, user} =
insert(:user) insert(:user)
|> User.upgrade_changeset(%{fields: fields}) |> User.update_changeset(%{fields: fields})
|> User.update_and_set_cache() |> User.update_and_set_cache()
assert %{ assert %{