MegaAntiCheat / client-backend

GNU General Public License v3.0
117 stars 24 forks source link

User note and alias are mutually-exclusive #125

Closed Seercat3160 closed 5 months ago

Seercat3160 commented 5 months ago

Issue Description

Adding a note to a user removes any set alias, and adding an alias removes any set note.

Reproduction Steps

  1. Set an alias for a user
  2. Add a note to them
  3. Notice that the alias is gone
  4. Add an alias
  5. Notice that the note is gone
Seercat3160 commented 5 months ago

I think I found the bug. From what I can tell, everything in the client-backend is working as it's supposed to, but there's an issue with how customData is handled in the WebUI.

In PlayerNotebox.tsx and ChangeAliasModel.tsx (from what I can tell these are responsible for updating the note and alias respectively), updatePlayer() is called only passing in the new alias or note as customData, and so the client-backend fully overwrites its stored customData with that, removing the other kinds (which is currently just the notes and aliases - thus no other effects than this where they seem mutually-exclusive).

This could be approached in a number of ways, but the best might be to have the webui update data on the client-backend in a delta of sorts, like an HTTP PATCH rather than PUT. Alternatively, updatePlayer() could have that done internally. However that would give the operation potential for change-after-read which could be an issue given that the player data is being updated and changed from various places at different times.

Bash-09 commented 5 months ago

The client is supposed to merge any incoming custom data with the existing data instead of completely overwriting it to prevent this kind of data loss. It would be annoying to have to keep track and send the entire custom data in the frontend to not accidentally overwrite anything.

I think when I refactored the web handling into the event loop, I must have accidentally made it replace the data instead of merge it. Unless it's always been like this. Anyways, I've got what should be a fix so I'll pop up a MR in a sec.

Bash-09 commented 5 months ago

Thanks for the bug report :)

Seercat3160 commented 5 months ago

No worries! By the way, when I said that I thought the client-backend was doing what it was supposed to do, that was because of the use of HTTP PUT which iirc is supposed to replace the object, as opposed to PATCH.

Bash-09 commented 5 months ago

Ah I see, I wasn't familiar with PATCH. I think our API spec is definitely not quite perfect lol, but it works.

lili7h commented 5 months ago

Originally, IIRC we used put because of its idempotency related to puting a state update multiple times not actually having any impact. In a perfect world, I would consider alias and tags different state 'objects' where in such a post does implicitly completely replace it, but because of the way the player record endpoint is combined, you get that muddiness.