Check room ID doesn't exist when creating a new room

[rreuvekamp]

Signed-off-by: Remi Reuvekamp git@remi.im

Closes #267

Pull Request Checklist

• I have added any new tests that need to pass to testfile as specified in docs/sytest.md

(None since this only changes internal behaviour. All others still pass; All tests PASSED (with 474 expected failures) (with 173 passes) (with 116 skipped tests) )

• Pull request includes a sign off

Based on #348

In a nutshell this pull request will:

• Add methods to the roomserver QueryAPI to mark a given room ID as reserved.
• Add a database table roomserver_reserved_room_ids to keep track of reservations.
• Make the ClientAPI reserve the chosen room ID before creating a new room, to avoid races.

There are still issues that need fixing:

• Function QueryReserveRoomID is racey as checking if the room with the given ID doesn't already exists happens before making the reservation. It would be ideal if the database enforces not being able to add a row to roomserver_reserved_room_ids if the roomID exists in roomserver_rooms, but I haven't found a solution. Any ideas?
• Reservations are not cleaned up. I suppose it should be the roomserver's responsibility to remove reservations when a room is actually created. The code which handles events that create a room is quite complicated to me. Would assignRoomNID in roomserver/storage/storage.go be a good place to delete a reservation? That still wouldn't fix the possible issue of 'orphan' reservations. Ideally reservations should expire after a while.

[anoadragon453]

Thanks for this PR! I've initially merged it forward to latest master to fix some CI errors.

[kegsay]

Firstly, thanks for your effort on this PR. Sorry it's taken quite so long to get around to it.

It seems a little wrong to be generating room IDs then querying the room server whether that's okay or not. Why not just ask the room server directly to give you an unallocated room ID?

In addition, I don't think this would work in a multi-room server scenario, where room servers > 1. Which server do you ask? How do they synchronise their reservations? This could be just "via the database" and "use the same database", but in the current set-up this is not the case as the tables reside in the normal room server. You will know which room server to ask, because you generate the room ID.

In my opinion, I don't think this is the right approach. Just use a UUID (~122 bits, 2^122 = ~5.3x10^36 combinations), or increase the number of characters in util.RandomString to produce the same number of bits. RandomString uses the alphanumerics A-Za-z0-9 for 62 different options per character, and there's 16 characters already: 62^16 = ~4.7x10^28 options. Increase it from 16 to 21 characters and you get 62^21 = ~4.3x10^37 combinations, exceeding UUIDs.

We don't require room IDs to be cryptographically random, so a normal pseudo-random function would be sufficient. Even the RFC for UUIDs doesn't require CSPRNGs:

Set all the other bits to randomly (or pseudo-randomly) chosen values.

Section 4.4: https://www.ietf.org/rfc/rfc4122.txt

This PR adds extra complexity where I feel we don't need to have it. I believe #267 shouldn't exist, and if we are genuinely worried about collisions, increase room IDs by 5 characters. cc @erikjohnston

[kegsay]

After discussing this with @anoadragon453 and others, I see no compelling reason to add this extra complexity to Dendrite. Therefore, I'll be closing this and #267 - sorry for the work you've already put into this.

[rreuvekamp]

Therefore, I'll be closing this and #267 - sorry for the work you've already put into this.

No problem mate! Cheers