From 487f1bb49d5eb5840c7dd70d95ac53f2b24eba21 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 16 Dec 2019 12:14:12 +0000 Subject: [PATCH 1/7] Use the filtered version of an event when responding to /context requests for that event Sometimes the filtering function can return a pruned version of an event (on top of either the event itself or an empty list), if it thinks the user should be able to see that there's an event there but not the content of that event. Therefore, the previous logic of 'if filtered is empty then we can use the event we retrieved from the database' is flawed, and we should use the event returned by the filtering function. --- synapse/handlers/room.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index 22768e97f..7f979e581 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -907,7 +907,10 @@ class RoomContextHandler(object): results["events_before"] = yield filter_evts(results["events_before"]) results["events_after"] = yield filter_evts(results["events_after"]) - results["event"] = event + # filter_evts can return a pruned event in case the user is allowed to see that + # there's something there but not see the content, so use the event that's in + # `filtered` rather than the event we retrieved from the datastore. + results["event"] = filtered[0] if results["events_after"]: last_event_id = results["events_after"][-1].event_id From ac87ddb242d146cd840b232fa6e8165c9dd01ae6 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 16 Dec 2019 12:15:37 +0000 Subject: [PATCH 2/7] Update the documentation of the filtering function --- synapse/visibility.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/visibility.py b/synapse/visibility.py index dffe943b2..100dc47a8 100644 --- a/synapse/visibility.py +++ b/synapse/visibility.py @@ -52,7 +52,8 @@ def filter_events_for_client( apply_retention_policies=True, ): """ - Check which events a user is allowed to see + Check which events a user is allowed to see. If the user can see the event but its + sender asked for their data to be erased, prune the content of the event. Args: storage From 8b9f5c21c35ff7a5491121ffc381bd8c97e879ce Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 16 Dec 2019 12:19:35 +0000 Subject: [PATCH 3/7] Changelog --- changelog.d/6553.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/6553.bugfix diff --git a/changelog.d/6553.bugfix b/changelog.d/6553.bugfix new file mode 100644 index 000000000..e8f55e2a7 --- /dev/null +++ b/changelog.d/6553.bugfix @@ -0,0 +1 @@ +Fix a bug causing responses to the `/context` client endpoint to not use the pruned version of the event the request is for. From 596dd9914dad1933ded1426bdec1e2b1e6874e39 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 16 Dec 2019 14:53:21 +0000 Subject: [PATCH 4/7] Add test case --- tests/rest/client/v1/test_rooms.py | 133 +++++++++++++++++++++++++++++ 1 file changed, 133 insertions(+) diff --git a/tests/rest/client/v1/test_rooms.py b/tests/rest/client/v1/test_rooms.py index 1ca7fa742..9cb505f31 100644 --- a/tests/rest/client/v1/test_rooms.py +++ b/tests/rest/client/v1/test_rooms.py @@ -29,6 +29,7 @@ import synapse.rest.admin from synapse.api.constants import EventContentFields, EventTypes, Membership from synapse.handlers.pagination import PurgeStatus from synapse.rest.client.v1 import login, profile, room +from synapse.rest.client.v2_alpha import account from synapse.util.stringutils import random_string from tests import unittest @@ -1597,3 +1598,135 @@ class LabelsTestCase(unittest.HomeserverTestCase): ) return event_id + + +class ContextTestCase(unittest.HomeserverTestCase): + + servlets = [ + synapse.rest.admin.register_servlets_for_client_rest_resource, + room.register_servlets, + login.register_servlets, + account.register_servlets, + ] + + def make_homeserver(self, reactor, clock): + self.hs = self.setup_test_homeserver() + + return self.hs + + def prepare(self, reactor, clock, homeserver): + self.user_id = self.register_user("user", "password") + self.tok = self.login("user", "password") + self.room_id = self.helper.create_room_as(self.user_id, tok=self.tok) + + self.other_user_id = self.register_user("user2", "password") + self.other_tok = self.login("user2", "password") + + self.helper.invite(self.room_id, self.user_id, self.other_user_id, tok=self.tok) + self.helper.join(self.room_id, self.other_user_id, tok=self.other_tok) + + def test_erased_sender(self): + """Test that an erasure request results in the requester's events being hidden + from any new member of the room. + """ + + # Send a bunch of events in the room. + + self.helper.send(self.room_id, "message 1", tok=self.tok) + self.helper.send(self.room_id, "message 2", tok=self.tok) + event_id = self.helper.send(self.room_id, "message 3", tok=self.tok)["event_id"] + self.helper.send(self.room_id, "message 4", tok=self.tok) + self.helper.send(self.room_id, "message 5", tok=self.tok) + + # Check that we can still see the messages before the erasure request. + + request, channel = self.make_request( + "GET", + '/rooms/%s/context/%s?filter={"types":["m.room.message"]}' + % (self.room_id, event_id), + access_token=self.tok, + ) + self.render(request) + self.assertEqual(channel.code, 200, channel.result) + + events_before = channel.json_body["events_before"] + + self.assertEqual(len(events_before), 2, events_before) + self.assertEqual( + events_before[0].get("content", {}).get("body"), + "message 2", + events_before[0], + ) + self.assertEqual( + events_before[1].get("content", {}).get("body"), + "message 1", + events_before[1], + ) + + self.assertEqual( + channel.json_body["event"].get("content", {}).get("body"), + "message 3", + channel.json_body["event"], + ) + + events_after = channel.json_body["events_after"] + + self.assertEqual(len(events_after), 2, events_after) + self.assertEqual( + events_after[0].get("content", {}).get("body"), + "message 4", + events_after[0], + ) + self.assertEqual( + events_after[1].get("content", {}).get("body"), + "message 5", + events_after[1], + ) + + # Deactivate the first account and erase the user's data. + + deactivate_account_handler = self.hs.get_deactivate_account_handler() + self.get_success( + deactivate_account_handler.deactivate_account(self.user_id, erase_data=True) + ) + + # Invite another user in the room. This is needed because messages will be + # pruned only if the user wasn't a member of the room when the messages were + # sent. + + invited_user_id = self.register_user("user3", "password") + invited_tok = self.login("user3", "password") + + self.helper.invite( + self.room_id, self.other_user_id, invited_user_id, tok=self.other_tok + ) + self.helper.join(self.room_id, invited_user_id, tok=invited_tok) + + # Check that a user that joined the room after the erasure request can't see + # the messages anymore. + + request, channel = self.make_request( + 'GET', + '/rooms/%s/context/%s?filter={"types":["m.room.message"]}' + % (self.room_id, event_id), + access_token=invited_tok, + ) + self.render(request) + self.assertEqual(channel.code, 200, channel.result) + + events_before = channel.json_body["events_before"] + + self.assertEqual(len(events_before), 2, events_before) + self.assertDictEqual(events_before[0].get("content"), {}, events_before[0]) + self.assertDictEqual(events_before[1].get("content"), {}, events_before[1]) + + self.assertDictEqual( + channel.json_body["event"].get("content"), {}, channel.json_body["event"] + ) + + events_after = channel.json_body["events_after"] + + self.assertEqual(len(events_after), 2, events_after) + self.assertDictEqual(events_after[0].get("content"), {}, events_after[0]) + self.assertEqual(events_after[1].get("content"), {}, events_after[1]) + From a29420f9f449da72d1d38bcab4cedc182e9f2ba0 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 16 Dec 2019 14:55:50 +0000 Subject: [PATCH 5/7] Lint --- tests/rest/client/v1/test_rooms.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/rest/client/v1/test_rooms.py b/tests/rest/client/v1/test_rooms.py index 9cb505f31..dca0fef97 100644 --- a/tests/rest/client/v1/test_rooms.py +++ b/tests/rest/client/v1/test_rooms.py @@ -1706,7 +1706,7 @@ class ContextTestCase(unittest.HomeserverTestCase): # the messages anymore. request, channel = self.make_request( - 'GET', + "GET", '/rooms/%s/context/%s?filter={"types":["m.room.message"]}' % (self.room_id, event_id), access_token=invited_tok, @@ -1729,4 +1729,3 @@ class ContextTestCase(unittest.HomeserverTestCase): self.assertEqual(len(events_after), 2, events_after) self.assertDictEqual(events_after[0].get("content"), {}, events_after[0]) self.assertEqual(events_after[1].get("content"), {}, events_after[1]) - From 284e690aa0e37c0d4d7516fc2f02b2b2fede4601 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 16 Dec 2019 14:56:05 +0000 Subject: [PATCH 6/7] Update changelog.d/6553.bugfix Co-Authored-By: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> --- changelog.d/6553.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/6553.bugfix b/changelog.d/6553.bugfix index e8f55e2a7..4fe576b87 100644 --- a/changelog.d/6553.bugfix +++ b/changelog.d/6553.bugfix @@ -1 +1 @@ -Fix a bug causing responses to the `/context` client endpoint to not use the pruned version of the event the request is for. +Fix a bug causing responses to the `/context` client endpoint to not use the pruned version of the event. From a82006954912ed96b0d47db43db44e76e5b052d6 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 16 Dec 2019 16:00:18 +0000 Subject: [PATCH 7/7] Incorporate review --- synapse/handlers/room.py | 2 +- tests/rest/client/v1/test_rooms.py | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index 7f979e581..60b8bbc7a 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -941,7 +941,7 @@ class RoomContextHandler(object): if event_filter: state_events = event_filter.filter(state_events) - results["state"] = state_events + results["state"] = yield filter_evts(state_events) # We use a dummy token here as we only care about the room portion of # the token, which we replace. diff --git a/tests/rest/client/v1/test_rooms.py b/tests/rest/client/v1/test_rooms.py index dca0fef97..e3af280ba 100644 --- a/tests/rest/client/v1/test_rooms.py +++ b/tests/rest/client/v1/test_rooms.py @@ -1609,11 +1609,6 @@ class ContextTestCase(unittest.HomeserverTestCase): account.register_servlets, ] - def make_homeserver(self, reactor, clock): - self.hs = self.setup_test_homeserver() - - return self.hs - def prepare(self, reactor, clock, homeserver): self.user_id = self.register_user("user", "password") self.tok = self.login("user", "password")