Don't specify an invalid "join_authorised_via_users_server" key for a test that doesn't require it

[neilalexander]

This breaks the signature checks in gomatrixserverlib/dendrite, because we expect the "join_authorised_via_users_server" key to be well-formed so we can extract the server name.

[clokep]

I'm unsure about this change, it was added in #208 as a regression test against something which was actually happening in the wild (clients were including that additional field when changing nicknames for rooms). I think for the test to be accurate it really needs the field to be included, not just missing -- although we might actually want to test both situations.

Changing the value so it can be parsed (instead of the wrong form) is likely OK, but shouldn't gomatrixserverlib bet treating this as "untrusted data" and not assuming it is well-formed?

[neilalexander]

shouldn't gomatrixserverlib bet treating this as "untrusted data" and not assuming it is well-formed?

gomatrixserverlib can only require the extra signature from the authorising server in the event auth if it can parse the server name from that field. If we fail to parse then we can’t add the second “needed” server name for the signature checking, which would result in the event possibly passing the signature checks when it otherwise shouldn’t.

The ”join_authorised_via_users_server” field isn’t specced as opaque so it isn’t clear what to do here otherwise?

https://github.com/matrix-org/gomatrixserverlib/blob/master/eventcrypto.go#L85-L96

[clokep]

gomatrixserverlib can only require the extra signature from the authorising server in the event auth if it can parse the server name from that field.

This test-case is testing that the extra signature isn't needed since it is a join -> join transition.

[neilalexander]

This test-case is testing that the extra signature isn't needed since it is a join -> join transition.

I am guessing then that Synapse needs to tangle the signature checking and event auth checks together for this to work? That is not how gomatrixserverlib works (Dendrite and GMSL on the whole are much more compartmentalised out of necessity) and this is obviously why things are ending up so difficult to implement.

[clokep]

I think what is missing here is that Synapse strips any join_authorised_via_users_server received over the client-server API (see matrix-org/synapse#10933 and the corresponding issue). I think we considered this an implementation detail, which is why it isn't part of the MSC, but clearly testing for that in complement doesn't necessarily make sense then.

I think it is necessary though for /myroom{nick,avatar} to work properly.

[clokep]

I think what is missing here is that Synapse strips any join_authorised_via_users_server received over the client-server API (see matrix-org/synapse#10933 and the corresponding issue). I think we considered this an implementation detail, which is why it isn't part of the MSC, but clearly testing for that in complement doesn't necessarily make sense then.

I think it is necessary though for /myroom{nick,avatar} to work properly.

I brought this up on the spec PR: matrix-org/matrix-spec-proposals#3387.

[kegsay]

So what's the status of this PR? Is it good and should be merged or does it test something we don't allow?

[clokep]

So what's the status of this PR? Is it good and should be merged or does it test something we don't allow?

I believe this PR is weakening a test that will likely cause Dendrite (and other homeservers) to break under the same conditions that Synapse did.

[kegsay]

Closing this for now then.