Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add support for stable prefixes for MSC2285: private read receipts #13273

Merged
Merged
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
83b7af1
Switch to stable prefixes for MSC2285
SimonBrandner Jul 14, 2022
01fbb6d
Add changelog
SimonBrandner Jul 14, 2022
cafe4be
Avoid leaking unstable private read receipts
SimonBrandner Jul 14, 2022
967f8ed
Delint
SimonBrandner Jul 14, 2022
a9f2329
Fix code
SimonBrandner Jul 14, 2022
08d874d
Merge remote-tracking branch 'upstream/develop' into SimonBrandner/fe…
SimonBrandner Jul 23, 2022
ba20652
Use a const
SimonBrandner Jul 23, 2022
d18a485
Convert list to tuple.
clokep Jul 25, 2022
8323358
Merge remote-tracking branch 'upstream/develop' into SimonBrandner/fe…
SimonBrandner Jul 28, 2022
404459f
Add background update to remove unstable private read receipts
SimonBrandner Jul 29, 2022
57251c8
Remove handling for unstable private read receipts
SimonBrandner Jul 29, 2022
e12706c
Don't bump `SCHEMA_VERSION`
SimonBrandner Jul 29, 2022
c8cf10e
Remove `READ_PRIVATE_UNSTABLE` from `ReceiptTypes`
SimonBrandner Jul 29, 2022
84299f6
Assume return value
SimonBrandner Jul 29, 2022
208e97a
Remove non-relevant test
SimonBrandner Jul 29, 2022
a7c3a1c
Don't bother with a migration script
SimonBrandner Jul 29, 2022
252146d
Remove non-schema thing
SimonBrandner Jul 29, 2022
3cbd80b
Only use single `'`
SimonBrandner Jul 29, 2022
8854e35
Use `!=`
SimonBrandner Jul 29, 2022
baa9e84
Merge pull request #1 from SimonBrandner/SimonBrandner/feat/db-rr
SimonBrandner Jul 29, 2022
4455232
Changelog
SimonBrandner Aug 4, 2022
de9005f
Merge remote-tracking branch 'upstream/develop' into SimonBrandner/fe…
SimonBrandner Aug 4, 2022
c0b3c90
Support both stable and unstable private RRs
SimonBrandner Aug 4, 2022
3612ceb
Test both stable and unstable private RRs
SimonBrandner Aug 4, 2022
a37a17d
Advertise unstable prefix
SimonBrandner Aug 4, 2022
14b36a3
Check for all receipt types
SimonBrandner Aug 4, 2022
746351c
Delint
SimonBrandner Aug 4, 2022
50bffe4
Further dlint
SimonBrandner Aug 4, 2022
1b66814
Remove `(`
SimonBrandner Aug 4, 2022
bcdbb5d
This won't work...
SimonBrandner Aug 4, 2022
0f498e0
Reformat...
SimonBrandner Aug 4, 2022
660c776
Get tests passing
SimonBrandner Aug 4, 2022
b1ddf1a
Merge remote-tracking branch 'origin/develop' into SimonBrandner/feat…
clokep Aug 4, 2022
3094b39
Fix type hints.
clokep Aug 4, 2022
011d6d2
Put experimental flag back in place
SimonBrandner Aug 5, 2022
ee6512f
Only advertise `org.matrix.msc2285` if enabled
SimonBrandner Aug 5, 2022
2cd29bd
Fix tests
SimonBrandner Aug 5, 2022
89d5bd4
Merge remote-tracking branch 'origin/SimonBrandner/feat/disable-rr' i…
SimonBrandner Aug 5, 2022
f0c531d
Fix tests
SimonBrandner Aug 5, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/13273.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add support for stable prefixes for [MSC2285 (private read receipts)](https://github.com/matrix-org/matrix-spec-proposals/pull/2285).
3 changes: 2 additions & 1 deletion synapse/api/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,8 @@ class GuestAccess:

class ReceiptTypes:
READ: Final = "m.read"
READ_PRIVATE: Final = "org.matrix.msc2285.read.private"
READ_PRIVATE: Final = "m.read.private"
UNSTABLE_READ_PRIVATE: Final = "org.matrix.msc2285.read.private"
FULLY_READ: Final = "m.fully_read"


Expand Down
3 changes: 0 additions & 3 deletions synapse/config/experimental.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,6 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
# MSC2716 (importing historical messages)
self.msc2716_enabled: bool = experimental.get("msc2716_enabled", False)

# MSC2285 (private read receipts)
self.msc2285_enabled: bool = experimental.get("msc2285_enabled", False)

# MSC3244 (room version capabilities)
self.msc3244_enabled: bool = experimental.get("msc3244_enabled", True)

Expand Down
11 changes: 4 additions & 7 deletions synapse/handlers/initial_sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,8 @@ async def _snapshot_all_rooms(
joined_rooms,
to_key=int(now_token.receipt_key),
)
if self.hs.config.experimental.msc2285_enabled:
receipt = ReceiptEventSource.filter_out_private_receipts(receipt, user_id)

receipt = ReceiptEventSource.filter_out_private_receipts(receipt, user_id)

tags_by_room = await self.store.get_tags_for_user(user_id)

Expand Down Expand Up @@ -456,11 +456,8 @@ async def get_receipts() -> List[JsonDict]:
)
if not receipts:
return []
if self.hs.config.experimental.msc2285_enabled:
receipts = ReceiptEventSource.filter_out_private_receipts(
receipts, user_id
)
return receipts

