EssentialsX / Essentials

The modern Essentials suite for Spigot and Paper.
https://essentialsx.net
GNU General Public License v3.0
1.95k stars 981 forks source link

Incompatibility with Towny economy: EssentialsX renames npc accounts with unallowed characters #2016

Closed leonardo-dgs closed 6 years ago

leonardo-dgs commented 6 years ago

Information

Full output of /ess version: [23:16:26 INFO]: Server version: 1.12.2-R0.1-SNAPSHOT git-Paper-1322 (MC: 1.12.2) [23:16:26 INFO]: EssentialsX version: 2.15.0.3 [23:16:26 INFO]: Vault version: 1.5.6-b49

Details

Essentials economy isn't compatible with Towny's towns accounts, because characters like "-" that can't be contained in usernames, are replaced with allowed characters (in this case "_"), so a normal user can login with an account with that name and cause problems.

mdcfe commented 6 years ago

We can't remove the sanitisation without breaking any existing accounts that have names that were previously sanitised, but this may be fixed by the ongoing Vault handler rewrite (which still has the potential to break these accounts).

All I can suggest is to not allow new towns to include those characters in their names.

leonardo-dgs commented 6 years ago

@md678685 Essentials economy isn't compatible at all with Towny, because Towny creates town and nation accounts prefixed with "town-" (or "nation-"), that Essentials replaces with "town_", so a player with that name can join. Anyway I don't understand why Essentials changes the account names, since, however, their real name still remains with the unallowed characters

mdcfe commented 6 years ago

That doesn't make them incompatible - Towny towns and nations still create accounts that they can then use, so they're partially compatible.

"Fixing" this would irreversibly damage data for existing Towny accounts as they would suddenly lose their balances, while leaving this as-is avoids yet another data loss issue relating to EssentialsX and Towny.

If the username sanitisation is causing an issue for your server, you can:

leonardo-dgs commented 6 years ago

@md678685 This problem can happen (even if with a minor probability) with premium servers, because a player can change his username to "town_townName"

mdcfe commented 6 years ago

As I said, pushing an update to change this will change what EssentialsX thinks the town name is from NPC:town_townname to NPC:town-townname, replacing them with new, empty accounts.

On an online-mode server, EssentialsX will use online mode UUIDs for players but offline-mode UUIDs for towns, so they will never clash.

However, even on an offline-mode server, the Towny accounts are prefixed with NPC: by EssentialsX, so a player would have to log in with the username NPC:town_<townname> for EssentialsX to give them the same offline UUID, but colons aren't valid in account names so this should never be an issue.

If you can produce the theoretical behaviour you're concerned about, please let us know, but this concern isn't an actual issue as far as the EssentialsX code is concerned.

leonardo-dgs commented 6 years ago

@md678685 I've tested it, the uuid changes, but the npc names are saved in usermap.csv without the prefix NPC:, so if a player with name town_<townname> join the server, the npc uuid is replaced with the player uuid, and the town account will no longer be accessible, so Essentials should save npc accounts in the file usermap.csv prefixed with NPC: to fix this problem

mdcfe commented 6 years ago

@Leomixer17 Again, doing that would mean all existing NPC accounts would be lost, including Towny accounts, as EssentialsX won't be able to find them unless we migrate the usermap, which I'm not sure is possible as I don't know whether migrations happen before the usermap loads, and we'd need to hold up the server startup when doing the migration to manually load every single userdata file and check if it's an NPC or not before updating the usermap.

mdcfe commented 6 years ago

Towny now prevents users joining with usernames that may clash with its NPC accounts, so this is no longer relevant.

leonardo-dgs commented 5 years ago

@md678685 in my opinion this issue should be reopened because this inconsistency can happens with every plugin that uses NPCs, not only with Towny. I think that should be done a converter for existing NPCs, and put a setting in the file upgrades-done.yml.