From c8fe6b5a78e580e1b06d026cdce54ea00a82cef7 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 28 Sep 2021 08:43:39 -0400 Subject: [PATCH 1/4] Use a constant for "join_authorised_via_users_server". --- synapse/api/constants.py | 3 +++ synapse/event_auth.py | 12 +++++++----- synapse/events/utils.py | 2 +- synapse/federation/federation_base.py | 6 +++--- synapse/federation/federation_client.py | 6 +++--- synapse/federation/federation_server.py | 6 +++--- synapse/handlers/federation.py | 9 +++++++-- synapse/handlers/room_member.py | 2 +- tests/events/test_utils.py | 7 ++++--- tests/test_event_auth.py | 9 +++++---- 10 files changed, 37 insertions(+), 25 deletions(-) diff --git a/synapse/api/constants.py b/synapse/api/constants.py index 39fd9954d507..a31f037748a3 100644 --- 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 index 5d7c6fa858fb..c12ac64674cf 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -116,11 +116,11 @@ def check( room_version_obj.msc3083_join_rules and 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") @@ -382,7 +382,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.") @@ -837,10 +839,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 index f86113a448c3..38fccd1efcf5 100644 --- 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 index 024e440ff401..0cd424e12aa1 100644 --- 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 index 584836c04ad1..2ab4dec88fe6 100644 --- 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 index 638959cbecdb..5f4383eebcd3 100644 --- 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 index b17ef2a9a104..adbd150e46e5 100644 --- 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 index 1a56c82fbd9e..04c001f58366 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -939,7 +939,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 index 5446fda5e7a3..1dea09e4800d 100644 --- 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,7 +373,7 @@ 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", }, }, @@ -380,7 +381,7 @@ def test_member(self): "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 index 6ebd01bcbe78..1a4d078780ec 100644 --- 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, From 5750d9aa713228e7fc3a2363c906e4c817db1dbd Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 28 Sep 2021 08:36:36 -0400 Subject: [PATCH 2/4] Strip out the join_authorised_via_users_server key from client-sent state events. --- synapse/handlers/room_member.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 04c001f58366..a8f371d4bad6 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -908,6 +908,11 @@ async def _should_perform_remote_join( Membership.JOIN, Membership.INVITE, ): + # 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. + content.pop(EventContentFields.AUTHORISING_USER, None) + return False, [] # If the local host has a user who can issue invites, then a local From 11d07a7e55e7c9534f964eb57fbf81ca9674c3ab Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 28 Sep 2021 09:13:29 -0400 Subject: [PATCH 3/4] Newsfragment --- changelog.d/10933.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10933.bugfix diff --git a/changelog.d/10933.bugfix b/changelog.d/10933.bugfix new file mode 100644 index 000000000000..e0694fea22f5 --- /dev/null +++ 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. From 90ccaf8b9422d8ee6bbc8f28e3d10f7f77ab41d6 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 29 Sep 2021 10:30:28 -0400 Subject: [PATCH 4/4] Always remove the join_authorised_via_users_server field. --- synapse/handlers/room_member.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index a8f371d4bad6..afa7e4727dc4 100644 --- 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" @@ -908,11 +916,6 @@ async def _should_perform_remote_join( Membership.JOIN, Membership.INVITE, ): - # 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. - content.pop(EventContentFields.AUTHORISING_USER, None) - return False, [] # If the local host has a user who can issue invites, then a local