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

Commit 2091c91

Browse files
authored
More refactoring in get_events_as_list (#5707)
We can now use `_get_events_from_cache_or_db` rather than going right back to the database, which means that (a) we can benefit from caching, and (b) it opens the way forward to more extensive checks on the original event. We now always require the original event to exist before we will serve up a redaction.
1 parent 375162b commit 2091c91

4 files changed

Lines changed: 198 additions & 27 deletions

File tree

changelog.d/5707.bugfix

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Fix some problems with authenticating redactions in recent room versions.

synapse/storage/events_worker.py

Lines changed: 37 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -236,38 +236,48 @@ def get_events_as_list(
236236
"allow_rejected=False"
237237
)
238238

239-
# Starting in room version v3, some redactions need to be rechecked if we
240-
# didn't have the redacted event at the time, so we recheck on read
241-
# instead.
239+
# We may not have had the original event when we received a redaction, so
240+
# we have to recheck auth now.
241+
242242
if not allow_rejected and entry.event.type == EventTypes.Redaction:
243-
if entry.event.internal_metadata.need_to_check_redaction():
244-
# XXX: we should probably use _get_events_from_cache_or_db here,
245-
# so that we can benefit from caching.
246-
orig_sender = yield self._simple_select_one_onecol(
247-
table="events",
248-
keyvalues={"event_id": entry.event.redacts},
249-
retcol="sender",
250-
allow_none=True,
243+
redacted_event_id = entry.event.redacts
244+
event_map = yield self._get_events_from_cache_or_db([redacted_event_id])
245+
original_event_entry = event_map.get(redacted_event_id)
246+
if not original_event_entry:
247+
# we don't have the redacted event (or it was rejected).
248+
#
249+
# We assume that the redaction isn't authorized for now; if the
250+
# redacted event later turns up, the redaction will be re-checked,
251+
# and if it is found valid, the original will get redacted before it
252+
# is served to the client.
253+
logger.debug(
254+
"Withholding redaction event %s since we don't (yet) have the "
255+
"original %s",
256+
event_id,
257+
redacted_event_id,
251258
)
259+
continue
252260

253-
expected_domain = get_domain_from_id(entry.event.sender)
254-
if (
255-
orig_sender
256-
and get_domain_from_id(orig_sender) == expected_domain
257-
):
258-
# This redaction event is allowed. Mark as not needing a
259-
# recheck.
260-
entry.event.internal_metadata.recheck_redaction = False
261-
else:
262-
# We either don't have the event that is being redacted (so we
263-
# assume that the event isn't authorised for now), or the
264-
# senders don't match (so it will never be authorised). Either
265-
# way, we shouldn't return it.
266-
#
267-
# (If we later receive the event, then we will redact it anyway,
268-
# since we have this redaction)
261+
original_event = original_event_entry.event
262+
263+
if entry.event.internal_metadata.need_to_check_redaction():
264+
original_domain = get_domain_from_id(original_event.sender)
265+
redaction_domain = get_domain_from_id(entry.event.sender)
266+
if original_domain != redaction_domain:
267+
# the senders don't match, so this is forbidden
268+
logger.info(
269+
"Withholding redaction %s whose sender domain %s doesn't "
270+
"match that of redacted event %s %s",
271+
event_id,
272+
redaction_domain,
273+
redacted_event_id,
274+
original_domain,
275+
)
269276
continue
270277

278+
# Update the cache to save doing the checks again.
279+
entry.event.internal_metadata.recheck_redaction = False
280+
271281
if check_redacted and entry.redacted_event:
272282
event = entry.redacted_event
273283
else:
Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,159 @@
1+
# -*- coding: utf-8 -*-
2+
# Copyright 2019 The Matrix.org Foundation C.I.C.
3+
#
4+
# Licensed under the Apache License, Version 2.0 (the "License");
5+
# you may not use this file except in compliance with the License.
6+
# You may obtain a copy of the License at
7+
#
8+
# http://www.apache.org/licenses/LICENSE-2.0
9+
#
10+
# Unless required by applicable law or agreed to in writing, software
11+
# distributed under the License is distributed on an "AS IS" BASIS,
12+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
# See the License for the specific language governing permissions and
14+
# limitations under the License.
15+
16+
from synapse.rest import admin
17+
from synapse.rest.client.v1 import login, room
18+
from synapse.rest.client.v2_alpha import sync
19+
20+
from tests.unittest import HomeserverTestCase
21+
22+
23+
class RedactionsTestCase(HomeserverTestCase):
24+
"""Tests that various redaction events are handled correctly"""
25+
26+
servlets = [
27+
admin.register_servlets,
28+
room.register_servlets,
29+
login.register_servlets,
30+
sync.register_servlets,
31+
]
32+
33+
def prepare(self, reactor, clock, hs):
34+
# register a couple of users
35+
self.mod_user_id = self.register_user("user1", "pass")
36+
self.mod_access_token = self.login("user1", "pass")
37+
self.other_user_id = self.register_user("otheruser", "pass")
38+
self.other_access_token = self.login("otheruser", "pass")
39+
40+
# Create a room
41+
self.room_id = self.helper.create_room_as(
42+
self.mod_user_id, tok=self.mod_access_token
43+
)
44+
45+
# Invite the other user
46+
self.helper.invite(
47+
room=self.room_id,
48+
src=self.mod_user_id,
49+
tok=self.mod_access_token,
50+
targ=self.other_user_id,
51+
)
52+
# The other user joins
53+
self.helper.join(
54+
room=self.room_id, user=self.other_user_id, tok=self.other_access_token
55+
)
56+
57+
def _redact_event(self, access_token, room_id, event_id, expect_code=200):
58+
"""Helper function to send a redaction event.
59+
60+
Returns the json body.
61+
"""
62+
path = "/_matrix/client/r0/rooms/%s/redact/%s" % (room_id, event_id)
63+
64+
request, channel = self.make_request(
65+
"POST", path, content={}, access_token=access_token
66+
)
67+
self.render(request)
68+
self.assertEqual(int(channel.result["code"]), expect_code)
69+
return channel.json_body
70+
71+
def _sync_room_timeline(self, access_token, room_id):
72+
request, channel = self.make_request(
73+
"GET", "sync", access_token=self.mod_access_token
74+
)
75+
self.render(request)
76+
self.assertEqual(channel.result["code"], b"200")
77+
room_sync = channel.json_body["rooms"]["join"][room_id]
78+
return room_sync["timeline"]["events"]
79+
80+
def test_redact_event_as_moderator(self):
81+
# as a regular user, send a message to redact
82+
b = self.helper.send(room_id=self.room_id, tok=self.other_access_token)
83+
msg_id = b["event_id"]
84+
85+
# as the moderator, send a redaction
86+
b = self._redact_event(self.mod_access_token, self.room_id, msg_id)
87+
redaction_id = b["event_id"]
88+
89+
# now sync
90+
timeline = self._sync_room_timeline(self.mod_access_token, self.room_id)
91+
92+
# the last event should be the redaction
93+
self.assertEqual(timeline[-1]["event_id"], redaction_id)
94+
self.assertEqual(timeline[-1]["redacts"], msg_id)
95+
96+
# and the penultimate should be the redacted original
97+
self.assertEqual(timeline[-2]["event_id"], msg_id)
98+
self.assertEqual(timeline[-2]["unsigned"]["redacted_by"], redaction_id)
99+
self.assertEqual(timeline[-2]["content"], {})
100+
101+
def test_redact_event_as_normal(self):
102+
# as a regular user, send a message to redact
103+
b = self.helper.send(room_id=self.room_id, tok=self.other_access_token)
104+
normal_msg_id = b["event_id"]
105+
106+
# also send one as the admin
107+
b = self.helper.send(room_id=self.room_id, tok=self.mod_access_token)
108+
admin_msg_id = b["event_id"]
109+
110+
# as a normal, try to redact the admin's event
111+
self._redact_event(
112+
self.other_access_token, self.room_id, admin_msg_id, expect_code=403
113+
)
114+
115+
# now try to redact our own event
116+
b = self._redact_event(self.other_access_token, self.room_id, normal_msg_id)
117+
redaction_id = b["event_id"]
118+
119+
# now sync
120+
timeline = self._sync_room_timeline(self.other_access_token, self.room_id)
121+
122+
# the last event should be the redaction of the normal event
123+
self.assertEqual(timeline[-1]["event_id"], redaction_id)
124+
self.assertEqual(timeline[-1]["redacts"], normal_msg_id)
125+
126+
# the penultimate should be the unredacted one from the admin
127+
self.assertEqual(timeline[-2]["event_id"], admin_msg_id)
128+
self.assertNotIn("redacted_by", timeline[-2]["unsigned"])
129+
self.assertTrue(timeline[-2]["content"]["body"], {})
130+
131+
# and the antepenultimate should be the redacted normal
132+
self.assertEqual(timeline[-3]["event_id"], normal_msg_id)
133+
self.assertEqual(timeline[-3]["unsigned"]["redacted_by"], redaction_id)
134+
self.assertEqual(timeline[-3]["content"], {})
135+
136+
def test_redact_nonexistent_event(self):
137+
# control case: an existing event
138+
b = self.helper.send(room_id=self.room_id, tok=self.other_access_token)
139+
msg_id = b["event_id"]
140+
b = self._redact_event(self.other_access_token, self.room_id, msg_id)
141+
redaction_id = b["event_id"]
142+
143+
# room moderators can send redactions for non-existent events
144+
self._redact_event(self.mod_access_token, self.room_id, "$zzz")
145+
146+
# ... but normals cannot
147+
self._redact_event(
148+
self.other_access_token, self.room_id, "$zzz", expect_code=404
149+
)
150+
151+
# when we sync, we should see only the valid redaction
152+
timeline = self._sync_room_timeline(self.other_access_token, self.room_id)
153+
self.assertEqual(timeline[-1]["event_id"], redaction_id)
154+
self.assertEqual(timeline[-1]["redacts"], msg_id)
155+
156+
# and the penultimate should be the redacted original
157+
self.assertEqual(timeline[-2]["event_id"], msg_id)
158+
self.assertEqual(timeline[-2]["unsigned"]["redacted_by"], redaction_id)
159+
self.assertEqual(timeline[-2]["content"], {})

tests/unittest.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -447,6 +447,7 @@ def register_user(self, username, password, admin=False):
447447
# Create the user
448448
request, channel = self.make_request("GET", "/_matrix/client/r0/admin/register")
449449
self.render(request)
450+
self.assertEqual(channel.code, 200)
450451
nonce = channel.json_body["nonce"]
451452

452453
want_mac = hmac.new(key=b"shared", digestmod=hashlib.sha1)

0 commit comments

Comments
 (0)