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

turt2live · 2019-09-06T02:38:42Z

See matrix-org/matrix-react-sdk#3395

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file
Pull request includes a sign off

This is done with my NV hat.

turt2live · 2019-09-06T04:17:43Z

synapse/handlers/initial_sync.py

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


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.

ara4n · 2019-09-06T17:17:43Z

changelog.d/5990.feature

@@ -0,0 +1 @@
+Support a way for clients to not send read receipts to other users/servers.


link to MSC?

richvdh

could do with some tests, please.

richvdh · 2019-09-09T15:18:01Z

synapse/app/federation_sender.py

@@ -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


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

richvdh · 2019-09-09T15:28:52Z

synapse/rest/client/v2_alpha/read_marker.py

@@ -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),


we should check this is a bool before accepting it

richvdh · 2019-09-09T15:29:03Z

synapse/rest/client/v2_alpha/receipts.py

+            receipt_type,
+            user_id=requester.user.to_string(),
+            event_id=event_id,
+            hidden=body.get("m.hidden", False),


again, needs a type check

richvdh · 2019-09-09T15:35:33Z

synapse/handlers/sync.py

-                ephemeral_by_room.setdefault(room_id, []).append(event_copy)
+
+                # filter out receipts the user shouldn't see
+                content = event_copy.get("content", {})


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 ?

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.

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.

turt2live · 2019-09-09T15:39:02Z

could do with some tests, please.

@richvdh are there any examples of tests for this kind of thing? I'm not going to be able to do this without heavy amounts of documentation and/or examples :/

turt2live · 2019-09-09T15:40:34Z

(also I don't know why this ended up in the review queue - I know it's terrible code and didn't intend to try and push this feature anywhere for months. Likewise for the MSC)

richvdh · 2019-09-09T15:50:37Z

@richvdh are there any examples of tests for this kind of thing? I'm not going to be able to do this without heavy amounts of documentation and/or examples :/

you could either add some sytests (look in tests/30rooms/21receipts.pl), or some python tests for the synapse code (best place to start there might be something like tests/rest/client/v2_alpha/test_sync.py). Or better yet, both...

also I don't know why this ended up in the review queue

I assumed the fact that you'd submitted a PR meant you wanted it reviewed and merged... generally we try to avoid having open PRs sitting around for months (though we often fail at it)

turt2live · 2019-09-09T15:57:24Z

ah, I tend to open PRs for things so I don't lose the code (and it might be useful for others who aren't me). Didn't really intend on adding this to the queue until the MSC was past FCP. Thanks for the early review though - will take a look at those tests, but is a p2 thing on my list.

turt2live · 2019-10-02T15:59:02Z

This has a massive bug in initial sync:

Traceback (most recent call last):
  File "/home/matrix/synapse/lib/python3.6/site-packages/synapse/http/server.py", line 77, in wrapped_request_handler
    await h(self, request)
  File "/home/matrix/synapse/lib/python3.6/site-packages/synapse/http/server.py", line 326, in _async_render
    callback_return = await callback_return
  File "/home/matrix/synapse/lib/python3.6/site-packages/twisted/internet/defer.py", line 1416, in _inlineCallbacks
    result = result.throwExceptionIntoGenerator(g)
  File "/home/matrix/synapse/lib/python3.6/site-packages/twisted/python/failure.py", line 491, in throwExceptionIntoGenerator
    return g.throw(self.type, self.value, self.tb)
  File "/home/matrix/synapse/lib/python3.6/site-packages/synapse/rest/client/v2_alpha/sync.py", line 169, in on_GET
    full_state=full_state,
  File "/home/matrix/synapse/lib/python3.6/site-packages/twisted/internet/defer.py", line 1416, in _inlineCallbacks
    result = result.throwExceptionIntoGenerator(g)
  File "/home/matrix/synapse/lib/python3.6/site-packages/twisted/python/failure.py", line 491, in throwExceptionIntoGenerator
    return g.throw(self.type, self.value, self.tb)
  File "/home/matrix/synapse/lib/python3.6/site-packages/synapse/handlers/sync.py", line 264, in wait_for_sync_for_user
    full_state,
  File "/home/matrix/synapse/lib/python3.6/site-packages/twisted/internet/defer.py", line 1416, in _inlineCallbacks
    result = result.throwExceptionIntoGenerator(g)
  File "/home/matrix/synapse/lib/python3.6/site-packages/twisted/python/failure.py", line 491, in throwExceptionIntoGenerator
    return g.throw(self.type, self.value, self.tb)
  File "/home/matrix/synapse/lib/python3.6/site-packages/synapse/handlers/sync.py", line 285, in _wait_for_sync_for_user
    sync_config, since_token, full_state=full_state
  File "/home/matrix/synapse/lib/python3.6/site-packages/twisted/internet/defer.py", line 1416, in _inlineCallbacks
    result = result.throwExceptionIntoGenerator(g)
  File "/home/matrix/synapse/lib/python3.6/site-packages/twisted/python/failure.py", line 491, in throwExceptionIntoGenerator
    return g.throw(self.type, self.value, self.tb)
  File "/home/matrix/synapse/lib/python3.6/site-packages/synapse/handlers/sync.py", line 989, in generate_sync_result
    sync_result_builder, account_data_by_room
  File "/home/matrix/synapse/lib/python3.6/site-packages/twisted/internet/defer.py", line 1416, in _inlineCallbacks
    result = result.throwExceptionIntoGenerator(g)
  File "/home/matrix/synapse/lib/python3.6/site-packages/twisted/python/failure.py", line 491, in throwExceptionIntoGenerator
    return g.throw(self.type, self.value, self.tb)
  File "/home/matrix/synapse/lib/python3.6/site-packages/synapse/handlers/sync.py", line 1345, in _generate_sync_entry_for_rooms
    since_token=sync_result_builder.since_token,
  File "/home/matrix/synapse/lib/python3.6/site-packages/twisted/internet/defer.py", line 1418, in _inlineCallbacks
    result = g.send(result)
  File "/home/matrix/synapse/lib/python3.6/site-packages/synapse/handlers/sync.py", line 395, in ephemeral_by_room
    hidden = data.get("hidden", False)
