From ba83fd22174ec12cc3ce8856b99b0d8edc19a2af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Sat, 5 Mar 2022 09:01:18 +0100 Subject: [PATCH] Implement changes to MSC2285 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/api/constants.py | 6 +- synapse/handlers/receipts.py | 51 ++++++-------- synapse/handlers/sync.py | 10 ++- synapse/push/push_tools.py | 3 +- synapse/rest/client/notifications.py | 3 +- synapse/rest/client/read_marker.py | 32 +++++---- synapse/rest/client/receipts.py | 60 ++++++++++------ synapse/storage/databases/main/receipts.py | 24 ++++--- tests/handlers/test_receipts.py | 68 ++++++++++--------- .../slave/storage/test_receipts.py | 4 +- tests/rest/client/test_sync.py | 19 ++---- 11 files changed, 151 insertions(+), 129 deletions(-) diff --git a/synapse/api/constants.py b/synapse/api/constants.py index 36ace7c6134f..b4534ddd1ca9 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 6250bb3bdf2b..b5971bdbfc1b 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 @@ -138,7 +138,7 @@ async def _handle_new_receipts(self, receipts: List[ReadReceipt]) -> bool: 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,7 +148,7 @@ async def received_client_receipt( 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]) @@ -156,7 +156,8 @@ async def received_client_receipt( return if self.federation_sender and not ( - self.hs.config.experimental.msc2285_enabled and hidden + self.hs.config.experimental.msc2285_enabled + and receipt_type == ReceiptTypes.READ_PRIVATE ): await self.federation_sender.send_read_receipt(receipt) @@ -178,35 +179,27 @@ def filter_out_hidden(events: List[JsonDict], user_id: str) -> List[JsonDict]: 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() + m_read = event_content.get(ReceiptTypes.READ, None) + if m_read: + new_event["content"][event_id] = {ReceiptTypes.READ: m_read} 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 + m_read_private = event_content.get(ReceiptTypes.READ_PRIVATE, None) + if m_read_private: + new_users = {} + for rr_user_id, user_rr in m_read_private.items(): + if rr_user_id == user_id: + new_users[rr_user_id] = user_rr.copy() + + # Set new users unless empty + if len(new_users.keys()) > 0: + new_event["content"][event_id] = { + ReceiptTypes.READ_PRIVATE: new_users + } + 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} + new_event["content"][event_id] = event_content # 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 0aa3052fd6ac..abd15658da9b 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -1065,8 +1065,16 @@ async def unread_notifs_for_room_id( 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_type=ReceiptTypes.READ_PRIVATE, ) + if not last_unread_event_id: + 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, + ) + ) return await self.store.get_unread_event_push_actions_by_room_for_user( room_id, sync_config.user.to_string(), last_unread_event_id diff --git a/synapse/push/push_tools.py b/synapse/push/push_tools.py index 957c9b780b94..b10d9b03ef17 100644 --- a/synapse/push/push_tools.py +++ b/synapse/push/push_tools.py @@ -13,7 +13,6 @@ # limitations under the License. from typing import Dict -from synapse.api.constants import ReceiptTypes from synapse.events import EventBase from synapse.push.presentable_names import calculate_room_name, name_from_member_event from synapse.storage import Storage @@ -24,7 +23,7 @@ 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) badge = len(invites) diff --git a/synapse/rest/client/notifications.py b/synapse/rest/client/notifications.py index ff040de6b840..8ee92d9d47b7 100644 --- a/synapse/rest/client/notifications.py +++ b/synapse/rest/client/notifications.py @@ -15,7 +15,6 @@ import logging from typing import TYPE_CHECKING, Tuple -from synapse.api.constants import ReceiptTypes from synapse.events.utils import ( SerializeEventConfig, format_event_for_client_v2_without_room_id, @@ -58,7 +57,7 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: ) receipts_by_room = await self.store.get_receipts_for_user_with_orderings( - user_id, ReceiptTypes.READ + user_id ) 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 f51be511d1f4..7d2449448b1c 100644 --- a/synapse/rest/client/read_marker.py +++ b/synapse/rest/client/read_marker.py @@ -15,8 +15,7 @@ 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.http.server import HttpServer from synapse.http.servlet import RestServlet, parse_json_object_from_request from synapse.http.site import SynapseRequest @@ -48,27 +47,32 @@ async def on_POST( 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): - raise SynapseError( - 400, - "Param %s must be a boolean, if given" - % ReadReceiptEventFields.MSC2285_HIDDEN, - Codes.BAD_JSON, - ) + 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, + ) + await self.receipts_handler.received_client_receipt( + room_id, + ReceiptTypes.READ_PRIVATE, + user_id=requester.user.to_string(), + event_id=read_event_id, + ) + + read_private_event_id = body.get(ReceiptTypes.READ_PRIVATE, None) + if read_private_event_id: + 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("m.fully_read", None) + 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 b24ad2d1be13..62932c32a767 100644 --- a/synapse/rest/client/receipts.py +++ b/synapse/rest/client/receipts.py @@ -16,8 +16,8 @@ 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 @@ def __init__(self, hs: "HomeServer"): 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,8 +54,15 @@ async def on_POST( ) -> Tuple[int, JsonDict]: requester = await self.auth.get_user_by_req(request) - if receipt_type != ReceiptTypes.READ: - raise SynapseError(400, "Receipt type must be 'm.read'") + if 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'", + ) # Do not allow older SchildiChat and Element Android clients (prior to Element/1.[012].x) to send an empty body. user_agent = get_request_user_agent(request) @@ -62,26 +70,36 @@ async def on_POST( 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, - ) + 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, + ) + elif receipt_type == ReceiptTypes.READ: + await self.receipts_handler.received_client_receipt( + room_id, + ReceiptTypes.READ, + user_id=requester.user.to_string(), + event_id=event_id, + ) + await self.receipts_handler.received_client_receipt( + room_id, + ReceiptTypes.READ_PRIVATE, + 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 bf0b903af2fc..51e09e545a17 100644 --- a/synapse/storage/databases/main/receipts.py +++ b/synapse/storage/databases/main/receipts.py @@ -134,22 +134,26 @@ async def get_last_receipt_event_id_for_user( allow_none=True, ) - @cached(num_args=2) - async def get_receipts_for_user( - self, user_id: str, receipt_type: str - ) -> Dict[str, str]: + @cached(num_args=1) + async def get_receipts_for_user(self, user_id: str) -> Dict[str, str]: 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"), + keyvalues={"user_id": user_id}, + retcols=("room_id", "event_id", "receipt_type"), desc="get_receipts_for_user", ) - return {row["room_id"]: row["event_id"] for row in rows} + # Give ReceiptTypes.READ_PRIVATE precedence + ret = {} + for row in rows: + if row["receipt_type"] == ReceiptTypes.READ_PRIVATE: + ret[row["room_id"]] = row["event_id"] + for row in rows: + if not ret.get(row["room_id"], None): + ret[row["room_id"]] = row["event_id"] + return ret - async def get_receipts_for_user_with_orderings( - self, user_id: str, receipt_type: str - ) -> JsonDict: + async def get_receipts_for_user_with_orderings(self, user_id: str) -> JsonDict: def f(txn: LoggingTransaction) -> List[Tuple[str, str, int, int]]: sql = ( "SELECT rl.room_id, rl.event_id," diff --git a/tests/handlers/test_receipts.py b/tests/handlers/test_receipts.py index 5081b97573a0..f00275109388 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 +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": { - "m.read": { + ReceiptTypes.READ_PRIVATE: { "@rikj:jki.re": { "ts": 1436451550453, - "hidden": True, } } } @@ -56,10 +51,9 @@ def test_does_not_filter_out_our_hidden_receipt(self): { "content": { "$1435641916hfgh4394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ_PRIVATE: { "@me:server.org": { "ts": 1436451550453, - "hidden": True, }, } } @@ -72,10 +66,9 @@ def test_does_not_filter_out_our_hidden_receipt(self): { "content": { "$1435641916hfgh4394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ_PRIVATE: { "@me:server.org": { "ts": 1436451550453, - ReadReceiptEventFields.MSC2285_HIDDEN: True, }, } } @@ -92,16 +85,17 @@ def test_filters_out_hidden_receipt_and_ignores_rest(self): { "content": { "$1dgdgrd5641916114394fHBLK:matrix.org": { - "m.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", @@ -111,7 +105,7 @@ def test_filters_out_hidden_receipt_and_ignores_rest(self): { "content": { "$1dgdgrd5641916114394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ: { "@user:jki.re": { "ts": 1436451550453, } @@ -130,15 +124,14 @@ def test_filters_out_event_with_only_hidden_receipts_and_ignores_the_rest(self): { "content": { "$14356419edgd14394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ_PRIVATE: { "@rikj:jki.re": { "ts": 1436451550453, - "hidden": True, }, } }, "$1435641916114394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ: { "@user:jki.re": { "ts": 1436451550453, } @@ -153,7 +146,7 @@ def test_filters_out_event_with_only_hidden_receipts_and_ignores_the_rest(self): { "content": { "$1435641916114394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ: { "@user:jki.re": { "ts": 1436451550453, } @@ -171,9 +164,9 @@ def test_handles_missing_content_of_m_read(self): [ { "content": { - "$14356419ggffg114394fHBLK:matrix.org": {"m.read": {}}, + "$14356419ggffg114394fHBLK:matrix.org": {ReceiptTypes.READ: {}}, "$1435641916114394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ: { "@user:jki.re": { "ts": 1436451550453, } @@ -187,9 +180,9 @@ def test_handles_missing_content_of_m_read(self): [ { "content": { - "$14356419ggffg114394fHBLK:matrix.org": {"m.read": {}}, + "$14356419ggffg114394fHBLK:matrix.org": {ReceiptTypes.READ: {}}, "$1435641916114394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ: { "@user:jki.re": { "ts": 1436451550453, } @@ -209,7 +202,7 @@ def test_handles_empty_event(self): "content": { "$143564gdfg6114394fHBLK:matrix.org": {}, "$1435641916114394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ: { "@user:jki.re": { "ts": 1436451550453, } @@ -225,7 +218,7 @@ def test_handles_empty_event(self): "content": { "$143564gdfg6114394fHBLK:matrix.org": {}, "$1435641916114394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ: { "@user:jki.re": { "ts": 1436451550453, } @@ -244,10 +237,9 @@ def test_filters_out_receipt_event_with_only_hidden_receipt_and_ignores_rest(sel { "content": { "$14356419edgd14394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ_PRIVATE: { "@rikj:jki.re": { "ts": 1436451550453, - "hidden": True, }, } }, @@ -258,7 +250,7 @@ def test_filters_out_receipt_event_with_only_hidden_receipt_and_ignores_rest(sel { "content": { "$1435641916114394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ: { "@user:jki.re": { "ts": 1436451550453, } @@ -273,7 +265,7 @@ def test_filters_out_receipt_event_with_only_hidden_receipt_and_ignores_rest(sel { "content": { "$1435641916114394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ: { "@user:jki.re": { "ts": 1436451550453, } @@ -297,7 +289,20 @@ def test_handles_string_data(self): { "content": { "$14356419edgd14394fHBLK:matrix.org": { - "m.read": { + ReceiptTypes.READ: { + "@rikj:jki.re": "string", + } + }, + }, + "room_id": "!jEsUZKDJdhlrceRyVU:example.org", + "type": "m.receipt", + }, + ], + [ + { + "content": { + "$14356419edgd14394fHBLK:matrix.org": { + ReceiptTypes.READ: { "@rikj:jki.re": "string", } }, @@ -306,7 +311,6 @@ def test_handles_string_data(self): "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 f47d94f690f9..f03bc9882eaa 100644 --- a/tests/replication/slave/storage/test_receipts.py +++ b/tests/replication/slave/storage/test_receipts.py @@ -26,9 +26,9 @@ class SlavedReceiptTestCase(BaseSlavedStoreTestCase): STORE_TYPE = SlavedReceiptsStore def test_receipt(self): - self.check("get_receipts_for_user", [USER_ID, "m.read"], {}) + self.check("get_receipts_for_user", [USER_ID], {}) self.get_success( self.master_store.insert_receipt(ROOM_ID, "m.read", USER_ID, [EVENT_ID], {}) ) self.replicate() - self.check("get_receipts_for_user", [USER_ID, "m.read"], {ROOM_ID: EVENT_ID}) + self.check("get_receipts_for_user", [USER_ID], {ROOM_ID: EVENT_ID}) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 435101395204..ddf03f75a0fc 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -20,12 +20,7 @@ from twisted.test.proto_helpers import MemoryReactor import synapse.rest.admin -from synapse.api.constants import ( - EventContentFields, - EventTypes, - ReadReceiptEventFields, - RelationTypes, -) +from synapse.api.constants import EventContentFields, EventTypes, RelationTypes from synapse.rest.client import devices, knock, login, read_marker, receipts, room, sync from synapse.server import HomeServer from synapse.types import JsonDict @@ -413,11 +408,11 @@ def test_hidden_read_receipts(self) -> None: 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") channel = self.make_request( "POST", - "/rooms/%s/receipt/m.read/%s" % (self.room_id, res["event_id"]), - body, + "/rooms/%s/receipt/org.matrix.msc2285.read.private/%s" + % (self.room_id, res["event_id"]), + {}, access_token=self.tok2, ) self.assertEqual(channel.code, 200) @@ -573,11 +568,11 @@ def test_unread_counts(self) -> None: 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, + "/rooms/%s/receipt/org.matrix.msc2285.read.private/%s" + % (self.room_id, res["event_id"]), + {}, access_token=self.tok, ) self.assertEqual(channel.code, 200, channel.json_body)