EssentialsX / Essentials

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

Balance Loss on Name Change #2400

Closed PetrichorCraft closed 5 years ago

PetrichorCraft commented 5 years ago

Information

Full output of /ess version: (I asked for help with this in Discord, and since then, I have updated EssentialsX. I received no help there, so I'm hoping I will get some here. To be safe, I'm posting both sets of /ess version & latest.log. This has happened on both versions.)

(This was the 2nd time it happened, first time I reported it)

[02:28:11 INFO]: Server version: 1.13.2-R0.1-SNAPSHOT git-Paper-492 (MC: 1.13.2) [02:28:11 INFO]: EssentialsX version: 2.16-pre2.3 [02:28:11 INFO]: PermissionsEx version: 1.23.4 [02:28:11 INFO]: Vault version: 1.7.1-b91 [02:28:11 INFO]: EssentialsXSpawn version: 2.16-pre2.3 [02:28:11 INFO]: You are running unsupported plugins!

(This was the 3rd time it happened, 2nd time I reported it)

[18:20:45 INFO]: Server version: 1.13.2-R0.1-SNAPSHOT git-Paper-492 (MC: 1.13.2) [18:20:45 INFO]: EssentialsX version: 2.16.0.31 [18:20:45 INFO]: PermissionsEx version: 1.23.4 [18:20:45 INFO]: Vault version: 1.7.1-b91 [18:20:45 INFO]: EssentialsXSpawn version: 2.16.0.31 [18:20:45 INFO]: You are running unsupported plugins!

Server log: (Again, two logs)

(First time reporting it) https://gist.github.com/PetrichorCraft/6cf4acfef77c00e427957dadda26fd75

(Second time reporting it - now) https://gist.github.com/PetrichorCraft/4efc77ae5e262155287cab5ae6bcaaa6 EssentialsX config https://gist.github.com/PetrichorCraft/f417be76f0d63ec35e199636ac59817e

Help request

Problem

When a player changes their Minecraft username, they lose their balance. Their sethomes are fine, they're kept at the same locations that they were set. This is not a fluke, it has happened 3 times so far, with all 3 people that have changed their usernames. When you do /bal OldUsername, it shows the balance of the new username. Likewise with /seen. EssentialsX is our economy manager. What I have tried

I've looked in the EssentialsX config.yml for anything related to name changes, or balance deletion. Nothing found. I've looked in the EssentialsX userdata files to see if the old balance was recorded in the files. It was not. (And the data files are sorted by UUID - and the sethomes are fine - so it's not like a new file was created for the players with the changed names) And I've gone onto the Discord server for help. (January 4)

triagonal commented 5 years ago

Hi, would you mind going into your /plugins/Essentials/userdata folder and seeing if a file named 8d714f9f-6475-3358-aa03-627188bbe5e7.yml is present? If so, could you upload that somewhere and reply with it here?

From these lines in your log, it seems like this issue may be related to #574

[14:44:45] [User Authenticator #47/INFO]: UUID of player Officer_Cam is d3630f94-bbc1-4619-bca9-f7fcb7164643
[14:44:46] [pool-25-thread-1/INFO]: [Essentials] Creating empty config: /home/container/plugins/Essentials/userdata/8d714f9f-6475-3358-aa03-627188bbe5e7.yml
[14:44:46] [Server thread/INFO]: Officer_Cam[/<redacted>:53448] logged in with entity id 1746567 at ([world]-8200.845030327007, 56.0, -4034.478643712719)
[14:44:46] [Server thread/INFO]: [Essentials] Found new UUID for Officer_Cam. Replacing 8d714f9f-6475-3358-aa03-627188bbe5e7 with d3630f94-bbc1-4619-bca9-f7fcb7164643
[14:44:46] [Server thread/INFO]: Officer_Cam (formerly known as CamStanley) joined the game
PetrichorCraft commented 5 years ago

Yeah, that does seem to be related. (note: He does still have access to his homes, even though they're not in this file)

https://pastebin.com/nXU4SGSX

triagonal commented 5 years ago

This definitely looks like the same issue. Until it's fixed, you can work around it by finding the npc file and giving the user back their balance, as you've probably already gathered:

[Essentials] Found new UUID for <playername>. Replacing <npc-uuid> with <player-uuid>

Where <npc-uuid>.yml will contain the missing balance.

PetrichorCraft commented 5 years ago

Thank you very much for the help!

mdcfe commented 5 years ago

From a quick glance, it looks like the economy is somehow migrating the player's balance to an offline mode UUID, then the user map is detecting that offline mode UUID and migrating it back to online mode. I'm looking into this further now.

Update 1

https://github.com/EssentialsX/Essentials/blob/023cf6a1aaf63614d4603020258b2040f725c1a5/Essentials/src/com/earth2me/essentials/api/Economy.java#L379-L393

https://github.com/EssentialsX/Essentials/blob/023cf6a1aaf63614d4603020258b2040f725c1a5/Essentials/src/com/earth2me/essentials/api/Economy.java#L39-L53

It looks like the plugin is somewhere trying to access the player's economy account while the user is connecting, forcing an NPC userdata file to be created through the createNPC method. I'm trying to work out where exactly this gets called during login now.

Update 2

IEssentials#getUser(String) relies on the usermap, but the plugin doesn't update the usermap with their new time until delayedJoin. This means no user is returned and so Economy.createNPC creates a new NPC file for that player. I'm still not sure where this gets called yet.

Update 3

To start with, I was confused - EssentialsX only internally calls Economy.createNPC when running automated tests, so I had no idea where this was coming from... until I checked Vault.

https://github.com/MilkBowl/Vault/blob/81c38e983b4dfb3decdd83385a92f477ba41bb1f/src/net/milkbowl/vault/economy/plugins/Economy_Essentials.java#L87-L93

Vault automatically calls Economy.createNPC if it determines that the player doesn't have an account, which is done by checking Economy.playerExists:

https://github.com/EssentialsX/Essentials/blob/023cf6a1aaf63614d4603020258b2040f725c1a5/Essentials/src/com/earth2me/essentials/api/Economy.java#L351-L360

This method relies on the aforementioned Essentials#getUser(String), which relies on the usermap. We should prevent NPC users being created if their UUID already corresponds to a valid Essentials user, even if they can't be found via username yet.

Update 4

After looking around a bit more, I believe getUserByName should be updated to try and search for users by UUID if they can't be found by name. This will avoid player accounts being reported as not existing to Vault.

In an ideal world, the whole Economy API would be migrated to using UUIDs, but this would require a greater effort and for now we should fix this particular issue.

mdcfe commented 5 years ago

I've opened a PR at #2432 which may fix this. If anyone who was running into issues could try this build on a replica test server that mostly matches their production server, it'd be appreciated.

Test builds

Only use these builds on a test server! Don't use them in production!

Commit Link
79035254ddc6219c93a57c67e9fe894545303af8 (latest) EssentialsX-2.16.0.46-debug.jar.zip
8b017f63cf77c4d80b8ad75d1046cfabcc90fa00 EssentialsX-2.16.0.45-debug.jar.zip
mdcfe commented 5 years ago

@PetrichorCraft Could you update to the latest build and see if the issue persists?

PetrichorCraft commented 5 years ago

Thank you so much! Yes, I just tested, and it's fixed!

mdcfe commented 5 years ago

Glad to hear this fix has worked. Thanks to everyone who helped test and replicate this.