Skip check_event_allowed callback for room creation events

[razvp]

Hello,

I'm writing on behalf of the Citadel product developed by ERCOM.

We are using the third party callback check_event_allowed to implement custom functionality and observed that events generated as part of room creation also trigger this callback.

A config parameter skip_check_event_allowed_for_room_creation would be useful. It should default to False, but if set to True it would make Synapse bypass the callback for the events generated by room creation.

If we contribute the code, is this something you'd be interested in merging?

[anoadragon453]

@razvp Are you able to expand upon your use case?

To implement this, I see the following options:

• Add a homeserver configuration option, as you suggested.
• Add a new module callback with an additional boolean argument that states whether the event was created as part of creating the room.

Both are a bit heavy, so I'm first curious whether we can solve your use case in a different way.

Also for your use case, should state events listed in the initial_state field as part of the createRoom request be marked as room creation events as well?

[razvp]

@razvp Are you able to expand upon your use case?

Some examples of use cases: block name change for specific types of rooms (even by room admins), don't allow inviting at all for specific types of rooms. We set the "type" of room in content of the m.room.create event (this callback conveniently provides the state_events from which we get the "type" of the room).

Also for your use case, should state events listed in the initial_state field as part of the createRoom request be marked as room creation events as well?

Yes, these should be considered part of room creation.

To implement this, I see the following options:

• Add a homeserver configuration option, as you suggested.
• Add a new module callback with an additional boolean argument that states whether the event was created as part of creating the room.

The implementation I had in mind is adding a skip_check_event_allowed boolean argument to the EventCreationHandler.create_new_client_event() method and use a default of False for it, so we modify just the call sites we are interested in. Here is a draft of the implementation which also takes into consideration initial_state, invite and invite_3pid fields of the createRoom payload and considers them part of room creation events. What do you think about this solution?

[anoadragon453]

A couple thoughts:

• I'd prefer to call the boolean part_of_room_creation rather than skip_check_event_allowed, as we may want to perform logic unrelated to the check_event_allowed method when an event is or isn't part of room creation.
• check_event_allowed is marked as experimental, so we technically can update the signature without considering it a breaking change.
  • However, it's been experimental for so long and making a backwards-compatible change is so cheap that we may as well.

The easiest option may be to add an additional boolean to check_event_allowed, and when calling the method Synapse will check whether a part_of_room_creation boolean exists on the module's method signature. If so, it will call the method with that bool, otherwise it will fall back to only calling check_event_allowed(event, state_events). This will be backwards-compatible with old modules, and not require an entirely new method, or config option.

What do you think?

[ASR-ASU]

Hello,

I'm writing on behalf of the Citadel product developed by ERCOM as well.

Isn't the part_of_room_creation name too restrictive? For now, we use this flag to indicate that the event was created by the room creation process. Using this name could cause misunderstanding if we need to set this flag to True for other cases (e.g. for an admin operation).

[anoadragon453]

For excluding checks on events sent by server admins, you could check the sender field of the event provided to check_event_allowed along with the ModuleApi's is_user_admin method.

part_of_room_creation is just one piece of data we're passing to the check_event_allowed method. There's no reason we couldn't add more data in the future as new use cases arise!

[razvp]

The easiest option may be to add an additional boolean to check_event_allowed, and when calling the method Synapse will check whether a part_of_room_creation boolean exists on the module's method signature. If so, it will call the method with that bool, otherwise it will fall back to only calling check_event_allowed(event, state_events). This will be backwards-compatible with old modules, and not require an entirely new method, or config option.

What do you think?

@anoadragon453 I think it's a good solution, as indeed part_of_room_creation might be useful in other contexts.

ASR-ASU made a good point regarding the admin operations. Server Admins can participate to chat through /_matrix/client/ endpoints, but can also generate events through /_synapse/admin/ endpoints. In check_event_allowed we need to know this separation.

What do you think about adding another bool from_admin_endpoint?

[anoadragon453]

@razvp to be clear, checking that the sender of the event is a homeserver admin in your module isn't enough? You need to distinguish whether the event was sent through the /matrix/client or the /_synapse/admin endpoint?

The server policies I'm familiar with typically only check whether an action was perform by an admin, not whether the endpoint they used is only accessible by admins.

For example, you can check whether an admin sent the event by doing the following:

async def check_event_allowed(
    self,
    event: "synapse.events.EventBase",
    state_events: "synapse.types.StateMap",
    part_of_room_creation: bool,
) -> Tuple[bool, Optional[dict]]:
    # Allow this event if it is part of the initial set sent during room creation,
    # even if the room was created by someone other than a homeserver admin.
    if part_of_room_creation:
        return True

    # Allow this event if it was sent by a homeserver admin.
    event_sender_user_id = event.user_id
    if self._module_api.is_user_admin(event_sender_user_id):
        return True

    # ... additional processing...

[ASR-ASU]

@anoadragon453 It might be interesting, for administrative operations, to give a fleet administrator the possibility to carry out certain actions, bypassing some rules that normally apply. What do you think?

[anoadragon453]

@ASR-ASU I'm not familiar with what a "fleet administrator" is. Is something a user who doesn't have server admin permissions, but still has permissions greater than other users?

Typically this would be modelled by a certain power level in the Matrix room, i.e. "fleet admins have power level 100" in the room. Then, you'd configure the power level requirements of each event type in the room accordingly. And your module would be written to assume anyone with power level 100 is a "fleet administrator". Does that make sense?