More refactoring in get_events_as_list

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 @@ def get_events_as_list(
                     "allow_rejected=False"
                 )
 
-            # Starting in room version v3, some redactions need to be rechecked if we
-            # didn't have the redacted event at the time, so we recheck on read
-            # instead.
+            # We may not have had the original event when we received a redaction, so
+            # we have to recheck auth now.
+
             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,
+                redacted_event_id = entry.event.redacts
+                event_map = yield self._get_events_from_cache_or_db([redacted_event_id])
+                original_event_entry = event_map.get(redacted_event_id)
+                if not original_event_entry:
+                    # we don't have the redacted event (or it was rejected).
+                    #
+                    # We assume that the redaction isn't authorized for now; if the
+                    # 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
 
-                    expected_domain = get_domain_from_id(entry.event.sender)
-                    if (
-                        orig_sender
-                        and get_domain_from_id(orig_sender) == expected_domain
-                    ):
-                        # This redaction event is allowed. Mark as not needing a
-                        # 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,
-                        # since we have this redaction)
+                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 @@ def register_user(self, username, password, admin=False):
         # 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)