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.

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

Ambiguity and possibly missing server behaviour in room upgrades #1324

Closed
ShadowJonathan opened this issue Nov 4, 2022 · 2 comments
Closed
Labels
clarification An area where the expected behaviour is understood, but the spec could do with being more explicit

Comments

@ShadowJonathan
Copy link
Contributor

ShadowJonathan commented Nov 4, 2022

Link to problem area: https://spec.matrix.org/v1.4/client-server-api/#server-behaviour-16

Issue

While converting 30rooms/60version_upgrade from sytest to complement, I encountered some of the following invariants being asserted, but being absent from the spec;

Remote aliases get copied to the new room

/upgrade moves remote aliases to the new room

This fails on dendrite, but I could not find a spec behaviour to point to when creating an issue for dendrite.

It asserts that, as far as I can see, that a remote server should observe a room upgrade and transfer all local aliases from the old room_id to the new one.

"Important state" is copied to the new room

/upgrade copies important state to the new room

This asserts that the following state is definitively copied to the new room:

  • m.room.topic
  • m.room.name
  • m.room.join_rules
  • m.room.guest_access
  • m.room.history_visibility
    • (also asserted by /upgrade should preserve room visibility for $vis rooms)
  • m.room.avatar
  • m.room.encryption
  • m.room.server_acl

The spec currently recommends (not requires) the above set of state events to be transferred to the new room, however, some of these (.encryption and .history_visibility) seem fairly critical for room functioning, so it feels like a wart or unnecessary ambiguity that these are recommended, not required.

Bans are copied to the new room

/upgrade copies ban events to the new room

Mentioned by sytest as it says on the tin, but no mention from the spec on this.

Power Levels to be copied

/upgrade copies the power levels to the new room
/upgrade copies >100 power levels to the new room

The spec makes the above recommendation to copy these, while - again - these seem pretty critical to the functioning of rooms, and so a server omission would be extremely unusual, yet it would be possible per the spec.

federation: true being copied over

/upgrade preserves room federation ability

The spec makes no mention of any keys within m.room.create to be copied or not, and so a room upgrade could potentially open a room up to federation, suddenly.

Push rules being copied over

$user_type user has push rules copied to upgraded room [local, remote]

The spec alludes to this with the following paragraph, but it feels non-exhaustive and ambiguous:

When a user joins the new room, the server should automatically transfer/replicate some of the user’s personalized settings such as notifications, tags, etc.

Also, this section includes "should", while the above sytest asserts "must".


I created this issue to help complement along, sytest seems to be a very strong authority on specification behaviour and the old process of "porting" synapse to the spec, and this area seems to have missed out.

Thus, here are the areas I notice I cannot port over to complement due to spec ambiguity, I hope they can help inform what areas of the spec to update to what behaviour, and/or what behaviours need an MSC to "correct", at this point.

@ShadowJonathan ShadowJonathan added the clarification An area where the expected behaviour is understood, but the spec could do with being more explicit label Nov 4, 2022
@turt2live
Copy link
Member

I think these all fit within the spec's claim that it's implementation-dependent on what gets copied. It's a shame that sytest ended up testing Synapse here, but hopefully that can be corrected.

Many of the features don't look insane to define in the spec, and therefore might be good MSCs to open.

The overall issues here are covered by a multitude of comments/issues throughout the repo though, usually with the keywords "room upgrades copy" (when talking about what should [not] be copied during upgrade) - I'm closing this in favour of the general epic.

@ShadowJonathan
Copy link
Contributor Author

For future reference, it seems that #456 references most (if not all) the issues listed here, and that it is the tracker issue for specification for these behaviours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification An area where the expected behaviour is understood, but the spec could do with being more explicit
Projects
None yet
Development

No branches or pull requests

2 participants