Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Rework room freeze and implement unfreezing the room #100

Merged
merged 12 commits into from
Jul 22, 2021

Conversation

babolivier
Copy link
Contributor

@babolivier babolivier commented Jun 24, 2021

A feature we currently have in the RoomAccessRules module is the ability to "freeze" room when the last admin has left it (#59). Freezing the room means changing the room's rules in a way that prevents anyone from sending messages in the room except for leaving or taking over and unfreezing the room.

The current solution for freezing a room doesn't allow the room to be unfrozen. This is because the Matrix spec explicitly forbids a user to change their own power level, or someone else's, to a higher value than their current one. As per https://spec.matrix.org/unstable/rooms/v1/#authorization-rules:

10. If type is m.room.power_levels:
  [...]
  4. For each entry being added, changed or removed in both the events and users keys:
    1. If the current value is higher than the sender’s current power level, reject.
    2. If the new value is higher than the sender’s current power level, reject.

This means that from a Matrix perspective the room is effectively broken, because even if the power level event allows anyone to send a new m.room.power_levels event, whoever sends it won't be able to change its content. This means unfreezing can't work.

This means the room freezing feature needs to be changed in the following way:

  • a new custom state event, io.element.room.frozen, is defined. Its content is {"frozen": [...]}, with frozen a boolean indicating whether the room is frozen, defaulting to false.
  • freezing a room updates the power levels by only changing the user_default key to 100 (rather than sending a brand new event with every permission up'd to 100), and removing any user whose PL isn't specifically set to 100 in the users object (in order to not prevent them from being able to unfreeze and take over the room
  • freezing a room also changes the value for io.element.room.frozen's frozen key in the room's state, setting it to true
  • the RoomAccessRules module forbids new events (except for leaving or unfreezing the room - see below for how the latter is defined) if frozen is true
  • new client UI changes are introduced, triggering if frozen is true in the room's state:
    • showing the user they can't send new messages in the room
    • users aren't showed as Admin in the room list - setting user_default to 100 will have the side-effect of showing every user as admin in the room list, and I think clients should hide that to avoid any confusion around the current state of the room.

Additionally, freezing the room would set its join rules to invite.

This would then allow unfreezing a room by sending a new power level events that:

  • gives the user unfreezing the room power level 100
  • reset the user_default key back to 0

Here's how this PR implements it:

  • when the last admin leaves the room, the RoomAccessRules module sends a io.element.room.frozen event; this in turns triggers a power levels update to be sent
  • if a user sends a io.element.room.frozen event manually, the module updates the power levels event accordingly
  • when a room is frozen any event is forbidden, except for events that allow someone to leave the room, or for events that relate to the freezing/unfreezing of the room

Future work (out of scope for this PR) include:

  • promoting the user(s) with the highest PL to admin once the last admin leaves the room - and only freezing once all other users in the room have the default PL
  • splitting this work out in a new module (which would require migrating the ThirdPartyEventRules interface to the new generic modules system first, since we'd then need to run two modules with this interface for synapse-dinsic)

@babolivier
Copy link
Contributor Author

Failing CI is blocked on #99

@babolivier babolivier requested a review from a team June 24, 2021 15:51
@babolivier babolivier marked this pull request as ready for review June 24, 2021 15:51
@richvdh
Copy link
Member

richvdh commented Jul 5, 2021

freezing a room updates the power levels by only changing the user_default key to 100

so every user gets a PL of 100? this seems a curious choice.

@babolivier
Copy link
Contributor Author

so every user gets a PL of 100? this seems a curious choice.

It seems like the only way to let someone name themselves as the new admin of the room (in the process of unfreezing it)

synapse/handlers/message.py Show resolved Hide resolved
synapse/third_party_rules/access_rules.py Outdated Show resolved Hide resolved
synapse/third_party_rules/access_rules.py Show resolved Hide resolved
synapse/third_party_rules/access_rules.py Show resolved Hide resolved
synapse/third_party_rules/access_rules.py Outdated Show resolved Hide resolved
synapse/third_party_rules/access_rules.py Outdated Show resolved Hide resolved
@richvdh
Copy link
Member

richvdh commented Jul 5, 2021

so every user gets a PL of 100? this seems a curious choice.

It seems like the only way to let someone name themselves as the new admin of the room (in the process of unfreezing it)

yeah fair enough. It just feels a bit surprising that anyone can now come in and take over the room. But maybe that is the intention?

@babolivier
Copy link
Contributor Author

so every user gets a PL of 100? this seems a curious choice.

It seems like the only way to let someone name themselves as the new admin of the room (in the process of unfreezing it)

yeah fair enough. It just feels a bit surprising that anyone can now come in and take over the room. But maybe that is the intention?

Yep exactly (see https://github.com/matrix-org/matrix-dinsic/issues/703 which states "any existing member can take the administration of the room")

babolivier added a commit to matrix-org/synapse that referenced this pull request Jul 8, 2021
…artyEventRules` module (#10316)

Because modules might send extra state events when processing an event (e.g. matrix-org/synapse-dinsic#100), and in some cases these extra events might get dropped if we don't recalculate the initial event's auth.
Otherwise we end up poisoning the event cache since we're getting a
reference when assigning (and so we're modifying the same content that's
in the cache for that event).
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

still a few changes needed from the previous review.

synapse/third_party_rules/access_rules.py Outdated Show resolved Hide resolved
synapse/third_party_rules/access_rules.py Outdated Show resolved Hide resolved
babolivier and others added 2 commits July 16, 2021 15:49
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm!

@babolivier babolivier merged commit 1a1a83a into dinsic Jul 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants