Make restricted room join tests clearer - #752
morguldir wants to merge 3 commits into
Conversation
Sorry, something went wrong.
…o strict ruma parsing requirements Signed-off-by: strawberry <strawberry@puppygock.gay>
Signed-off-by: strawberry <strawberry@puppygock.gay>
Signed-off-by: morguldir <morguldir@protonmail.com>
| // 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", |
There was a problem hiding this comment.
What happens if a ruma-based server gets garbage here then? Does it still work?
Sorry, something went wrong.
There was a problem hiding this comment.
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 🧐
Sorry, something went wrong.
There was a problem hiding this comment.
I'm not really following your comment, sorry. Shouldn't the field be removed no matter what? So what isn't working for ruma?
Sorry, something went wrong.
There was a problem hiding this comment.
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
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
|
||
| return ev.Get("content").Get("membership").Str == "leave" | ||
| })) | ||
|
|
There was a problem hiding this comment.
This is a duplicate of the stuff underneath? Why add this?
Sorry, something went wrong.
There was a problem hiding this comment.
It makes sure the leave worked in both rooms now
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Minor request, otherwise LGTM!
Edit: also could you please fix the git conflict.
Sorry, something went wrong.
|
|
||
| return ev.Get("content").Get("membership").Str == "leave" | ||
| })) | ||
|
|
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| // 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", |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
Reviewers
Requested changes must be addressed to merge this pull request.
Firstly this PR makes the tests work on more implementations, for example ruma and GMS won't really understand join_authorised_via_users_server not looking like a user id, similar to what #224 tried to do
Making it a user id that isn't correct still makes sure that the server can handle clients updating the member event directly with /myroomnick or similar
Some errors were also silently being ignored, which made it harder to tell which part of the test was actually failing
Pull Request Checklist