search

CitizensDev / Citizens2

Citizens - the premier plugin and API for creating server-side NPCs in Minecraft.
https://citizensnpcs.co
Open Software License 3.0
583 stars 313 forks source link

Owner Trait Needs Rewrite To UUID Basis #2674

Closed GriffinCodes closed 2 years ago

GriffinCodes commented 3 years ago

A player recently changed their name, and could no longer edit the NPCs they created before the name change (&aYou must be the owner of this NPC to execute that command.). In the saves file, the owner section had their uuid and old name. Running /npc owner showed their old name.

config.yml

image

Original issue: https://github.com/ProjectEdenGG/Bugs/issues/38

mcmonkey4eva commented 3 years ago

This is half bug half feature - it's technically a feature request for /npc owner (name) to grab the UUID when possible (if the name corresponds to a real player, either one online or in the server player files, presumably via https://hub.spigotmc.org/javadocs/spigot/org/bukkit/Bukkit.html#getOfflinePlayer(java.lang.String)), though also can be considered a bug insofar as this behavior would be expected by most users.

GriffinCodes commented 3 years ago

I may be misunderstanding but I don't agree with the title change; the saves.yml is storing both the uuid and (old) name: image

but what I'm really worried about is the fact that players can't edit their NPCs after changing their name

mcmonkey4eva commented 3 years ago

Per discussion on Discord in #citizens and #developers, the proper solution here should actually be rewriting Owner trait to be entirely UUID based and removing the String name entirely. Presumably uuid=null can be the replacement stand in for an NPC being owned by "SERVER" (which helpfully negates the bork that this guy https://namemc.com/profile/Server.1 technically owns everybody's NPCs)

Several issues exist in the existing code, on top of the prior mentioned /npc owner (name) not grabbing a UUID, there's also Owner#isOwnedBy(CommandSender) not actually checking the UUID at all unless the name matches, which is the root of the original issue GriffinCodes encountered here. Also the isOwnedBy methods should not check name at all if UUID is non-null, as theoretically somebody could take over a previous player's name (if the original player renames and a different player then renames to that old name, which is allowed by Mojang)

String getOwner() can probably be swapped to return uuid == null ? "SERVER" : Bukkit.getOfflinePlayer(uuid).getName(); to avoid API breakage.

mcmonkey4eva commented 2 years ago

The commits https://github.com/CitizensDev/CitizensAPI/commit/fc065df0b8084f80e26dc55ac808e26d66649ff7 and https://github.com/CitizensDev/Citizens2/commit/4ecf09050a5dd016d7e1c0838ae4cf5f4a93752c and https://github.com/CitizensDev/Citizens2/commit/25f341ef63a8dd12a6980c8ef4ca37f3c8f6e595 from fullwall rewrite Owner to be entirely UUID based. The last commit also makes sure /npc owner (name) still works, and simply stores the UUID of the named player. This update is available in build 2388+ from https://ci.citizensnpcs.co/job/Citizens2/