From 7855b97c4867f17f847a61164379e00b2be71b5c Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Thu, 13 Jun 2024 10:35:54 -0400 Subject: [PATCH 01/28] Support MSC3757 Restricting who can overwrite a state event --- synapse/api/room_versions.py | 21 +++++++++++++++++++++ synapse/event_auth.py | 27 +++++++++++++++++++++------ 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/synapse/api/room_versions.py b/synapse/api/room_versions.py index fbc1d58ecb1..6e0fbd17ec6 100644 --- a/synapse/api/room_versions.py +++ b/synapse/api/room_versions.py @@ -320,6 +320,26 @@ class RoomVersions: enforce_int_power_levels=True, msc3931_push_features=(PushRuleRoomFlag.EXTENSIBLE_EVENTS,), ) + MSC3757v10 = RoomVersion( + # MSC3757 (Restricting who can overwrite a state event) based on room version "10" + "org.matrix.msc3757.10", + RoomDisposition.UNSTABLE, + EventFormatVersions.ROOM_V4_PLUS, + StateResolutionVersions.V2, + enforce_key_validity=True, + special_case_aliases_auth=False, + strict_canonicaljson=True, + limit_notifications_power_levels=True, + implicit_room_creator=False, + updated_redaction_rules=False, + restricted_join_rule=True, + restricted_join_rule_fix=True, + knock_join_rule=True, + msc3389_relation_redactions=False, + knock_restricted_join_rule=True, + enforce_int_power_levels=True, + msc3931_push_features=(), + ) V11 = RoomVersion( "11", RoomDisposition.STABLE, @@ -355,6 +375,7 @@ class RoomVersions: RoomVersions.V9, RoomVersions.V10, RoomVersions.V11, + RoomVersions.MSC3757v10, ) } diff --git a/synapse/event_auth.py b/synapse/event_auth.py index f5abcde2dbe..11a8dea7db5 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -388,6 +388,7 @@ def check_state_dependent_auth_rules( RoomVersions.V9, RoomVersions.V10, RoomVersions.MSC1767v10, + RoomVersions.MSC3757v10, } @@ -790,9 +791,10 @@ def get_send_level( def _can_send_event(event: "EventBase", auth_events: StateMap["EventBase"]) -> bool: + state_key = event.get_state_key() power_levels_event = get_power_level_event(auth_events) - send_level = get_send_level(event.type, event.get("state_key"), power_levels_event) + send_level = get_send_level(event.type, state_key, power_levels_event) user_level = get_user_power_level(event.user_id, auth_events) if user_level < send_level: @@ -803,11 +805,24 @@ def _can_send_event(event: "EventBase", auth_events: StateMap["EventBase"]) -> b errcode=Codes.INSUFFICIENT_POWER, ) - # Check state_key - if hasattr(event, "state_key"): - if event.state_key.startswith("@"): - if event.state_key != event.user_id: - raise AuthError(403, "You are not allowed to set others state") + if ( + state_key is not None + and state_key.startswith("@") + and state_key != event.user_id + ): + if event.room_version is RoomVersions.MSC3757v10: + colon_idx = state_key.find(":") + if colon_idx != -1: + suffix_idx = state_key.find("_", colon_idx) + state_key_user_id = ( + state_key[:suffix_idx] if suffix_idx != -1 else state_key + ) + if ( + state_key_user_id == event.user_id + or user_level > get_user_power_level(state_key_user_id, auth_events) + ): + return True + raise AuthError(403, "You are not allowed to set others state") return True From 6ec215b38d4ad3a20b977070b0521b5c50daa5c1 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Wed, 31 Jul 2024 23:17:34 -0400 Subject: [PATCH 02/28] Add changelog --- changelog.d/17513.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17513.feature diff --git a/changelog.d/17513.feature b/changelog.d/17513.feature new file mode 100644 index 00000000000..21441c7211b --- /dev/null +++ b/changelog.d/17513.feature @@ -0,0 +1 @@ +Add implementation of restricting who can overwrite a state event as proposed by [MSC3757](https://github.com/matrix-org/matrix-spec-proposals/pull/3757). From 6a6921377a3597e8054818e05f07434ea7000bba Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Wed, 7 Aug 2024 15:40:40 -0400 Subject: [PATCH 03/28] Run power level tests on MSC3757 rooms --- tests/rest/client/test_power_levels.py | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/tests/rest/client/test_power_levels.py b/tests/rest/client/test_power_levels.py index 1584c2e96ce..4e1ac9c0566 100644 --- a/tests/rest/client/test_power_levels.py +++ b/tests/rest/client/test_power_levels.py @@ -19,10 +19,14 @@ # # from http import HTTPStatus +from typing import Optional + +from parameterized import parameterized_class from twisted.test.proto_helpers import MemoryReactor from synapse.api.errors import Codes +from synapse.api.room_versions import RoomVersions from synapse.events.utils import CANONICALJSON_MAX_INT, CANONICALJSON_MIN_INT from synapse.rest import admin from synapse.rest.client import login, room, sync @@ -32,9 +36,13 @@ from tests.unittest import HomeserverTestCase +@parameterized_class( + ("room_version",), [(None,), (RoomVersions.MSC3757v10.identifier,)] +) class PowerLevelsTestCase(HomeserverTestCase): """Tests that power levels are enforced in various situations""" + room_version: Optional[str] servlets = [ admin.register_servlets, room.register_servlets, @@ -58,7 +66,9 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: # Create a room self.room_id = self.helper.create_room_as( - self.admin_user_id, tok=self.admin_access_token + self.admin_user_id, + tok=self.admin_access_token, + room_version=self.room_version, ) # Invite the other users @@ -149,7 +159,9 @@ def test_non_admins_cannot_send_server_acl(self) -> None: def test_non_admins_cannot_tombstone_room(self) -> None: # Create another room that will serve as our "upgraded room" self.upgraded_room_id = self.helper.create_room_as( - self.admin_user_id, tok=self.admin_access_token + self.admin_user_id, + tok=self.admin_access_token, + room_version=self.room_version, ) # have the mod try to send a tombstone event @@ -203,7 +215,9 @@ def test_admins_can_send_server_acl(self) -> None: def test_admins_can_tombstone_room(self) -> None: # Create another room that will serve as our "upgraded room" self.upgraded_room_id = self.helper.create_room_as( - self.admin_user_id, tok=self.admin_access_token + self.admin_user_id, + tok=self.admin_access_token, + room_version=self.room_version, ) # have the admin try to send a tombstone event From e48123823c2606cbda9aedccbf242642b12c04b8 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Thu, 8 Aug 2024 10:32:46 -0400 Subject: [PATCH 04/28] Check if @-prefixed state key is not a user ID --- synapse/event_auth.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/synapse/event_auth.py b/synapse/event_auth.py index 11a8dea7db5..71a3c50629b 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -817,6 +817,12 @@ def _can_send_event(event: "EventBase", auth_events: StateMap["EventBase"]) -> b state_key_user_id = ( state_key[:suffix_idx] if suffix_idx != -1 else state_key ) + if not UserID.is_valid(state_key_user_id): + raise UnstableSpecAuthError( + 403, + "State key is not a valid user ID", + errcode=Codes.BAD_JSON, + ) if ( state_key_user_id == event.user_id or user_level > get_user_power_level(state_key_user_id, auth_events) From 248025d1aa8ba52f5fa7a0ab98f7c6fc38de309e Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Thu, 8 Aug 2024 10:39:39 -0400 Subject: [PATCH 05/28] Test MSC3757 auth rules --- tests/rest/client/test_power_levels.py | 53 ++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/tests/rest/client/test_power_levels.py b/tests/rest/client/test_power_levels.py index 4e1ac9c0566..3e789d0a687 100644 --- a/tests/rest/client/test_power_levels.py +++ b/tests/rest/client/test_power_levels.py @@ -18,6 +18,7 @@ # [This file includes modifications made by New Vector Limited] # # +import re from http import HTTPStatus from typing import Optional @@ -35,6 +36,8 @@ from tests.unittest import HomeserverTestCase +_room_version_msc3757_re = re.compile(r"org.matrix.msc3757\b") + @parameterized_class( ("room_version",), [(None,), (RoomVersions.MSC3757v10.identifier,)] @@ -110,6 +113,12 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: tok=self.admin_access_token, ) + @property + def _allows_owned_state(self) -> bool: + return self.room_version is not None and bool( + _room_version_msc3757_re.match(self.room_version) + ) + def test_non_admins_cannot_enable_room_encryption(self) -> None: # have the mod try to enable room encryption self.helper.send_state( @@ -307,3 +316,47 @@ def test_cannot_set_unsafe_small_power_levels(self) -> None: Codes.BAD_JSON, body, ) + + def test_can_set_own_state(self) -> None: + self.helper.send_state( + self.room_id, + "org.matrix.msc3757.test", + {}, + state_key=f"{self.mod_user_id}_suffix", + tok=self.mod_access_token, + expect_code=( + HTTPStatus.OK if self._allows_owned_state else HTTPStatus.FORBIDDEN + ), + ) + + def test_admins_can_set_others_state(self) -> None: + self.helper.send_state( + self.room_id, + "org.matrix.msc3757.test", + {}, + state_key=f"{self.mod_user_id}_suffix", + tok=self.admin_access_token, + expect_code=( + HTTPStatus.OK if self._allows_owned_state else HTTPStatus.FORBIDDEN + ), + ) + + def test_non_admins_cannot_set_others_state(self) -> None: + self.helper.send_state( + self.room_id, + "org.matrix.msc3757.test", + {}, + state_key=f"{self.admin_user_id}_suffix", + tok=self.mod_access_token, + expect_code=HTTPStatus.FORBIDDEN, + ) + + def test_cannot_set_state_with_non_user_id_key(self) -> None: + self.helper.send_state( + self.room_id, + "org.matrix.msc3757.test", + {}, + state_key=f"{self.admin_user_id}@suffix", + tok=self.admin_access_token, + expect_code=HTTPStatus.FORBIDDEN, + ) From a12238d635bf91baa81366de3c77c67923b30fa0 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Thu, 8 Aug 2024 10:47:46 -0400 Subject: [PATCH 06/28] Support MSC3757 for room versions 9 and 11 Support v9 because that is what the MSC says should be supported. Keep supporting v10 because it is the current default room version. Also support v11 because it is the latest room version. --- synapse/api/room_versions.py | 42 ++++++++++++++++++++++++++ synapse/event_auth.py | 7 ++++- tests/rest/client/test_power_levels.py | 11 ++++++- 3 files changed, 58 insertions(+), 2 deletions(-) diff --git a/synapse/api/room_versions.py b/synapse/api/room_versions.py index 6e0fbd17ec6..3faa9e16fda 100644 --- a/synapse/api/room_versions.py +++ b/synapse/api/room_versions.py @@ -281,6 +281,26 @@ class RoomVersions: enforce_int_power_levels=False, msc3931_push_features=(), ) + MSC3757v9 = RoomVersion( + # MSC3757 (Restricting who can overwrite a state event) based on room version "9" + "org.matrix.msc3757.9", + RoomDisposition.UNSTABLE, + EventFormatVersions.ROOM_V4_PLUS, + StateResolutionVersions.V2, + enforce_key_validity=True, + special_case_aliases_auth=False, + strict_canonicaljson=True, + limit_notifications_power_levels=True, + implicit_room_creator=False, + updated_redaction_rules=False, + restricted_join_rule=True, + restricted_join_rule_fix=True, + knock_join_rule=True, + msc3389_relation_redactions=False, + knock_restricted_join_rule=False, + enforce_int_power_levels=False, + msc3931_push_features=(), + ) V10 = RoomVersion( "10", RoomDisposition.STABLE, @@ -359,6 +379,26 @@ class RoomVersions: enforce_int_power_levels=True, msc3931_push_features=(), ) + MSC3757v11 = RoomVersion( + # MSC3757 (Restricting who can overwrite a state event) based on room version "11" + "org.matrix.msc3757.11", + RoomDisposition.UNSTABLE, + EventFormatVersions.ROOM_V4_PLUS, + StateResolutionVersions.V2, + enforce_key_validity=True, + special_case_aliases_auth=False, + strict_canonicaljson=True, + limit_notifications_power_levels=True, + implicit_room_creator=True, # Used by MSC3820 + updated_redaction_rules=True, # Used by MSC3820 + restricted_join_rule=True, + restricted_join_rule_fix=True, + knock_join_rule=True, + msc3389_relation_redactions=False, + knock_restricted_join_rule=True, + enforce_int_power_levels=True, + msc3931_push_features=(), + ) KNOWN_ROOM_VERSIONS: Dict[str, RoomVersion] = { @@ -375,7 +415,9 @@ class RoomVersions: RoomVersions.V9, RoomVersions.V10, RoomVersions.V11, + RoomVersions.MSC3757v9, RoomVersions.MSC3757v10, + RoomVersions.MSC3757v11, ) } diff --git a/synapse/event_auth.py b/synapse/event_auth.py index 71a3c50629b..dafacd3f1f3 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -386,6 +386,7 @@ def check_state_dependent_auth_rules( RoomVersions.V7, RoomVersions.V8, RoomVersions.V9, + RoomVersions.MSC3757v9, RoomVersions.V10, RoomVersions.MSC1767v10, RoomVersions.MSC3757v10, @@ -810,7 +811,11 @@ def _can_send_event(event: "EventBase", auth_events: StateMap["EventBase"]) -> b and state_key.startswith("@") and state_key != event.user_id ): - if event.room_version is RoomVersions.MSC3757v10: + if event.room_version in ( + RoomVersions.MSC3757v9, + RoomVersions.MSC3757v10, + RoomVersions.MSC3757v11, + ): colon_idx = state_key.find(":") if colon_idx != -1: suffix_idx = state_key.find("_", colon_idx) diff --git a/tests/rest/client/test_power_levels.py b/tests/rest/client/test_power_levels.py index 3e789d0a687..1c6e9c3bef8 100644 --- a/tests/rest/client/test_power_levels.py +++ b/tests/rest/client/test_power_levels.py @@ -40,7 +40,16 @@ @parameterized_class( - ("room_version",), [(None,), (RoomVersions.MSC3757v10.identifier,)] + ("room_version",), + [ + (None,) if rv is None else (rv.identifier,) + for rv in [ + None, + RoomVersions.MSC3757v9, + RoomVersions.MSC3757v10, + RoomVersions.MSC3757v11, + ] + ], ) class PowerLevelsTestCase(HomeserverTestCase): """Tests that power levels are enforced in various situations""" From 663da8d18e7c4531d271bb6225b9e988d455a0ab Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 12 Aug 2024 14:51:41 -0400 Subject: [PATCH 07/28] Skip redundant characters for some string searches --- synapse/event_auth.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/event_auth.py b/synapse/event_auth.py index dafacd3f1f3..b3b832902ef 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -816,9 +816,9 @@ def _can_send_event(event: "EventBase", auth_events: StateMap["EventBase"]) -> b RoomVersions.MSC3757v10, RoomVersions.MSC3757v11, ): - colon_idx = state_key.find(":") + colon_idx = state_key.find(":", 1) if colon_idx != -1: - suffix_idx = state_key.find("_", colon_idx) + suffix_idx = state_key.find("_", colon_idx + 1) state_key_user_id = ( state_key[:suffix_idx] if suffix_idx != -1 else state_key ) From c4cc4d8af07b2e06222bd43db65fae5a2a55b7fe Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Wed, 14 Aug 2024 21:27:15 -0400 Subject: [PATCH 08/28] Add explicit field to room version for MSC3757 --- synapse/api/room_versions.py | 17 +++++++++++++++++ synapse/event_auth.py | 6 +----- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/synapse/api/room_versions.py b/synapse/api/room_versions.py index 3faa9e16fda..15ea15456c6 100644 --- a/synapse/api/room_versions.py +++ b/synapse/api/room_versions.py @@ -107,6 +107,8 @@ class RoomVersion: # support the flag. Unknown flags are ignored by the evaluator, making conditions # fail if used. msc3931_push_features: Tuple[str, ...] # values from PushRuleRoomFlag + # MSC3757: Restricting who can overwrite a state event + msc3757_enabled: bool class RoomVersions: @@ -128,6 +130,7 @@ class RoomVersions: knock_restricted_join_rule=False, enforce_int_power_levels=False, msc3931_push_features=(), + msc3757_enabled=False, ) V2 = RoomVersion( "2", @@ -147,6 +150,7 @@ class RoomVersions: knock_restricted_join_rule=False, enforce_int_power_levels=False, msc3931_push_features=(), + msc3757_enabled=False, ) V3 = RoomVersion( "3", @@ -166,6 +170,7 @@ class RoomVersions: knock_restricted_join_rule=False, enforce_int_power_levels=False, msc3931_push_features=(), + msc3757_enabled=False, ) V4 = RoomVersion( "4", @@ -185,6 +190,7 @@ class RoomVersions: knock_restricted_join_rule=False, enforce_int_power_levels=False, msc3931_push_features=(), + msc3757_enabled=False, ) V5 = RoomVersion( "5", @@ -204,6 +210,7 @@ class RoomVersions: knock_restricted_join_rule=False, enforce_int_power_levels=False, msc3931_push_features=(), + msc3757_enabled=False, ) V6 = RoomVersion( "6", @@ -223,6 +230,7 @@ class RoomVersions: knock_restricted_join_rule=False, enforce_int_power_levels=False, msc3931_push_features=(), + msc3757_enabled=False, ) V7 = RoomVersion( "7", @@ -242,6 +250,7 @@ class RoomVersions: knock_restricted_join_rule=False, enforce_int_power_levels=False, msc3931_push_features=(), + msc3757_enabled=False, ) V8 = RoomVersion( "8", @@ -261,6 +270,7 @@ class RoomVersions: knock_restricted_join_rule=False, enforce_int_power_levels=False, msc3931_push_features=(), + msc3757_enabled=False, ) V9 = RoomVersion( "9", @@ -280,6 +290,7 @@ class RoomVersions: knock_restricted_join_rule=False, enforce_int_power_levels=False, msc3931_push_features=(), + msc3757_enabled=False, ) MSC3757v9 = RoomVersion( # MSC3757 (Restricting who can overwrite a state event) based on room version "9" @@ -300,6 +311,7 @@ class RoomVersions: knock_restricted_join_rule=False, enforce_int_power_levels=False, msc3931_push_features=(), + msc3757_enabled=True, ) V10 = RoomVersion( "10", @@ -319,6 +331,7 @@ class RoomVersions: knock_restricted_join_rule=True, enforce_int_power_levels=True, msc3931_push_features=(), + msc3757_enabled=False, ) MSC1767v10 = RoomVersion( # MSC1767 (Extensible Events) based on room version "10" @@ -339,6 +352,7 @@ class RoomVersions: knock_restricted_join_rule=True, enforce_int_power_levels=True, msc3931_push_features=(PushRuleRoomFlag.EXTENSIBLE_EVENTS,), + msc3757_enabled=False, ) MSC3757v10 = RoomVersion( # MSC3757 (Restricting who can overwrite a state event) based on room version "10" @@ -359,6 +373,7 @@ class RoomVersions: knock_restricted_join_rule=True, enforce_int_power_levels=True, msc3931_push_features=(), + msc3757_enabled=True, ) V11 = RoomVersion( "11", @@ -378,6 +393,7 @@ class RoomVersions: knock_restricted_join_rule=True, enforce_int_power_levels=True, msc3931_push_features=(), + msc3757_enabled=False, ) MSC3757v11 = RoomVersion( # MSC3757 (Restricting who can overwrite a state event) based on room version "11" @@ -398,6 +414,7 @@ class RoomVersions: knock_restricted_join_rule=True, enforce_int_power_levels=True, msc3931_push_features=(), + msc3757_enabled=True, ) diff --git a/synapse/event_auth.py b/synapse/event_auth.py index b3b832902ef..0203aa1ed8d 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -811,11 +811,7 @@ def _can_send_event(event: "EventBase", auth_events: StateMap["EventBase"]) -> b and state_key.startswith("@") and state_key != event.user_id ): - if event.room_version in ( - RoomVersions.MSC3757v9, - RoomVersions.MSC3757v10, - RoomVersions.MSC3757v11, - ): + if event.room_version.msc3757_enabled: colon_idx = state_key.find(":", 1) if colon_idx != -1: suffix_idx = state_key.find("_", colon_idx + 1) From 56578631cab285b0e7f84d6c348d578086e568fb Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Wed, 14 Aug 2024 22:02:08 -0400 Subject: [PATCH 09/28] Parametrize fewer power level unit tests --- tests/rest/client/test_power_levels.py | 42 +++++++++++--------------- 1 file changed, 18 insertions(+), 24 deletions(-) diff --git a/tests/rest/client/test_power_levels.py b/tests/rest/client/test_power_levels.py index 1c6e9c3bef8..677b7c11486 100644 --- a/tests/rest/client/test_power_levels.py +++ b/tests/rest/client/test_power_levels.py @@ -18,7 +18,6 @@ # [This file includes modifications made by New Vector Limited] # # -import re from http import HTTPStatus from typing import Optional @@ -36,25 +35,11 @@ from tests.unittest import HomeserverTestCase -_room_version_msc3757_re = re.compile(r"org.matrix.msc3757\b") - -@parameterized_class( - ("room_version",), - [ - (None,) if rv is None else (rv.identifier,) - for rv in [ - None, - RoomVersions.MSC3757v9, - RoomVersions.MSC3757v10, - RoomVersions.MSC3757v11, - ] - ], -) -class PowerLevelsTestCase(HomeserverTestCase): +class BasePowerLevelsTestCase(HomeserverTestCase): """Tests that power levels are enforced in various situations""" - room_version: Optional[str] + room_version: Optional[str] = None servlets = [ admin.register_servlets, room.register_servlets, @@ -122,12 +107,8 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: tok=self.admin_access_token, ) - @property - def _allows_owned_state(self) -> bool: - return self.room_version is not None and bool( - _room_version_msc3757_re.match(self.room_version) - ) +class PowerLevelsTestCase(BasePowerLevelsTestCase): def test_non_admins_cannot_enable_room_encryption(self) -> None: # have the mod try to enable room encryption self.helper.send_state( @@ -326,6 +307,19 @@ def test_cannot_set_unsafe_small_power_levels(self) -> None: body, ) +@parameterized_class( + ("room_version", "allows_owned_state"), + [ + (rv.identifier, rv.msc3757_enabled) + for rv in [ + RoomVersions.V10, + RoomVersions.MSC3757v10, + ] + ], +) +class MSC3757PowerLevelsTestCase(BasePowerLevelsTestCase): + allows_owned_state: bool + def test_can_set_own_state(self) -> None: self.helper.send_state( self.room_id, @@ -334,7 +328,7 @@ def test_can_set_own_state(self) -> None: state_key=f"{self.mod_user_id}_suffix", tok=self.mod_access_token, expect_code=( - HTTPStatus.OK if self._allows_owned_state else HTTPStatus.FORBIDDEN + HTTPStatus.OK if self.allows_owned_state else HTTPStatus.FORBIDDEN ), ) @@ -346,7 +340,7 @@ def test_admins_can_set_others_state(self) -> None: state_key=f"{self.mod_user_id}_suffix", tok=self.admin_access_token, expect_code=( - HTTPStatus.OK if self._allows_owned_state else HTTPStatus.FORBIDDEN + HTTPStatus.OK if self.allows_owned_state else HTTPStatus.FORBIDDEN ), ) From e9d22bab6bf05fce62887d3a63dcd2a6df0fdb92 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Wed, 14 Aug 2024 22:31:06 -0400 Subject: [PATCH 10/28] Lint --- tests/rest/client/test_power_levels.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/rest/client/test_power_levels.py b/tests/rest/client/test_power_levels.py index 677b7c11486..b4c1effcb56 100644 --- a/tests/rest/client/test_power_levels.py +++ b/tests/rest/client/test_power_levels.py @@ -307,6 +307,7 @@ def test_cannot_set_unsafe_small_power_levels(self) -> None: body, ) + @parameterized_class( ("room_version", "allows_owned_state"), [ From 228d6b4cde7337016d6001285db427420882cb50 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Tue, 3 Sep 2024 23:42:42 -0400 Subject: [PATCH 11/28] Return 400 for @-prefixed, non-userID state key Also set errcode to M_BAD_JSON. Do this only for MSC3757 rooms. --- synapse/event_auth.py | 23 ++++++++++++----------- tests/rest/client/test_power_levels.py | 17 +++++++++++++++-- 2 files changed, 27 insertions(+), 13 deletions(-) diff --git a/synapse/event_auth.py b/synapse/event_auth.py index 0203aa1ed8d..1bebed049e4 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -812,23 +812,24 @@ def _can_send_event(event: "EventBase", auth_events: StateMap["EventBase"]) -> b and state_key != event.user_id ): if event.room_version.msc3757_enabled: + state_key_user_id: Union[str, None] = None colon_idx = state_key.find(":", 1) if colon_idx != -1: suffix_idx = state_key.find("_", colon_idx + 1) state_key_user_id = ( state_key[:suffix_idx] if suffix_idx != -1 else state_key ) - if not UserID.is_valid(state_key_user_id): - raise UnstableSpecAuthError( - 403, - "State key is not a valid user ID", - errcode=Codes.BAD_JSON, - ) - if ( - state_key_user_id == event.user_id - or user_level > get_user_power_level(state_key_user_id, auth_events) - ): - return True + if state_key_user_id is None or not UserID.is_valid(state_key_user_id): + raise SynapseError( + 400, + "State key neither equals a valid user ID, nor starts with one plus an underscore", + errcode=Codes.BAD_JSON, + ) + if ( + state_key_user_id == event.user_id + or user_level > get_user_power_level(state_key_user_id, auth_events) + ): + return True raise AuthError(403, "You are not allowed to set others state") return True diff --git a/tests/rest/client/test_power_levels.py b/tests/rest/client/test_power_levels.py index b4c1effcb56..4705206f806 100644 --- a/tests/rest/client/test_power_levels.py +++ b/tests/rest/client/test_power_levels.py @@ -356,11 +356,24 @@ def test_non_admins_cannot_set_others_state(self) -> None: ) def test_cannot_set_state_with_non_user_id_key(self) -> None: - self.helper.send_state( + if self.allows_owned_state: + expect_code = HTTPStatus.BAD_REQUEST + errcode = Codes.BAD_JSON + else: + expect_code = HTTPStatus.FORBIDDEN + errcode = Codes.FORBIDDEN + + body = self.helper.send_state( self.room_id, "org.matrix.msc3757.test", {}, state_key=f"{self.admin_user_id}@suffix", tok=self.admin_access_token, - expect_code=HTTPStatus.FORBIDDEN, + expect_code=expect_code, + ) + + self.assertEqual( + body["errcode"], + errcode, + body, ) From e17587105e31effa506aab6082128b3243bc0e41 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Tue, 3 Sep 2024 23:45:44 -0400 Subject: [PATCH 12/28] Define test event type string once --- tests/rest/client/test_power_levels.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/rest/client/test_power_levels.py b/tests/rest/client/test_power_levels.py index 4705206f806..13302ba63d0 100644 --- a/tests/rest/client/test_power_levels.py +++ b/tests/rest/client/test_power_levels.py @@ -319,12 +319,14 @@ def test_cannot_set_unsafe_small_power_levels(self) -> None: ], ) class MSC3757PowerLevelsTestCase(BasePowerLevelsTestCase): + _STATE_EVENT_TYPE = "org.matrix.msc3757.test" + allows_owned_state: bool def test_can_set_own_state(self) -> None: self.helper.send_state( self.room_id, - "org.matrix.msc3757.test", + self._STATE_EVENT_TYPE, {}, state_key=f"{self.mod_user_id}_suffix", tok=self.mod_access_token, @@ -336,7 +338,7 @@ def test_can_set_own_state(self) -> None: def test_admins_can_set_others_state(self) -> None: self.helper.send_state( self.room_id, - "org.matrix.msc3757.test", + self._STATE_EVENT_TYPE, {}, state_key=f"{self.mod_user_id}_suffix", tok=self.admin_access_token, @@ -348,7 +350,7 @@ def test_admins_can_set_others_state(self) -> None: def test_non_admins_cannot_set_others_state(self) -> None: self.helper.send_state( self.room_id, - "org.matrix.msc3757.test", + self._STATE_EVENT_TYPE, {}, state_key=f"{self.admin_user_id}_suffix", tok=self.mod_access_token, @@ -365,7 +367,7 @@ def test_cannot_set_state_with_non_user_id_key(self) -> None: body = self.helper.send_state( self.room_id, - "org.matrix.msc3757.test", + self._STATE_EVENT_TYPE, {}, state_key=f"{self.admin_user_id}@suffix", tok=self.admin_access_token, From a841921fe3cc2eef06e08f6dc057730b8051919b Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Tue, 3 Sep 2024 23:58:58 -0400 Subject: [PATCH 13/28] Lint --- synapse/event_auth.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/synapse/event_auth.py b/synapse/event_auth.py index 1bebed049e4..43a381db940 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -825,9 +825,8 @@ def _can_send_event(event: "EventBase", auth_events: StateMap["EventBase"]) -> b "State key neither equals a valid user ID, nor starts with one plus an underscore", errcode=Codes.BAD_JSON, ) - if ( - state_key_user_id == event.user_id - or user_level > get_user_power_level(state_key_user_id, auth_events) + if state_key_user_id == event.user_id or user_level > get_user_power_level( + state_key_user_id, auth_events ): return True raise AuthError(403, "You are not allowed to set others state") From 772d35a769133a9de059aa471fff42b695bbcad3 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Thu, 5 Sep 2024 09:09:23 -0400 Subject: [PATCH 14/28] Refactor state key parsing --- synapse/event_auth.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/synapse/event_auth.py b/synapse/event_auth.py index 43a381db940..496e003487c 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -812,14 +812,15 @@ def _can_send_event(event: "EventBase", auth_events: StateMap["EventBase"]) -> b and state_key != event.user_id ): if event.room_version.msc3757_enabled: - state_key_user_id: Union[str, None] = None - colon_idx = state_key.find(":", 1) - if colon_idx != -1: + try: + colon_idx = state_key.index(":", 1) suffix_idx = state_key.find("_", colon_idx + 1) state_key_user_id = ( state_key[:suffix_idx] if suffix_idx != -1 else state_key ) - if state_key_user_id is None or not UserID.is_valid(state_key_user_id): + if not UserID.is_valid(state_key_user_id): + raise ValueError + except Exception: raise SynapseError( 400, "State key neither equals a valid user ID, nor starts with one plus an underscore", From ba2ce94bbc5d3fb1d94864a41d568a0d8007d55a Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Thu, 5 Sep 2024 09:11:03 -0400 Subject: [PATCH 15/28] Split & comment allow conditions --- synapse/event_auth.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/synapse/event_auth.py b/synapse/event_auth.py index 496e003487c..9c4d6200151 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -826,9 +826,11 @@ def _can_send_event(event: "EventBase", auth_events: StateMap["EventBase"]) -> b "State key neither equals a valid user ID, nor starts with one plus an underscore", errcode=Codes.BAD_JSON, ) - if state_key_user_id == event.user_id or user_level > get_user_power_level( - state_key_user_id, auth_events - ): + if state_key_user_id == event.user_id: + # sender is owner of the state key, allow + return True + if user_level > get_user_power_level(state_key_user_id, auth_events): + # sender has higher PL than the owner of the state key, allow return True raise AuthError(403, "You are not allowed to set others state") From 77af3873d2ceed343b20dc7deee94ea0f0c7bb1a Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Thu, 5 Sep 2024 11:42:27 -0400 Subject: [PATCH 16/28] Drop support for room version 9 Support only the current default (10) and latest (11) room versions --- synapse/api/room_versions.py | 22 ---------------------- synapse/event_auth.py | 1 - tests/rest/client/test_power_levels.py | 4 ++-- 3 files changed, 2 insertions(+), 25 deletions(-) diff --git a/synapse/api/room_versions.py b/synapse/api/room_versions.py index 15ea15456c6..4bde385f786 100644 --- a/synapse/api/room_versions.py +++ b/synapse/api/room_versions.py @@ -292,27 +292,6 @@ class RoomVersions: msc3931_push_features=(), msc3757_enabled=False, ) - MSC3757v9 = RoomVersion( - # MSC3757 (Restricting who can overwrite a state event) based on room version "9" - "org.matrix.msc3757.9", - RoomDisposition.UNSTABLE, - EventFormatVersions.ROOM_V4_PLUS, - StateResolutionVersions.V2, - enforce_key_validity=True, - special_case_aliases_auth=False, - strict_canonicaljson=True, - limit_notifications_power_levels=True, - implicit_room_creator=False, - updated_redaction_rules=False, - restricted_join_rule=True, - restricted_join_rule_fix=True, - knock_join_rule=True, - msc3389_relation_redactions=False, - knock_restricted_join_rule=False, - enforce_int_power_levels=False, - msc3931_push_features=(), - msc3757_enabled=True, - ) V10 = RoomVersion( "10", RoomDisposition.STABLE, @@ -432,7 +411,6 @@ class RoomVersions: RoomVersions.V9, RoomVersions.V10, RoomVersions.V11, - RoomVersions.MSC3757v9, RoomVersions.MSC3757v10, RoomVersions.MSC3757v11, ) diff --git a/synapse/event_auth.py b/synapse/event_auth.py index 9c4d6200151..f17caafc7f3 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -386,7 +386,6 @@ def check_state_dependent_auth_rules( RoomVersions.V7, RoomVersions.V8, RoomVersions.V9, - RoomVersions.MSC3757v9, RoomVersions.V10, RoomVersions.MSC1767v10, RoomVersions.MSC3757v10, diff --git a/tests/rest/client/test_power_levels.py b/tests/rest/client/test_power_levels.py index 13302ba63d0..f717009c83f 100644 --- a/tests/rest/client/test_power_levels.py +++ b/tests/rest/client/test_power_levels.py @@ -313,8 +313,8 @@ def test_cannot_set_unsafe_small_power_levels(self) -> None: [ (rv.identifier, rv.msc3757_enabled) for rv in [ - RoomVersions.V10, - RoomVersions.MSC3757v10, + RoomVersions.V11, + RoomVersions.MSC3757v11, ] ], ) From 6f429b6272533fca06dc1dfa44284473911a2044 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Thu, 5 Sep 2024 15:32:41 -0400 Subject: [PATCH 17/28] Refactor unit tests --- tests/rest/client/test_owned_state.py | 227 +++++++++++++++++++++++++ tests/rest/client/test_power_levels.py | 94 +--------- 2 files changed, 231 insertions(+), 90 deletions(-) create mode 100644 tests/rest/client/test_owned_state.py diff --git a/tests/rest/client/test_owned_state.py b/tests/rest/client/test_owned_state.py new file mode 100644 index 00000000000..f14746fcb2b --- /dev/null +++ b/tests/rest/client/test_owned_state.py @@ -0,0 +1,227 @@ +# +# This file is licensed under the Affero General Public License (AGPL) version 3. +# +# Copyright (C) 2024 New Vector, Ltd +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as +# published by the Free Software Foundation, either version 3 of the +# License, or (at your option) any later version. +# +# See the GNU Affero General Public License for more details: +# . +# +from http import HTTPStatus + +from parameterized import parameterized_class + +from twisted.test.proto_helpers import MemoryReactor + +from synapse.api.errors import Codes +from synapse.api.room_versions import KNOWN_ROOM_VERSIONS +from synapse.rest import admin +from synapse.rest.client import login, room +from synapse.server import HomeServer +from synapse.types import JsonDict +from synapse.util import Clock + +from tests.unittest import HomeserverTestCase + +_STATE_EVENT_TEST_TYPE = "com.example.test" + +# To stress-test parsing, include separator & sigil characters +_STATE_KEY_SUFFIX = "_state_key_suffix:!@#$123" + + +class OwnedStateBase(HomeserverTestCase): + servlets = [ + admin.register_servlets, + room.register_servlets, + login.register_servlets, + ] + + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + self.admin_user_id = self.register_user("admin", "pass") + self.admin_access_token = self.login("admin", "pass") + self.user1_user_id = self.register_user("user1", "pass") + self.user1_access_token = self.login("user1", "pass") + + self.room_id = self.helper.create_room_as( + self.admin_user_id, + tok=self.admin_access_token, + is_public=True, + extra_content={ + "power_level_content_override": { + "events": { + _STATE_EVENT_TEST_TYPE: 0, + }, + }, + }, + ) + + self.helper.join( + room=self.room_id, user=self.user1_user_id, tok=self.user1_access_token + ) + + +class WithoutOwnedStateTestCase(OwnedStateBase): + def test_user_can_set_state_with_own_userid_key(self) -> None: + self.helper.send_state( + self.room_id, + _STATE_EVENT_TEST_TYPE, + {}, + state_key=f"{self.user1_user_id}", + tok=self.user1_access_token, + expect_code=HTTPStatus.OK, + ) + + def test_admin_cannot_set_state_with_own_suffixed_key(self) -> None: + self.helper.send_state( + self.room_id, + _STATE_EVENT_TEST_TYPE, + {}, + state_key=f"{self.admin_user_id}{_STATE_KEY_SUFFIX}", + tok=self.admin_access_token, + expect_code=HTTPStatus.FORBIDDEN, + ) + + def test_admin_cannot_set_state_with_other_userid_key(self) -> None: + self.helper.send_state( + self.room_id, + _STATE_EVENT_TEST_TYPE, + {}, + state_key=f"{self.user1_user_id}", + tok=self.admin_access_token, + expect_code=HTTPStatus.FORBIDDEN, + ) + + def test_admin_cannot_set_state_with_other_suffixed_key(self) -> None: + self.helper.send_state( + self.room_id, + _STATE_EVENT_TEST_TYPE, + {}, + state_key=f"{self.user1_user_id}{_STATE_KEY_SUFFIX}", + tok=self.admin_access_token, + expect_code=HTTPStatus.FORBIDDEN, + ) + + def test_admin_cannot_set_state_with_malformed_userid_key(self) -> None: + body = self.helper.send_state( + self.room_id, + _STATE_EVENT_TEST_TYPE, + {}, + state_key="@oops", + tok=self.admin_access_token, + expect_code=HTTPStatus.FORBIDDEN, + ) + + self.assertEqual( + body["errcode"], + Codes.FORBIDDEN, + body, + ) + + +@parameterized_class( + ("room_version",), + [(i,) for i, v in KNOWN_ROOM_VERSIONS.items() if v.msc3757_enabled], +) +class MSC3757OwnedStateTestCase(OwnedStateBase): + room_version: str + + def default_config(self) -> JsonDict: + config = super().default_config() + config["default_room_version"] = self.room_version + return config + + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + super().prepare(reactor, clock, hs) + + self.user2_user_id = self.register_user("user2", "pass") + self.user2_access_token = self.login("user2", "pass") + + self.helper.join( + room=self.room_id, user=self.user2_user_id, tok=self.user2_access_token + ) + + def test_user_can_set_state_with_own_suffixed_key(self) -> None: + self.helper.send_state( + self.room_id, + _STATE_EVENT_TEST_TYPE, + {}, + state_key=f"{self.user1_user_id}{_STATE_KEY_SUFFIX}", + tok=self.user1_access_token, + expect_code=HTTPStatus.OK, + ) + + def test_admin_can_set_state_with_other_userid_key(self) -> None: + self.helper.send_state( + self.room_id, + _STATE_EVENT_TEST_TYPE, + {}, + state_key=f"{self.user1_user_id}", + tok=self.admin_access_token, + expect_code=HTTPStatus.OK, + ) + + def test_admin_can_set_state_with_other_suffixed_key(self) -> None: + self.helper.send_state( + self.room_id, + _STATE_EVENT_TEST_TYPE, + {}, + state_key=f"{self.user1_user_id}{_STATE_KEY_SUFFIX}", + tok=self.admin_access_token, + expect_code=HTTPStatus.OK, + ) + + def test_user_cannot_set_state_with_other_userid_key(self) -> None: + self.helper.send_state( + self.room_id, + _STATE_EVENT_TEST_TYPE, + {}, + state_key=f"{self.user2_user_id}", + tok=self.user1_access_token, + expect_code=HTTPStatus.FORBIDDEN, + ) + + def test_user_cannot_set_state_with_other_suffixed_key(self) -> None: + self.helper.send_state( + self.room_id, + _STATE_EVENT_TEST_TYPE, + {}, + state_key=f"{self.user2_user_id}", + tok=self.user1_access_token, + expect_code=HTTPStatus.FORBIDDEN, + ) + + def test_admin_cannot_set_state_with_malformed_userid_key(self) -> None: + body = self.helper.send_state( + self.room_id, + _STATE_EVENT_TEST_TYPE, + {}, + state_key="@oops", + tok=self.admin_access_token, + expect_code=HTTPStatus.BAD_REQUEST, + ) + + self.assertEqual( + body["errcode"], + Codes.BAD_JSON, + body, + ) + + def test_admin_cannot_set_state_with_improperly_suffixed_key(self) -> None: + body = self.helper.send_state( + self.room_id, + _STATE_EVENT_TEST_TYPE, + {}, + state_key=f"{self.admin_user_id}@{_STATE_KEY_SUFFIX[1:]}", + tok=self.admin_access_token, + expect_code=HTTPStatus.BAD_REQUEST, + ) + + self.assertEqual( + body["errcode"], + Codes.BAD_JSON, + body, + ) diff --git a/tests/rest/client/test_power_levels.py b/tests/rest/client/test_power_levels.py index f717009c83f..1584c2e96ce 100644 --- a/tests/rest/client/test_power_levels.py +++ b/tests/rest/client/test_power_levels.py @@ -19,14 +19,10 @@ # # from http import HTTPStatus -from typing import Optional - -from parameterized import parameterized_class from twisted.test.proto_helpers import MemoryReactor from synapse.api.errors import Codes -from synapse.api.room_versions import RoomVersions from synapse.events.utils import CANONICALJSON_MAX_INT, CANONICALJSON_MIN_INT from synapse.rest import admin from synapse.rest.client import login, room, sync @@ -36,10 +32,9 @@ from tests.unittest import HomeserverTestCase -class BasePowerLevelsTestCase(HomeserverTestCase): +class PowerLevelsTestCase(HomeserverTestCase): """Tests that power levels are enforced in various situations""" - room_version: Optional[str] = None servlets = [ admin.register_servlets, room.register_servlets, @@ -63,9 +58,7 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: # Create a room self.room_id = self.helper.create_room_as( - self.admin_user_id, - tok=self.admin_access_token, - room_version=self.room_version, + self.admin_user_id, tok=self.admin_access_token ) # Invite the other users @@ -107,8 +100,6 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: tok=self.admin_access_token, ) - -class PowerLevelsTestCase(BasePowerLevelsTestCase): def test_non_admins_cannot_enable_room_encryption(self) -> None: # have the mod try to enable room encryption self.helper.send_state( @@ -158,9 +149,7 @@ def test_non_admins_cannot_send_server_acl(self) -> None: def test_non_admins_cannot_tombstone_room(self) -> None: # Create another room that will serve as our "upgraded room" self.upgraded_room_id = self.helper.create_room_as( - self.admin_user_id, - tok=self.admin_access_token, - room_version=self.room_version, + self.admin_user_id, tok=self.admin_access_token ) # have the mod try to send a tombstone event @@ -214,9 +203,7 @@ def test_admins_can_send_server_acl(self) -> None: def test_admins_can_tombstone_room(self) -> None: # Create another room that will serve as our "upgraded room" self.upgraded_room_id = self.helper.create_room_as( - self.admin_user_id, - tok=self.admin_access_token, - room_version=self.room_version, + self.admin_user_id, tok=self.admin_access_token ) # have the admin try to send a tombstone event @@ -306,76 +293,3 @@ def test_cannot_set_unsafe_small_power_levels(self) -> None: Codes.BAD_JSON, body, ) - - -@parameterized_class( - ("room_version", "allows_owned_state"), - [ - (rv.identifier, rv.msc3757_enabled) - for rv in [ - RoomVersions.V11, - RoomVersions.MSC3757v11, - ] - ], -) -class MSC3757PowerLevelsTestCase(BasePowerLevelsTestCase): - _STATE_EVENT_TYPE = "org.matrix.msc3757.test" - - allows_owned_state: bool - - def test_can_set_own_state(self) -> None: - self.helper.send_state( - self.room_id, - self._STATE_EVENT_TYPE, - {}, - state_key=f"{self.mod_user_id}_suffix", - tok=self.mod_access_token, - expect_code=( - HTTPStatus.OK if self.allows_owned_state else HTTPStatus.FORBIDDEN - ), - ) - - def test_admins_can_set_others_state(self) -> None: - self.helper.send_state( - self.room_id, - self._STATE_EVENT_TYPE, - {}, - state_key=f"{self.mod_user_id}_suffix", - tok=self.admin_access_token, - expect_code=( - HTTPStatus.OK if self.allows_owned_state else HTTPStatus.FORBIDDEN - ), - ) - - def test_non_admins_cannot_set_others_state(self) -> None: - self.helper.send_state( - self.room_id, - self._STATE_EVENT_TYPE, - {}, - state_key=f"{self.admin_user_id}_suffix", - tok=self.mod_access_token, - expect_code=HTTPStatus.FORBIDDEN, - ) - - def test_cannot_set_state_with_non_user_id_key(self) -> None: - if self.allows_owned_state: - expect_code = HTTPStatus.BAD_REQUEST - errcode = Codes.BAD_JSON - else: - expect_code = HTTPStatus.FORBIDDEN - errcode = Codes.FORBIDDEN - - body = self.helper.send_state( - self.room_id, - self._STATE_EVENT_TYPE, - {}, - state_key=f"{self.admin_user_id}@suffix", - tok=self.admin_access_token, - expect_code=expect_code, - ) - - self.assertEqual( - body["errcode"], - errcode, - body, - ) From defeb6efb65b5c67d37dd4cdcd082994b4e42b4d Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Thu, 5 Sep 2024 15:33:30 -0400 Subject: [PATCH 18/28] Use one expression for commented allow conditions --- synapse/event_auth.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/synapse/event_auth.py b/synapse/event_auth.py index f17caafc7f3..e2050d5c1e0 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -825,11 +825,12 @@ def _can_send_event(event: "EventBase", auth_events: StateMap["EventBase"]) -> b "State key neither equals a valid user ID, nor starts with one plus an underscore", errcode=Codes.BAD_JSON, ) - if state_key_user_id == event.user_id: - # sender is owner of the state key, allow - return True - if user_level > get_user_power_level(state_key_user_id, auth_events): - # sender has higher PL than the owner of the state key, allow + if ( + # sender is owner of the state key + state_key_user_id == event.user_id + # sender has higher PL than the owner of the state key + or user_level > get_user_power_level(state_key_user_id, auth_events) + ): return True raise AuthError(403, "You are not allowed to set others state") From 426f52b61e5fec5d41e9845095366782dc92fb36 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 9 Sep 2024 10:10:44 -0400 Subject: [PATCH 19/28] Narrow caught error type --- synapse/event_auth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/event_auth.py b/synapse/event_auth.py index e2050d5c1e0..29b29b93601 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -819,7 +819,7 @@ def _can_send_event(event: "EventBase", auth_events: StateMap["EventBase"]) -> b ) if not UserID.is_valid(state_key_user_id): raise ValueError - except Exception: + except ValueError: raise SynapseError( 400, "State key neither equals a valid user ID, nor starts with one plus an underscore", From 8c203af53ef7c975deff9891429212e499accb43 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Wed, 11 Sep 2024 00:47:01 -0400 Subject: [PATCH 20/28] Remove license header from new test file --- tests/rest/client/test_owned_state.py | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/tests/rest/client/test_owned_state.py b/tests/rest/client/test_owned_state.py index f14746fcb2b..435a945e80c 100644 --- a/tests/rest/client/test_owned_state.py +++ b/tests/rest/client/test_owned_state.py @@ -1,16 +1,3 @@ -# -# This file is licensed under the Affero General Public License (AGPL) version 3. -# -# Copyright (C) 2024 New Vector, Ltd -# -# This program is free software: you can redistribute it and/or modify -# it under the terms of the GNU Affero General Public License as -# published by the Free Software Foundation, either version 3 of the -# License, or (at your option) any later version. -# -# See the GNU Affero General Public License for more details: -# . -# from http import HTTPStatus from parameterized import parameterized_class From 0d901f7c8ab5e3082b98a33a09eddd22ac64450e Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 16 Sep 2024 16:28:45 -0400 Subject: [PATCH 21/28] Run Complement tests Requires matrix-org/complement#733 --- scripts-dev/complement.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts-dev/complement.sh b/scripts-dev/complement.sh index 4ad547bc7e5..75c9d0725ad 100755 --- a/scripts-dev/complement.sh +++ b/scripts-dev/complement.sh @@ -220,6 +220,7 @@ test_packages=( ./tests/msc3874 ./tests/msc3890 ./tests/msc3391 + ./tests/msc3757 ./tests/msc3930 ./tests/msc3902 ./tests/msc3967 From b16fa102a6715fc1092ae98ec3aa82baf2de2e3b Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 23 Sep 2024 16:36:23 -0400 Subject: [PATCH 22/28] Make suffix test actually use suffix --- tests/rest/client/test_owned_state.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/rest/client/test_owned_state.py b/tests/rest/client/test_owned_state.py index 435a945e80c..27b691b6508 100644 --- a/tests/rest/client/test_owned_state.py +++ b/tests/rest/client/test_owned_state.py @@ -176,7 +176,7 @@ def test_user_cannot_set_state_with_other_suffixed_key(self) -> None: self.room_id, _STATE_EVENT_TEST_TYPE, {}, - state_key=f"{self.user2_user_id}", + state_key=f"{self.user2_user_id}{_STATE_KEY_SUFFIX}", tok=self.user1_access_token, expect_code=HTTPStatus.FORBIDDEN, ) From 77fc9d22686895fc01a46a1a870aeb719be32ddb Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 23 Sep 2024 16:36:57 -0400 Subject: [PATCH 23/28] Test suffixed key without separator --- tests/rest/client/test_owned_state.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/rest/client/test_owned_state.py b/tests/rest/client/test_owned_state.py index 27b691b6508..0f7b215ccbc 100644 --- a/tests/rest/client/test_owned_state.py +++ b/tests/rest/client/test_owned_state.py @@ -181,6 +181,16 @@ def test_user_cannot_set_state_with_other_suffixed_key(self) -> None: expect_code=HTTPStatus.FORBIDDEN, ) + def test_user_cannot_set_state_with_unseparated_suffixed_key(self) -> None: + self.helper.send_state( + self.room_id, + _STATE_EVENT_TEST_TYPE, + {}, + state_key=f"{self.user1_user_id}{_STATE_KEY_SUFFIX[1:]}", + tok=self.user1_access_token, + expect_code=HTTPStatus.FORBIDDEN, + ) + def test_admin_cannot_set_state_with_malformed_userid_key(self) -> None: body = self.helper.send_state( self.room_id, From c0dceafa158072d4bddbe60018b0da0c0f13508e Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 23 Sep 2024 16:44:24 -0400 Subject: [PATCH 24/28] Lock non-owned-state tests to non-MSC room version --- tests/rest/client/test_owned_state.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/rest/client/test_owned_state.py b/tests/rest/client/test_owned_state.py index 0f7b215ccbc..f740185f6bb 100644 --- a/tests/rest/client/test_owned_state.py +++ b/tests/rest/client/test_owned_state.py @@ -5,7 +5,7 @@ from twisted.test.proto_helpers import MemoryReactor from synapse.api.errors import Codes -from synapse.api.room_versions import KNOWN_ROOM_VERSIONS +from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersions from synapse.rest import admin from synapse.rest.client import login, room from synapse.server import HomeServer @@ -52,6 +52,11 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: class WithoutOwnedStateTestCase(OwnedStateBase): + def default_config(self) -> JsonDict: + config = super().default_config() + config["default_room_version"] = RoomVersions.V10.identifier + return config + def test_user_can_set_state_with_own_userid_key(self) -> None: self.helper.send_state( self.room_id, From e1fa3ef151a8d8e0e4b693ebc3135d32d41548af Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 23 Sep 2024 17:19:39 -0400 Subject: [PATCH 25/28] Test user ID in middle of state key --- tests/rest/client/test_owned_state.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/rest/client/test_owned_state.py b/tests/rest/client/test_owned_state.py index f740185f6bb..6f4da881aca 100644 --- a/tests/rest/client/test_owned_state.py +++ b/tests/rest/client/test_owned_state.py @@ -196,6 +196,17 @@ def test_user_cannot_set_state_with_unseparated_suffixed_key(self) -> None: expect_code=HTTPStatus.FORBIDDEN, ) + def test_user_cannot_set_state_with_misplaced_userid_in_key(self) -> None: + self.helper.send_state( + self.room_id, + _STATE_EVENT_TEST_TYPE, + {}, + # Still put @ at start of state key, because without it, there is no write protection at all + state_key=f"@prefix_{self.user1_user_id}{_STATE_KEY_SUFFIX}", + tok=self.user1_access_token, + expect_code=HTTPStatus.FORBIDDEN, + ) + def test_admin_cannot_set_state_with_malformed_userid_key(self) -> None: body = self.helper.send_state( self.room_id, From 7bcd7a054fc07cf697d6a97e15b0d0b7d13d79cc Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Wed, 25 Sep 2024 10:02:41 -0400 Subject: [PATCH 26/28] Rename admin to "room creator" to avoid confusion with a server administrator --- tests/rest/client/test_owned_state.py | 44 +++++++++++++-------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/tests/rest/client/test_owned_state.py b/tests/rest/client/test_owned_state.py index 6f4da881aca..1158e7bc3d4 100644 --- a/tests/rest/client/test_owned_state.py +++ b/tests/rest/client/test_owned_state.py @@ -28,14 +28,14 @@ class OwnedStateBase(HomeserverTestCase): ] def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: - self.admin_user_id = self.register_user("admin", "pass") - self.admin_access_token = self.login("admin", "pass") + self.creator_user_id = self.register_user("creator", "pass") + self.creator_access_token = self.login("creator", "pass") self.user1_user_id = self.register_user("user1", "pass") self.user1_access_token = self.login("user1", "pass") self.room_id = self.helper.create_room_as( - self.admin_user_id, - tok=self.admin_access_token, + self.creator_user_id, + tok=self.creator_access_token, is_public=True, extra_content={ "power_level_content_override": { @@ -67,43 +67,43 @@ def test_user_can_set_state_with_own_userid_key(self) -> None: expect_code=HTTPStatus.OK, ) - def test_admin_cannot_set_state_with_own_suffixed_key(self) -> None: + def test_room_creator_cannot_set_state_with_own_suffixed_key(self) -> None: self.helper.send_state( self.room_id, _STATE_EVENT_TEST_TYPE, {}, - state_key=f"{self.admin_user_id}{_STATE_KEY_SUFFIX}", - tok=self.admin_access_token, + state_key=f"{self.creator_user_id}{_STATE_KEY_SUFFIX}", + tok=self.creator_access_token, expect_code=HTTPStatus.FORBIDDEN, ) - def test_admin_cannot_set_state_with_other_userid_key(self) -> None: + def test_room_creator_cannot_set_state_with_other_userid_key(self) -> None: self.helper.send_state( self.room_id, _STATE_EVENT_TEST_TYPE, {}, state_key=f"{self.user1_user_id}", - tok=self.admin_access_token, + tok=self.creator_access_token, expect_code=HTTPStatus.FORBIDDEN, ) - def test_admin_cannot_set_state_with_other_suffixed_key(self) -> None: + def test_room_creator_cannot_set_state_with_other_suffixed_key(self) -> None: self.helper.send_state( self.room_id, _STATE_EVENT_TEST_TYPE, {}, state_key=f"{self.user1_user_id}{_STATE_KEY_SUFFIX}", - tok=self.admin_access_token, + tok=self.creator_access_token, expect_code=HTTPStatus.FORBIDDEN, ) - def test_admin_cannot_set_state_with_malformed_userid_key(self) -> None: + def test_room_creator_cannot_set_state_with_malformed_userid_key(self) -> None: body = self.helper.send_state( self.room_id, _STATE_EVENT_TEST_TYPE, {}, state_key="@oops", - tok=self.admin_access_token, + tok=self.creator_access_token, expect_code=HTTPStatus.FORBIDDEN, ) @@ -146,23 +146,23 @@ def test_user_can_set_state_with_own_suffixed_key(self) -> None: expect_code=HTTPStatus.OK, ) - def test_admin_can_set_state_with_other_userid_key(self) -> None: + def test_room_creator_can_set_state_with_other_userid_key(self) -> None: self.helper.send_state( self.room_id, _STATE_EVENT_TEST_TYPE, {}, state_key=f"{self.user1_user_id}", - tok=self.admin_access_token, + tok=self.creator_access_token, expect_code=HTTPStatus.OK, ) - def test_admin_can_set_state_with_other_suffixed_key(self) -> None: + def test_room_creator_can_set_state_with_other_suffixed_key(self) -> None: self.helper.send_state( self.room_id, _STATE_EVENT_TEST_TYPE, {}, state_key=f"{self.user1_user_id}{_STATE_KEY_SUFFIX}", - tok=self.admin_access_token, + tok=self.creator_access_token, expect_code=HTTPStatus.OK, ) @@ -207,13 +207,13 @@ def test_user_cannot_set_state_with_misplaced_userid_in_key(self) -> None: expect_code=HTTPStatus.FORBIDDEN, ) - def test_admin_cannot_set_state_with_malformed_userid_key(self) -> None: + def test_room_creator_cannot_set_state_with_malformed_userid_key(self) -> None: body = self.helper.send_state( self.room_id, _STATE_EVENT_TEST_TYPE, {}, state_key="@oops", - tok=self.admin_access_token, + tok=self.creator_access_token, expect_code=HTTPStatus.BAD_REQUEST, ) @@ -223,13 +223,13 @@ def test_admin_cannot_set_state_with_malformed_userid_key(self) -> None: body, ) - def test_admin_cannot_set_state_with_improperly_suffixed_key(self) -> None: + def test_room_creator_cannot_set_state_with_improperly_suffixed_key(self) -> None: body = self.helper.send_state( self.room_id, _STATE_EVENT_TEST_TYPE, {}, - state_key=f"{self.admin_user_id}@{_STATE_KEY_SUFFIX[1:]}", - tok=self.admin_access_token, + state_key=f"{self.creator_user_id}@{_STATE_KEY_SUFFIX[1:]}", + tok=self.creator_access_token, expect_code=HTTPStatus.BAD_REQUEST, ) From d28c17beca151cb593b80469037ce1096c4c2ee6 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Wed, 25 Sep 2024 10:09:36 -0400 Subject: [PATCH 27/28] Test state with key of a user ID not in the room --- tests/rest/client/test_owned_state.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/rest/client/test_owned_state.py b/tests/rest/client/test_owned_state.py index 1158e7bc3d4..e1022dd7a3b 100644 --- a/tests/rest/client/test_owned_state.py +++ b/tests/rest/client/test_owned_state.py @@ -97,6 +97,22 @@ def test_room_creator_cannot_set_state_with_other_suffixed_key(self) -> None: expect_code=HTTPStatus.FORBIDDEN, ) + def test_room_creator_cannot_set_state_with_nonmember_userid_key(self) -> None: + body = self.helper.send_state( + self.room_id, + _STATE_EVENT_TEST_TYPE, + {}, + state_key="@notinroom:hs2", + tok=self.creator_access_token, + expect_code=HTTPStatus.FORBIDDEN, + ) + + self.assertEqual( + body["errcode"], + Codes.FORBIDDEN, + body, + ) + def test_room_creator_cannot_set_state_with_malformed_userid_key(self) -> None: body = self.helper.send_state( self.room_id, @@ -207,6 +223,16 @@ def test_user_cannot_set_state_with_misplaced_userid_in_key(self) -> None: expect_code=HTTPStatus.FORBIDDEN, ) + def test_room_creator_can_set_state_with_nonmember_userid_key(self) -> None: + self.helper.send_state( + self.room_id, + _STATE_EVENT_TEST_TYPE, + {}, + state_key="@notinroom:hs2", + tok=self.creator_access_token, + expect_code=HTTPStatus.OK, + ) + def test_room_creator_cannot_set_state_with_malformed_userid_key(self) -> None: body = self.helper.send_state( self.room_id, From 792b0657d2862381bc50685b13edc3a108fbd0a5 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Wed, 25 Sep 2024 10:22:24 -0400 Subject: [PATCH 28/28] Always check the errcode in failure responses --- tests/rest/client/test_owned_state.py | 56 +++++++++++++++++++++++---- 1 file changed, 49 insertions(+), 7 deletions(-) diff --git a/tests/rest/client/test_owned_state.py b/tests/rest/client/test_owned_state.py index e1022dd7a3b..5fb57676762 100644 --- a/tests/rest/client/test_owned_state.py +++ b/tests/rest/client/test_owned_state.py @@ -68,7 +68,7 @@ def test_user_can_set_state_with_own_userid_key(self) -> None: ) def test_room_creator_cannot_set_state_with_own_suffixed_key(self) -> None: - self.helper.send_state( + body = self.helper.send_state( self.room_id, _STATE_EVENT_TEST_TYPE, {}, @@ -77,8 +77,14 @@ def test_room_creator_cannot_set_state_with_own_suffixed_key(self) -> None: expect_code=HTTPStatus.FORBIDDEN, ) + self.assertEqual( + body["errcode"], + Codes.FORBIDDEN, + body, + ) + def test_room_creator_cannot_set_state_with_other_userid_key(self) -> None: - self.helper.send_state( + body = self.helper.send_state( self.room_id, _STATE_EVENT_TEST_TYPE, {}, @@ -87,8 +93,14 @@ def test_room_creator_cannot_set_state_with_other_userid_key(self) -> None: expect_code=HTTPStatus.FORBIDDEN, ) + self.assertEqual( + body["errcode"], + Codes.FORBIDDEN, + body, + ) + def test_room_creator_cannot_set_state_with_other_suffixed_key(self) -> None: - self.helper.send_state( + body = self.helper.send_state( self.room_id, _STATE_EVENT_TEST_TYPE, {}, @@ -97,6 +109,12 @@ def test_room_creator_cannot_set_state_with_other_suffixed_key(self) -> None: expect_code=HTTPStatus.FORBIDDEN, ) + self.assertEqual( + body["errcode"], + Codes.FORBIDDEN, + body, + ) + def test_room_creator_cannot_set_state_with_nonmember_userid_key(self) -> None: body = self.helper.send_state( self.room_id, @@ -183,7 +201,7 @@ def test_room_creator_can_set_state_with_other_suffixed_key(self) -> None: ) def test_user_cannot_set_state_with_other_userid_key(self) -> None: - self.helper.send_state( + body = self.helper.send_state( self.room_id, _STATE_EVENT_TEST_TYPE, {}, @@ -192,8 +210,14 @@ def test_user_cannot_set_state_with_other_userid_key(self) -> None: expect_code=HTTPStatus.FORBIDDEN, ) + self.assertEqual( + body["errcode"], + Codes.FORBIDDEN, + body, + ) + def test_user_cannot_set_state_with_other_suffixed_key(self) -> None: - self.helper.send_state( + body = self.helper.send_state( self.room_id, _STATE_EVENT_TEST_TYPE, {}, @@ -202,8 +226,14 @@ def test_user_cannot_set_state_with_other_suffixed_key(self) -> None: expect_code=HTTPStatus.FORBIDDEN, ) + self.assertEqual( + body["errcode"], + Codes.FORBIDDEN, + body, + ) + def test_user_cannot_set_state_with_unseparated_suffixed_key(self) -> None: - self.helper.send_state( + body = self.helper.send_state( self.room_id, _STATE_EVENT_TEST_TYPE, {}, @@ -212,8 +242,14 @@ def test_user_cannot_set_state_with_unseparated_suffixed_key(self) -> None: expect_code=HTTPStatus.FORBIDDEN, ) + self.assertEqual( + body["errcode"], + Codes.FORBIDDEN, + body, + ) + def test_user_cannot_set_state_with_misplaced_userid_in_key(self) -> None: - self.helper.send_state( + body = self.helper.send_state( self.room_id, _STATE_EVENT_TEST_TYPE, {}, @@ -223,6 +259,12 @@ def test_user_cannot_set_state_with_misplaced_userid_in_key(self) -> None: expect_code=HTTPStatus.FORBIDDEN, ) + self.assertEqual( + body["errcode"], + Codes.FORBIDDEN, + body, + ) + def test_room_creator_can_set_state_with_nonmember_userid_key(self) -> None: self.helper.send_state( self.room_id,