Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add proper server names to join in unban tests #1341

Merged
merged 1 commit into from
Mar 28, 2023
Merged

Add proper server names to join in unban tests #1341

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions tests/30rooms/07ban.pl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, ) )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@deepbluev7 deepbluev7 Mar 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

->main::expect_http_403; # Must be unbanned first
})->then( sub {
do_request_json_for( $creator,
Expand Down Expand Up @@ -103,7 +103,7 @@
};
})->then( sub {
# Must be unbanned first
matrix_join_room( $banned_user, $room_id )->main::check_http_code(
matrix_join_room( $banned_user, $room_id, ( server_name => $creator->server_name, ) )->main::check_http_code(
403 => "ok",
200 => "redo",
);
Expand Down