AttributeError: 'str' object has no attribute 'get'

symptog · 2020-04-06T15:31:45Z

I've created a fork and rebased this branch onto the master. I've also fixed some proposed changes.
Can you have a look @turt2live @richvdh
symptog@88131a0

To fix the sync:
What type is this variable?
https://github.com/symptog/synapse/blob/travis/hidden_rr/synapse/handlers/sync.py#L427

clokep · 2020-06-11T16:45:48Z

I've created a fork and rebased this branch onto the master. I've also fixed some proposed changes.
Can you have a look @turt2live @richvdh
symptog@88131a0

It would probably be best to create a separate PR and we can replace this one if necessary!

To fix the sync:
What type is this variable?
symptog/synapse:hidden_rr/synapse/handlers/sync.py@travis#L427

It looks like it is a User, but I'm not 100% sure (checkout the spec).

turt2live · 2020-06-11T16:48:07Z

Sorry, I'm going to close this as it's massively bitrotted and unlikely to be helpful for the synapse team to review in this state.

@symptog if you're able to take this on, a new PR is appreciated.

turt2live added 6 commits September 5, 2019 20:37

Support optionally not sending read receipts to other users/servers

aab3f5d

changelog

53cf721

import the important things in life

27eac52

Filter out hidden read receipts for federation workers

30f0824

Filter out hidden read receipts from sync too

1705265

Appease the linter

c4f3e97

turt2live mentioned this pull request Sep 6, 2019

Support sending hidden read receipts matrix-org/matrix-react-sdk#3395

Merged

turt2live marked this pull request as ready for review September 6, 2019 04:12

turt2live mentioned this pull request Sep 6, 2019

MSC2285: Private read receipts matrix-org/matrix-spec-proposals#2285

Merged

turt2live commented Sep 6, 2019

View reviewed changes

richvdh requested a review from a team September 6, 2019 08:28

ara4n reviewed Sep 6, 2019

View reviewed changes

richvdh suggested changes Sep 9, 2019

View reviewed changes

turt2live closed this Jun 11, 2020

dnut mentioned this pull request Jul 19, 2020

Option to stop sending read receipts to other users element-hq/element-web#2527

Closed

SimonBrandner mentioned this pull request Jul 16, 2021

Support for MSC2285 (hidden read receipts) #10413

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

turt2live commented Sep 6, 2019 •

edited

Loading

turt2live Sep 6, 2019

ara4n Sep 6, 2019

richvdh left a comment

richvdh Sep 9, 2019 •

edited

Loading

richvdh Sep 9, 2019

richvdh Sep 9, 2019

richvdh Sep 9, 2019

turt2live Sep 9, 2019

richvdh Sep 9, 2019

turt2live commented Sep 9, 2019

turt2live commented Sep 9, 2019

richvdh commented Sep 9, 2019

turt2live commented Sep 9, 2019 •

edited

Loading

turt2live commented Oct 2, 2019

symptog commented Apr 6, 2020 •

edited

Loading

clokep commented Jun 11, 2020

turt2live commented Jun 11, 2020

		@@ -0,0 +1 @@
		Support a way for clients to not send read receipts to other users/servers.

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

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

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

Conversation

turt2live commented Sep 6, 2019 • edited Loading

Pull Request Checklist

turt2live Sep 6, 2019

Choose a reason for hiding this comment

ara4n Sep 6, 2019

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

richvdh Sep 9, 2019 • edited Loading

Choose a reason for hiding this comment

richvdh Sep 9, 2019

Choose a reason for hiding this comment

richvdh Sep 9, 2019

Choose a reason for hiding this comment

richvdh Sep 9, 2019

Choose a reason for hiding this comment

turt2live Sep 9, 2019

Choose a reason for hiding this comment

richvdh Sep 9, 2019

Choose a reason for hiding this comment

turt2live commented Sep 9, 2019

turt2live commented Sep 9, 2019

richvdh commented Sep 9, 2019

turt2live commented Sep 9, 2019 • edited Loading

turt2live commented Oct 2, 2019

symptog commented Apr 6, 2020 • edited Loading

clokep commented Jun 11, 2020

turt2live commented Jun 11, 2020

turt2live commented Sep 6, 2019 •

edited

Loading

richvdh Sep 9, 2019 •

edited

Loading

turt2live commented Sep 9, 2019 •

edited

Loading

symptog commented Apr 6, 2020 •

edited

Loading