diff options
| -rw-r--r-- | changelog.d/12168.feature | 1 | ||||
| -rw-r--r-- | synapse/api/constants.py | 6 | ||||
| -rw-r--r-- | synapse/handlers/receipts.py | 65 | ||||
| -rw-r--r-- | synapse/handlers/sync.py | 2 | ||||
| -rw-r--r-- | synapse/push/push_tools.py | 4 | ||||
| -rw-r--r-- | synapse/rest/client/notifications.py | 2 | ||||
| -rw-r--r-- | synapse/rest/client/read_marker.py | 32 | ||||
| -rw-r--r-- | synapse/rest/client/receipts.py | 51 | ||||
| -rw-r--r-- | synapse/storage/databases/main/receipts.py | 142 | ||||
| -rw-r--r-- | tests/handlers/test_receipts.py | 129 | ||||
| -rw-r--r-- | tests/replication/slave/storage/test_receipts.py | 238 | ||||
| -rw-r--r-- | tests/rest/client/test_sync.py | 161 |
12 files changed, 647 insertions, 186 deletions
diff --git a/changelog.d/12168.feature b/changelog.d/12168.feature new file mode 100644 index 0000000000..cd5c45029e --- /dev/null +++ b/changelog.d/12168.feature @@ -0,0 +1 @@ +Implement [changes](https://github.com/matrix-org/matrix-spec-proposals/pull/2285/commits/4a77139249c2e830aec3c7d6bd5501a514d1cc27) to [MSC2285 (hidden read receipts)](https://github.com/matrix-org/matrix-spec-proposals/pull/2285). Contributed by @SimonBrandner. diff --git a/synapse/api/constants.py b/synapse/api/constants.py index 0172eb60b8..0ccd4c9558 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -255,7 +255,5 @@ class GuestAccess: class ReceiptTypes: READ: Final = "m.read" - - -class ReadReceiptEventFields: - MSC2285_HIDDEN: Final = "org.matrix.msc2285.hidden" + READ_PRIVATE: Final = "org.matrix.msc2285.read.private" + FULLY_READ: Final = "m.fully_read" diff --git a/synapse/handlers/receipts.py b/synapse/handlers/receipts.py index cfe860decc..ae41fd674e 100644 --- a/synapse/handlers/receipts.py +++ b/synapse/handlers/receipts.py @@ -14,7 +14,7 @@ import logging from typing import TYPE_CHECKING, Iterable, List, Optional, Tuple -from synapse.api.constants import ReadReceiptEventFields, ReceiptTypes +from synapse.api.constants import ReceiptTypes from synapse.appservice import ApplicationService from synapse.streams import EventSource from synapse.types import JsonDict, ReadReceipt, UserID, get_domain_from_id @@ -112,7 +112,7 @@ class ReceiptsHandler: ) if not res: - # res will be None if this read receipt is 'old' + # res will be None if this receipt is 'old' continue stream_id, max_persisted_id = res @@ -138,7 +138,7 @@ class ReceiptsHandler: return True async def received_client_receipt( - self, room_id: str, receipt_type: str, user_id: str, event_id: str, hidden: bool + self, room_id: str, receipt_type: str, user_id: str, event_id: str ) -> None: """Called when a client tells us a local user has read up to the given event_id in the room. @@ -148,16 +148,14 @@ class ReceiptsHandler: receipt_type=receipt_type, user_id=user_id, event_ids=[event_id], - data={"ts": int(self.clock.time_msec()), "hidden": hidden}, + data={"ts": int(self.clock.time_msec())}, ) is_new = await self._handle_new_receipts([receipt]) if not is_new: return - if self.federation_sender and not ( - self.hs.config.experimental.msc2285_enabled and hidden - ): + if self.federation_sender and receipt_type != ReceiptTypes.READ_PRIVATE: await self.federation_sender.send_read_receipt(receipt) @@ -168,6 +166,13 @@ class ReceiptEventSource(EventSource[int, JsonDict]): @staticmethod def filter_out_hidden(events: List[JsonDict], user_id: str) -> List[JsonDict]: + """ + This method takes in what is returned by + get_linearized_receipts_for_rooms() and goes through read receipts + filtering out m.read.private receipts if they were not sent by the + current user. + """ + visible_events = [] # filter out hidden receipts the user shouldn't see @@ -176,37 +181,21 @@ class ReceiptEventSource(EventSource[int, JsonDict]): new_event = event.copy() new_event["content"] = {} - for event_id in content.keys(): - event_content = content.get(event_id, {}) - m_read = event_content.get(ReceiptTypes.READ, {}) - - # If m_read is missing copy over the original event_content as there is nothing to process here - if not m_read: - new_event["content"][event_id] = event_content.copy() - continue - - new_users = {} - for rr_user_id, user_rr in m_read.items(): - try: - hidden = user_rr.get("hidden") - except AttributeError: - # Due to https://github.com/matrix-org/synapse/issues/10376 - # there are cases where user_rr is a string, in those cases - # we just ignore the read receipt - continue - - if hidden is not True or rr_user_id == user_id: - new_users[rr_user_id] = user_rr.copy() - # If hidden has a value replace hidden with the correct prefixed key - if hidden is not None: - new_users[rr_user_id].pop("hidden") - new_users[rr_user_id][ - ReadReceiptEventFields.MSC2285_HIDDEN - ] = hidden - - # Set new users unless empty - if len(new_users.keys()) > 0: - new_event["content"][event_id] = {ReceiptTypes.READ: new_users} + for event_id, event_content in content.items(): + receipt_event = {} + for receipt_type, receipt_content in event_content.items(): + if receipt_type == ReceiptTypes.READ_PRIVATE: + user_rr = receipt_content.get(user_id, None) + if user_rr: + receipt_event[ReceiptTypes.READ_PRIVATE] = { + user_id: user_rr.copy() + } + else: + receipt_event[receipt_type] = receipt_content.copy() + + # Only include the receipt event if it is non-empty. + if receipt_event: + new_event["content"][event_id] = receipt_event # Append new_event to visible_events unless empty if len(new_event["content"].keys()) > 0: diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index 5125126a80..2c555a66d0 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -1045,7 +1045,7 @@ class SyncHandler: last_unread_event_id = await self.store.get_last_receipt_event_id_for_user( user_id=sync_config.user.to_string(), room_id=room_id, - receipt_type=ReceiptTypes.READ, + receipt_types=(ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE), ) return await self.store.get_unread_event_push_actions_by_room_for_user( diff --git a/synapse/push/push_tools.py b/synapse/push/push_tools.py index 957c9b780b..a1bf5b20dd 100644 --- a/synapse/push/push_tools.py +++ b/synapse/push/push_tools.py @@ -24,7 +24,9 @@ async def get_badge_count(store: DataStore, user_id: str, group_by_room: bool) - invites = await store.get_invited_rooms_for_local_user(user_id) joins = await store.get_rooms_for_user(user_id) - my_receipts_by_room = await store.get_receipts_for_user(user_id, ReceiptTypes.READ) + my_receipts_by_room = await store.get_receipts_for_user( + user_id, (ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE) + ) badge = len(invites) diff --git a/synapse/rest/client/notifications.py b/synapse/rest/client/notifications.py index ff040de6b8..24bc7c9095 100644 --- a/synapse/rest/client/notifications.py +++ b/synapse/rest/client/notifications.py @@ -58,7 +58,7 @@ class NotificationsServlet(RestServlet): ) receipts_by_room = await self.store.get_receipts_for_user_with_orderings( - user_id, ReceiptTypes.READ + user_id, [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE] ) notif_event_ids = [pa.event_id for pa in push_actions] diff --git a/synapse/rest/client/read_marker.py b/synapse/rest/client/read_marker.py index f51be511d1..1583e903cd 100644 --- a/synapse/rest/client/read_marker.py +++ b/synapse/rest/client/read_marker.py @@ -15,8 +15,8 @@ import logging from typing import TYPE_CHECKING, Tuple -from synapse.api.constants import ReadReceiptEventFields, ReceiptTypes -from synapse.api.errors import Codes, SynapseError +from synapse.api.constants import ReceiptTypes +from synapse.api.errors import SynapseError from synapse.http.server import HttpServer from synapse.http.servlet import RestServlet, parse_json_object_from_request from synapse.http.site import SynapseRequest @@ -36,6 +36,7 @@ class ReadMarkerRestServlet(RestServlet): def __init__(self, hs: "HomeServer"): super().__init__() self.auth = hs.get_auth() + self.config = hs.config self.receipts_handler = hs.get_receipts_handler() self.read_marker_handler = hs.get_read_marker_handler() self.presence_handler = hs.get_presence_handler() @@ -48,27 +49,38 @@ class ReadMarkerRestServlet(RestServlet): await self.presence_handler.bump_presence_active_time(requester.user) body = parse_json_object_from_request(request) - read_event_id = body.get(ReceiptTypes.READ, None) - hidden = body.get(ReadReceiptEventFields.MSC2285_HIDDEN, False) - if not isinstance(hidden, bool): + valid_receipt_types = {ReceiptTypes.READ, ReceiptTypes.FULLY_READ} + if self.config.experimental.msc2285_enabled: + valid_receipt_types.add(ReceiptTypes.READ_PRIVATE) + + if set(body.keys()) > valid_receipt_types: raise SynapseError( 400, - "Param %s must be a boolean, if given" - % ReadReceiptEventFields.MSC2285_HIDDEN, - Codes.BAD_JSON, + "Receipt type must be 'm.read', 'org.matrix.msc2285.read.private' or 'm.fully_read'" + if self.config.experimental.msc2285_enabled + else "Receipt type must be 'm.read' or 'm.fully_read'", ) + read_event_id = body.get(ReceiptTypes.READ, None) if read_event_id: await self.receipts_handler.received_client_receipt( room_id, ReceiptTypes.READ, user_id=requester.user.to_string(), event_id=read_event_id, - hidden=hidden, ) - read_marker_event_id = body.get("m.fully_read", None) + read_private_event_id = body.get(ReceiptTypes.READ_PRIVATE, None) + if read_private_event_id and self.config.experimental.msc2285_enabled: + await self.receipts_handler.received_client_receipt( + room_id, + ReceiptTypes.READ_PRIVATE, + user_id=requester.user.to_string(), + event_id=read_private_event_id, + ) + + read_marker_event_id = body.get(ReceiptTypes.FULLY_READ, None) if read_marker_event_id: await self.read_marker_handler.received_client_read_marker( room_id, diff --git a/synapse/rest/client/receipts.py b/synapse/rest/client/receipts.py index b24ad2d1be..f9caab6635 100644 --- a/synapse/rest/client/receipts.py +++ b/synapse/rest/client/receipts.py @@ -16,8 +16,8 @@ import logging import re from typing import TYPE_CHECKING, Tuple -from synapse.api.constants import ReadReceiptEventFields, ReceiptTypes -from synapse.api.errors import Codes, SynapseError +from synapse.api.constants import ReceiptTypes +from synapse.api.errors import SynapseError from synapse.http import get_request_user_agent from synapse.http.server import HttpServer from synapse.http.servlet import RestServlet, parse_json_object_from_request @@ -46,6 +46,7 @@ class ReceiptRestServlet(RestServlet): self.hs = hs self.auth = hs.get_auth() self.receipts_handler = hs.get_receipts_handler() + self.read_marker_handler = hs.get_read_marker_handler() self.presence_handler = hs.get_presence_handler() async def on_POST( @@ -53,7 +54,19 @@ class ReceiptRestServlet(RestServlet): ) -> Tuple[int, JsonDict]: requester = await self.auth.get_user_by_req(request) - if receipt_type != ReceiptTypes.READ: + if self.hs.config.experimental.msc2285_enabled and receipt_type not in [ + ReceiptTypes.READ, + ReceiptTypes.READ_PRIVATE, + ReceiptTypes.FULLY_READ, + ]: + raise SynapseError( + 400, + "Receipt type must be 'm.read', 'org.matrix.msc2285.read.private' or 'm.fully_read'", + ) + elif ( + not self.hs.config.experimental.msc2285_enabled + and receipt_type != ReceiptTypes.READ + ): raise SynapseError(400, "Receipt type must be 'm.read'") # Do not allow older SchildiChat and Element Android clients (prior to Element/1.[012].x) to send an empty body. @@ -62,26 +75,24 @@ class ReceiptRestServlet(RestServlet): if "Android" in user_agent: if pattern.match(user_agent) or "Riot" in user_agent: allow_empty_body = True - body = parse_json_object_from_request(request, allow_empty_body) - hidden = body.get(ReadReceiptEventFields.MSC2285_HIDDEN, False) - - if not isinstance(hidden, bool): - raise SynapseError( - 400, - "Param %s must be a boolean, if given" - % ReadReceiptEventFields.MSC2285_HIDDEN, - Codes.BAD_JSON, - ) + # This call makes sure possible empty body is handled correctly + parse_json_object_from_request(request, allow_empty_body) await self.presence_handler.bump_presence_active_time(requester.user) - await self.receipts_handler.received_client_receipt( - room_id, - receipt_type, - user_id=requester.user.to_string(), - event_id=event_id, - hidden=hidden, - ) + if receipt_type == ReceiptTypes.FULLY_READ: + await self.read_marker_handler.received_client_read_marker( + room_id, + user_id=requester.user.to_string(), + event_id=event_id, + ) + else: + await self.receipts_handler.received_client_receipt( + room_id, + receipt_type, + user_id=requester.user.to_string(), + event_id=event_id, + ) return 200, {} diff --git a/synapse/storage/databases/main/receipts.py b/synapse/storage/databases/main/receipts.py index 7d96f4feda..9e3d838eab 100644 --- a/synapse/storage/databases/main/receipts.py +++ b/synapse/storage/databases/main/receipts.py @@ -144,43 +144,77 @@ class ReceiptsWorkerStore(SQLBaseStore): desc="get_receipts_for_room", ) - @cached() async def get_last_receipt_event_id_for_user( - self, user_id: str, room_id: str, receipt_type: str + self, user_id: str, room_id: str, receipt_types: Iterable[str] ) -> Optional[str]: """ - Fetch the event ID for the latest receipt in a room with the given receipt type. + Fetch the event ID for the latest receipt in a room with one of the given receipt types. Args: user_id: The user to fetch receipts for. room_id: The room ID to fetch the receipt for. - receipt_type: The receipt type to fetch. + receipt_type: The receipt types to fetch. Earlier receipt types + are given priority if multiple receipts point to the same event. Returns: - The event ID of the latest receipt, if one exists; otherwise `None`. + The latest receipt, if one exists. """ - return await self.db_pool.simple_select_one_onecol( - table="receipts_linearized", - keyvalues={ - "room_id": room_id, - "receipt_type": receipt_type, - "user_id": user_id, - }, - retcol="event_id", - desc="get_own_receipt_for_user", - allow_none=True, - ) + latest_event_id: Optional[str] = None + latest_stream_ordering = 0 + for receipt_type in receipt_types: + result = await self._get_last_receipt_event_id_for_user( + user_id, room_id, receipt_type + ) + if result is None: + continue + event_id, stream_ordering = result + + if latest_event_id is None or latest_stream_ordering < stream_ordering: + latest_event_id = event_id + latest_stream_ordering = stream_ordering + + return latest_event_id @cached() + async def _get_last_receipt_event_id_for_user( + self, user_id: str, room_id: str, receipt_type: str + ) -> Optional[Tuple[str, int]]: + """ + Fetch the event ID and stream ordering for the latest receipt. + + Args: + user_id: The user to fetch receipts for. + room_id: The room ID to fetch the receipt for. + receipt_type: The receipt type to fetch. + + Returns: + The event ID and stream ordering of the latest receipt, if one exists; + otherwise `None`. + """ + sql = """ + SELECT event_id, stream_ordering + FROM receipts_linearized + INNER JOIN events USING (room_id, event_id) + WHERE user_id = ? + AND room_id = ? + AND receipt_type = ? + """ + + def f(txn: LoggingTransaction) -> Optional[Tuple[str, int]]: + txn.execute(sql, (user_id, room_id, receipt_type)) + return cast(Optional[Tuple[str, int]], txn.fetchone()) + + return await self.db_pool.runInteraction("get_own_receipt_for_user", f) + async def get_receipts_for_user( - self, user_id: str, receipt_type: str + self, user_id: str, receipt_types: Iterable[str] ) -> Dict[str, str]: """ Fetch the event IDs for the latest receipts sent by the given user. Args: user_id: The user to fetch receipts for. - receipt_type: The receipt type to fetch. + receipt_types: The receipt types to check. Returns: A map of room ID to the event ID of the latest receipt for that room. @@ -188,16 +222,48 @@ class ReceiptsWorkerStore(SQLBaseStore): If the user has not sent a receipt to a room then it will not appear in the returned dictionary. """ - rows = await self.db_pool.simple_select_list( - table="receipts_linearized", - keyvalues={"user_id": user_id, "receipt_type": receipt_type}, - retcols=("room_id", "event_id"), - desc="get_receipts_for_user", + results = await self.get_receipts_for_user_with_orderings( + user_id, receipt_types ) - return {row["room_id"]: row["event_id"] for row in rows} + # Reduce the result to room ID -> event ID. + return { + room_id: room_result["event_id"] for room_id, room_result in results.items() + } async def get_receipts_for_user_with_orderings( + self, user_id: str, receipt_types: Iterable[str] + ) -> JsonDict: + """ + Fetch receipts for all rooms that the given user is joined to. + + Args: + user_id: The user to fetch receipts for. + receipt_types: The receipt types to fetch. Earlier receipt types + are given priority if multiple receipts point to the same event. + + Returns: + A map of room ID to the latest receipt (for the given types). + """ + results: JsonDict = {} + for receipt_type in receipt_types: + partial_result = await self._get_receipts_for_user_with_orderings( + user_id, receipt_type + ) + for room_id, room_result in partial_result.items(): + # If the room has not yet been seen, or the receipt is newer, + # use it. + if ( + room_id not in results + or results[room_id]["stream_ordering"] + < room_result["stream_ordering"] + ): + results[room_id] = room_result + + return results + + @cached() + async def _get_receipts_for_user_with_orderings( self, user_id: str, receipt_type: str ) -> JsonDict: """ @@ -220,8 +286,9 @@ class ReceiptsWorkerStore(SQLBaseStore): " WHERE rl.room_id = e.room_id" " AND rl.event_id = e.event_id" " AND user_id = ?" + " AND receipt_type = ?" ) - txn.execute(sql, (user_id,)) + txn.execute(sql, (user_id, receipt_type)) return cast(List[Tuple[str, str, int, int]], txn.fetchall()) rows = await self.db_pool.runInteraction( @@ -552,9 +619,9 @@ class ReceiptsWorkerStore(SQLBaseStore): def invalidate_caches_for_receipt( self, room_id: str, receipt_type: str, user_id: str ) -> None: - self.get_receipts_for_user.invalidate((user_id, receipt_type)) + self._get_receipts_for_user_with_orderings.invalidate((user_id, receipt_type)) self._get_linearized_receipts_for_room.invalidate((room_id,)) - self.get_last_receipt_event_id_for_user.invalidate( + self._get_last_receipt_event_id_for_user.invalidate( (user_id, room_id, receipt_type) ) self._invalidate_get_users_with_receipts_in_room(room_id, receipt_type, user_id) @@ -590,8 +657,8 @@ class ReceiptsWorkerStore(SQLBaseStore): """Inserts a receipt into the database if it's newer than the current one. Returns: - None if the RR is older than the current RR - otherwise, the rx timestamp of the event that the RR corresponds to + None if the receipt is older than the current receipt + otherwise, the rx timestamp of the event that the receipt corresponds to (or 0 if the event is unknown) """ assert self._can_write_to_receipts @@ -612,7 +679,7 @@ class ReceiptsWorkerStore(SQLBaseStore): if stream_ordering is not None: sql = ( "SELECT stream_ordering, event_id FROM events" - " INNER JOIN receipts_linearized as r USING (event_id, room_id)" + " INNER JOIN receipts_linearized AS r USING (event_id, room_id)" " WHERE r.room_id = ? AND r.receipt_type = ? AND r.user_id = ?" ) txn.execute(sql, (room_id, receipt_type, user_id)) @@ -653,7 +720,10 @@ class ReceiptsWorkerStore(SQLBaseStore): lock=False, ) - if receipt_type == ReceiptTypes.READ and stream_ordering is not None: + if ( + receipt_type in (ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE) + and stream_ordering is not None + ): self._remove_old_push_actions_before_txn( # type: ignore[attr-defined] txn, room_id=room_id, user_id=user_id, stream_ordering=stream_ordering ) @@ -672,6 +742,10 @@ class ReceiptsWorkerStore(SQLBaseStore): Automatically does conversion between linearized and graph representations. + + Returns: + The new receipts stream ID and token, if the receipt is newer than + what was previously persisted. None, otherwise. """ assert self._can_write_to_receipts @@ -719,6 +793,7 @@ class ReceiptsWorkerStore(SQLBaseStore): stream_id=stream_id, ) + # If the receipt was older than the currently persisted one, nothing to do. if event_ts is None: return None @@ -774,7 +849,10 @@ class ReceiptsWorkerStore(SQLBaseStore): receipt_type, user_id, ) - txn.call_after(self.get_receipts_for_user.invalidate, (user_id, receipt_type)) + txn.call_after( + self._get_receipts_for_user_with_orderings.invalidate, + (user_id, receipt_type), + ) # FIXME: This shouldn't invalidate the whole cache txn.call_after(self._get_linearized_receipts_for_room.invalidate, (room_id,)) diff --git a/tests/handlers/test_receipts.py b/tests/handlers/test_receipts.py index 65ab7db0c8..c12a9120f0 100644 --- a/tests/handlers/test_receipts.py +++ b/tests/handlers/test_receipts.py @@ -15,7 +15,7 @@ from typing import List -from synapse.api.constants import ReadReceiptEventFields, ReceiptTypes +from synapse.api.constants import ReceiptTypes from synapse.types import JsonDict from tests import unittest @@ -25,20 +25,15 @@ class ReceiptsTestCase(unittest.HomeserverTestCase): def prepare(self, reactor, clock, hs): self.event_source = hs.get_event_sources().sources.receipt - # In the first param of _test_filters_hidden we use "hidden" instead of - # ReadReceiptEventFields.MSC2285_HIDDEN. We do this because we're mocking - # the data from the database which doesn't use the prefix - def test_filters_out_hidden_receipt(self): self._test_filters_hidden( [ { "content": { "$1435641916114394fHBLK:matrix.org": { - ReceiptTypes.READ: { + ReceiptTypes.READ_PRIVATE: { "@rikj:jki.re": { "ts": 1436451550453, - "hidden": True, } } } @@ -50,58 +45,23 @@ class ReceiptsTestCase(unittest.HomeserverTestCase): [], ) - def test_does_not_filter_out_our_hidden_receipt(self): - self._test_filters_hidden( - [ - { - "content": { - "$1435641916hfgh4394fHBLK:matrix.org": { - ReceiptTypes.READ: { - "@me:server.org": { - "ts": 1436451550453, - "hidden": True, - }, - } - } - }, - "room_id": "!jEsUZKDJdhlrceRyVU:example.org", - "type": "m.receipt", - } - ], - [ - { - "content": { - "$1435641916hfgh4394fHBLK:matrix.org": { - ReceiptTypes.READ: { - "@me:server.org": { - "ts": 1436451550453, - ReadReceiptEventFields.MSC2285_HIDDEN: True, - }, - } - } - }, - "room_id": "!jEsUZKDJdhlrceRyVU:example.org", - "type": "m.receipt", - } - ], - ) - def test_filters_out_hidden_receipt_and_ignores_rest(self): self._test_filters_hidden( [ { "content": { "$1dgdgrd5641916114394fHBLK:matrix.org": { - ReceiptTypes.READ: { + ReceiptTypes.READ_PRIVATE: { "@rikj:jki.re": { "ts": 1436451550453, - "hidden": True, }, + }, + ReceiptTypes.READ: { "@user:jki.re": { "ts": 1436451550453, }, - } - } + }, + }, }, "room_id": "!jEsUZKDJdhlrceRyVU:example.org", "type": "m.receipt", @@ -130,10 +90,9 @@ class ReceiptsTestCase(unittest.HomeserverTestCase): { "content": { "$14356419edgd14394fHBLK:matrix.org": { - ReceiptTypes.READ: { + ReceiptTypes.READ_PRIVATE: { "@rikj:jki.re": { "ts": 1436451550453, - "hidden": True, }, } }, @@ -223,7 +182,6 @@ class ReceiptsTestCase(unittest.HomeserverTestCase): [ { "content": { - "$143564gdfg6114394fHBLK:matrix.org": {}, "$1435641916114394fHBLK:matrix.org": { ReceiptTypes.READ: { "@user:jki.re": { @@ -244,10 +202,9 @@ class ReceiptsTestCase(unittest.HomeserverTestCase): { "content": { "$14356419edgd14394fHBLK:matrix.org": { - ReceiptTypes.READ: { + ReceiptTypes.READ_PRIVATE: { "@rikj:jki.re": { "ts": 1436451550453, - "hidden": True, }, } }, @@ -306,7 +263,73 @@ class ReceiptsTestCase(unittest.HomeserverTestCase): "type": "m.receipt", }, ], - [], + [ + { + "content": { + "$14356419edgd14394fHBLK:matrix.org": { + ReceiptTypes.READ: { + "@rikj:jki.re": "string", + } + }, + }, + "room_id": "!jEsUZKDJdhlrceRyVU:example.org", + "type": "m.receipt", + }, + ], + ) + + def test_leaves_our_hidden_and_their_public(self): + self._test_filters_hidden( + [ + { + "content": { + "$1dgdgrd5641916114394fHBLK:matrix.org": { + ReceiptTypes.READ_PRIVATE: { + "@me:server.org": { + "ts": 1436451550453, + }, + }, + ReceiptTypes.READ: { + "@rikj:jki.re": { + "ts": 1436451550453, + }, + }, + "a.receipt.type": { + "@rikj:jki.re": { + "ts": 1436451550453, + }, + }, + }, + }, + "room_id": "!jEsUZKDJdhlrceRyVU:example.org", + "type": "m.receipt", + } + ], + [ + { + "content": { + "$1dgdgrd5641916114394fHBLK:matrix.org": { + ReceiptTypes.READ_PRIVATE: { + "@me:server.org": { + "ts": 1436451550453, + }, + }, + ReceiptTypes.READ: { + "@rikj:jki.re": { + "ts": 1436451550453, + }, + }, + "a.receipt.type": { + "@rikj:jki.re": { + "ts": 1436451550453, + }, + }, + } + }, + "room_id": "!jEsUZKDJdhlrceRyVU:example.org", + "type": "m.receipt", + } + ], ) def _test_filters_hidden( diff --git a/tests/replication/slave/storage/test_receipts.py b/tests/replication/slave/storage/test_receipts.py index de19e75b9d..5bbbd5fbcb 100644 --- a/tests/replication/slave/storage/test_receipts.py +++ b/tests/replication/slave/storage/test_receipts.py @@ -14,26 +14,246 @@ from synapse.api.constants import ReceiptTypes from synapse.replication.slave.storage.receipts import SlavedReceiptsStore +from synapse.types import UserID, create_requester + +from tests.test_utils.event_injection import create_event from ._base import BaseSlavedStoreTestCase -USER_ID = "@feeling:blue" -ROOM_ID = "!room:blue" -EVENT_ID = "$event:blue" +OTHER_USER_ID = "@other:test" +OUR_USER_ID = "@our:test" class SlavedReceiptTestCase(BaseSlavedStoreTestCase): STORE_TYPE = SlavedReceiptsStore - def test_receipt(self): - self.check("get_receipts_for_user", [USER_ID, ReceiptTypes.READ], {}) + def prepare(self, reactor, clock, homeserver): + super().prepare(reactor, clock, homeserver) + self.room_creator = homeserver.get_room_creation_handler() + self.persist_event_storage = self.hs.get_storage().persistence + + # Create a test user + self.ourUser = UserID.from_string(OUR_USER_ID) + self.ourRequester = create_requester(self.ourUser) + + # Create a second test user + self.otherUser = UserID.from_string(OTHER_USER_ID) + self.otherRequester = create_requester(self.otherUser) + + # Create a test room + info, _ = self.get_success(self.room_creator.create_room(self.ourRequester, {})) + self.room_id1 = info["room_id"] + + # Create a second test room + info, _ = self.get_success(self.room_creator.create_room(self.ourRequester, {})) + self.room_id2 = info["room_id"] + + # Join the second user to the first room + memberEvent, memberEventContext = self.get_success( + create_event( + self.hs, + room_id=self.room_id1, + type="m.room.member", + sender=self.otherRequester.user.to_string(), + state_key=self.otherRequester.user.to_string(), + content={"membership": "join"}, + ) + ) + self.get_success( + self.persist_event_storage.persist_event(memberEvent, memberEventContext) + ) + + # Join the second user to the second room + memberEvent, memberEventContext = self.get_success( + create_event( + self.hs, + room_id=self.room_id2, + type="m.room.member", + sender=self.otherRequester.user.to_string(), + state_key=self.otherRequester.user.to_string(), + content={"membership": "join"}, + ) + ) + self.get_success( + self.persist_event_storage.persist_event(memberEvent, memberEventContext) + ) + + def test_return_empty_with_no_data(self): + res = self.get_success( + self.master_store.get_receipts_for_user( + OUR_USER_ID, [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE] + ) + ) + self.assertEqual(res, {}) + + res = self.get_success( + self.master_store.get_receipts_for_user_with_orderings( + OUR_USER_ID, + [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE], + ) + ) + self.assertEqual(res, {}) + + res = self.get_success( + self.master_store.get_last_receipt_event_id_for_user( + OUR_USER_ID, + self.room_id1, + [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE], + ) + ) + self.assertEqual(res, None) + + def test_get_receipts_for_user(self): + # Send some events into the first room + event1_1_id = self.create_and_send_event( + self.room_id1, UserID.from_string(OTHER_USER_ID) + ) + event1_2_id = self.create_and_send_event( + self.room_id1, UserID.from_string(OTHER_USER_ID) + ) + + # Send public read receipt for the first event + self.get_success( + self.master_store.insert_receipt( + self.room_id1, ReceiptTypes.READ, OUR_USER_ID, [event1_1_id], {} + ) + ) + # Send private read receipt for the second event self.get_success( self.master_store.insert_receipt( - ROOM_ID, ReceiptTypes.READ, USER_ID, [EVENT_ID], {} + self.room_id1, ReceiptTypes.READ_PRIVATE, OUR_USER_ID, [event1_2_id], {} + ) + ) + + # Test we get the latest event when we want both private and public receipts + res = self.get_success( + self.master_store.get_receipts_for_user( + OUR_USER_ID, [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE] ) ) - self.replicate() - self.check( - "get_receipts_for_user", [USER_ID, ReceiptTypes.READ], {ROOM_ID: EVENT_ID} + self.assertEqual(res, {self.room_id1: event1_2_id}) + + # Test we get the older event when we want only public receipt + res = self.get_success( + self.master_store.get_receipts_for_user(OUR_USER_ID, [ReceiptTypes.READ]) + ) + self.assertEqual(res, {self.room_id1: event1_1_id}) + + # Test we get the latest event when we want only the public receipt + res = self.get_success( + self.master_store.get_receipts_for_user( + OUR_USER_ID, [ReceiptTypes.READ_PRIVATE] + ) + ) + self.assertEqual(res, {self.room_id1: event1_2_id}) + + # Test receipt updating + self.get_success( + self.master_store.insert_receipt( + self.room_id1, ReceiptTypes.READ, OUR_USER_ID, [event1_2_id], {} + ) + ) + res = self.get_success( + self.master_store.get_receipts_for_user(OUR_USER_ID, [ReceiptTypes.READ]) + ) + self.assertEqual(res, {self.room_id1: event1_2_id}) + + # Send some events into the second room + event2_1_id = self.create_and_send_event( + self.room_id2, UserID.from_string(OTHER_USER_ID) + ) + + # Test new room is reflected in what the method returns + self.get_success( + self.master_store.insert_receipt( + self.room_id2, ReceiptTypes.READ_PRIVATE, OUR_USER_ID, [event2_1_id], {} + ) + ) + res = self.get_success( + self.master_store.get_receipts_for_user( + OUR_USER_ID, [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE] + ) + ) + self.assertEqual(res, {self.room_id1: event1_2_id, self.room_id2: event2_1_id}) + + def test_get_last_receipt_event_id_for_user(self): + # Send some events into the first room + event1_1_id = self.create_and_send_event( + self.room_id1, UserID.from_string(OTHER_USER_ID) + ) + event1_2_id = self.create_and_send_event( + self.room_id1, UserID.from_string(OTHER_USER_ID) + ) + + # Send public read receipt for the first event + self.get_success( + self.master_store.insert_receipt( + self.room_id1, ReceiptTypes.READ, OUR_USER_ID, [event1_1_id], {} + ) + ) + # Send private read receipt for the second event + self.get_success( + self.master_store.insert_receipt( + self.room_id1, ReceiptTypes.READ_PRIVATE, OUR_USER_ID, [event1_2_id], {} + ) + ) + + # Test we get the latest event when we want both private and public receipts + res = self.get_success( + self.master_store.get_last_receipt_event_id_for_user( + OUR_USER_ID, + self.room_id1, + [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE], + ) + ) + self.assertEqual(res, event1_2_id) + + # Test we get the older event when we want only public receipt + res = self.get_success( + self.master_store.get_last_receipt_event_id_for_user( + OUR_USER_ID, self.room_id1, [ReceiptTypes.READ] + ) + ) + self.assertEqual(res, event1_1_id) + + # Test we get the latest event when we want only the private receipt + res = self.get_success( + self.master_store.get_last_receipt_event_id_for_user( + OUR_USER_ID, self.room_id1, [ReceiptTypes.READ_PRIVATE] + ) + ) + self.assertEqual(res, event1_2_id) + + # Test receipt updating + self.get_success( + self.master_store.insert_receipt( + self.room_id1, ReceiptTypes.READ, OUR_USER_ID, [event1_2_id], {} + ) + ) + res = self.get_success( + self.master_store.get_last_receipt_event_id_for_user( + OUR_USER_ID, self.room_id1, [ReceiptTypes.READ] + ) + ) + self.assertEqual(res, event1_2_id) + + # Send some events into the second room + event2_1_id = self.create_and_send_event( + self.room_id2, UserID.from_string(OTHER_USER_ID) + ) + + # Test new room is reflected in what the method returns + self.get_success( + self.master_store.insert_receipt( + self.room_id2, ReceiptTypes.READ_PRIVATE, OUR_USER_ID, [event2_1_id], {} + ) + ) + res = self.get_success( + self.master_store.get_last_receipt_event_id_for_user( + OUR_USER_ID, + self.room_id2, + [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE], + ) ) + self.assertEqual(res, event2_1_id) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index cb765455c1..67c94dd18f 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -23,7 +23,6 @@ import synapse.rest.admin from synapse.api.constants import ( EventContentFields, EventTypes, - ReadReceiptEventFields, ReceiptTypes, RelationTypes, ) @@ -347,7 +346,7 @@ class SyncKnockTestCase( # Knock on a room channel = self.make_request( "POST", - "/_matrix/client/r0/knock/%s" % (self.room_id,), + f"/_matrix/client/r0/knock/{self.room_id}", b"{}", self.knocker_tok, ) @@ -412,18 +411,79 @@ class ReadReceiptsTestCase(unittest.HomeserverTestCase): # Send a message as the first user res = self.helper.send(self.room_id, body="hello", tok=self.tok) - # Send a read receipt to tell the server the first user's message was read - body = json.dumps({ReadReceiptEventFields.MSC2285_HIDDEN: True}).encode("utf8") + # Send a private read receipt to tell the server the first user's message was read channel = self.make_request( "POST", - "/rooms/%s/receipt/m.read/%s" % (self.room_id, res["event_id"]), - body, + f"/rooms/{self.room_id}/receipt/org.matrix.msc2285.read.private/{res['event_id']}", + {}, access_token=self.tok2, ) self.assertEqual(channel.code, 200) - # Test that the first user can't see the other user's hidden read receipt - self.assertEqual(self._get_read_receipt(), None) + # Test that the first user can't see the other user's private read receipt + self.assertIsNone(self._get_read_receipt()) + + @override_config({"experimental_features": {"msc2285_enabled": True}}) + def test_public_receipt_can_override_private(self) -> None: + """ + Sending a public read receipt to the same event which has a private read + receipt should cause that receipt to become public. + """ + # Send a message as the first user + res = self.helper.send(self.room_id, body="hello", tok=self.tok) + + # Send a private read receipt + channel = self.make_request( + "POST", + f"/rooms/{self.room_id}/receipt/{ReceiptTypes.READ_PRIVATE}/{res['event_id']}", + {}, + access_token=self.tok2, + ) + self.assertEqual(channel.code, 200) + self.assertIsNone(self._get_read_receipt()) + + # Send a public read receipt + channel = self.make_request( + "POST", + f"/rooms/{self.room_id}/receipt/{ReceiptTypes.READ}/{res['event_id']}", + {}, + access_token=self.tok2, + ) + self.assertEqual(channel.code, 200) + + # Test that we did override the private read receipt + self.assertNotEqual(self._get_read_receipt(), None) + + @override_config({"experimental_features": {"msc2285_enabled": True}}) + def test_private_receipt_cannot_override_public(self) -> None: + """ + Sending a private read receipt to the same event which has a public read + receipt should cause no change. + """ + # Send a message as the first user + res = self.helper.send(self.room_id, body="hello", tok=self.tok) + + # Send a public read receipt + channel = self.make_request( + "POST", + f"/rooms/{self.room_id}/receipt/{ReceiptTypes.READ}/{res['event_id']}", + {}, + access_token=self.tok2, + ) + self.assertEqual(channel.code, 200) + self.assertNotEqual(self._get_read_receipt(), None) + + # Send a private read receipt + channel = self.make_request( + "POST", + f"/rooms/{self.room_id}/receipt/{ReceiptTypes.READ_PRIVATE}/{res['event_id']}", + {}, + access_token=self.tok2, + ) + self.assertEqual(channel.code, 200) + + # Test that we didn't override the public read receipt + self.assertIsNone(self._get_read_receipt()) @parameterized.expand( [ @@ -455,7 +515,7 @@ class ReadReceiptsTestCase(unittest.HomeserverTestCase): # Send a read receipt for this message with an empty body channel = self.make_request( "POST", - "/rooms/%s/receipt/m.read/%s" % (self.room_id, res["event_id"]), + f"/rooms/{self.room_id}/receipt/m.read/{res['event_id']}", access_token=self.tok2, custom_headers=[("User-Agent", user_agent)], ) @@ -479,6 +539,9 @@ class ReadReceiptsTestCase(unittest.HomeserverTestCase): # Store the next batch for the next request. self.next_batch = channel.json_body["next_batch"] + if channel.json_body.get("rooms", None) is None: + return None + # Return the read receipt ephemeral_events = channel.json_body["rooms"]["join"][self.room_id][ "ephemeral" @@ -499,7 +562,10 @@ class UnreadMessagesTestCase(unittest.HomeserverTestCase): def default_config(self) -> JsonDict: config = super().default_config() - config["experimental_features"] = {"msc2654_enabled": True} + config["experimental_features"] = { + "msc2654_enabled": True, + "msc2285_enabled": True, + } return config def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: @@ -564,7 +630,7 @@ class UnreadMessagesTestCase(unittest.HomeserverTestCase): body = json.dumps({ReceiptTypes.READ: res["event_id"]}).encode("utf8") channel = self.make_request( "POST", - "/rooms/%s/read_markers" % self.room_id, + f"/rooms/{self.room_id}/read_markers", body, access_token=self.tok, ) @@ -578,11 +644,10 @@ class UnreadMessagesTestCase(unittest.HomeserverTestCase): self._check_unread_count(1) # Send a read receipt to tell the server we've read the latest event. - body = json.dumps({ReadReceiptEventFields.MSC2285_HIDDEN: True}).encode("utf8") channel = self.make_request( "POST", - "/rooms/%s/receipt/m.read/%s" % (self.room_id, res["event_id"]), - body, + f"/rooms/{self.room_id}/receipt/org.matrix.msc2285.read.private/{res['event_id']}", + {}, access_token=self.tok, ) self.assertEqual(channel.code, 200, channel.json_body) @@ -644,13 +709,73 @@ class UnreadMessagesTestCase(unittest.HomeserverTestCase): self._check_unread_count(4) # Check that tombstone events changes increase the unread counter. - self.helper.send_state( + res1 = self.helper.send_state( self.room_id, EventTypes.Tombstone, {"replacement_room": "!someroom:test"}, tok=self.tok2, ) self._check_unread_count(5) + res2 = self.helper.send(self.room_id, "hello", tok=self.tok2) + + # Make sure both m.read and org.matrix.msc2285.read.private advance + channel = self.make_request( + "POST", + f"/rooms/{self.room_id}/receipt/m.read/{res1['event_id']}", + {}, + access_token=self.tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + self._check_unread_count(1) + + channel = self.make_request( + "POST", + f"/rooms/{self.room_id}/receipt/org.matrix.msc2285.read.private/{res2['event_id']}", + {}, + access_token=self.tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + self._check_unread_count(0) + + # We test for both receipt types that influence notification counts + @parameterized.expand([ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE]) + def test_read_receipts_only_go_down(self, receipt_type: ReceiptTypes) -> None: + # Join the new user + self.helper.join(room=self.room_id, user=self.user2, tok=self.tok2) + + # Send messages + res1 = self.helper.send(self.room_id, "hello", tok=self.tok2) + res2 = self.helper.send(self.room_id, "hello", tok=self.tok2) + + # Read last event + channel = self.make_request( + "POST", + f"/rooms/{self.room_id}/receipt/{receipt_type}/{res2['event_id']}", + {}, + access_token=self.tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + self._check_unread_count(0) + + # Make sure neither m.read nor org.matrix.msc2285.read.private make the + # read receipt go up to an older event + channel = self.make_request( + "POST", + f"/rooms/{self.room_id}/receipt/org.matrix.msc2285.read.private/{res1['event_id']}", + {}, + access_token=self.tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + self._check_unread_count(0) + + channel = self.make_request( + "POST", + f"/rooms/{self.room_id}/receipt/m.read/{res1['event_id']}", + {}, + access_token=self.tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + self._check_unread_count(0) def _check_unread_count(self, expected_count: int) -> None: """Syncs and compares the unread count with the expected value.""" @@ -663,9 +788,11 @@ class UnreadMessagesTestCase(unittest.HomeserverTestCase): self.assertEqual(channel.code, 200, channel.json_body) - room_entry = channel.json_body["rooms"]["join"][self.room_id] + room_entry = ( + channel.json_body.get("rooms", {}).get("join", {}).get(self.room_id, {}) + ) self.assertEqual( - room_entry["org.matrix.msc2654.unread_count"], + room_entry.get("org.matrix.msc2654.unread_count", 0), expected_count, room_entry, ) |