Stabilize support for MSC3970: updated transaction semantics (scope to `device_id`) (#15629)

For now this maintains compatible with old Synapses by falling back
to using transaction semantics on a per-access token. A future version
of Synapse will drop support for this.
pull/16073/head
Patrick Cloke 2023-08-04 07:47:18 -04:00 committed by GitHub
parent 0a5f4f7665
commit d98a43d922
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 48 additions and 59 deletions

View File

@ -0,0 +1 @@
Scope transaction IDs to devices (implement [MSC3970](https://github.com/matrix-org/matrix-spec-proposals/pull/3970)).

View File

@ -216,12 +216,6 @@ class MSC3861:
("session_lifetime",), ("session_lifetime",),
) )
if not root.experimental.msc3970_enabled:
raise ConfigError(
"experimental_features.msc3970_enabled must be 'true' when OAuth delegation is enabled",
("experimental_features", "msc3970_enabled"),
)
@attr.s(auto_attribs=True, frozen=True, slots=True) @attr.s(auto_attribs=True, frozen=True, slots=True)
class MSC3866Config: class MSC3866Config:
@ -397,9 +391,6 @@ class ExperimentalConfig(Config):
"Invalid MSC3861 configuration", ("experimental", "msc3861") "Invalid MSC3861 configuration", ("experimental", "msc3861")
) from exc ) from exc
# MSC3970: Scope transaction IDs to devices
self.msc3970_enabled = experimental.get("msc3970_enabled", self.msc3861.enabled)
# Check that none of the other config options conflict with MSC3861 when enabled # Check that none of the other config options conflict with MSC3861 when enabled
self.msc3861.check_config_conflicts(self.root) self.msc3861.check_config_conflicts(self.root)

View File

