Add proper server names to join in unban tests #1341
Conversation
Otherwise the join will fail for reasons other than the one we test for on Synapse. Required for matrix-org/synapse#15323 Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
| @@ -29,7 +29,7 @@ | |||
| }) | |||
| } | |||
| })->then( sub { | |||
| matrix_join_room( $banned_user, $room_id ) | |||
| matrix_join_room( $banned_user, $room_id, ( server_name => $creator->server_name, ) ) | |||
There was a problem hiding this comment.
is this actually a desirable change?
As far as I understood it, at this point the room still has a member on this server: the creator (AFAICT the creater and banned user are on the same homeserver?).
Does this now mean that joining a room is broken if you don't provide the server_name despite it still being local?
There was a problem hiding this comment.
Joining rooms via roomid was always broken for me on Synapse. Dendrite has some special logic for this to extract an additional servername from the roomid, Synapse does not.
Also, providing the extra roomid does not change anything. It just means that this test now actually tests, if you can join that room instead of failing because you can't join a room via roomid. It just fixes the error message, that you don't get a 400 for not providing a server_name instead of a 403 for not being allowed to join the room. (Which I think is correct, because if you can't join the room, why should you get a different error message, if a different member is still in it? You shouldn't be able to know that?)
There was a problem hiding this comment.
I guess the problem I have in mind here is that this test does not seem like it should be doing a remote join, since there is already a local user.
Whilst you're right that this isn't the right test for that, it's symptomatic of a problem if we have to change this to get the test working :)
I don't know whether the spec says we need to accept local joins without a server name or not — I had a brief look but couldn't find a comment to that effect.
There was a problem hiding this comment.
As far as I understood it, at this point the room still has a member on this server: the creator (AFAICT the creater and banned user are on the same homeserver?).
Actually, I just checked, no, they are not in the test case I fixed. I just changed the other test for consistency, but that change isn't needed there. It never even tries a remote join. So basically, that behaviour did not change with matrix-org/synapse#15323 for the local user test, but it did for the remote test. For the remote test we need the server_name now, but for the local test I just added it, because doing a join on a roomid without a server_name is unsupported by synapse for remote rooms and it makes more sense to always pass it.
Does this now mean that joining a room is broken if you don't provide the server_name despite it still being local?
No, it works fine without that line. Remote joins are broken without server_name though.
Otherwise the join will fail for reasons other than the one we test for on Synapse.
Required for matrix-org/synapse#15323