From 7bf026cc89e9baf456b9e858d6adc0b0914fb6cd Mon Sep 17 00:00:00 2001 From: Nicolas Werner Date: Sat, 25 Mar 2023 15:15:54 +0100 Subject: [PATCH 1/3] Fix joining rooms you have been unbanned from Since forever synapse did not allow you to join a room after you have been unbanned from it over federation. This was not actually because of the unban event not federating. Synapse simply used outdated state to validate the join transition. This skips the validation if we are not in the room and for that reason won't have the current room state. Fixes #1563 Signed-off-by: Nicolas Werner --- synapse/handlers/federation_event.py | 2 +- synapse/handlers/room_member.py | 109 ++++++++++++++------------- 2 files changed, 58 insertions(+), 53 deletions(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index b7136f8d1ccb..648843cdbe9b 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -583,7 +583,7 @@ async def process_remote_join( await self._check_event_auth(origin, event, context) if context.rejected: - raise SynapseError(400, "Join event was rejected") + raise SynapseError(403, "Join event was rejected") # the remote server is responsible for sending our join event to the rest # of the federation. Indeed, attempting to do so will result in problems diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 509c5578895b..1d8b0aee6ff7 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -850,63 +850,68 @@ async def update_membership_locked( # `is_partial_state_room` also indicates whether `partial_state_before_join` is # partial. - # TODO: Refactor into dictionary of explicitly allowed transitions - # between old and new state, with specific error messages for some - # transitions and generic otherwise - old_state_id = partial_state_before_join.get( - (EventTypes.Member, target.to_string()) - ) - if old_state_id: - old_state = await self.store.get_event(old_state_id, allow_none=True) - old_membership = old_state.content.get("membership") if old_state else None - if action == "unban" and old_membership != "ban": - raise SynapseError( - 403, - "Cannot unban user who was not banned" - " (membership=%s)" % old_membership, - errcode=Codes.BAD_STATE, - ) - if old_membership == "ban" and action not in ["ban", "unban", "leave"]: - raise SynapseError( - 403, - "Cannot %s user who was banned" % (action,), - errcode=Codes.BAD_STATE, - ) - - if old_state: - same_content = content == old_state.content - same_membership = old_membership == effective_membership_state - same_sender = requester.user.to_string() == old_state.sender - if same_sender and same_membership and same_content: - # duplicate event. - # we know it was persisted, so must have a stream ordering. - assert old_state.internal_metadata.stream_ordering - return ( - old_state.event_id, - old_state.internal_metadata.stream_ordering, - ) + is_host_in_room = await self._is_host_in_room(partial_state_before_join) - if old_membership in ["ban", "leave"] and action == "kick": - raise AuthError(403, "The target user is not in the room") + # if we are not in the room, we won't have the current state + if is_host_in_room: + # TODO: Refactor into dictionary of explicitly allowed transitions + # between old and new state, with specific error messages for some + # transitions and generic otherwise + old_state_id = partial_state_before_join.get( + (EventTypes.Member, target.to_string()) + ) - # we don't allow people to reject invites to the server notice - # room, but they can leave it once they are joined. - if ( - old_membership == Membership.INVITE - and effective_membership_state == Membership.LEAVE - ): - is_blocked = await self.store.is_server_notice_room(room_id) - if is_blocked: + if old_state_id: + old_state = await self.store.get_event(old_state_id, allow_none=True) + old_membership = ( + old_state.content.get("membership") if old_state else None + ) + if action == "unban" and old_membership != "ban": raise SynapseError( - HTTPStatus.FORBIDDEN, - "You cannot reject this invite", - errcode=Codes.CANNOT_LEAVE_SERVER_NOTICE_ROOM, + 403, + "Cannot unban user who was not banned" + " (membership=%s)" % old_membership, + errcode=Codes.BAD_STATE, + ) + if old_membership == "ban" and action not in ["ban", "unban", "leave"]: + raise SynapseError( + 403, + "Cannot %s user who was banned" % (action,), + errcode=Codes.BAD_STATE, ) - else: - if action == "kick": - raise AuthError(403, "The target user is not in the room") - is_host_in_room = await self._is_host_in_room(partial_state_before_join) + if old_state: + same_content = content == old_state.content + same_membership = old_membership == effective_membership_state + same_sender = requester.user.to_string() == old_state.sender + if same_sender and same_membership and same_content: + # duplicate event. + # we know it was persisted, so must have a stream ordering. + assert old_state.internal_metadata.stream_ordering + return ( + old_state.event_id, + old_state.internal_metadata.stream_ordering, + ) + + if old_membership in ["ban", "leave"] and action == "kick": + raise AuthError(403, "The target user is not in the room") + + # we don't allow people to reject invites to the server notice + # room, but they can leave it once they are joined. + if ( + old_membership == Membership.INVITE + and effective_membership_state == Membership.LEAVE + ): + is_blocked = await self.store.is_server_notice_room(room_id) + if is_blocked: + raise SynapseError( + HTTPStatus.FORBIDDEN, + "You cannot reject this invite", + errcode=Codes.CANNOT_LEAVE_SERVER_NOTICE_ROOM, + ) + else: + if action == "kick": + raise AuthError(403, "The target user is not in the room") if effective_membership_state == Membership.JOIN: if requester.is_guest: From e3c5aea23f4c270f1257e362751f237be73e465d Mon Sep 17 00:00:00 2001 From: Nicolas Werner Date: Sat, 25 Mar 2023 15:26:38 +0100 Subject: [PATCH 2/3] Add changelog Signed-off-by: Nicolas Werner --- changelog.d/15323.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/15323.bugfix diff --git a/changelog.d/15323.bugfix b/changelog.d/15323.bugfix new file mode 100644 index 000000000000..15c5289ef62c --- /dev/null +++ b/changelog.d/15323.bugfix @@ -0,0 +1 @@ +Fix joining rooms you have been unbanned from over federation. Introduced before Synapse bugs were tracked on Github. Contributed by Nico. From 961b40fc8584bf40b58e721b7a5cbeb00a78d082 Mon Sep 17 00:00:00 2001 From: reivilibre Date: Wed, 29 Mar 2023 08:47:17 +0100 Subject: [PATCH 3/3] Update changelog.d/15323.bugfix --- changelog.d/15323.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/15323.bugfix b/changelog.d/15323.bugfix index 15c5289ef62c..bc1ab35532fd 100644 --- a/changelog.d/15323.bugfix +++ b/changelog.d/15323.bugfix @@ -1 +1 @@ -Fix joining rooms you have been unbanned from over federation. Introduced before Synapse bugs were tracked on Github. Contributed by Nico. +Fix a long-standing bug preventing users from joining rooms, that they had been unbanned from, over federation. Contributed by Nico.