Merge tag 'v1.13.0rc2' into develop

Synapse 1.13.0rc2 (2020-05-14)
==============================

Bugfixes
--------

- Fix a long-standing bug which could cause messages not to be sent over federation, when state events with state keys matching user IDs (such as custom user statuses) were received. ([\#7376](#7376))
- Restore compatibility with non-compliant clients during the user interactive authentication process, fixing a problem introduced in v1.13.0rc1. ([\#7483](#7483))

Internal Changes
----------------

- Fix linting errors in new version of Flake8. ([\#7470](#7470))

Author: richvdh

CHANGES.md

@@ -1,3 +1,18 @@
+Synapse 1.13.0rc2 (2020-05-14)
+==============================
+
+Bugfixes
+--------
+
+- Fix a long-standing bug which could cause messages not to be sent over federation, when state events with state keys matching user IDs (such as custom user statuses) were received. ([\#7376](https://github.com/matrix-org/synapse/issues/7376))
+- Restore compatibility with non-compliant clients during the user interactive authentication process, fixing a problem introduced in v1.13.0rc1. ([\#7483](https://github.com/matrix-org/synapse/issues/7483))
+
+Internal Changes
+----------------
+
+- Fix linting errors in new version of Flake8. ([\#7470](https://github.com/matrix-org/synapse/issues/7470))
+
+
 Synapse 1.13.0rc1 (2020-05-11)
 ==============================
 

synapse/__init__.py

@@ -36,7 +36,7 @@
 except ImportError:
     pass
 
-__version__ = "1.13.0rc1"
+__version__ = "1.13.0rc2"
 
 if bool(os.environ.get("SYNAPSE_TEST_PATCH_LOG_CONTEXTS", False)):
     # We import here so that we don't have to install a bunch of deps when

synapse/handlers/auth.py

@@ -252,7 +252,6 @@ async def check_auth(
         clientdict: Dict[str, Any],
         clientip: str,
         description: str,
-        validate_clientdict: bool = True,
     ) -> Tuple[dict, dict, str]:
         """
         Takes a dictionary sent by the client in the login / registration
@@ -278,10 +277,6 @@ async def check_auth(
             description: A human readable string to be displayed to the user that
                          describes the operation happening on their account.
 
-            validate_clientdict: Whether to validate that the operation happening
-                                 on the account has not changed. If this is false,
-                                 the client dict is persisted instead of validated.
-
         Returns:
             A tuple of (creds, params, session_id).
 
@@ -346,26 +341,30 @@ async def check_auth(
 
             # Ensure that the queried operation does not vary between stages of
             # the UI authentication session. This is done by generating a stable
-            # comparator based on the URI, method, and client dict (minus the
-            # auth dict) and storing it during the initial query. Subsequent
+            # comparator and storing it during the initial query. Subsequent
             # queries ensure that this comparator has not changed.
-            if validate_clientdict:
-                session_comparator = (session.uri, session.method, session.clientdict)
-                comparator = (uri, method, clientdict)
-            else:
-                session_comparator = (session.uri, session.method)  # type: ignore
-                comparator = (uri, method)  # type: ignore
-
-            if session_comparator != comparator:
+            #
+            # The comparator is based on the requested URI and HTTP method. The
+            # client dict (minus the auth dict) should also be checked, but some
+            # clients are not spec compliant, just warn for now if the client
+            # dict changes.
+            if (session.uri, session.method) != (uri, method):
                 raise SynapseError(
                     403,
                     "Requested operation has changed during the UI authentication session.",
                 )
 
-            # For backwards compatibility the registration endpoint persists
-            # changes to the client dict instead of validating them.
-            if not validate_clientdict:
-                await self.store.set_ui_auth_clientdict(sid, clientdict)
+            if session.clientdict != clientdict:
+                logger.warning(
+                    "Requested operation has changed during the UI "
+                    "authentication session. A future version of Synapse "
+                    "will remove this capability."
+                )
+
+            # For backwards compatibility, changes to the client dict are
+            # persisted as clients modify them throughout their user interactive
+            # authentication flow.
+            await self.store.set_ui_auth_clientdict(sid, clientdict)
 
         if not authdict:
             raise InteractiveAuthIncompleteError(

synapse/rest/client/v2_alpha/register.py

@@ -516,7 +516,6 @@ async def on_POST(self, request):
             body,
             self.hs.get_ip_from_request(request),
             "register a new account",
-            validate_clientdict=False,
         )
 
         # Check that we're not trying to register a denied 3pid.

synapse/storage/data_stores/main/roommember.py

@@ -566,7 +566,8 @@ def _get_joined_users_from_context(
                         if key[0] == EventTypes.Member
                     ]
                     for etype, state_key in context.delta_ids:
-                        users_in_room.pop(state_key, None)
+                        if etype == EventTypes.Member:
+                            users_in_room.pop(state_key, None)
 
         # We check if we have any of the member event ids in the event cache
         # before we ask the DB

tests/rest/client/v2_alpha/test_auth.py

@@ -133,47 +133,6 @@ def test_fallback_captcha(self):
         # We're given a registered user.
         self.assertEqual(channel.json_body["user_id"], "@user:test")
 
-    def test_legacy_registration(self):
-        """
-        Registration allows the parameters to vary through the process.
-        """
-
-        # Make the initial request to register. (Later on a different password
-        # will be used.)
-        # Returns a 401 as per the spec
-        channel = self.register(
-            401, {"username": "user", "type": "m.login.password", "password": "bar"},
-        )
-
-        # Grab the session
-        session = channel.json_body["session"]
-        # Assert our configured public key is being given
-        self.assertEqual(
-            channel.json_body["params"]["m.login.recaptcha"]["public_key"], "brokencake"
-        )
-
-        # Complete the recaptcha step.
-        self.recaptcha(session, 200)
-
-        # also complete the dummy auth
-        self.register(200, {"auth": {"session": session, "type": "m.login.dummy"}})
-
-        # Now we should have fulfilled a complete auth flow, including
-        # the recaptcha fallback step. Make the initial request again, but
-        # with a changed password. This still completes.
-        channel = self.register(
-            200,
-            {
-                "username": "user",
-                "type": "m.login.password",
-                "password": "foo",  # Note that this is different.
-                "auth": {"session": session},
-            },
-        )
-
-        # We're given a registered user.
-        self.assertEqual(channel.json_body["user_id"], "@user:test")
-
     def test_complete_operation_unknown_session(self):
         """
         Attempting to mark an invalid session as complete should error.
@@ -282,9 +241,15 @@ def test_ui_auth(self):
             },
         )
 
-    def test_cannot_change_body(self):
+    def test_can_change_body(self):
         """
-        The initial requested client dict cannot be modified during the user interactive authentication session.
+        The client dict can be modified during the user interactive authentication session.
+
+        Note that it is not spec compliant to modify the client dict during a
+        user interactive authentication session, but many clients currently do.
+
+        When Synapse is updated to be spec compliant, the call to re-use the
+        session ID should be rejected.
         """
         # Create a second login.
         self.login("test", self.user_pass)
@@ -302,9 +267,9 @@ def test_cannot_change_body(self):
         self.assertIn({"stages": ["m.login.password"]}, channel.json_body["flows"])
 
         # Make another request providing the UI auth flow, but try to delete the
-        # second device. This results in an error.
+        # second device.
         self.delete_devices(
-            403,
+            200,
             {
                 "devices": [device_ids[1]],
                 "auth": {

tests/storage/test_roommember.py

@@ -22,6 +22,8 @@
 from synapse.types import Requester, UserID
 
 from tests import unittest
+from tests.test_utils import event_injection
+from tests.utils import TestHomeServer
 
 
 class RoomMemberStoreTestCase(unittest.HomeserverTestCase):
@@ -38,7 +40,7 @@ def make_homeserver(self, reactor, clock):
         )
         return hs
 
-    def prepare(self, reactor, clock, hs):
+    def prepare(self, reactor, clock, hs: TestHomeServer):
 
         # We can't test the RoomMemberStore on its own without the other event
         # storage logic
@@ -114,6 +116,52 @@ def test_count_known_servers_stat_counter_enabled(self):
         # It now knows about Charlie's server.
         self.assertEqual(self.store._known_servers_count, 2)
 
+    def test_get_joined_users_from_context(self):
+        room = self.helper.create_room_as(self.u_alice, tok=self.t_alice)
+        bob_event = event_injection.inject_member_event(
+            self.hs, room, self.u_bob, Membership.JOIN
+        )
+
+        # first, create a regular event
+        event, context = event_injection.create_event(
+            self.hs,
+            room_id=room,
+            sender=self.u_alice,
+            prev_event_ids=[bob_event.event_id],
+            type="m.test.1",
+            content={},
+        )
+
+        users = self.get_success(
+            self.store.get_joined_users_from_context(event, context)
+        )
+        self.assertEqual(users.keys(), {self.u_alice, self.u_bob})
+
+        # Regression test for #7376: create a state event whose key matches bob's
+        # user_id, but which is *not* a membership event, and persist that; then check
+        # that `get_joined_users_from_context` returns the correct users for the next event.
+        non_member_event = event_injection.inject_event(
+            self.hs,
+            room_id=room,
+            sender=self.u_bob,
+            prev_event_ids=[bob_event.event_id],
+            type="m.test.2",
+            state_key=self.u_bob,
+            content={},
+        )
+        event, context = event_injection.create_event(
+            self.hs,
+            room_id=room,
+            sender=self.u_alice,
+            prev_event_ids=[non_member_event.event_id],
+            type="m.test.3",
+            content={},
+        )
+        users = self.get_success(
+            self.store.get_joined_users_from_context(event, context)
+        )
+        self.assertEqual(users.keys(), {self.u_alice, self.u_bob})
+
 
 class CurrentStateMembershipUpdateTestCase(unittest.HomeserverTestCase):
     def prepare(self, reactor, clock, homeserver):