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.

changelog.d/5707.bugfix

@@ -0,0 +1 @@
+Fix some problems with authenticating redactions in recent room versions.

synapse/storage/events_worker.py

@@ -236,38 +236,48 @@ class EventsWorkerStore(SQLBaseStore):
                     "allow_rejected=False"
                 )
 
-            # Starting in room version v3, some redactions need to be rechecked if we
+            # We may not have had the original event when we received a redaction, so
-            # didn't have the redacted event at the time, so we recheck on read
+            # we have to recheck auth now.
-            # instead.
-            if not allow_rejected and entry.event.type == EventTypes.Redaction:
-                if entry.event.internal_metadata.need_to_check_redaction():
-                    # XXX: we should probably use _get_events_from_cache_or_db here,
-                    # so that we can benefit from caching.
-                    orig_sender = yield self._simple_select_one_onecol(
-                        table="events",
-                        keyvalues={"event_id": entry.event.redacts},
-                        retcol="sender",
-                        allow_none=True,
-                    )
 
-                    expected_domain = get_domain_from_id(entry.event.sender)
+            if not allow_rejected and entry.event.type == EventTypes.Redaction:
-                    if (
+                redacted_event_id = entry.event.redacts
-                        orig_sender
+                event_map = yield self._get_events_from_cache_or_db([redacted_event_id])
-                        and get_domain_from_id(orig_sender) == expected_domain
+                original_event_entry = event_map.get(redacted_event_id)
-                    ):
+                if not original_event_entry:
-                        # This redaction event is allowed. Mark as not needing a
+                    # we don't have the redacted event (or it was rejected).
-                        # recheck.
-                        entry.event.internal_metadata.recheck_redaction = False
-                    else:
-                        # We either don't have the event that is being redacted (so we
-                        # assume that the event isn't authorised for now), or the
-                        # senders don't match (so it will never be authorised). Either
-                        # way, we shouldn't return it.
                     #
-                        # (If we later receive the event, then we will redact it anyway,
+                    # We assume that the redaction isn't authorized for now; if the
-                        # since we have this redaction)
+                    # redacted event later turns up, the redaction will be re-checked,
+                    # and if it is found valid, the original will get redacted before it
+                    # is served to the client.
+                    logger.debug(
+                        "Withholding redaction event %s since we don't (yet) have the "
+                        "original %s",
+                        event_id,
+                        redacted_event_id,
+                    )
                     continue
 
+                original_event = original_event_entry.event
+
+                if entry.event.internal_metadata.need_to_check_redaction():
+                    original_domain = get_domain_from_id(original_event.sender)
+                    redaction_domain = get_domain_from_id(entry.event.sender)
+                    if original_domain != redaction_domain:
+                        # the senders don't match, so this is forbidden
+                        logger.info(
+                            "Withholding redaction %s whose sender domain %s doesn't "
+                            "match that of redacted event %s %s",
+                            event_id,
+                            redaction_domain,
+                            redacted_event_id,
+                            original_domain,
+                        )
+                        continue
+
+                    # Update the cache to save doing the checks again.
+                    entry.event.internal_metadata.recheck_redaction = False
+
             if check_redacted and entry.redacted_event:
                 event = entry.redacted_event
             else:

tests/rest/client/test_redactions.py