@ -394,7 +394,6 @@ def serialize_event(
time_now_ms: int, time_now_ms: int,
*, *,
config: SerializeEventConfig = _DEFAULT_SERIALIZE_EVENT_CONFIG, config: SerializeEventConfig = _DEFAULT_SERIALIZE_EVENT_CONFIG,
msc3970_enabled: bool = False,
) -> JsonDict: ) -> JsonDict:
"""Serialize event for clients """Serialize event for clients
@ -402,8 +401,6 @@ def serialize_event(
e e
time_now_ms time_now_ms
config: Event serialization config 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: Returns:
The serialized event dictionary. The serialized event dictionary.
@ -429,38 +426,46 @@ def serialize_event(
e.unsigned["redacted_because"], e.unsigned["redacted_because"],
time_now_ms, time_now_ms,
config=config, config=config,
msc3970_enabled=msc3970_enabled,
) )
# If we have a txn_id saved in the internal_metadata, we should include it in the # If we have a txn_id saved in the internal_metadata, we should include it in the
# unsigned section of the event if it was sent by the same session as the one # unsigned section of the event if it was sent by the same session as the one
# requesting the event. # requesting the event.
txn_id: Optional[str] = getattr(e.internal_metadata, "txn_id", None) txn_id: Optional[str] = getattr(e.internal_metadata, "txn_id", None)
if txn_id is not None and config.requester is not None: if (
# For the MSC3970 rules to be applied, we *need* to have the device ID in the txn_id is not None
# event internal metadata. Since we were not recording them before, if it hasn't and config.requester is not None
# been recorded, we fallback to the old behaviour. 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
# to using the access token instead.
event_device_id: Optional[str] = getattr(e.internal_metadata, "device_id", None) 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: if event_device_id == config.requester.device_id:
d["unsigned"]["transaction_id"] = txn_id d["unsigned"]["transaction_id"] = txn_id
else: else:
# The pre-MSC3970 behaviour is to only include the transaction ID if the # Fallback behaviour: only include the transaction ID if the event
# event was sent from the same access token. For regular users, we can use # was sent from the same access token.
# 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 # For regular users, the access token ID can be used to determine this.
# sent by the same user as the one requesting the event. # This includes access tokens minted with the admin API.
#
# 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( event_token_id: Optional[int] = getattr(
e.internal_metadata, "token_id", None e.internal_metadata, "token_id", None
) )
if config.requester.user.to_string() == e.sender and ( if (
( (
event_token_id is not None event_token_id is not None
and config.requester.access_token_id is not None and config.requester.access_token_id is not None
and event_token_id == config.requester.access_token_id and event_token_id == config.requester.access_token_id
) )
or config.requester.is_guest or config.requester.is_guest
or config.requester.app_service
): ):
d["unsigned"]["transaction_id"] = txn_id d["unsigned"]["transaction_id"] = txn_id
@ -504,9 +509,6 @@ class EventClientSerializer:
clients. clients.
""" """
def __init__(self, *, msc3970_enabled: bool = False):
self._msc3970_enabled = msc3970_enabled
def serialize_event( def serialize_event(
self, self,
event: Union[JsonDict, EventBase], event: Union[JsonDict, EventBase],
@ -531,9 +533,7 @@ class EventClientSerializer:
if not isinstance(event, EventBase): if not isinstance(event, EventBase):
return event return event
serialized_event = serialize_event( serialized_event = serialize_event(event, time_now, config=config)
event, time_now, config=config, msc3970_enabled=self._msc3970_enabled
)
# Check if there are any bundled aggregations to include with the event. # Check if there are any bundled aggregations to include with the event.
if bundle_aggregations: if bundle_aggregations:

View File

@ -561,8 +561,6 @@ class EventCreationHandler:
expiry_ms=30 * 60 * 1000, expiry_ms=30 * 60 * 1000,
) )
self._msc3970_enabled = hs.config.experimental.msc3970_enabled
async def create_event( async def create_event(
self, self,
requester: Requester, requester: Requester,
@ -897,9 +895,8 @@ class EventCreationHandler:
""" """
existing_event_id = None existing_event_id = None
if self._msc3970_enabled and requester.device_id: # According to the spec, transactions are scoped to a user's device ID.
# When MSC3970 is enabled, we lookup for events sent by the same device first, if requester.device_id:
# and fallback to the old behaviour if none were found.
existing_event_id = ( existing_event_id = (
await self.store.get_event_id_from_transaction_id_and_device_id( await self.store.get_event_id_from_transaction_id_and_device_id(
room_id, room_id,
@ -911,8 +908,9 @@ class EventCreationHandler:
if existing_event_id: if existing_event_id:
return existing_event_id return existing_event_id
# Pre-MSC3970, we looked up for events that were sent by the same session by # Some requsters don't have device IDs (appservice, guests, and access
# using the access token ID. # tokens minted with the admin API), fallback to checking the access token
# ID, which should be close enough.
if requester.access_token_id: if requester.access_token_id:
existing_event_id = ( existing_event_id = (
await self.store.get_event_id_from_transaction_id_and_token_id( await self.store.get_event_id_from_transaction_id_and_token_id(

View File

@ -50,8 +50,6 @@ class HttpTransactionCache:
# for at *LEAST* 30 mins, and at *MOST* 60 mins. # for at *LEAST* 30 mins, and at *MOST* 60 mins.
self.cleaner = self.clock.looping_call(self._cleanup, CLEANUP_PERIOD_MS) 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: def _get_transaction_key(self, request: IRequest, requester: Requester) -> Hashable:
"""A helper function which returns a transaction key that can be used """A helper function which returns a transaction key that can be used
with TransactionCache for idempotent requests. with TransactionCache for idempotent requests.
@ -78,18 +76,20 @@ class HttpTransactionCache:
elif requester.app_service is not None: elif requester.app_service is not None:
return (path, "appservice", requester.app_service.id) return (path, "appservice", requester.app_service.id)
# With MSC3970, we use the user ID and device ID as the transaction key # Use the user ID and device ID as the transaction key.
elif self._msc3970_enabled: elif requester.device_id:
assert requester.user, "Requester must have a user" assert requester.user, "Requester must have a user"
assert requester.device_id, "Requester must have a device_id" assert requester.device_id, "Requester must have a device_id"
return (path, "user", requester.user, requester.device_id) return (path, "user", requester.user, requester.device_id)
# Otherwise, the pre-MSC3970 behaviour is to use the access token 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: else:
assert ( assert (
requester.access_token_id is not None requester.access_token_id is not None
), "Requester must have an access_token_id" ), "Requester must have an access_token_id"
return (path, "user", requester.access_token_id) return (path, "user_admin", requester.access_token_id)
def fetch_or_execute_request( def fetch_or_execute_request(
self, self,

View File

@ -785,9 +785,7 @@ class HomeServer(metaclass=abc.ABCMeta):
@cache_in_self @cache_in_self
def get_event_client_serializer(self) -> EventClientSerializer: def get_event_client_serializer(self) -> EventClientSerializer:
return EventClientSerializer( return EventClientSerializer()
msc3970_enabled=self.config.experimental.msc3970_enabled
)
@cache_in_self @cache_in_self
def get_password_policy_handler(self) -> PasswordPolicyHandler: def get_password_policy_handler(self) -> PasswordPolicyHandler:

View File

@ -127,8 +127,6 @@ class PersistEventsStore:
self._backfill_id_gen: AbstractStreamIdGenerator = self.store._backfill_id_gen self._backfill_id_gen: AbstractStreamIdGenerator = self.store._backfill_id_gen
self._stream_id_gen: AbstractStreamIdGenerator = self.store._stream_id_gen self._stream_id_gen: AbstractStreamIdGenerator = self.store._stream_id_gen
self._msc3970_enabled = hs.config.experimental.msc3970_enabled
@trace @trace
async def _persist_events_and_state_updates( async def _persist_events_and_state_updates(
self, self,
@ -1012,9 +1010,11 @@ class PersistEventsStore:
) )
) )
# Pre-MSC3970, we rely on the access_token_id to scope the txn_id for events. # Synapse usually relies on the device_id to scope transactions for events,
# Since this is an experimental flag, we still store the mapping even if the # except for users without device IDs (appservice, guests, and access
# flag is disabled. # 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: if to_insert_token_id:
self.db_pool.simple_insert_many_txn( self.db_pool.simple_insert_many_txn(
txn, txn,
@ -1030,10 +1030,7 @@ class PersistEventsStore:
values=to_insert_token_id, values=to_insert_token_id,
) )
# With MSC3970, we rely on the device_id instead to scope the txn_id for events. if to_insert_device_id:
# 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:
self.db_pool.simple_insert_many_txn( self.db_pool.simple_insert_many_txn(
txn, txn,
table="event_txn_id_device_id", table="event_txn_id_device_id",

View File

@ -12,7 +12,7 @@
# See the License for the specific language governing permissions and # See the License for the specific language governing permissions and
# limitations under the License. # 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 """Represents the expectations made by the codebase about the database schema
This should be incremented whenever the codebase changes its requirements on the This should be incremented whenever the codebase changes its requirements on the
@ -110,6 +110,9 @@ Changes in SCHEMA_VERSION = 78
Changes in SCHEMA_VERSION = 79 Changes in SCHEMA_VERSION = 79
- Add tables to handle in DB read-write locks. - Add tables to handle in DB read-write locks.
- Add some mitigations for a painful race between foreground and background updates, cf #15677. - 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.
""" """

View File

@ -117,11 +117,12 @@ class Requester:
Attributes: Attributes:
user: id of the user making the request user: id of the user making the request
access_token_id: *ID* of the access token used for this access_token_id: *ID* of the access token used for this request, or
request, or None if it came via the appservice API or similar None for appservices, guests, and tokens generated by the admin API
is_guest: True if the user making this request is a guest user 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. 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, or
None for appservices, guests, and tokens generated by the admin API
app_service: the AS requesting on behalf of the user app_service: the AS requesting on behalf of the user
authenticated_entity: The entity that authenticated when making the request. 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 This is different to the user_id when an admin user or the server is