From ceec7f77d4255025a7481c17cf562495682a645e Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 25 Jun 2021 01:09:25 -0500 Subject: [PATCH 01/11] Add base starting insertion point when no chunk ID is provided This is so we can have the marker event point to this initial insertion event and be able to traverse the events in the first chunk. --- synapse/rest/client/v1/room.py | 76 +++++++++++++++++++++++++--------- 1 file changed, 56 insertions(+), 20 deletions(-) diff --git a/synapse/rest/client/v1/room.py b/synapse/rest/client/v1/room.py index 92ebe838fd84..25af9cd4292d 100644 --- a/synapse/rest/client/v1/room.py +++ b/synapse/rest/client/v1/room.py @@ -349,6 +349,30 @@ async def inherit_depth_from_prev_ids(self, prev_event_ids) -> int: return depth + def _create_insertion_event_dict(self, sender: str, origin_server_ts: int): + """ + Creates an event dict for an "insertion" event with the proper fields + and a random chunk ID. + Args: + sender: The event author MXID + origin_server_ts: Timestamp when the event was sent + Returns: + Tuple of event ID and stream ordering position + """ + + next_chunk_id = random_string(64) + insertion_event = { + "type": EventTypes.MSC2716_INSERTION, + "sender": sender, + "content": { + EventContentFields.MSC2716_NEXT_CHUNK_ID: next_chunk_id, + EventContentFields.MSC2716_HISTORICAL: True, + }, + "origin_server_ts": origin_server_ts, + } + + return insertion_event + async def on_POST(self, request, room_id): requester = await self.auth.get_user_by_req(request, allow_guest=False) @@ -449,30 +473,40 @@ async def on_POST(self, request, room_id): events_to_create = body["events"] - # If provided, connect the chunk to the last insertion point - # The chunk ID passed in comes from the chunk_id in the - # "insertion" event from the previous chunk. + # Figure out which chunk to connect to. If they passed in + # chunk_id_from_query let's use it. The chunk ID passed in comes + # from the chunk_id in the "insertion" event from the previous chunk. + last_event_in_chunk = events_to_create[-1] + chunk_id_to_connect_to = chunk_id_from_query if chunk_id_from_query: - last_event_in_chunk = events_to_create[-1] - last_event_in_chunk["content"][ - EventContentFields.MSC2716_CHUNK_ID - ] = chunk_id_from_query + # TODO: Verify the chunk_id_from_query corresponds to an insertion event + pass + # Otherwise, create an insertion event to be based off of and connect + # to as a starting point. + else: + base_insertion_event = self._create_insertion_event_dict( + sender=requester.user.to_string(), + origin_server_ts=last_event_in_chunk["origin_server_ts"], + ) + events_to_create.append(base_insertion_event) + chunk_id_to_connect_to = base_insertion_event["content"][ + EventContentFields.MSC2716_NEXT_CHUNK_ID + ] + + # Connect this current chunk to the insertion event from the previous chunk + last_event_in_chunk["content"][ + EventContentFields.MSC2716_CHUNK_ID + ] = chunk_id_to_connect_to - # Add an "insertion" event to the start of each chunk (next to the oldest + # Add an "insertion" event to the start of each chunk (next to the oldest-in-time # event in the chunk) so the next chunk can be connected to this one. - next_chunk_id = random_string(64) - insertion_event = { - "type": EventTypes.MSC2716_INSERTION, - "sender": requester.user.to_string(), - "content": { - EventContentFields.MSC2716_NEXT_CHUNK_ID: next_chunk_id, - EventContentFields.MSC2716_HISTORICAL: True, - }, + insertion_event = self._create_insertion_event_dict( + sender=requester.user.to_string(), # Since the insertion event is put at the start of the chunk, - # where the oldest event is, copy the origin_server_ts from + # where the oldest-in-time event is, copy the origin_server_ts from # the first event we're inserting - "origin_server_ts": events_to_create[0]["origin_server_ts"], - } + origin_server_ts=events_to_create[0]["origin_server_ts"], + ) # Prepend the insertion event to the start of the chunk events_to_create = [insertion_event] + events_to_create @@ -536,7 +570,9 @@ async def on_POST(self, request, room_id): return 200, { "state_events": auth_event_ids, "events": event_ids, - "next_chunk_id": next_chunk_id, + "next_chunk_id": insertion_event["content"][ + EventContentFields.MSC2716_NEXT_CHUNK_ID + ], } def on_GET(self, request, room_id): From 308366090434852c73170fc13ecea9693571bb46 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 25 Jun 2021 01:26:17 -0500 Subject: [PATCH 02/11] Add changelog --- changelog.d/10250.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10250.bugfix diff --git a/changelog.d/10250.bugfix b/changelog.d/10250.bugfix new file mode 100644 index 000000000000..a8107dafb230 --- /dev/null +++ b/changelog.d/10250.bugfix @@ -0,0 +1 @@ +Add base starting insertion event when no chunk ID is specified in the historical batch send API. From 9d60613dd73cf8f7d1935e1a92e6003637f66547 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 7 Jul 2021 16:22:30 -0500 Subject: [PATCH 03/11] Add more better comments --- synapse/rest/client/v1/room.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/synapse/rest/client/v1/room.py b/synapse/rest/client/v1/room.py index 25af9cd4292d..fabc40fbd420 100644 --- a/synapse/rest/client/v1/room.py +++ b/synapse/rest/client/v1/room.py @@ -350,17 +350,18 @@ async def inherit_depth_from_prev_ids(self, prev_event_ids) -> int: return depth def _create_insertion_event_dict(self, sender: str, origin_server_ts: int): - """ - Creates an event dict for an "insertion" event with the proper fields + """Creates an event dict for an "insertion" event with the proper fields and a random chunk ID. + Args: sender: The event author MXID origin_server_ts: Timestamp when the event was sent + Returns: Tuple of event ID and stream ordering position """ - next_chunk_id = random_string(64) + next_chunk_id = random_string(8) insertion_event = { "type": EventTypes.MSC2716_INSERTION, "sender": sender, @@ -481,8 +482,13 @@ async def on_POST(self, request, room_id): if chunk_id_from_query: # TODO: Verify the chunk_id_from_query corresponds to an insertion event pass - # Otherwise, create an insertion event to be based off of and connect - # to as a starting point. + # Otherwise, create an insertion event to act as a starting point. + # + # We don't always have an insertion event to start hanging more history + # off of (ideally there would be one in the main DAG, but that's not the + # case if we're wanting to add history to e.g. existing rooms without + # an insertion event), in which case we just create a new insertion event + # that can then get pointed to by a "marker" event later. else: base_insertion_event = self._create_insertion_event_dict( sender=requester.user.to_string(), From 14cd25d0d589edda09e73d6e9cca7aef63237384 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 29 Jun 2021 17:03:54 -0500 Subject: [PATCH 04/11] Fix messages from multiple senders in historical chunk Follow-up to https://github.com/matrix-org/synapse/pull/9247 Part of MSC2716: https://github.com/matrix-org/matrix-doc/pull/2716 --- Previously, Synapse would throw a 403, `Cannot force another user to join.`, because we were trying to use `?user_id` from a single virtual user which did not match with messages from other users in the chunk. --- synapse/event_auth.py | 10 ++++++++++ synapse/rest/client/v1/room.py | 27 +++++++++++++++++++++------ 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/synapse/event_auth.py b/synapse/event_auth.py index 89bcf8151589..55a5d3c20e65 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -343,7 +343,17 @@ def _is_membership_change_allowed( # * They are accepting a previously sent invitation. # * They are already joined (it's a NOOP). # * The room is public or restricted. + logger.info( + "check join aewffaewafewf %s %s", + event.user_id, + target_user_id, + ) if event.user_id != target_user_id: + logger.error( + "Cannot force another user to join aewffaewafewf %s %s", + event.user_id, + target_user_id, + ) raise AuthError(403, "Cannot force another user to join.") elif target_banned: raise AuthError(403, "You are banned from this room") diff --git a/synapse/rest/client/v1/room.py b/synapse/rest/client/v1/room.py index fabc40fbd420..2fa29f23037f 100644 --- a/synapse/rest/client/v1/room.py +++ b/synapse/rest/client/v1/room.py @@ -14,6 +14,7 @@ # limitations under the License. """ This module contains REST servlets to do with rooms: /rooms/ """ +import copy import logging import re from typing import TYPE_CHECKING, Dict, List, Optional, Tuple @@ -47,6 +48,7 @@ from synapse.streams.config import PaginationConfig from synapse.types import ( JsonDict, + Requester, RoomAlias, RoomID, StreamToken, @@ -309,7 +311,14 @@ def __init__(self, hs): self.room_member_handler = hs.get_room_member_handler() self.auth = hs.get_auth() - async def inherit_depth_from_prev_ids(self, prev_event_ids) -> int: + def _copy_requester_and_override_user_id(self, requester, new_user_id): + serialized_requester = requester.serialize() + serialized_requester["user_id"] = new_user_id + new_requester = Requester.deserialize(self.store, serialized_requester) + + return new_requester + + async def _inherit_depth_from_prev_ids(self, prev_event_ids) -> int: ( most_recent_prev_event_id, most_recent_prev_event_depth, @@ -439,7 +448,9 @@ async def on_POST(self, request, room_id): if event_dict["type"] == EventTypes.Member: membership = event_dict["content"].get("membership", None) event_id, _ = await self.room_member_handler.update_membership( - requester, + self._copy_requester_and_override_user_id( + requester, state_event["sender"] + ), target=UserID.from_string(event_dict["state_key"]), room_id=room_id, action=membership, @@ -459,7 +470,9 @@ async def on_POST(self, request, room_id): event, _, ) = await self.event_creation_handler.create_and_send_nonmember_event( - requester, + self._copy_requester_and_override_user_id( + requester, state_event["sender"] + ), event_dict, outlier=True, prev_event_ids=[fake_prev_event_id], @@ -516,7 +529,9 @@ async def on_POST(self, request, room_id): # Prepend the insertion event to the start of the chunk events_to_create = [insertion_event] + events_to_create - inherited_depth = await self.inherit_depth_from_prev_ids(prev_events_from_query) + inherited_depth = await self._inherit_depth_from_prev_ids( + prev_events_from_query + ) event_ids = [] prev_event_ids = prev_events_from_query @@ -538,7 +553,7 @@ async def on_POST(self, request, room_id): } event, context = await self.event_creation_handler.create_event( - requester, + self._copy_requester_and_override_user_id(requester, ev["sender"]), event_dict, prev_event_ids=event_dict.get("prev_events"), auth_event_ids=auth_event_ids, @@ -568,7 +583,7 @@ async def on_POST(self, request, room_id): # where topological_ordering is just depth. for (event, context) in reversed(events_to_persist): ev = await self.event_creation_handler.handle_new_client_event( - requester=requester, + self._copy_requester_and_override_user_id(requester, event["sender"]), event=event, context=context, ) From 254a2f3d94b62aaa0655ef1cfa0ce78671e71de8 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 29 Jun 2021 17:14:40 -0500 Subject: [PATCH 05/11] Remove debug lines --- synapse/event_auth.py | 10 ---------- synapse/rest/client/v1/room.py | 1 - 2 files changed, 11 deletions(-) diff --git a/synapse/event_auth.py b/synapse/event_auth.py index 55a5d3c20e65..89bcf8151589 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -343,17 +343,7 @@ def _is_membership_change_allowed( # * They are accepting a previously sent invitation. # * They are already joined (it's a NOOP). # * The room is public or restricted. - logger.info( - "check join aewffaewafewf %s %s", - event.user_id, - target_user_id, - ) if event.user_id != target_user_id: - logger.error( - "Cannot force another user to join aewffaewafewf %s %s", - event.user_id, - target_user_id, - ) raise AuthError(403, "Cannot force another user to join.") elif target_banned: raise AuthError(403, "You are banned from this room") diff --git a/synapse/rest/client/v1/room.py b/synapse/rest/client/v1/room.py index 2fa29f23037f..2c427a67ff9d 100644 --- a/synapse/rest/client/v1/room.py +++ b/synapse/rest/client/v1/room.py @@ -14,7 +14,6 @@ # limitations under the License. """ This module contains REST servlets to do with rooms: /rooms/ """ -import copy import logging import re from typing import TYPE_CHECKING, Dict, List, Optional, Tuple From bb92d8745c4bcdc612a6f2a467c75d84a2dc3109 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 29 Jun 2021 17:18:13 -0500 Subject: [PATCH 06/11] Add changelog --- changelog.d/10276.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10276.bugfix diff --git a/changelog.d/10276.bugfix b/changelog.d/10276.bugfix new file mode 100644 index 000000000000..42adc57ad1c5 --- /dev/null +++ b/changelog.d/10276.bugfix @@ -0,0 +1 @@ +Fix historical batch send endpoint (MSC2716) rejecting batches with messages from multiple senders. From d7eb5381ad86d8f0c4974acfcf6d933a1573467d Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 7 Jul 2021 16:47:16 -0500 Subject: [PATCH 07/11] Make a fake requester with just what we need See https://github.com/matrix-org/synapse/pull/10276#discussion_r660999080 --- synapse/rest/client/v1/room.py | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/synapse/rest/client/v1/room.py b/synapse/rest/client/v1/room.py index 2c427a67ff9d..5b7b7fd0919b 100644 --- a/synapse/rest/client/v1/room.py +++ b/synapse/rest/client/v1/room.py @@ -47,12 +47,12 @@ from synapse.streams.config import PaginationConfig from synapse.types import ( JsonDict, - Requester, RoomAlias, RoomID, StreamToken, ThirdPartyInstanceID, UserID, + create_requester, ) from synapse.util import json_decoder from synapse.util.stringutils import parse_and_validate_server_name, random_string @@ -310,13 +310,6 @@ def __init__(self, hs): self.room_member_handler = hs.get_room_member_handler() self.auth = hs.get_auth() - def _copy_requester_and_override_user_id(self, requester, new_user_id): - serialized_requester = requester.serialize() - serialized_requester["user_id"] = new_user_id - new_requester = Requester.deserialize(self.store, serialized_requester) - - return new_requester - async def _inherit_depth_from_prev_ids(self, prev_event_ids) -> int: ( most_recent_prev_event_id, @@ -447,8 +440,8 @@ async def on_POST(self, request, room_id): if event_dict["type"] == EventTypes.Member: membership = event_dict["content"].get("membership", None) event_id, _ = await self.room_member_handler.update_membership( - self._copy_requester_and_override_user_id( - requester, state_event["sender"] + create_requester( + state_event["sender"], app_service=requester.app_service ), target=UserID.from_string(event_dict["state_key"]), room_id=room_id, @@ -469,8 +462,8 @@ async def on_POST(self, request, room_id): event, _, ) = await self.event_creation_handler.create_and_send_nonmember_event( - self._copy_requester_and_override_user_id( - requester, state_event["sender"] + create_requester( + state_event["sender"], app_service=requester.app_service ), event_dict, outlier=True, @@ -552,7 +545,7 @@ async def on_POST(self, request, room_id): } event, context = await self.event_creation_handler.create_event( - self._copy_requester_and_override_user_id(requester, ev["sender"]), + create_requester(ev["sender"], app_service=requester.app_service), event_dict, prev_event_ids=event_dict.get("prev_events"), auth_event_ids=auth_event_ids, @@ -582,7 +575,7 @@ async def on_POST(self, request, room_id): # where topological_ordering is just depth. for (event, context) in reversed(events_to_persist): ev = await self.event_creation_handler.handle_new_client_event( - self._copy_requester_and_override_user_id(requester, event["sender"]), + create_requester(event["sender"], app_service=requester.app_service), event=event, context=context, ) From 4dfa4ff80a09d2058736be5e13aead71cb253773 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 7 Jul 2021 23:08:39 -0500 Subject: [PATCH 08/11] Make base insertion event float off on its own See https://github.com/matrix-org/synapse/pull/10250#issuecomment-875711889 --- synapse/handlers/message.py | 8 ++++++++ synapse/rest/client/v1/room.py | 36 ++++++++++++++++++++++++++++------ 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 66e40a915d04..61e6f4ef252c 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -518,6 +518,9 @@ async def create_event( outlier: Indicates whether the event is an `outlier`, i.e. if it's from an arbitrary point and floating in the DAG as opposed to being inline with the current DAG. + historical: Indicates whether the message is being inserted + back in time around some existing events. This is used to skip + a few checks and mark the event as backfilled. depth: Override the depth used to order the event in the DAG. Should normally be set to None, which will cause the depth to be calculated based on the prev_events. @@ -772,6 +775,7 @@ async def create_and_send_nonmember_event( txn_id: Optional[str] = None, ignore_shadow_ban: bool = False, outlier: bool = False, + historical: bool = False, depth: Optional[int] = None, ) -> Tuple[EventBase, int]: """ @@ -799,6 +803,9 @@ async def create_and_send_nonmember_event( outlier: Indicates whether the event is an `outlier`, i.e. if it's from an arbitrary point and floating in the DAG as opposed to being inline with the current DAG. + historical: Indicates whether the message is being inserted + back in time around some existing events. This is used to skip + a few checks and mark the event as backfilled. depth: Override the depth used to order the event in the DAG. Should normally be set to None, which will cause the depth to be calculated based on the prev_events. @@ -847,6 +854,7 @@ async def create_and_send_nonmember_event( prev_event_ids=prev_event_ids, auth_event_ids=auth_event_ids, outlier=outlier, + historical=historical, depth=depth, ) diff --git a/synapse/rest/client/v1/room.py b/synapse/rest/client/v1/room.py index fabc40fbd420..9c58e3689ee3 100644 --- a/synapse/rest/client/v1/room.py +++ b/synapse/rest/client/v1/room.py @@ -349,12 +349,15 @@ async def inherit_depth_from_prev_ids(self, prev_event_ids) -> int: return depth - def _create_insertion_event_dict(self, sender: str, origin_server_ts: int): + def _create_insertion_event_dict( + self, sender: str, room_id: str, origin_server_ts: int + ): """Creates an event dict for an "insertion" event with the proper fields and a random chunk ID. Args: sender: The event author MXID + room_id: The room ID that the event belongs to origin_server_ts: Timestamp when the event was sent Returns: @@ -365,6 +368,7 @@ def _create_insertion_event_dict(self, sender: str, origin_server_ts: int): insertion_event = { "type": EventTypes.MSC2716_INSERTION, "sender": sender, + "room_id": room_id, "content": { EventContentFields.MSC2716_NEXT_CHUNK_ID: next_chunk_id, EventContentFields.MSC2716_HISTORICAL: True, @@ -474,11 +478,15 @@ async def on_POST(self, request, room_id): events_to_create = body["events"] + prev_event_ids = prev_events_from_query + inherited_depth = await self.inherit_depth_from_prev_ids(prev_events_from_query) + # Figure out which chunk to connect to. If they passed in # chunk_id_from_query let's use it. The chunk ID passed in comes # from the chunk_id in the "insertion" event from the previous chunk. last_event_in_chunk = events_to_create[-1] chunk_id_to_connect_to = chunk_id_from_query + base_insertion_event = None if chunk_id_from_query: # TODO: Verify the chunk_id_from_query corresponds to an insertion event pass @@ -490,11 +498,25 @@ async def on_POST(self, request, room_id): # an insertion event), in which case we just create a new insertion event # that can then get pointed to by a "marker" event later. else: - base_insertion_event = self._create_insertion_event_dict( + base_insertion_event_dict = self._create_insertion_event_dict( sender=requester.user.to_string(), + room_id=room_id, origin_server_ts=last_event_in_chunk["origin_server_ts"], ) - events_to_create.append(base_insertion_event) + base_insertion_event_dict["prev_events"] = prev_event_ids.copy() + + ( + base_insertion_event, + _, + ) = await self.event_creation_handler.create_and_send_nonmember_event( + requester, + base_insertion_event_dict, + prev_event_ids=base_insertion_event_dict.get("prev_events"), + auth_event_ids=auth_event_ids, + historical=True, + depth=inherited_depth, + ) + chunk_id_to_connect_to = base_insertion_event["content"][ EventContentFields.MSC2716_NEXT_CHUNK_ID ] @@ -508,6 +530,7 @@ async def on_POST(self, request, room_id): # event in the chunk) so the next chunk can be connected to this one. insertion_event = self._create_insertion_event_dict( sender=requester.user.to_string(), + room_id=room_id, # Since the insertion event is put at the start of the chunk, # where the oldest-in-time event is, copy the origin_server_ts from # the first event we're inserting @@ -516,10 +539,7 @@ async def on_POST(self, request, room_id): # Prepend the insertion event to the start of the chunk events_to_create = [insertion_event] + events_to_create - inherited_depth = await self.inherit_depth_from_prev_ids(prev_events_from_query) - event_ids = [] - prev_event_ids = prev_events_from_query events_to_persist = [] for ev in events_to_create: assert_params_in_dict(ev, ["type", "origin_server_ts", "content", "sender"]) @@ -573,6 +593,10 @@ async def on_POST(self, request, room_id): context=context, ) + # Add the base_insertion_event to the bottom of the list we return + if base_insertion_event is not None: + event_ids.append(base_insertion_event.event_id) + return 200, { "state_events": auth_event_ids, "events": event_ids, From dae35d175093bccaf5db5db78004949d71902f2f Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 8 Jul 2021 20:22:57 -0500 Subject: [PATCH 09/11] Validate that the app service can actually control the given user See https://github.com/matrix-org/synapse/pull/10276#issuecomment-876316455 --- synapse/rest/client/v1/room.py | 50 ++++++++++++++++++++++++++++------ 1 file changed, 42 insertions(+), 8 deletions(-) diff --git a/synapse/rest/client/v1/room.py b/synapse/rest/client/v1/room.py index b1de7efac90a..2130ee8b2b2c 100644 --- a/synapse/rest/client/v1/room.py +++ b/synapse/rest/client/v1/room.py @@ -29,6 +29,9 @@ SynapseError, ) from synapse.api.filtering import Filter + + +from synapse.appservice import ApplicationService from synapse.events.utils import format_event_for_client_v2 from synapse.http.servlet import ( RestServlet, @@ -47,6 +50,7 @@ from synapse.streams.config import PaginationConfig from synapse.types import ( JsonDict, + Requester, RoomAlias, RoomID, StreamToken, @@ -379,6 +383,32 @@ def _create_insertion_event_dict( return insertion_event + async def _create_requester_from_app_service( + self, user_id: str, app_service: ApplicationService + ) -> Requester: + """Creates a new requester for the given user_id + and validates that the app service is allowed to control + the given user. + + Args: + user_id: The author MXID that the app service is controlling + app_service: The app service that controls the user + + Returns: + Requester object + """ + + if app_service.sender == user_id: + pass + elif not app_service.is_interested_in_user(user_id): + raise AuthError(403, "Application service cannot masquerade as this user (%s)." % user_id) + elif not (await self.store.get_user_by_id(user_id)): + raise AuthError( + 403, "Application service has not registered this user (%s)" % user_id + ) + + return create_requester(user_id, app_service=app_service) + async def on_POST(self, request, room_id): requester = await self.auth.get_user_by_req(request, allow_guest=False) @@ -444,8 +474,8 @@ async def on_POST(self, request, room_id): if event_dict["type"] == EventTypes.Member: membership = event_dict["content"].get("membership", None) event_id, _ = await self.room_member_handler.update_membership( - create_requester( - state_event["sender"], app_service=requester.app_service + await self._create_requester_from_app_service( + state_event["sender"], requester.app_service ), target=UserID.from_string(event_dict["state_key"]), room_id=room_id, @@ -466,8 +496,8 @@ async def on_POST(self, request, room_id): event, _, ) = await self.event_creation_handler.create_and_send_nonmember_event( - create_requester( - state_event["sender"], app_service=requester.app_service + await self._create_requester_from_app_service( + state_event["sender"], requester.app_service ), event_dict, outlier=True, @@ -516,9 +546,9 @@ async def on_POST(self, request, room_id): base_insertion_event, _, ) = await self.event_creation_handler.create_and_send_nonmember_event( - create_requester( + await self._create_requester_from_app_service( base_insertion_event_dict["sender"], - app_service=requester.app_service, + requester.app_service, ), base_insertion_event_dict, prev_event_ids=base_insertion_event_dict.get("prev_events"), @@ -568,7 +598,9 @@ async def on_POST(self, request, room_id): } event, context = await self.event_creation_handler.create_event( - create_requester(ev["sender"], app_service=requester.app_service), + await self._create_requester_from_app_service( + ev["sender"], requester.app_service + ), event_dict, prev_event_ids=event_dict.get("prev_events"), auth_event_ids=auth_event_ids, @@ -598,7 +630,9 @@ async def on_POST(self, request, room_id): # where topological_ordering is just depth. for (event, context) in reversed(events_to_persist): ev = await self.event_creation_handler.handle_new_client_event( - create_requester(event["sender"], app_service=requester.app_service), + await self._create_requester_from_app_service( + event["sender"], requester.app_service + ), event=event, context=context, ) From 35965752dfd7a488dbe8535ca1c92a30430ebe86 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 8 Jul 2021 20:36:02 -0500 Subject: [PATCH 10/11] Add some better comments on what we're trying to check for --- synapse/rest/client/v1/room.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/synapse/rest/client/v1/room.py b/synapse/rest/client/v1/room.py index a0d6904dd4fa..7ff4ad6bf5c7 100644 --- a/synapse/rest/client/v1/room.py +++ b/synapse/rest/client/v1/room.py @@ -29,8 +29,6 @@ SynapseError, ) from synapse.api.filtering import Filter - - from synapse.appservice import ApplicationService from synapse.events.utils import format_event_for_client_v2 from synapse.http.servlet import ( @@ -398,13 +396,16 @@ async def _create_requester_from_app_service( Requester object """ + # It's ok if the app service is trying to use the sender from their registration if app_service.sender == user_id: pass + # Check to make sure the app service is allowed to control the user elif not app_service.is_interested_in_user(user_id): raise AuthError( 403, "Application service cannot masquerade as this user (%s)." % user_id, ) + # Check to make sure the user is already registered on the homeserver elif not (await self.store.get_user_by_id(user_id)): raise AuthError( 403, "Application service has not registered this user (%s)" % user_id From 21a5d6a0b5801a0b9d6a61380a396a2c9687f17e Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 12 Jul 2021 16:45:06 -0500 Subject: [PATCH 11/11] Share validation logic --- synapse/api/auth.py | 37 ++++++++++++++++++++++++++++++---- synapse/rest/client/v1/room.py | 27 +++++++------------------ 2 files changed, 40 insertions(+), 24 deletions(-) diff --git a/synapse/api/auth.py b/synapse/api/auth.py index 307f5f9a9463..42476a18e504 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -240,6 +240,37 @@ async def get_user_by_req( except KeyError: raise MissingClientTokenError() + async def validate_appservice_can_control_user_id( + self, app_service: ApplicationService, user_id: str + ): + """Validates that the app service is allowed to control + the given user. + + Args: + app_service: The app service that controls the user + user_id: The author MXID that the app service is controlling + + Raises: + AuthError: If the application service is not allowed to control the user + (user namespace regex does not match, wrong homeserver, etc) + or if the user has not been registered yet. + """ + + # It's ok if the app service is trying to use the sender from their registration + if app_service.sender == user_id: + pass + # Check to make sure the app service is allowed to control the user + elif not app_service.is_interested_in_user(user_id): + raise AuthError( + 403, + "Application service cannot masquerade as this user (%s)." % user_id, + ) + # Check to make sure the user is already registered on the homeserver + elif not (await self.store.get_user_by_id(user_id)): + raise AuthError( + 403, "Application service has not registered this user (%s)" % user_id + ) + async def _get_appservice_user_id( self, request: Request ) -> Tuple[Optional[str], Optional[ApplicationService]]: @@ -261,13 +292,11 @@ async def _get_appservice_user_id( return app_service.sender, app_service user_id = request.args[b"user_id"][0].decode("utf8") + await self.validate_appservice_can_control_user_id(app_service, user_id) + if app_service.sender == user_id: return app_service.sender, app_service - if not app_service.is_interested_in_user(user_id): - raise AuthError(403, "Application service cannot masquerade as this user.") - if not (await self.store.get_user_by_id(user_id)): - raise AuthError(403, "Application service has not registered this user") return user_id, app_service async def get_user_by_access_token( diff --git a/synapse/rest/client/v1/room.py b/synapse/rest/client/v1/room.py index 7ff4ad6bf5c7..ebf4e3223089 100644 --- a/synapse/rest/client/v1/room.py +++ b/synapse/rest/client/v1/room.py @@ -381,7 +381,7 @@ def _create_insertion_event_dict( return insertion_event - async def _create_requester_from_app_service( + async def _create_requester_for_user_id_from_app_service( self, user_id: str, app_service: ApplicationService ) -> Requester: """Creates a new requester for the given user_id @@ -396,20 +396,7 @@ async def _create_requester_from_app_service( Requester object """ - # It's ok if the app service is trying to use the sender from their registration - if app_service.sender == user_id: - pass - # Check to make sure the app service is allowed to control the user - elif not app_service.is_interested_in_user(user_id): - raise AuthError( - 403, - "Application service cannot masquerade as this user (%s)." % user_id, - ) - # Check to make sure the user is already registered on the homeserver - elif not (await self.store.get_user_by_id(user_id)): - raise AuthError( - 403, "Application service has not registered this user (%s)" % user_id - ) + await self.auth.validate_appservice_can_control_user_id(app_service, user_id) return create_requester(user_id, app_service=app_service) @@ -478,7 +465,7 @@ async def on_POST(self, request, room_id): if event_dict["type"] == EventTypes.Member: membership = event_dict["content"].get("membership", None) event_id, _ = await self.room_member_handler.update_membership( - await self._create_requester_from_app_service( + await self._create_requester_for_user_id_from_app_service( state_event["sender"], requester.app_service ), target=UserID.from_string(event_dict["state_key"]), @@ -500,7 +487,7 @@ async def on_POST(self, request, room_id): event, _, ) = await self.event_creation_handler.create_and_send_nonmember_event( - await self._create_requester_from_app_service( + await self._create_requester_for_user_id_from_app_service( state_event["sender"], requester.app_service ), event_dict, @@ -550,7 +537,7 @@ async def on_POST(self, request, room_id): base_insertion_event, _, ) = await self.event_creation_handler.create_and_send_nonmember_event( - await self._create_requester_from_app_service( + await self._create_requester_for_user_id_from_app_service( base_insertion_event_dict["sender"], requester.app_service, ), @@ -602,7 +589,7 @@ async def on_POST(self, request, room_id): } event, context = await self.event_creation_handler.create_event( - await self._create_requester_from_app_service( + await self._create_requester_for_user_id_from_app_service( ev["sender"], requester.app_service ), event_dict, @@ -634,7 +621,7 @@ async def on_POST(self, request, room_id): # where topological_ordering is just depth. for (event, context) in reversed(events_to_persist): ev = await self.event_creation_handler.handle_new_client_event( - await self._create_requester_from_app_service( + await self._create_requester_for_user_id_from_app_service( event["sender"], requester.app_service ), event=event,