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

Jump to bottom

MSC4167: Copy bans on room upgrade #4167

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

MSC4167: Copy bans on room upgrade #4167

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
56f8b80
Proposal to copy ban membership states on room upgrade
Kladki Jul 6, 2024
514d111
add historical precedence
Kladki Jul 8, 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
40 changes: 40 additions & 0 deletions proposals/4167-copy-bans-on-room-upgrade.md
View file
Open in desktop
Copy link
Member

@turt2live turt2live Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Implementation requirements:

  • Server

Copy link
Member

Choose a reason for hiding this comment

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

Implementations:

Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
# MSC4167: Copy bans on room upgrade
Copy link
Member

Choose a reason for hiding this comment

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

It's unclear to me if we want to be more prescriptive in the room upgrade process: matrix-org/matrix-spec#1906 exists for discussion.


When a [room upgrade](https://spec.matrix.org/v1.11/client-server-api/#room-upgrades) is performed,
many state events are copied over to the new room, to minimize the amount of work the user has to do
after the upgrade. However, the spec currently states that "Membership events should not be transferred
to the new room due to technical limitations of servers not being able to impersonate people from other
homeservers". While this is most notably true for `join` membership events, this is not true for `ban`
Kladki marked this conversation as resolved.
Show resolved Hide resolved
membership, which users likely want copied over to the new room.
Copy link
Contributor

Choose a reason for hiding this comment

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

There is the argument to be made that users want the ability to control this behavior.

Why? Because if the room in question uses a Bot like Draupnir or Mjolnir they have no need to clone the bans potentially as they do all their bans via the bot. If all the bans are done via the bot why clutter the room state the room might argue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok then, adding a query parameter like copy_membership could be a good way to toggle this then. I don't specify bans with this parameter as in the future there may be other membership states which could be copied over, such as mutes. Does that sound good?

Also I'm not sure whether to mention in the MSC whether mutes should also be copied over once in the spec, and what to do about other future membership states.

Copy link
Contributor

Choose a reason for hiding this comment

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

To not dig too deep into the intracies of #3909 and #4106 they have a problem in that being muted under them is counted as being a room member. Now if you copied the leave mute states that could work without issue. And you could in theory allow preemptively setting it via a trivial from my perspective change to #3909 that would be inherited by #4106

Current day mutes as in mutes before 3909 are coppied as they are baked into the powerlevels event.


This behavior [has been part of Synapse](https://github.com/matrix-org/synapse/pull/4642) since only a
few months after [support for room upgrades was added](https://github.com/matrix-org/synapse/pull/4091),
with Dendrite doing this
[ever since they had support for room upgrades](https://github.com/matrix-org/dendrite/pull/2307).

## Proposal

When a room upgrade is performed, servers SHOULD copy membership states with the `ban` `membership` to
the new room, to ensure that users banned in the old room are still banned in the new room.

## Potential issues
Copy link
Member

Choose a reason for hiding this comment

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

There are potential safety concerns with this behaviour, fwiw. Namely:

Copy link
Contributor

Choose a reason for hiding this comment

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

Its good that Draupnir and Mjolnir allows you to avoid these issues if you are willing to accept the tradeoff that comes with reactive sanctions instead of preemptive sanctions.


In rooms with many banned users, it may take a while to perform the upgrade, as all the bans will need
to be performed. However, not doing this would allow previously banned users to access the room, which
is why this is deemed as worth it.
Comment on lines +22 to +24
Copy link

Choose a reason for hiding this comment

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

Additional potential issue: remote servers iirc are informed of the membership change, causing the banned user to potentially receive "You were banned from : " prompts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

remote servers iirc are informed of the membership change

Why do you think that? Even if it were true, this scenario doesn't really make sense in situations where the remote server isn't even aware of a room.

Copy link
Contributor

@Gnuxie Gnuxie Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

if you have @alice:matrix.org present within a room, and @bob:matrix.org becomes banned, then @bob:matrix.org will be force joined to the room and notified that they are banned (The event is sent to them down /sync iirc). This is because @bob:matrix.org's server (matrix.org) is sent the ban because @alice:matrix.org is participating in the room. Which is an issue because now bob is aware or has been reminded of the new room's existence.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really know if there's anything in the spec that causes that behaviour or of it's all just implicit and Synapse is doing something without thinking too much, which is still a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to check, this only happens with users on the same server as the user upgrading the room, right? Because last I checked if you /send events to remote servers from rooms they don't know about, they should get rejected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notifying a user that they were banned in a room they never participated in (or are not currently participating in, i.e. their membership is leave) seems like strange behavior anyways, and I don't think it is something that should happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to check, this only happens with users on the same server as the user upgrading the room, right?

Haven't tested.

seems like strange behavior anyways, and I don't think it is something that should happen.

It does happen though until there is some work to clarify or make sure that this doesn't happen (if it is even possible to do so since it might be quite hard for a server to work out the extent that a user was participating in the room or not).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it might be quite hard for a server to work out the extent that a user was participating in the room or not

It shouldn't be, since the server already needs to know their membership to send the event over /sync anyways (since the rooms field has different sub-fields for events depending on their current membership).


## Alternatives

None considered.

## Security considerations

None considered.

## Unstable prefix

None required, as no new fields or endpoints are being proposed.

## Dependencies

None.