From 38314caf284c13ba6916eb4801d320f352c95caa Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 2 Sep 2021 11:52:19 +0100 Subject: [PATCH 1/5] Move `kick_guest_users` to RoomMemberHandler --- synapse/handlers/_base.py | 50 ++------------------------- synapse/handlers/room_member.py | 60 ++++++++++++++++++++++++++++++--- 2 files changed, 58 insertions(+), 52 deletions(-) diff --git a/synapse/handlers/_base.py b/synapse/handlers/_base.py index 6a05a6530599..c602b667686c 100644 --- a/synapse/handlers/_base.py +++ b/synapse/handlers/_base.py @@ -15,10 +15,8 @@ import logging from typing import TYPE_CHECKING, Optional -import synapse.types -from synapse.api.constants import EventTypes, Membership +from synapse.api.constants import EventTypes from synapse.api.ratelimiting import Ratelimiter -from synapse.types import UserID if TYPE_CHECKING: from synapse.server import HomeServer @@ -135,48 +133,4 @@ async def maybe_kick_guest_users(self, event, context=None): current_state = list(current_state_map.values()) logger.info("maybe_kick_guest_users %r", current_state) - await self.kick_guest_users(current_state) - - async def kick_guest_users(self, current_state): - for member_event in current_state: - try: - if member_event.type != EventTypes.Member: - continue - - target_user = UserID.from_string(member_event.state_key) - if not self.hs.is_mine(target_user): - continue - - if member_event.content["membership"] not in { - Membership.JOIN, - Membership.INVITE, - }: - continue - - if ( - "kind" not in member_event.content - or member_event.content["kind"] != "guest" - ): - continue - - # We make the user choose to leave, rather than have the - # event-sender kick them. This is partially because we don't - # need to worry about power levels, and partially because guest - # users are a concept which doesn't hugely work over federation, - # and having homeservers have their own users leave keeps more - # of that decision-making and control local to the guest-having - # homeserver. - requester = synapse.types.create_requester( - target_user, is_guest=True, authenticated_entity=self.server_name - ) - handler = self.hs.get_room_member_handler() - await handler.update_membership( - requester, - target_user, - member_event.room_id, - "leave", - ratelimit=False, - require_consent=False, - ) - except Exception as e: - logger.exception("Error kicking guest user: %s" % (e,)) + await self.hs.get_room_member_handler().kick_guest_users(current_state) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 401b84aad1eb..b6f7402b7390 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -44,6 +44,7 @@ RoomID, StateMap, UserID, + create_requester, get_domain_from_id, ) from synapse.util.async_helpers import Linearizer @@ -70,6 +71,7 @@ def __init__(self, hs: "HomeServer"): self.auth = hs.get_auth() self.state_handler = hs.get_state_handler() self.config = hs.config + self._server_name = hs.hostname self.federation_handler = hs.get_federation_handler() self.directory_handler = hs.get_directory_handler() @@ -115,9 +117,8 @@ def __init__(self, hs: "HomeServer"): burst_count=hs.config.ratelimiting.rc_invites_per_user.burst_count, ) - # This is only used to get at ratelimit function, and - # maybe_kick_guest_users. It's fine there are multiple of these as - # it doesn't store state. + # This is only used to get at the ratelimit function. It's fine there are + # multiple of these as it doesn't store state. self.base_handler = BaseHandler(hs) @abc.abstractmethod @@ -1099,6 +1100,58 @@ async def _can_guest_join(self, current_state_ids: StateMap[str]) -> bool: and guest_access.content["guest_access"] == "can_join" ) + async def kick_guest_users(self, current_state: Iterable[EventBase]) -> None: + """Kick any local guest users from the room. + + This is called when the room state changes from guests allowed to not-allowed. + + Params: + current_state: the current state of the room. We will iterate this to look + for guest users to kick. + """ + for member_event in current_state: + try: + if member_event.type != EventTypes.Member: + continue + + if not self.hs.is_mine_id(member_event.state_key): + continue + + if member_event.content["membership"] not in { + Membership.JOIN, + Membership.INVITE, + }: + continue + + if ( + "kind" not in member_event.content + or member_event.content["kind"] != "guest" + ): + continue + + # We make the user choose to leave, rather than have the + # event-sender kick them. This is partially because we don't + # need to worry about power levels, and partially because guest + # users are a concept which doesn't hugely work over federation, + # and having homeservers have their own users leave keeps more + # of that decision-making and control local to the guest-having + # homeserver. + target_user = UserID.from_string(member_event.state_key) + requester = create_requester( + target_user, is_guest=True, authenticated_entity=self._server_name + ) + handler = self.hs.get_room_member_handler() + await handler.update_membership( + requester, + target_user, + member_event.room_id, + "leave", + ratelimit=False, + require_consent=False, + ) + except Exception as e: + logger.exception("Error kicking guest user: %s" % (e,)) + async def lookup_room_alias( self, room_alias: RoomAlias ) -> Tuple[RoomID, List[str]]: @@ -1352,7 +1405,6 @@ def __init__(self, hs: "HomeServer"): self.distributor = hs.get_distributor() self.distributor.declare("user_left_room") - self._server_name = hs.hostname async def _is_remote_room_too_complex( self, room_id: str, remote_room_hosts: List[str] From a9cc6d7013ab9a8b57a70e9a7d05c0a8c223c3b3 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 2 Sep 2021 14:56:40 +0100 Subject: [PATCH 2/5] Move `_maybe_kick_guest_users` out of `BaseHandler` ... and into both `FederationEventHandler` and `EventCreationHandler`. I figure having two copies isn't so bad given much of the body is different. --- synapse/handlers/_base.py | 22 ---------------------- synapse/handlers/federation_event.py | 16 +++++++++++++--- synapse/handlers/message.py | 22 ++++++++++++++++++++-- 3 files changed, 33 insertions(+), 27 deletions(-) diff --git a/synapse/handlers/_base.py b/synapse/handlers/_base.py index c602b667686c..955cfa220724 100644 --- a/synapse/handlers/_base.py +++ b/synapse/handlers/_base.py @@ -15,7 +15,6 @@ import logging from typing import TYPE_CHECKING, Optional -from synapse.api.constants import EventTypes from synapse.api.ratelimiting import Ratelimiter if TYPE_CHECKING: @@ -113,24 +112,3 @@ async def ratelimit(self, requester, update=True, is_admin_redaction=False): burst_count=burst_count, update=update, ) - - async def maybe_kick_guest_users(self, event, context=None): - # Technically this function invalidates current_state by changing it. - # Hopefully this isn't that important to the caller. - if event.type == EventTypes.GuestAccess: - guest_access = event.content.get("guest_access", "forbidden") - if guest_access != "can_join": - if context: - current_state_ids = await context.get_current_state_ids() - current_state_dict = await self.store.get_events( - list(current_state_ids.values()) - ) - current_state = list(current_state_dict.values()) - else: - current_state_map = await self.state_handler.get_current_state( - event.room_id - ) - current_state = list(current_state_map.values()) - - logger.info("maybe_kick_guest_users %r", current_state) - await self.hs.get_room_member_handler().kick_guest_users(current_state) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 9f055f00cff3..013d761be0fb 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -1321,9 +1321,7 @@ async def _check_event_auth( if not context.rejected: await self._check_for_soft_fail(event, state, backfilled, origin=origin) - - if event.type == EventTypes.GuestAccess and not context.rejected: - await self.maybe_kick_guest_users(event) + await self._maybe_kick_guest_users(event) # If we are going to send this event over federation we precaclculate # the joined hosts. @@ -1334,6 +1332,18 @@ async def _check_event_auth( return context + async def _maybe_kick_guest_users(self, event: EventBase) -> None: + if event.type != EventTypes.GuestAccess: + return + + guest_access = event.content.get("guest_access") + if guest_access == "can_join": + return + + current_state_map = await self.state_handler.get_current_state(event.room_id) + current_state = list(current_state_map.values()) + await self.hs.get_room_member_handler().kick_guest_users(current_state) + async def _check_for_soft_fail( self, event: EventBase, diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 101a29c6d3cc..79b2ef0fb70e 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -426,7 +426,7 @@ def __init__(self, hs: "HomeServer"): self.send_event = ReplicationSendEventRestServlet.make_client(hs) - # This is only used to get at ratelimit function, and maybe_kick_guest_users + # This is only used to get at ratelimit function self.base_handler = BaseHandler(hs) # We arbitrarily limit concurrent event creation for a room to 5. @@ -1306,7 +1306,7 @@ async def persist_and_notify_client_event( requester, is_admin_redaction=is_admin_redaction ) - await self.base_handler.maybe_kick_guest_users(event, context) + await self._maybe_kick_guest_users(event, context) if event.type == EventTypes.CanonicalAlias: # Validate a newly added alias or newly added alt_aliases. @@ -1471,6 +1471,24 @@ def _notify(): return event + async def _maybe_kick_guest_users( + self, event: EventBase, context: EventContext + ) -> None: + if event.type != EventTypes.GuestAccess: + return + + guest_access = event.content.get("guest_access") + if guest_access == "can_join": + return + + current_state_ids = await context.get_current_state_ids() + current_state_dict = await self.store.get_events( + list(current_state_ids.values()) + ) + current_state = list(current_state_dict.values()) + logger.info("maybe_kick_guest_users %r", current_state) + await self.hs.get_room_member_handler().kick_guest_users(current_state) + async def _bump_active_time(self, user: UserID) -> None: try: presence = self.hs.get_presence_handler() From 75aca7319fdb7acb400b8e0d954c8529f9755365 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 2 Sep 2021 15:01:49 +0100 Subject: [PATCH 3/5] factor out some constants for `guest_access` events --- synapse/api/constants.py | 9 +++++++++ synapse/handlers/federation_event.py | 5 +++-- synapse/handlers/message.py | 5 +++-- synapse/handlers/room.py | 5 ++++- synapse/handlers/room_list.py | 12 +++++++++--- synapse/handlers/room_member.py | 5 +++-- synapse/handlers/stats.py | 6 ++++-- 7 files changed, 35 insertions(+), 12 deletions(-) diff --git a/synapse/api/constants.py b/synapse/api/constants.py index 5e34eb7e13e9..5f0f34119b06 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -201,6 +201,9 @@ class EventContentFields: # The creator of the room, as used in `m.room.create` events. ROOM_CREATOR = "creator" + # Used in m.room.guest_access events. + GUEST_ACCESS = "guest_access" + # Used on normal messages to indicate they were historically imported after the fact MSC2716_HISTORICAL = "org.matrix.msc2716.historical" # For "insertion" events to indicate what the next chunk ID should be in @@ -235,5 +238,11 @@ class HistoryVisibility: WORLD_READABLE = "world_readable" +class GuestAccess: + CAN_JOIN = "can_join" + # anything that is not "can_join" is considered "forbidden", but for completeness: + FORBIDDEN = "forbidden" + + class ReadReceiptEventFields: MSC2285_HIDDEN = "org.matrix.msc2285.hidden" diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 013d761be0fb..a8e3568e707f 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -36,6 +36,7 @@ from synapse.api.constants import ( EventContentFields, EventTypes, + GuestAccess, Membership, RejectedReason, RoomEncryptionAlgorithms, @@ -1336,8 +1337,8 @@ async def _maybe_kick_guest_users(self, event: EventBase) -> None: if event.type != EventTypes.GuestAccess: return - guest_access = event.content.get("guest_access") - if guest_access == "can_join": + guest_access = event.content.get(EventContentFields.GUEST_ACCESS) + if guest_access == GuestAccess.CAN_JOIN: return current_state_map = await self.state_handler.get_current_state(event.room_id) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 79b2ef0fb70e..2d95a07d4382 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -27,6 +27,7 @@ from synapse.api.constants import ( EventContentFields, EventTypes, + GuestAccess, Membership, RelationTypes, UserTypes, @@ -1477,8 +1478,8 @@ async def _maybe_kick_guest_users( if event.type != EventTypes.GuestAccess: return - guest_access = event.content.get("guest_access") - if guest_access == "can_join": + guest_access = event.content.get(EventContentFields.GUEST_ACCESS) + if guest_access == GuestAccess.CAN_JOIN: return current_state_ids = await context.get_current_state_ids() diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index b33fe09f77eb..622c5ed5e39d 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -25,7 +25,9 @@ from typing import TYPE_CHECKING, Any, Awaitable, Dict, List, Optional, Tuple from synapse.api.constants import ( + EventContentFields, EventTypes, + GuestAccess, HistoryVisibility, JoinRules, Membership, @@ -988,7 +990,8 @@ async def send(etype: str, content: JsonDict, **kwargs) -> int: if config["guest_can_join"]: if (EventTypes.GuestAccess, "") not in initial_state: last_sent_stream_id = await send( - etype=EventTypes.GuestAccess, content={"guest_access": "can_join"} + etype=EventTypes.GuestAccess, + content={EventContentFields.GUEST_ACCESS: GuestAccess.CAN_JOIN}, ) for (etype, state_key), content in initial_state.items(): diff --git a/synapse/handlers/room_list.py b/synapse/handlers/room_list.py index 6d433fad41b0..92bb75c84824 100644 --- a/synapse/handlers/room_list.py +++ b/synapse/handlers/room_list.py @@ -19,7 +19,13 @@ import msgpack from unpaddedbase64 import decode_base64, encode_base64 -from synapse.api.constants import EventTypes, HistoryVisibility, JoinRules +from synapse.api.constants import ( + EventContentFields, + EventTypes, + GuestAccess, + HistoryVisibility, + JoinRules, +) from synapse.api.errors import ( Codes, HttpResponseException, @@ -336,8 +342,8 @@ async def generate_room_entry( guest_event = current_state.get((EventTypes.GuestAccess, "")) guest = None if guest_event: - guest = guest_event.content.get("guest_access", None) - result["guest_can_join"] = guest == "can_join" + guest = guest_event.content.get(EventContentFields.GUEST_ACCESS) + result["guest_can_join"] = guest == GuestAccess.CAN_JOIN avatar_event = current_state.get(("m.room.avatar", "")) if avatar_event: diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index b6f7402b7390..4390201641ef 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -23,6 +23,7 @@ AccountDataTypes, EventContentFields, EventTypes, + GuestAccess, Membership, ) from synapse.api.errors import ( @@ -1096,8 +1097,8 @@ async def _can_guest_join(self, current_state_ids: StateMap[str]) -> bool: return bool( guest_access and guest_access.content - and "guest_access" in guest_access.content - and guest_access.content["guest_access"] == "can_join" + and guest_access.content.get(EventContentFields.GUEST_ACCESS) + == GuestAccess.CAN_JOIN ) async def kick_guest_users(self, current_state: Iterable[EventBase]) -> None: diff --git a/synapse/handlers/stats.py b/synapse/handlers/stats.py index 3fd89af2a4e2..3a4c41c9ff52 100644 --- a/synapse/handlers/stats.py +++ b/synapse/handlers/stats.py @@ -18,7 +18,7 @@ from typing_extensions import Counter as CounterType -from synapse.api.constants import EventTypes, Membership +from synapse.api.constants import EventContentFields, EventTypes, Membership from synapse.metrics import event_processing_positions from synapse.metrics.background_process_metrics import run_as_background_process from synapse.types import JsonDict @@ -273,7 +273,9 @@ async def _handle_deltas( elif typ == EventTypes.CanonicalAlias: room_state["canonical_alias"] = event_content.get("alias") elif typ == EventTypes.GuestAccess: - room_state["guest_access"] = event_content.get("guest_access") + room_state["guest_access"] = event_content.get( + EventContentFields.GUEST_ACCESS + ) for room_id, state in room_to_state_updates.items(): logger.debug("Updating room_stats_state for %s: %s", room_id, state) From a8f4b40a94fd34709fa0df34fa266f3978cb18e1 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 2 Sep 2021 15:03:18 +0100 Subject: [PATCH 4/5] changelog --- changelog.d/10744.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10744.misc diff --git a/changelog.d/10744.misc b/changelog.d/10744.misc new file mode 100644 index 000000000000..f0f789ce3c02 --- /dev/null +++ b/changelog.d/10744.misc @@ -0,0 +1 @@ +Move `kick_guest_users` into `RoomMemberHandler`. From f687b2284e97cd3e82f60ec968bba6f7512adf12 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 3 Sep 2021 10:29:40 +0100 Subject: [PATCH 5/5] pacify mypy --- synapse/handlers/message.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 2d95a07d4382..9b2b3d94d9ca 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -1483,6 +1483,10 @@ async def _maybe_kick_guest_users( return current_state_ids = await context.get_current_state_ids() + + # since this is a client-generated event, it cannot be an outlier and we must + # therefore have the state ids. + assert current_state_ids is not None current_state_dict = await self.store.get_events( list(current_state_ids.values()) )