Noita-Together / noita-together

Play alone together
MIT License
48 stars 15 forks source link

Issues with room creation with some usernames #164

Open innerpeach opened 5 months ago

innerpeach commented 5 months ago

There is no space to enter a name in room creation.

Failed to create room Invalid name. I can't create a room!

tehkab commented 5 months ago

Most users are not permitted to give a room a custom name. For those users it will always be of the format "${username}'s room"

What is your twitch username?

innerpeach commented 5 months ago

name is inner_peach. Twitch's Korean service was terminated as of February this year, and I was wondering if that might be the cause. I'm a Korean.

2024년 4월 12일 (금) 오전 12:29, kabby @.***>님이 작성:

Most users are not permitted to give a room a custom name. For those users it will always be of the format "${username}'s room"

What is your twitch username?

— Reply to this email directly, view it on GitHub https://github.com/Noita-Together/noita-together/issues/164#issuecomment-2049965498, or unsubscribe https://github.com/notifications/unsubscribe-auth/AR5XOV7OMVM4NML7KUQ2HHTY42T5NAVCNFSM6AAAAABF6MFCAGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBZHE3DKNBZHA . You are receiving this because you authored the thread.Message ID: @.***>

tehkab commented 5 months ago

@myndzi is this person's display name(?) possibly causing a problem creating a room in the backend?

https://www.twitch.tv/inner_peach (presumably?)

myndzi commented 5 months ago

Sounds likely. The JWT gets signed with a "preferred_username" that comes from the display name: https://github.com/Noita-Together/noita-together/blob/main/nt-web-app/utils/jwtUtils.ts#L10

However, the room name filter requires ascii text (which excludes unicode / international characters)

I'm not certain why it was done that way to begin with, but I basically duplicated the existing check: https://github.com/Noita-Together/noita-together/blob/main/nt-web-app/websocket/lobby/Lobby.js#L153

One solution is to sign the JWT with the login in addition to the display name of the user, which should follow stricter "identifier" rules. The username rules don't appear to be documented; an old Reddit post suggests typical identifier rules (alphanumeric + underscore): https://www.reddit.com/r/Twitch/comments/32w5b2/username_requirements/

This would mean that we never send the lobby server a name that would fail its validation check.

Another solution is to loosen or remove the validation check (I'm not sure why it's there TBH anyway)

A third solution is to do something like generate random room names from a wordlist, instead of using the username as part of the room name.

I don't really have any specific preferences here.

SkyeOfBreeze commented 5 months ago

@myndzi The third option seems like it would be cool. We already have a column for the user's name, so it would not be that hard to find a room still by person, but they could also share the name of the room too

myndzi commented 5 months ago

@myndzi The third option seems like it would be cool. We already have a column for the user's name, so it would not be that hard to find a room still by person, but they could also share the name of the room too

In that case, somebody needs to come up with a list of words, and (unlike Twitch), we're aware that there are limited combinations, so we should also have a plan for how to deal with repeats.

We can take advantage of math here in a couple ways.

1) We can select a random integer that can be deconstructed into a unique combination: Say we use two word lists (first word: A, last word: B) len(A) = 25, len(B) = 17

That makes 25*17 total combinations (425)

We can select a random combination with N = random() % 425 and deconstruct it with idx(A) = floor(N / 17) and idx(B) = floor(N % 17)

The downside here is that we can be more likely to hit duplicates sooner

2) Alternately, we can shuffle the lists on boot and cycle through them independently at some pace that's relatively prime (so that you don't wind up creating 17 rooms with the same first word). The upside is we're guaranteed to go through all combinations before we ever hit a repeat, and when we do repeat it will be the oldest-generated room name. The downside is room combinations always get created in the same order, so it's potentially less effective to "move on to the next one" if we hit one that's in use.

Both of those are edge cases, though they are edge cases we should deal with. An easy answer is to just try up to N times to get a unique room name, and if we fail, give up and throw the host's user id on the end - since we're guaranteed that the host's userid is not present in any room name (a user can only host one room)

myndzi commented 5 months ago

Using a birthday problem approximation, we can calculate the likelihood of a collision when choosing random room names at a certain number of concurrent rooms:

> var nRooms = 100; var nCombinations = (171*393); console.log(1 - Math.E ** -( (nRooms*(nRooms-1)) / (2*nCombinations)  ) );
0.07101011288982295

// 7% chance of a collision with 100 concurrent rooms

> var nRooms = 300; var nCombinations = (171*393); console.log(1 - Math.E ** -( (nRooms*(nRooms-1)) / (2*nCombinations)  ) );
0.4869494601791271

// 50% chance of collision with 300 concurrent rooms

Note that the numbers I used here are all spell and enemy names, but we might want to filter the list down to avoid using variants, e.g. "spark bolt", "spark bolt with trigger", "spark bolt with timer", which reduces the number of selections we can make.

We can also just add a short random number to every number to increase the number of unique combinations. With a number from 0-999, the probabilities change to:

> var nRooms = 100; var nCombinations = (171*393*1000); console.log(1 - Math.E ** -( (nRooms*(nRooms-1)) / (2*nCombinations)  ) );
0.00007365471336617802
// 0.007% chance of collision at 100 rooms

> var nRooms = 300; var nCombinations = (171*393*1000); console.log(1 - Math.E ** -( (nRooms*(nRooms-1)) / (2*nCombinations)  ) );
0.0006671582713814184
// 0.067% chance of collision at 300 rooms

> var nRooms = 1000; var nCombinations = (171*393*1000); console.log(1 - Math.E ** -( (nRooms*(nRooms-1)) / (2*nCombinations)  ) );
0.007405149663286648
// 0.74% chance of collision at 1000 rooms

So I think we can do something like this:

Userids doesn't feel like that great an idea in retrospect since, while it currently would be fine, it would kind of create a dependency where if we did live in a world where a user could host multiple rooms in the future, this code would have to change too.

Given the probabilities, 4 digits seems enough to have a very small chance of failure (assuming the full spell list), so if we do hit a collision, the disambiguating number should be quite short except in truly pathological (or adversarial) cases.