@@ -0,0 +1,159 @@
+# -*- coding: utf-8 -*-
+# Copyright 2019 The Matrix.org Foundation C.I.C.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+from synapse.rest import admin
+from synapse.rest.client.v1 import login, room
+from synapse.rest.client.v2_alpha import sync
+
+from tests.unittest import HomeserverTestCase
+
+
+class RedactionsTestCase(HomeserverTestCase):
+    """Tests that various redaction events are handled correctly"""
+
+    servlets = [
+        admin.register_servlets,
+        room.register_servlets,
+        login.register_servlets,
+        sync.register_servlets,
+    ]
+
+    def prepare(self, reactor, clock, hs):
+        # register a couple of users
+        self.mod_user_id = self.register_user("user1", "pass")
+        self.mod_access_token = self.login("user1", "pass")
+        self.other_user_id = self.register_user("otheruser", "pass")
+        self.other_access_token = self.login("otheruser", "pass")
+
+        # Create a room
+        self.room_id = self.helper.create_room_as(
+            self.mod_user_id, tok=self.mod_access_token
+        )
+
+        # Invite the other user
+        self.helper.invite(
+            room=self.room_id,
+            src=self.mod_user_id,
+            tok=self.mod_access_token,
+            targ=self.other_user_id,
+        )
+        # The other user joins
+        self.helper.join(
+            room=self.room_id, user=self.other_user_id, tok=self.other_access_token
+        )
+
+    def _redact_event(self, access_token, room_id, event_id, expect_code=200):
+        """Helper function to send a redaction event.
+
+        Returns the json body.
+        """
+        path = "/_matrix/client/r0/rooms/%s/redact/%s" % (room_id, event_id)
+
+        request, channel = self.make_request(
+            "POST", path, content={}, access_token=access_token
+        )
+        self.render(request)
+        self.assertEqual(int(channel.result["code"]), expect_code)
+        return channel.json_body
+
+    def _sync_room_timeline(self, access_token, room_id):
+        request, channel = self.make_request(
+            "GET", "sync", access_token=self.mod_access_token
+        )
+        self.render(request)
+        self.assertEqual(channel.result["code"], b"200")
+        room_sync = channel.json_body["rooms"]["join"][room_id]
+        return room_sync["timeline"]["events"]
+
+    def test_redact_event_as_moderator(self):
+        # as a regular user, send a message to redact
+        b = self.helper.send(room_id=self.room_id, tok=self.other_access_token)
+        msg_id = b["event_id"]
+
+        # as the moderator, send a redaction
+        b = self._redact_event(self.mod_access_token, self.room_id, msg_id)
+        redaction_id = b["event_id"]
+
+        # now sync
+        timeline = self._sync_room_timeline(self.mod_access_token, self.room_id)
+
+        # the last event should be the redaction
+        self.assertEqual(timeline[-1]["event_id"], redaction_id)
+        self.assertEqual(timeline[-1]["redacts"], msg_id)
+
+        # and the penultimate should be the redacted original
+        self.assertEqual(timeline[-2]["event_id"], msg_id)
+        self.assertEqual(timeline[-2]["unsigned"]["redacted_by"], redaction_id)
+        self.assertEqual(timeline[-2]["content"], {})
+
+    def test_redact_event_as_normal(self):
+        # as a regular user, send a message to redact
+        b = self.helper.send(room_id=self.room_id, tok=self.other_access_token)
+        normal_msg_id = b["event_id"]
+
+        # also send one as the admin
+        b = self.helper.send(room_id=self.room_id, tok=self.mod_access_token)
+        admin_msg_id = b["event_id"]
+
+        # as a normal, try to redact the admin's event
+        self._redact_event(
+            self.other_access_token, self.room_id, admin_msg_id, expect_code=403
+        )
+
+        # now try to redact our own event
+        b = self._redact_event(self.other_access_token, self.room_id, normal_msg_id)
+        redaction_id = b["event_id"]
+
+        # now sync
+        timeline = self._sync_room_timeline(self.other_access_token, self.room_id)
+
+        # the last event should be the redaction of the normal event
+        self.assertEqual(timeline[-1]["event_id"], redaction_id)
+        self.assertEqual(timeline[-1]["redacts"], normal_msg_id)
+
+        # the penultimate should be the unredacted one from the admin
+        self.assertEqual(timeline[-2]["event_id"], admin_msg_id)
+        self.assertNotIn("redacted_by", timeline[-2]["unsigned"])
+        self.assertTrue(timeline[-2]["content"]["body"], {})
+
+        # and the antepenultimate should be the redacted normal
+        self.assertEqual(timeline[-3]["event_id"], normal_msg_id)
+        self.assertEqual(timeline[-3]["unsigned"]["redacted_by"], redaction_id)
+        self.assertEqual(timeline[-3]["content"], {})
+
+    def test_redact_nonexistent_event(self):
+        # control case: an existing event
+        b = self.helper.send(room_id=self.room_id, tok=self.other_access_token)
+        msg_id = b["event_id"]
+        b = self._redact_event(self.other_access_token, self.room_id, msg_id)
+        redaction_id = b["event_id"]
+
+        # room moderators can send redactions for non-existent events
+        self._redact_event(self.mod_access_token, self.room_id, "$zzz")
+
+        # ... but normals cannot
+        self._redact_event(
+            self.other_access_token, self.room_id, "$zzz", expect_code=404
+        )
+
+        # when we sync, we should see only the valid redaction
+        timeline = self._sync_room_timeline(self.other_access_token, self.room_id)
+        self.assertEqual(timeline[-1]["event_id"], redaction_id)
+        self.assertEqual(timeline[-1]["redacts"], msg_id)
+
+        # and the penultimate should be the redacted original
+        self.assertEqual(timeline[-2]["event_id"], msg_id)
+        self.assertEqual(timeline[-2]["unsigned"]["redacted_by"], redaction_id)
+        self.assertEqual(timeline[-2]["content"], {})

tests/unittest.py

@@ -447,6 +447,7 @@ class HomeserverTestCase(TestCase):
         # Create the user
         request, channel = self.make_request("GET", "/_matrix/client/r0/admin/register")
         self.render(request)
+        self.assertEqual(channel.code, 200)
         nonce = channel.json_body["nonce"]
 
         want_mac = hmac.new(key=b"shared", digestmod=hashlib.sha1)