From be63affbbc150ea917038b04fc5d60f2b9c3fb18 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 18 May 2023 11:59:25 -0400 Subject: [PATCH 01/10] Stabilize support for MSC3970. --- changelog.d/15629.feature | 1 + synapse/config/experimental.py | 3 --- synapse/events/utils.py | 13 ++----------- synapse/handlers/message.py | 4 +--- synapse/handlers/room_member.py | 4 +--- synapse/rest/client/transactions.py | 11 +---------- synapse/server.py | 4 +--- synapse/storage/databases/main/events.py | 4 +--- 8 files changed, 8 insertions(+), 36 deletions(-) create mode 100644 changelog.d/15629.feature diff --git a/changelog.d/15629.feature b/changelog.d/15629.feature new file mode 100644 index 000000000000..16264effca57 --- /dev/null +++ b/changelog.d/15629.feature @@ -0,0 +1 @@ +Scope transaction IDs to devices (implement [MSC3970](https://github.com/matrix-org/matrix-spec-proposals/pull/3970)). diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index 6e453bd963ea..724d2c744f66 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -194,9 +194,6 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: "msc3981_recurse_relations", False ) - # MSC3970: Scope transaction IDs to devices - self.msc3970_enabled = experimental.get("msc3970_enabled", False) - # MSC4009: E.164 Matrix IDs self.msc4009_e164_mxids = experimental.get("msc4009_e164_mxids", False) diff --git a/synapse/events/utils.py b/synapse/events/utils.py index e6d040176be2..dd0689c42391 100644 --- a/synapse/events/utils.py +++ b/synapse/events/utils.py @@ -361,7 +361,6 @@ def serialize_event( time_now_ms: int, *, config: SerializeEventConfig = _DEFAULT_SERIALIZE_EVENT_CONFIG, - msc3970_enabled: bool = False, ) -> JsonDict: """Serialize event for clients @@ -369,8 +368,6 @@ def serialize_event( e time_now_ms config: Event serialization config - msc3970_enabled: Whether MSC3970 is enabled. It changes whether we should - include the `transaction_id` in the event's `unsigned` section. Returns: The serialized event dictionary. @@ -396,7 +393,6 @@ def serialize_event( e.unsigned["redacted_because"], time_now_ms, config=config, - msc3970_enabled=msc3970_enabled, ) # If we have a txn_id saved in the internal_metadata, we should include it in the @@ -408,7 +404,7 @@ def serialize_event( # event internal metadata. Since we were not recording them before, if it hasn't # been recorded, we fallback to the old behaviour. event_device_id: Optional[str] = getattr(e.internal_metadata, "device_id", None) - if msc3970_enabled and event_device_id is not None: + if event_device_id is not None: if event_device_id == config.requester.device_id: d["unsigned"]["transaction_id"] = txn_id @@ -460,9 +456,6 @@ class EventClientSerializer: clients. """ - def __init__(self, *, msc3970_enabled: bool = False): - self._msc3970_enabled = msc3970_enabled - def serialize_event( self, event: Union[JsonDict, EventBase], @@ -487,9 +480,7 @@ def serialize_event( if not isinstance(event, EventBase): return event - serialized_event = serialize_event( - event, time_now, config=config, msc3970_enabled=self._msc3970_enabled - ) + serialized_event = serialize_event(event, time_now, config=config) # Check if there are any bundled aggregations to include with the event. if bundle_aggregations: diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 0b61c2272b90..73ef0a22158a 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -560,8 +560,6 @@ def __init__(self, hs: "HomeServer"): expiry_ms=30 * 60 * 1000, ) - self._msc3970_enabled = hs.config.experimental.msc3970_enabled - async def create_event( self, requester: Requester, @@ -906,7 +904,7 @@ async def get_event_from_transaction( An event if one could be found, None otherwise. """ - if self._msc3970_enabled and requester.device_id: + if requester.device_id: # When MSC3970 is enabled, we lookup for events sent by the same device first, # and fallback to the old behaviour if none were found. existing_event_id = ( diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index af0ca5c26d33..2c36aa684235 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -174,8 +174,6 @@ def __init__(self, hs: "HomeServer"): self.request_ratelimiter = hs.get_request_ratelimiter() hs.get_notifier().add_new_join_in_room_callback(self._on_user_joined_room) - self._msc3970_enabled = hs.config.experimental.msc3970_enabled - def _on_user_joined_room(self, event_id: str, room_id: str) -> None: """Notify the rate limiter that a room join has occurred. @@ -424,7 +422,7 @@ async def _local_membership_update( # do it up front for efficiency.) if txn_id: existing_event_id = None - if self._msc3970_enabled and requester.device_id: + if requester.device_id: # When MSC3970 is enabled, we lookup for events sent by the same device # first, and fallback to the old behaviour if none were found. existing_event_id = ( diff --git a/synapse/rest/client/transactions.py b/synapse/rest/client/transactions.py index 0d8a63d8beda..40946782c55d 100644 --- a/synapse/rest/client/transactions.py +++ b/synapse/rest/client/transactions.py @@ -50,8 +50,6 @@ def __init__(self, hs: "HomeServer"): # for at *LEAST* 30 mins, and at *MOST* 60 mins. self.cleaner = self.clock.looping_call(self._cleanup, CLEANUP_PERIOD_MS) - self._msc3970_enabled = hs.config.experimental.msc3970_enabled - def _get_transaction_key(self, request: IRequest, requester: Requester) -> Hashable: """A helper function which returns a transaction key that can be used with TransactionCache for idempotent requests. @@ -79,18 +77,11 @@ def _get_transaction_key(self, request: IRequest, requester: Requester) -> Hasha return (path, "appservice", requester.app_service.id) # With MSC3970, we use the user ID and device ID as the transaction key - elif self._msc3970_enabled: + else: assert requester.user, "Requester must have a user" assert requester.device_id, "Requester must have a device_id" return (path, "user", requester.user, requester.device_id) - # Otherwise, the pre-MSC3970 behaviour is to use the access token ID - else: - assert ( - requester.access_token_id is not None - ), "Requester must have an access_token_id" - return (path, "user", requester.access_token_id) - def fetch_or_execute_request( self, request: IRequest, diff --git a/synapse/server.py b/synapse/server.py index b307295789cd..d82be9f0c00a 100644 --- a/synapse/server.py +++ b/synapse/server.py @@ -777,9 +777,7 @@ def get_oidc_handler(self) -> "OidcHandler": @cache_in_self def get_event_client_serializer(self) -> EventClientSerializer: - return EventClientSerializer( - msc3970_enabled=self.config.experimental.msc3970_enabled - ) + return EventClientSerializer() @cache_in_self def get_password_policy_handler(self) -> PasswordPolicyHandler: diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index e2e6eb479f61..726632874657 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -127,8 +127,6 @@ def __init__( self._backfill_id_gen: AbstractStreamIdGenerator = self.store._backfill_id_gen self._stream_id_gen: AbstractStreamIdGenerator = self.store._stream_id_gen - self._msc3970_enabled = hs.config.experimental.msc3970_enabled - @trace async def _persist_events_and_state_updates( self, @@ -1033,7 +1031,7 @@ def _persist_transaction_ids_txn( # With MSC3970, we rely on the device_id instead to scope the txn_id for events. # We're only inserting if MSC3970 is *enabled*, because else the pre-MSC3970 # behaviour would allow for a UNIQUE constraint violation on this table - if to_insert_device_id and self._msc3970_enabled: + if to_insert_device_id: self.db_pool.simple_insert_many_txn( txn, table="event_txn_id_device_id", From 68ce9adb526379176386bc75b149f68600102f59 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 22 May 2023 15:43:22 -0400 Subject: [PATCH 02/10] Improve comments. --- synapse/handlers/room_member.py | 4 ++-- synapse/storage/databases/main/events.py | 11 +++++------ synapse/types/__init__.py | 3 ++- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 2c36aa684235..381b4c42a4bf 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -422,9 +422,9 @@ async def _local_membership_update( # do it up front for efficiency.) if txn_id: existing_event_id = None + # Look for an event sent by the same device. If none is found, fallback + # to an event sent by the same access token. if requester.device_id: - # When MSC3970 is enabled, we lookup for events sent by the same device - # first, and fallback to the old behaviour if none were found. existing_event_id = ( await self.store.get_event_id_from_transaction_id_and_device_id( room_id, diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index 726632874657..df4218ae2cb5 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -1010,9 +1010,10 @@ def _persist_transaction_ids_txn( ) ) - # Pre-MSC3970, we rely on the access_token_id to scope the txn_id for events. - # Since this is an experimental flag, we still store the mapping even if the - # flag is disabled. + # Previously Synapsed used the access_token_id to scope the txn_id for events. + # + # TODO Remove this once Synapse cannot be rolled back to a version without + # device IDs being stored. if to_insert_token_id: self.db_pool.simple_insert_many_txn( txn, @@ -1028,9 +1029,7 @@ def _persist_transaction_ids_txn( values=to_insert_token_id, ) - # With MSC3970, we rely on the device_id instead to scope the txn_id for events. - # We're only inserting if MSC3970 is *enabled*, because else the pre-MSC3970 - # behaviour would allow for a UNIQUE constraint violation on this table + # Synapse now relies on the device_id to scope the txn_id for events. if to_insert_device_id: self.db_pool.simple_insert_many_txn( txn, diff --git a/synapse/types/__init__.py b/synapse/types/__init__.py index 42baf8ac6bb4..04d2a655fa77 100644 --- a/synapse/types/__init__.py +++ b/synapse/types/__init__.py @@ -121,7 +121,8 @@ class Requester: request, or None if it came via the appservice API or similar is_guest: True if the user making this request is a guest user shadow_banned: True if the user making this request has been shadow-banned. - device_id: device_id which was set at authentication time + device_id: device_id which was set at authentication time, this will be + None for appservices, guests, and tokens generated by the admin API app_service: the AS requesting on behalf of the user authenticated_entity: The entity that authenticated when making the request. This is different to the user_id when an admin user or the server is From e095d920402d22217edf972800933bf550c881ff Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 22 May 2023 15:43:32 -0400 Subject: [PATCH 03/10] Add back transaction else clause. --- synapse/rest/client/transactions.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/synapse/rest/client/transactions.py b/synapse/rest/client/transactions.py index 40946782c55d..5388d9913855 100644 --- a/synapse/rest/client/transactions.py +++ b/synapse/rest/client/transactions.py @@ -76,12 +76,21 @@ def _get_transaction_key(self, request: IRequest, requester: Requester) -> Hasha elif requester.app_service is not None: return (path, "appservice", requester.app_service.id) - # With MSC3970, we use the user ID and device ID as the transaction key - else: + # We use the user ID and device ID as the transaction key. + elif requester.device_id: assert requester.user, "Requester must have a user" assert requester.device_id, "Requester must have a device_id" return (path, "user", requester.user, requester.device_id) + # Some requsters don't have device IDs, these are mostly handled above + # (appservice and guest users), but does not cover access tokens minted + # by the admin API. Use the access token ID instead. + else: + assert ( + requester.access_token_id is not None + ), "Requester must have an access_token_id" + return (path, "user_admin", requester.access_token_id) + def fetch_or_execute_request( self, request: IRequest, From a34fbf3b6c48b0fbca42d33e98b4839e8a07b845 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 22 May 2023 15:47:20 -0400 Subject: [PATCH 04/10] Improve more comments. --- synapse/events/utils.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/synapse/events/utils.py b/synapse/events/utils.py index dd0689c42391..d56d8572b73a 100644 --- a/synapse/events/utils.py +++ b/synapse/events/utils.py @@ -400,20 +400,22 @@ def serialize_event( # requesting the event. txn_id: Optional[str] = getattr(e.internal_metadata, "txn_id", None) if txn_id is not None and config.requester is not None: - # For the MSC3970 rules to be applied, we *need* to have the device ID in the - # event internal metadata. Since we were not recording them before, if it hasn't - # been recorded, we fallback to the old behaviour. + # Old events do not have the device ID stored in the internal metadata, + # if it hasn't been recorded, fallback to using the access token instead. event_device_id: Optional[str] = getattr(e.internal_metadata, "device_id", None) if event_device_id is not None: if event_device_id == config.requester.device_id: d["unsigned"]["transaction_id"] = txn_id else: - # The pre-MSC3970 behaviour is to only include the transaction ID if the - # event was sent from the same access token. For regular users, we can use - # the access token ID to determine this. For guests, we can't, but since - # each guest only has one access token, we can just check that the event was - # sent by the same user as the one requesting the event. + # Fallback behaviour: only include the transaction ID if the event + # was sent from the same access token. + # + # For regular users, the access token ID can be used to determine this. + # + # For guests, we can't check the access token ID, but since each guest + # only has one access token, just check the event was sent by the same + # user as the one requesting the event. event_token_id: Optional[int] = getattr( e.internal_metadata, "token_id", None ) From 283d3c5091b8354a7fa0691e28ed32a64eafcf82 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 31 May 2023 12:20:17 -0400 Subject: [PATCH 05/10] Fix typo. Co-authored-by: reivilibre --- synapse/storage/databases/main/events.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index df4218ae2cb5..a2593e711907 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -1010,7 +1010,7 @@ def _persist_transaction_ids_txn( ) ) - # Previously Synapsed used the access_token_id to scope the txn_id for events. + # Previously Synapse used the access_token_id to scope the txn_id for events. # # TODO Remove this once Synapse cannot be rolled back to a version without # device IDs being stored. From e0a72c0a48d7ceaa318cb388f55ee9dd10eb416f Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 1 Jun 2023 09:28:58 -0400 Subject: [PATCH 06/10] Consolidate confusing logic. --- synapse/handlers/message.py | 39 ++++++++++++++++++++++++++------- synapse/handlers/room_member.py | 26 ++++------------------ 2 files changed, 35 insertions(+), 30 deletions(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 73ef0a22158a..22fbafccf540 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -885,14 +885,13 @@ async def deduplicate_state_event( return prev_event return None - async def get_event_from_transaction( + async def get_event_id_from_transaction( self, requester: Requester, txn_id: str, room_id: str, - ) -> Optional[EventBase]: - """For the given transaction ID and room ID, check if there is a matching event. - If so, fetch it and return it. + ) -> Optional[str]: + """For the given transaction ID and room ID, check if there is a matching event ID. Args: requester: The requester making the request in the context of which we want @@ -901,8 +900,9 @@ async def get_event_from_transaction( room_id: The room ID. Returns: - An event if one could be found, None otherwise. + An event ID if one could be found, None otherwise. """ + existing_event_id = None if requester.device_id: # When MSC3970 is enabled, we lookup for events sent by the same device first, @@ -916,7 +916,7 @@ async def get_event_from_transaction( ) ) if existing_event_id: - return await self.store.get_event(existing_event_id) + return existing_event_id # Pre-MSC3970, we looked up for events that were sent by the same session by # using the access token ID. @@ -929,9 +929,32 @@ async def get_event_from_transaction( txn_id, ) ) - if existing_event_id: - return await self.store.get_event(existing_event_id) + return existing_event_id + + async def get_event_from_transaction( + self, + requester: Requester, + txn_id: str, + room_id: str, + ) -> Optional[EventBase]: + """For the given transaction ID and room ID, check if there is a matching event. + If so, fetch it and return it. + + Args: + requester: The requester making the request in the context of which we want + to fetch the event. + txn_id: The transaction ID. + room_id: The room ID. + + Returns: + An event if one could be found, None otherwise. + """ + existing_event_id = await self.get_event_id_from_transaction( + requester, txn_id, room_id + ) + if existing_event_id: + return await self.store.get_event(existing_event_id) return None async def create_and_send_nonmember_event( diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 381b4c42a4bf..e8dec6200da3 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -421,29 +421,11 @@ async def _local_membership_update( # do this check just before we persist an event as well, but may as well # do it up front for efficiency.) if txn_id: - existing_event_id = None - # Look for an event sent by the same device. If none is found, fallback - # to an event sent by the same access token. - if requester.device_id: - existing_event_id = ( - await self.store.get_event_id_from_transaction_id_and_device_id( - room_id, - requester.user.to_string(), - requester.device_id, - txn_id, - ) - ) - - if requester.access_token_id and not existing_event_id: - existing_event_id = ( - await self.store.get_event_id_from_transaction_id_and_token_id( - room_id, - requester.user.to_string(), - requester.access_token_id, - txn_id, - ) + existing_event_id = ( + await self.event_creation_handler.get_event_id_from_transaction( + requester, txn_id, room_id ) - + ) if existing_event_id: event_pos = await self.store.get_position_for_event(existing_event_id) return existing_event_id, event_pos.stream From 06d46e309465049559b2575ceb64c3e478e1d100 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 1 Jun 2023 10:12:05 -0400 Subject: [PATCH 07/10] Clean-up comments. --- synapse/events/utils.py | 12 ++++++++++-- synapse/handlers/message.py | 8 ++++---- synapse/rest/client/transactions.py | 2 +- synapse/storage/databases/main/events.py | 8 +++----- 4 files changed, 18 insertions(+), 12 deletions(-) diff --git a/synapse/events/utils.py b/synapse/events/utils.py index b8ff2b78007c..4cc6e0c54a6b 100644 --- a/synapse/events/utils.py +++ b/synapse/events/utils.py @@ -444,8 +444,10 @@ def serialize_event( # requesting the event. txn_id: Optional[str] = getattr(e.internal_metadata, "txn_id", None) if txn_id is not None and config.requester is not None: - # Old events do not have the device ID stored in the internal metadata, - # if it hasn't been recorded, fallback to using the access token instead. + # Some events do not have the device ID stored in the internal metadata, + # this includes old events as well as those created by appservice, guests, + # or with tokens minted with the admin API. For those events, fallback + # to using the access token instead. event_device_id: Optional[str] = getattr(e.internal_metadata, "device_id", None) if event_device_id is not None: if event_device_id == config.requester.device_id: @@ -456,10 +458,15 @@ def serialize_event( # was sent from the same access token. # # For regular users, the access token ID can be used to determine this. + # This includes access tokens minted with the admin API. # # For guests, we can't check the access token ID, but since each guest # only has one access token, just check the event was sent by the same # user as the one requesting the event. + # + # For appservice users, we can't check the access token ID, just + # check the event was sent by the same user as the one requesting + # the event. event_token_id: Optional[int] = getattr( e.internal_metadata, "token_id", None ) @@ -470,6 +477,7 @@ def serialize_event( and event_token_id == config.requester.access_token_id ) or config.requester.is_guest + or config.requester.app_service ): d["unsigned"]["transaction_id"] = txn_id diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 22fbafccf540..8b945b34269b 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -904,9 +904,8 @@ async def get_event_id_from_transaction( """ existing_event_id = None + # According to the spec, transactions are scoped to a user's device ID. if requester.device_id: - # When MSC3970 is enabled, we lookup for events sent by the same device first, - # and fallback to the old behaviour if none were found. existing_event_id = ( await self.store.get_event_id_from_transaction_id_and_device_id( room_id, @@ -918,8 +917,9 @@ async def get_event_id_from_transaction( if existing_event_id: return existing_event_id - # Pre-MSC3970, we looked up for events that were sent by the same session by - # using the access token ID. + # Some requsters don't have device IDs (appservice, guests, and access + # tokens minted with the admin API), fallback to checking the access token + # ID, which should be close enough. if requester.access_token_id: existing_event_id = ( await self.store.get_event_id_from_transaction_id_and_token_id( diff --git a/synapse/rest/client/transactions.py b/synapse/rest/client/transactions.py index 5388d9913855..3d814c404d98 100644 --- a/synapse/rest/client/transactions.py +++ b/synapse/rest/client/transactions.py @@ -76,7 +76,7 @@ def _get_transaction_key(self, request: IRequest, requester: Requester) -> Hasha elif requester.app_service is not None: return (path, "appservice", requester.app_service.id) - # We use the user ID and device ID as the transaction key. + # Use the user ID and device ID as the transaction key. elif requester.device_id: assert requester.user, "Requester must have a user" assert requester.device_id, "Requester must have a device_id" diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index a2593e711907..f03edc6de051 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -1010,10 +1010,9 @@ def _persist_transaction_ids_txn( ) ) - # Previously Synapse used the access_token_id to scope the txn_id for events. - # - # TODO Remove this once Synapse cannot be rolled back to a version without - # device IDs being stored. + # Synapse usually relies on the device_id to scope transactions for events, + # except for users without device IDs (appservice, guests, and access + # tokens minted with the admin API) which use the access token ID instead. if to_insert_token_id: self.db_pool.simple_insert_many_txn( txn, @@ -1029,7 +1028,6 @@ def _persist_transaction_ids_txn( values=to_insert_token_id, ) - # Synapse now relies on the device_id to scope the txn_id for events. if to_insert_device_id: self.db_pool.simple_insert_many_txn( txn, From a67561bc39143b33aa91bfa93c1d322d233818c7 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 31 Jul 2023 13:26:14 -0400 Subject: [PATCH 08/10] Update comments to be consistent. --- synapse/types/__init__.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/types/__init__.py b/synapse/types/__init__.py index 33ad6018e7cf..39a1ae4ac347 100644 --- a/synapse/types/__init__.py +++ b/synapse/types/__init__.py @@ -117,11 +117,11 @@ class Requester: Attributes: user: id of the user making the request - access_token_id: *ID* of the access token used for this - request, or None if it came via the appservice API or similar + access_token_id: *ID* of the access token used for this request, or + None for appservices, guests, and tokens generated by the admin API is_guest: True if the user making this request is a guest user shadow_banned: True if the user making this request has been shadow-banned. - device_id: device_id which was set at authentication time, this will be + device_id: device_id which was set at authentication time, or None for appservices, guests, and tokens generated by the admin API app_service: the AS requesting on behalf of the user authenticated_entity: The entity that authenticated when making the request. From bb0879049e6874964a984048c05004ea7ed1aa23 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 31 Jul 2023 16:03:37 -0400 Subject: [PATCH 09/10] Only return transaction IDs if the sender is the same as the requester. --- synapse/events/utils.py | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/synapse/events/utils.py b/synapse/events/utils.py index 19759c41b5ee..ebd2d141004b 100644 --- a/synapse/events/utils.py +++ b/synapse/events/utils.py @@ -432,7 +432,11 @@ def serialize_event( # unsigned section of the event if it was sent by the same session as the one # requesting the event. txn_id: Optional[str] = getattr(e.internal_metadata, "txn_id", None) - if txn_id is not None and config.requester is not None: + if ( + txn_id is not None + and config.requester is not None + and config.requester.user.to_string() == e.sender + ): # Some events do not have the device ID stored in the internal metadata, # this includes old events as well as those created by appservice, guests, # or with tokens minted with the admin API. For those events, fallback @@ -449,17 +453,12 @@ def serialize_event( # For regular users, the access token ID can be used to determine this. # This includes access tokens minted with the admin API. # - # For guests, we can't check the access token ID, but since each guest - # only has one access token, just check the event was sent by the same - # user as the one requesting the event. - # - # For appservice users, we can't check the access token ID, just - # check the event was sent by the same user as the one requesting - # the event. + # For guests and appservice users, we can't check the access token ID + # so assume it is the same session. event_token_id: Optional[int] = getattr( e.internal_metadata, "token_id", None ) - if config.requester.user.to_string() == e.sender and ( + if ( ( event_token_id is not None and config.requester.access_token_id is not None From 937788955f9e2a35cb9e3a179c066d8cf5d00d6e Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 1 Aug 2023 08:37:15 -0400 Subject: [PATCH 10/10] Bump schema version. --- synapse/storage/databases/main/events.py | 2 ++ synapse/storage/schema/__init__.py | 5 ++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index 3799a8044067..c1353b18c1cd 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -1013,6 +1013,8 @@ def _persist_transaction_ids_txn( # Synapse usually relies on the device_id to scope transactions for events, # except for users without device IDs (appservice, guests, and access # tokens minted with the admin API) which use the access token ID instead. + # + # TODO https://github.com/matrix-org/synapse/issues/16042 if to_insert_token_id: self.db_pool.simple_insert_many_txn( txn, diff --git a/synapse/storage/schema/__init__.py b/synapse/storage/schema/__init__.py index d3ec648f6d0f..7de9949a5b7f 100644 --- a/synapse/storage/schema/__init__.py +++ b/synapse/storage/schema/__init__.py @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -SCHEMA_VERSION = 79 # remember to update the list below when updating +SCHEMA_VERSION = 80 # remember to update the list below when updating """Represents the expectations made by the codebase about the database schema This should be incremented whenever the codebase changes its requirements on the @@ -110,6 +110,9 @@ Changes in SCHEMA_VERSION = 79 - Add tables to handle in DB read-write locks. - Add some mitigations for a painful race between foreground and background updates, cf #15677. + +Changes in SCHEMA_VERSION = 80 + - The event_txn_id_device_id is always written to for new events. """