Skip to content

Make restricted room join tests clearer #752

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Make restricted room join tests clearer #752

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6ec9a3a
remove pointless invalid join_authorised_via_users_server field due t…
girlbossceo Dec 9, 2024
14cc5be
add back join_authorised_via_users_server correctly formatted
girlbossceo Dec 15, 2024
4830c39
make restricted room join tests fail earlier when something goes wrong
morguldir Dec 31, 2024
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
21 changes: 15 additions & 6 deletions tests/restricted_rooms_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func checkRestrictedRoom(t *testing.T, alice *client.CSAPI, bob *client.CSAPI, a

t.Run("Join should succeed when joined to allowed room", func(t *testing.T) {
// Join the allowed room.
bob.JoinRoom(t, allowed_room, []string{"hs1"})
bob.MustJoinRoom(t, allowed_room, []string{"hs1"})

// Confirm that we joined the allowed room by changing displayname and
// waiting for confirmation in the /sync response. (This is an attempt
Expand All @@ -86,7 +86,7 @@ func checkRestrictedRoom(t *testing.T, alice *client.CSAPI, bob *client.CSAPI, a
)

// We should now be able to join the restricted room.
bob.JoinRoom(t, room, []string{"hs1"})
bob.MustJoinRoom(t, room, []string{"hs1"})

// Joining the same room again should work fine (e.g. to change your display name).
bob.SendEventSynced(
Expand All @@ -99,8 +99,8 @@ func checkRestrictedRoom(t *testing.T, alice *client.CSAPI, bob *client.CSAPI, a
Content: map[string]interface{}{
"membership": "join",
"displayname": "Bobby",
// This should be ignored since this is a join -> join transition.
"join_authorised_via_users_server": "unused",
// This should be ignored by the server since this is a join -> join transition
"join_authorised_via_users_server": "@unused:unused.local",
Copy link
Member

Choose a reason for hiding this comment

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

What happens if a ruma-based server gets garbage here then? Does it still work?

Copy link
Author

Choose a reason for hiding this comment

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

It can create the event then, but it won’t really work properly unless the server removes the field when handling the PUT like synapse does

So when verifying the signature of a federation event actually looking like that it won’t work still

This part i think is intended though, although seemingly synapse changed that behaviour at some point too 🧐

Copy link
Member

Choose a reason for hiding this comment

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

I'm not really following your comment, sorry. Shouldn't the field be removed no matter what? So what isn't working for ruma?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry yeah it should, but if another server forgets to remove it, then ruma still tries to check the signature, but synapse seems to still allow the event

Copy link
Member

Choose a reason for hiding this comment

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

Irrespective of the behaviour of Synapse and Ruma, this field should either be set to a valid structure for this field (a Matrix ID), or it should explicitly call out that this field is set to an invalid value on purpose.

As the latter does not appear the case, I agree with this change. However I'm not opposed to another test that checks homeservers correctly handle invalid content in this field.

},
},
)
Expand All @@ -114,6 +114,15 @@ func checkRestrictedRoom(t *testing.T, alice *client.CSAPI, bob *client.CSAPI, a
// Wait until Alice sees Bob leave the allowed room. This ensures that Alice's HS
// has processed the leave before Bob tries rejoining, so that it rejects his
// attempt to join the room.
alice.MustSyncUntil(t, client.SyncReq{}, client.SyncTimelineHas(
room, func(ev gjson.Result) bool {
if ev.Get("type").Str != "m.room.member" || ev.Get("sender").Str != bob.UserID {
return false
}

return ev.Get("content").Get("membership").Str == "leave"
}))

Copy link
Member

Choose a reason for hiding this comment

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

This is a duplicate of the stuff underneath? Why add this?

Copy link
Author

Choose a reason for hiding this comment

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

It makes sure the leave worked in both rooms now

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this break if a single sync contained both leaves?

MustSyncUntil supports multiple SyncCheckOpt instances, so you can provide two functions that verify that bob has left the rooms in question.

You can also replace SyncTimelineHas with SyncLeftFrom to save yourself the manual event content checking.

alice.MustSyncUntil(t, client.SyncReq{}, client.SyncTimelineHas(
allowed_room, func(ev gjson.Result) bool {
if ev.Get("type").Str != "m.room.member" || ev.Get("sender").Str != bob.UserID {
Expand All @@ -130,11 +139,11 @@ func checkRestrictedRoom(t *testing.T, alice *client.CSAPI, bob *client.CSAPI, a
t.Run("Join should succeed when invited", func(t *testing.T) {
// Invite the user and joining should work.
alice.MustInviteRoom(t, room, bob.UserID)
bob.JoinRoom(t, room, []string{"hs1"})
bob.MustJoinRoom(t, room, []string{"hs1"})

// Leave the room again, and join the allowed room.
bob.MustLeaveRoom(t, room)
bob.JoinRoom(t, allowed_room, []string{"hs1"})
bob.MustJoinRoom(t, allowed_room, []string{"hs1"})
})

t.Run("Join should fail with mangled join rules", func(t *testing.T) {
Expand Down