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

Support optionally not sending read receipts to other users/servers #5990

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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/5990.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Support a way for clients to not send read receipts to other users/servers.
Copy link
Member

Choose a reason for hiding this comment

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

link to MSC?

2 changes: 2 additions & 0 deletions synapse/app/federation_sender.py
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,8 @@ def _on_new_receipts(self, rows):
# we only want to send on receipts for our own users
if not self._is_mine_id(receipt.user_id):
continue
if receipt.data.get("hidden", False):
return # do not send over federation
Copy link
Member

@richvdh richvdh Sep 9, 2019

Choose a reason for hiding this comment

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

we don't want to skip all receipts in the list, aiui:

Suggested change
return # do not send over federation
continue # do not send over federation

receipt_info = ReadReceipt(
receipt.room_id,
receipt.receipt_type,
Expand Down
7 changes: 6 additions & 1 deletion synapse/handlers/initial_sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,12 @@ def get_receipts():
)
if not receipts:
receipts = []
return receipts

return [
r
for r in receipts
if not r.data.get("hidden", False) or r.user_id == user_id
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not entirely convinced that this works because I didn't test initialSync. I am hoping to pawn all this imperfect python over to someone else eventually.

]

presence, receipts, (messages, token) = yield make_deferred_yieldable(
defer.gatherResults(
Expand Down
7 changes: 4 additions & 3 deletions synapse/handlers/receipts.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ def _handle_new_receipts(self, receipts):
return True

@defer.inlineCallbacks
def received_client_receipt(self, room_id, receipt_type, user_id, event_id):
def received_client_receipt(self, room_id, receipt_type, user_id, event_id, hidden):
"""Called when a client tells us a local user has read up to the given
event_id in the room.
"""
Expand All @@ -115,14 +115,15 @@ def received_client_receipt(self, room_id, receipt_type, user_id, event_id):
receipt_type=receipt_type,
user_id=user_id,
event_ids=[event_id],
data={"ts": int(self.clock.time_msec())},
data={"ts": int(self.clock.time_msec()), "hidden": hidden},
)

is_new = yield self._handle_new_receipts([receipt])
if not is_new:
return

yield self.federation.send_read_receipt(receipt)
if not hidden:
yield self.federation.send_read_receipt(receipt)

@defer.inlineCallbacks
def get_receipts_for_room(self, room_id, to_key):
Expand Down
29 changes: 28 additions & 1 deletion synapse/handlers/sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,7 @@ def ephemeral_by_room(self, sync_result_builder, now_token, since_token=None):
"""

sync_config = sync_result_builder.sync_config
user_id = sync_result_builder.sync_config.user.to_string()

with Measure(self.clock, "ephemeral_by_room"):
typing_key = since_token.typing_key if since_token else "0"
Expand Down Expand Up @@ -376,7 +377,33 @@ def ephemeral_by_room(self, sync_result_builder, now_token, since_token=None):
room_id = event["room_id"]
# exclude room id, as above
event_copy = {k: v for (k, v) in iteritems(event) if k != "room_id"}
ephemeral_by_room.setdefault(room_id, []).append(event_copy)

# filter out receipts the user shouldn't see
content = event_copy.get("content", {})
Copy link
Member

Choose a reason for hiding this comment

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

I've not really managed to grok what this lump of code is doing. If it's just filtering events, shouldn't that be handled in receipt_source.get_new_events ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally yes, however there's a comment in a similar area around typing notifications that says the results from that are cached. Didn't want to risk all read receipts being dropped due to a caching issue.

Copy link
Member

Choose a reason for hiding this comment

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

the comment says you shouldn't modify returned event objects, which is true... I don't think there's any reason you can't modify get_new_events to return a different set of events (modulo checking where else it is used).

This code feels super-complicated at the moment.

event_ids = content.keys()
reconstructed = event_copy.copy()
reconstructed["content"] = {} # clear old content
for event_id in event_ids:
m_read = content[event_id].get("m.read", None)
if m_read is None:
# clone it now - it's not something we can process
reconstructed["content"][event_id] = content[event_id]
continue
user_ids = m_read.keys()
for rr_user_id in user_ids:
data = m_read[rr_user_id]
hidden = data.get("hidden", False)
if rr_user_id == user_id or not hidden:
# append the key to the reconstructed receipt
new_content = reconstructed["content"]
if new_content.get(event_id, None) is None:
new_content[event_id] = {"m.read": {}}
ev_content = new_content[event_id]["m.read"]
if ev_content.get(rr_user_id, None) is None:
ev_content[rr_user_id] = {}
ev_content[rr_user_id] = data
if len(reconstructed["content"].keys()) > 0:
ephemeral_by_room.setdefault(room_id, []).append(reconstructed)

return now_token, ephemeral_by_room

Expand Down
1 change: 1 addition & 0 deletions synapse/rest/client/v2_alpha/read_marker.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ def on_POST(self, request, room_id):
"m.read",
user_id=requester.user.to_string(),
event_id=read_event_id,
hidden=body.get("m.hidden", False),
Copy link
Member

Choose a reason for hiding this comment

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

we should check this is a bool before accepting it

)

read_marker_event_id = body.get("m.fully_read", None)
Expand Down
10 changes: 8 additions & 2 deletions synapse/rest/client/v2_alpha/receipts.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
from twisted.internet import defer

from synapse.api.errors import SynapseError
from synapse.http.servlet import RestServlet
from synapse.http.servlet import RestServlet, parse_json_object_from_request

from ._base import client_patterns

Expand Down Expand Up @@ -46,10 +46,16 @@ def on_POST(self, request, room_id, receipt_type, event_id):
if receipt_type != "m.read":
raise SynapseError(400, "Receipt type must be 'm.read'")

body = parse_json_object_from_request(request)

yield self.presence_handler.bump_presence_active_time(requester.user)

yield self.receipts_handler.received_client_receipt(
room_id, receipt_type, user_id=requester.user.to_string(), event_id=event_id
room_id,
receipt_type,
user_id=requester.user.to_string(),
event_id=event_id,
hidden=body.get("m.hidden", False),
Copy link
Member

Choose a reason for hiding this comment

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

again, needs a type check

)

return 200, {}
Expand Down