Strip "join_authorised_via_users_server" from join events which do no…

…t need it. (#10933) This fixes a "Event not signed by authorising server" error when transition room member from join -> join, e.g. when updating a display name or avatar URL for restricted rooms.
matrix-org · Oct 1, 2021 · 32072dc · 32072dc
1 parent 3412f5c
commit 32072dc
Show file tree

Hide file tree

Showing 11 changed files with 46 additions and 25 deletions.
diff --git a/changelog.d/10933.bugfix b/changelog.d/10933.bugfix
@@ -0,0 +1 @@
+Fix a bug introduced in Synapse v1.40.0 where changing a user's display name or avatar in a restricted room would cause an authentication error.
diff --git a/synapse/api/constants.py b/synapse/api/constants.py
@@ -217,6 +217,9 @@ class EventContentFields:
     # For "marker" events
     MSC2716_MARKER_INSERTION = "org.matrix.msc2716.marker.insertion"
 
+    # The authorising user for joining a restricted room.
+    AUTHORISING_USER = "join_authorised_via_users_server"
+
 
 class RoomTypes:
     """Understood values of the room_type field of m.room.create events."""

diff --git a/synapse/event_auth.py b/synapse/event_auth.py
@@ -115,11 +115,11 @@ def check(
         is_invite_via_allow_rule = (
             event.type == EventTypes.Member
             and event.membership == Membership.JOIN
-            and "join_authorised_via_users_server" in event.content
+            and EventContentFields.AUTHORISING_USER in event.content
         )
         if is_invite_via_allow_rule:
             authoriser_domain = get_domain_from_id(
-                event.content["join_authorised_via_users_server"]
+                event.content[EventContentFields.AUTHORISING_USER]
             )
             if not event.signatures.get(authoriser_domain):
                 raise AuthError(403, "Event not signed by authorising server")
@@ -381,7 +381,9 @@ def _is_membership_change_allowed(
             # Note that if the caller is in the room or invited, then they do
             # not need to meet the allow rules.
             if not caller_in_room and not caller_invited:
-                authorising_user = event.content.get("join_authorised_via_users_server")
+                authorising_user = event.content.get(
+                    EventContentFields.AUTHORISING_USER
+                )
 
                 if authorising_user is None:
                     raise AuthError(403, "Join event is missing authorising user.")
@@ -836,10 +838,10 @@ def auth_types_for_event(
                 auth_types.add(key)
 
         if room_version.msc3083_join_rules and membership == Membership.JOIN:
-            if "join_authorised_via_users_server" in event.content:
+            if EventContentFields.AUTHORISING_USER in event.content:
                 key = (
                     EventTypes.Member,
-                    event.content["join_authorised_via_users_server"],
+                    event.content[EventContentFields.AUTHORISING_USER],
                 )
                 auth_types.add(key)
 

diff --git a/synapse/events/utils.py b/synapse/events/utils.py
@@ -105,7 +105,7 @@ def add_fields(*fields):
     if event_type == EventTypes.Member:
         add_fields("membership")
         if room_version.msc3375_redaction_rules:
-            add_fields("join_authorised_via_users_server")
+            add_fields(EventContentFields.AUTHORISING_USER)
     elif event_type == EventTypes.Create:
         # MSC2176 rules state that create events cannot be redacted.
         if room_version.msc2176_redaction_rules:

diff --git a/synapse/federation/federation_base.py b/synapse/federation/federation_base.py
@@ -15,7 +15,7 @@
 import logging
 from collections import namedtuple
 
-from synapse.api.constants import MAX_DEPTH, EventTypes, Membership
+from synapse.api.constants import MAX_DEPTH, EventContentFields, EventTypes, Membership
 from synapse.api.errors import Codes, SynapseError
 from synapse.api.room_versions import EventFormatVersions, RoomVersion
 from synapse.crypto.event_signing import check_event_content_hash
@@ -184,10 +184,10 @@ async def _check_sigs_on_pdu(
         room_version.msc3083_join_rules
         and pdu.type == EventTypes.Member
         and pdu.membership == Membership.JOIN
-        and "join_authorised_via_users_server" in pdu.content
+        and EventContentFields.AUTHORISING_USER in pdu.content
     ):
         authorising_server = get_domain_from_id(
-            pdu.content["join_authorised_via_users_server"]
+            pdu.content[EventContentFields.AUTHORISING_USER]
         )
         try:
             await keyring.verify_event_for_server(

diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py
@@ -37,7 +37,7 @@
 import attr
 from prometheus_client import Counter
 
-from synapse.api.constants import EventTypes, Membership
+from synapse.api.constants import EventContentFields, EventTypes, Membership
 from synapse.api.errors import (
     CodeMessageException,
     Codes,
@@ -875,9 +875,9 @@ async def _execute(pdu: EventBase) -> None:
             # If the join is being authorised via allow rules, we need to send
             # the /send_join back to the same server that was originally used
             # with /make_join.
-            if "join_authorised_via_users_server" in pdu.content:
+            if EventContentFields.AUTHORISING_USER in pdu.content:
                 destinations = [
-                    get_domain_from_id(pdu.content["join_authorised_via_users_server"])
+                    get_domain_from_id(pdu.content[EventContentFields.AUTHORISING_USER])
                 ]
 
         return await self._try_destination_list(

diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py
@@ -34,7 +34,7 @@
 from twisted.internet.abstract import isIPAddress
 from twisted.python import failure
 
-from synapse.api.constants import EduTypes, EventTypes, Membership
+from synapse.api.constants import EduTypes, EventContentFields, EventTypes, Membership
 from synapse.api.errors import (
     AuthError,
     Codes,
@@ -765,11 +765,11 @@ async def _on_send_membership_event(
         if (
             room_version.msc3083_join_rules
             and event.membership == Membership.JOIN
-            and "join_authorised_via_users_server" in event.content
+            and EventContentFields.AUTHORISING_USER in event.content
         ):
             # We can only authorise our own users.
             authorising_server = get_domain_from_id(
-                event.content["join_authorised_via_users_server"]
+                event.content[EventContentFields.AUTHORISING_USER]
             )
             if authorising_server != self.server_name:
                 raise SynapseError(

diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py
@@ -27,7 +27,12 @@
 from twisted.internet import defer
 
 from synapse import event_auth
-from synapse.api.constants import EventTypes, Membership, RejectedReason
+from synapse.api.constants import (
+    EventContentFields,
+    EventTypes,
+    Membership,
+    RejectedReason,
+)
 from synapse.api.errors import (
     AuthError,
     CodeMessageException,
@@ -712,7 +717,7 @@ async def on_make_join_request(
 
                 if include_auth_user_id:
                     event_content[
-                        "join_authorised_via_users_server"
+                        EventContentFields.AUTHORISING_USER
                     ] = await self._event_auth_handler.get_user_which_could_invite(
                         room_id,
                         state_ids,

diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py
@@ -573,6 +573,14 @@ async def update_membership_locked(
                 errcode=Codes.BAD_JSON,
             )
 
+        # The event content should *not* include the authorising user as
+        # it won't be properly signed. Strip it out since it might come
+        # back from a client updating a display name / avatar.
+        #
+        # This only applies to restricted rooms, but there should be no reason
+        # for a client to include it. Unconditionally remove it.
+        content.pop(EventContentFields.AUTHORISING_USER, None)
+
         effective_membership_state = action
         if action in ["kick", "unban"]:
             effective_membership_state = "leave"
@@ -939,7 +947,7 @@ async def _should_perform_remote_join(
         # be included in the event content in order to efficiently validate
         # the event.
         content[
-            "join_authorised_via_users_server"
+            EventContentFields.AUTHORISING_USER
         ] = await self.event_auth_handler.get_user_which_could_invite(
             room_id,
             current_state_ids,

diff --git a/tests/events/test_utils.py b/tests/events/test_utils.py
@@ -12,6 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+from synapse.api.constants import EventContentFields
 from synapse.api.room_versions import RoomVersions
 from synapse.events import make_event_from_dict
 from synapse.events.utils import (
@@ -352,7 +353,7 @@ def test_member(self):
                 "event_id": "$test:domain",
                 "content": {
                     "membership": "join",
-                    "join_authorised_via_users_server": "@user:domain",
+                    EventContentFields.AUTHORISING_USER: "@user:domain",
                     "other_key": "stripped",
                 },
             },
@@ -372,15 +373,15 @@ def test_member(self):
                 "type": "m.room.member",
                 "content": {
                     "membership": "join",
-                    "join_authorised_via_users_server": "@user:domain",
+                    EventContentFields.AUTHORISING_USER: "@user:domain",
                     "other_key": "stripped",
                 },
             },
             {
                 "type": "m.room.member",
                 "content": {
                     "membership": "join",
-                    "join_authorised_via_users_server": "@user:domain",
+                    EventContentFields.AUTHORISING_USER: "@user:domain",
                 },
                 "signatures": {},
                 "unsigned": {},

diff --git a/tests/test_event_auth.py b/tests/test_event_auth.py
@@ -16,6 +16,7 @@
 from typing import Optional
 
 from synapse import event_auth
+from synapse.api.constants import EventContentFields
 from synapse.api.errors import AuthError
 from synapse.api.room_versions import RoomVersions
 from synapse.events import EventBase, make_event_from_dict
@@ -380,7 +381,7 @@ def test_join_rules_msc3083_restricted(self):
         authorised_join_event = _join_event(
             pleb,
             additional_content={
-                "join_authorised_via_users_server": "@creator:example.com"
+                EventContentFields.AUTHORISING_USER: "@creator:example.com"
             },
         )
         event_auth.check(
@@ -404,7 +405,7 @@ def test_join_rules_msc3083_restricted(self):
             _join_event(
                 pleb,
                 additional_content={
-                    "join_authorised_via_users_server": "@inviter:foo.test"
+                    EventContentFields.AUTHORISING_USER: "@inviter:foo.test"
                 },
             ),
             pl_auth_events,
@@ -431,7 +432,7 @@ def test_join_rules_msc3083_restricted(self):
                 _join_event(
                     pleb,
                     additional_content={
-                        "join_authorised_via_users_server": "@other:example.com"
+                        EventContentFields.AUTHORISING_USER: "@other:example.com"
                     },
                 ),
                 auth_events,
@@ -448,7 +449,7 @@ def test_join_rules_msc3083_restricted(self):
                     "join",
                     sender=creator,
                     additional_content={
-                        "join_authorised_via_users_server": "@inviter:foo.test"
+                        EventContentFields.AUTHORISING_USER: "@inviter:foo.test"
                     },
                 ),
                 auth_events,