return ReceiptEventSource.filter_out_private_receipts(receipts, user_id)

presence, receipts, (messages, token) = await make_deferred_yieldable(
gather_results(
Expand Down
36 changes: 26 additions & 10 deletions synapse/handlers/receipts.py
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,10 @@ async def received_client_receipt(
if not is_new:
return

if self.federation_sender and receipt_type != ReceiptTypes.READ_PRIVATE:
if self.federation_sender and receipt_type not in (
ReceiptTypes.READ_PRIVATE,
ReceiptTypes.UNSTABLE_READ_PRIVATE,
):
await self.federation_sender.send_read_receipt(receipt)


Expand Down Expand Up @@ -203,24 +206,38 @@ def filter_out_private_receipts(
for event_id, orig_event_content in room.get("content", {}).items():
event_content = orig_event_content
# If there are private read receipts, additional logic is necessary.
if ReceiptTypes.READ_PRIVATE in event_content:
if (
ReceiptTypes.READ_PRIVATE in event_content
or ReceiptTypes.UNSTABLE_READ_PRIVATE in event_content
):
# Make a copy without private read receipts to avoid leaking
# other user's private read receipts..
event_content = {
receipt_type: receipt_value
for receipt_type, receipt_value in event_content.items()
if receipt_type != ReceiptTypes.READ_PRIVATE
if receipt_type
not in (
ReceiptTypes.READ_PRIVATE,
ReceiptTypes.UNSTABLE_READ_PRIVATE,
)
}

# Copy the current user's private read receipt from the
# original content, if it exists.
user_private_read_receipt = orig_event_content[
ReceiptTypes.READ_PRIVATE
].get(user_id, None)
user_private_read_receipt = orig_event_content.get(
ReceiptTypes.READ_PRIVATE, {}
).get(user_id, None)
SimonBrandner marked this conversation as resolved.
Show resolved Hide resolved
if user_private_read_receipt:
event_content[ReceiptTypes.READ_PRIVATE] = {
user_id: user_private_read_receipt
}
user_unstable_private_read_receipt = orig_event_content.get(
ReceiptTypes.UNSTABLE_READ_PRIVATE, {}
).get(user_id, None)
if user_unstable_private_read_receipt:
event_content[ReceiptTypes.UNSTABLE_READ_PRIVATE] = {
user_id: user_unstable_private_read_receipt
}

# Include the event if there is at least one non-private read
# receipt or the current user has a private read receipt.
Expand Down Expand Up @@ -256,10 +273,9 @@ async def get_new_events(
room_ids, from_key=from_key, to_key=to_key
)

if self.config.experimental.msc2285_enabled:
events = ReceiptEventSource.filter_out_private_receipts(
events, user.to_string()
)
events = ReceiptEventSource.filter_out_private_receipts(
events, user.to_string()
)

return events, to_key

Expand Down
5 changes: 4 additions & 1 deletion synapse/replication/tcp/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,10 @@ async def _on_new_receipts(
if not self._is_mine_id(receipt.user_id):
continue
# Private read receipts never get sent over federation.
if receipt.receipt_type == ReceiptTypes.READ_PRIVATE:
if receipt.receipt_type in (
ReceiptTypes.READ_PRIVATE,
ReceiptTypes.UNSTABLE_READ_PRIVATE,
):
continue
receipt_info = ReadReceipt(
receipt.room_id,
Expand Down
7 changes: 6 additions & 1 deletion synapse/rest/client/notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,12 @@ 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, ReceiptTypes.READ_PRIVATE]
user_id,
[
ReceiptTypes.READ,
ReceiptTypes.READ_PRIVATE,
ReceiptTypes.UNSTABLE_READ_PRIVATE,
],
)

notif_event_ids = [pa.event_id for pa in push_actions]
Expand Down
9 changes: 6 additions & 3 deletions synapse/rest/client/read_marker.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,12 @@ def __init__(self, hs: "HomeServer"):
self.read_marker_handler = hs.get_read_marker_handler()
self.presence_handler = hs.get_presence_handler()

self._known_receipt_types = {ReceiptTypes.READ, ReceiptTypes.FULLY_READ}
if hs.config.experimental.msc2285_enabled:
self._known_receipt_types.add(ReceiptTypes.READ_PRIVATE)
SimonBrandner marked this conversation as resolved.
Show resolved Hide resolved
self._known_receipt_types = {
ReceiptTypes.READ,
ReceiptTypes.FULLY_READ,
ReceiptTypes.READ_PRIVATE,
ReceiptTypes.UNSTABLE_READ_PRIVATE,
}

async def on_POST(
self, request: SynapseRequest, room_id: str
Expand Down
11 changes: 6 additions & 5 deletions synapse/rest/client/receipts.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,12 @@ def __init__(self, hs: "HomeServer"):
self.read_marker_handler = hs.get_read_marker_handler()
self.presence_handler = hs.get_presence_handler()

self._known_receipt_types = {ReceiptTypes.READ}
if hs.config.experimental.msc2285_enabled:
self._known_receipt_types.update(
(ReceiptTypes.READ_PRIVATE, ReceiptTypes.FULLY_READ)
)
self._known_receipt_types = {
ReceiptTypes.READ,
ReceiptTypes.READ_PRIVATE,
ReceiptTypes.UNSTABLE_READ_PRIVATE,
ReceiptTypes.FULLY_READ,
}

async def on_POST(
self, request: SynapseRequest, room_id: str, receipt_type: str, event_id: str
Expand Down
3 changes: 2 additions & 1 deletion synapse/rest/client/versions.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@ def on_GET(self, request: Request) -> Tuple[int, JsonDict]:
# Supports the busy presence state described in MSC3026.
"org.matrix.msc3026.busy_presence": self.config.experimental.msc3026_enabled,
# Supports receiving private read receipts as per MSC2285
"org.matrix.msc2285": self.config.experimental.msc2285_enabled,
"org.matrix.msc2285.stable": True, # TODO: Remove when MSC2285 becomes a part of the spec
"org.matrix.msc2285": True, # TODO: Remove after one release cycle
SimonBrandner marked this conversation as resolved.
Show resolved Hide resolved
# Supports filtering of /publicRooms by room type as per MSC3827
"org.matrix.msc3827.stable": True,
# Adds support for importing historical messages as per MSC2716
Expand Down
85 changes: 70 additions & 15 deletions synapse/storage/databases/main/event_push_actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@

from synapse.api.constants import ReceiptTypes
from synapse.metrics.background_process_metrics import wrap_as_background_process
from synapse.storage._base import SQLBaseStore, db_to_json
from synapse.storage._base import SQLBaseStore, db_to_json, make_in_list_sql_clause
from synapse.storage.database import (
DatabasePool,
LoggingDatabaseConnection,
Expand Down Expand Up @@ -259,7 +259,11 @@ def _get_unread_counts_by_receipt_txn(
txn,
user_id,
room_id,
receipt_types=(ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE),
receipt_types=(
ReceiptTypes.READ,
ReceiptTypes.READ_PRIVATE,
ReceiptTypes.UNSTABLE_READ_PRIVATE,
),
)

stream_ordering = None
Expand Down Expand Up @@ -448,25 +452,37 @@ async def get_unread_push_actions_for_user_in_range_for_http(
The list will be ordered by ascending stream_ordering.
The list will have between 0~limit entries.
"""

# find rooms that have a read receipt in them and return the next
# push actions
def get_after_receipt(
txn: LoggingTransaction,
) -> List[Tuple[str, str, int, str, bool]]:
# find rooms that have a read receipt in them and return the next
# push actions
sql = """

receipt_types_clause, args = make_in_list_sql_clause(
self.database_engine,
"receipt_type",
(
ReceiptTypes.READ,
ReceiptTypes.READ_PRIVATE,
ReceiptTypes.UNSTABLE_READ_PRIVATE,
),
)

sql = f"""
SELECT ep.event_id, ep.room_id, ep.stream_ordering, ep.actions,
ep.highlight
FROM (
SELECT room_id,
MAX(stream_ordering) as stream_ordering
FROM events
INNER JOIN receipts_linearized USING (room_id, event_id)
WHERE receipt_type = 'm.read' AND user_id = ?
WHERE {receipt_types_clause} AND user_id = ?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the ramifications of this change (and the ones below it) are that we might have HTTP pushed / emailed for something that was actually read using a private read receipt? Does that sound accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I am not sure I understand the code at hand though my thinking was that since both public and private RRs influence notifs in the same way, they should be added here. Was my thinking incorrect?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was my thinking incorrect?

That's my understanding as well. I'm trying to figure out the user-impact of fixing this bug.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I double checked this; I think (before fixing this to look at all read receipt types) you could end up in this situation:

  1. There are some new events in a room.
  2. The user reads them and sends a private read receipt.
  3. They have an email or HTTP pusher configured.
  4. The private read receipt would not be considered read when checking for events to push.
  5. The new events would be pushed even though they're before the private read receipt.

Good that we found it, but doesn't seem to be the end of the world!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good that we found it

Thank you very much for doing that! I've totally missed it

GROUP BY room_id
) AS rl,
event_push_actions AS ep
event_push_actions AS ep
WHERE
ep.room_id = rl.room_id
AND ep.stream_ordering > rl.stream_ordering
Expand All @@ -476,7 +492,9 @@ def get_after_receipt(
AND ep.notif = 1
ORDER BY ep.stream_ordering ASC LIMIT ?
"""
args = [user_id, user_id, min_stream_ordering, max_stream_ordering, limit]
args.extend(
(user_id, user_id, min_stream_ordering, max_stream_ordering, limit)
)
txn.execute(sql, args)
return cast(List[Tuple[str, str, int, str, bool]], txn.fetchall())

Expand All @@ -490,15 +508,25 @@ def get_after_receipt(
def get_no_receipt(
txn: LoggingTransaction,
) -> List[Tuple[str, str, int, str, bool]]:
sql = """
receipt_types_clause, args = make_in_list_sql_clause(
self.database_engine,
"receipt_type",
(
ReceiptTypes.READ,
ReceiptTypes.READ_PRIVATE,
ReceiptTypes.UNSTABLE_READ_PRIVATE,
),
)

sql = f"""
SELECT ep.event_id, ep.room_id, ep.stream_ordering, ep.actions,
ep.highlight
FROM event_push_actions AS ep
INNER JOIN events AS e USING (room_id, event_id)
WHERE
ep.room_id NOT IN (
SELECT room_id FROM receipts_linearized
WHERE receipt_type = 'm.read' AND user_id = ?
WHERE {receipt_types_clause} AND user_id = ?
GROUP BY room_id
)
AND ep.user_id = ?
Expand All @@ -507,7 +535,9 @@ def get_no_receipt(
AND ep.notif = 1
ORDER BY ep.stream_ordering ASC LIMIT ?
"""
args = [user_id, user_id, min_stream_ordering, max_stream_ordering, limit]
args.extend(
(user_id, user_id, min_stream_ordering, max_stream_ordering, limit)
)
txn.execute(sql, args)
return cast(List[Tuple[str, str, int, str, bool]], txn.fetchall())

Expand Down Expand Up @@ -557,20 +587,31 @@ async def get_unread_push_actions_for_user_in_range_for_email(
The list will be ordered by descending received_ts.
The list will have between 0~limit entries.
"""

# find rooms that have a read receipt in them and return the most recent
# push actions
def get_after_receipt(
txn: LoggingTransaction,
) -> List[Tuple[str, str, int, str, bool, int]]:
sql = """
receipt_types_clause, args = make_in_list_sql_clause(
self.database_engine,
"receipt_type",
(
ReceiptTypes.READ,
ReceiptTypes.READ_PRIVATE,
ReceiptTypes.UNSTABLE_READ_PRIVATE,
),
)

sql = f"""
SELECT ep.event_id, ep.room_id, ep.stream_ordering, ep.actions,
ep.highlight, e.received_ts
FROM (
SELECT room_id,
MAX(stream_ordering) as stream_ordering
FROM events
INNER JOIN receipts_linearized USING (room_id, event_id)
WHERE receipt_type = 'm.read' AND user_id = ?
WHERE {receipt_types_clause} AND user_id = ?
GROUP BY room_id
) AS rl,
event_push_actions AS ep
Expand All @@ -584,7 +625,9 @@ def get_after_receipt(
AND ep.notif = 1
ORDER BY ep.stream_ordering DESC LIMIT ?
"""
args = [user_id, user_id, min_stream_ordering, max_stream_ordering, limit]
args.extend(
(user_id, user_id, min_stream_ordering, max_stream_ordering, limit)
)
txn.execute(sql, args)
return cast(List[Tuple[str, str, int, str, bool, int]], txn.fetchall())

Expand All @@ -598,15 +641,25 @@ def get_after_receipt(
def get_no_receipt(
txn: LoggingTransaction,
) -> List[Tuple[str, str, int, str, bool, int]]:
sql = """
receipt_types_clause, args = make_in_list_sql_clause(
self.database_engine,
"receipt_type",
(
ReceiptTypes.READ,
ReceiptTypes.READ_PRIVATE,
ReceiptTypes.UNSTABLE_READ_PRIVATE,
),
)

sql = f"""
SELECT ep.event_id, ep.room_id, ep.stream_ordering, ep.actions,
ep.highlight, e.received_ts
FROM event_push_actions AS ep
INNER JOIN events AS e USING (room_id, event_id)
WHERE
ep.room_id NOT IN (
SELECT room_id FROM receipts_linearized
WHERE receipt_type = 'm.read' AND user_id = ?
WHERE {receipt_types_clause} AND user_id = ?
GROUP BY room_id
)
AND ep.user_id = ?
Expand All @@ -615,7 +668,9 @@ def get_no_receipt(
AND ep.notif = 1
ORDER BY ep.stream_ordering DESC LIMIT ?
"""
args = [user_id, user_id, min_stream_ordering, max_stream_ordering, limit]
args.extend(
(user_id, user_id, min_stream_ordering, max_stream_ordering, limit)
)
txn.execute(sql, args)
return cast(List[Tuple[str, str, int, str, bool, int]], txn.fetchall())

Expand Down
Loading