diff options
| -rw-r--r-- | changelog.d/12657.bugfix | 1 | ||||
| -rw-r--r-- | synapse/events/utils.py | 61 | ||||
| -rw-r--r-- | synapse/handlers/room.py | 14 | ||||
| -rw-r--r-- | tests/events/test_utils.py | 41 | ||||
| -rw-r--r-- | tests/rest/client/test_upgrade_room.py | 44 |
5 files changed, 137 insertions, 24 deletions
diff --git a/changelog.d/12657.bugfix b/changelog.d/12657.bugfix new file mode 100644 index 0000000000..7547ca40a7 --- /dev/null +++ b/changelog.d/12657.bugfix @@ -0,0 +1 @@ +Fix a long-standing bug where rooms containing power levels with string values could not be upgraded. diff --git a/synapse/events/utils.py b/synapse/events/utils.py index 026dcde8d8..ac91c5eb57 100644 --- a/synapse/events/utils.py +++ b/synapse/events/utils.py @@ -22,6 +22,7 @@ from typing import ( Iterable, List, Mapping, + MutableMapping, Optional, Union, ) @@ -580,10 +581,20 @@ class EventClientSerializer: ] -def copy_power_levels_contents( - old_power_levels: Mapping[str, Union[int, Mapping[str, int]]] +_PowerLevel = Union[str, int] + + +def copy_and_fixup_power_levels_contents( + old_power_levels: Mapping[str, Union[_PowerLevel, Mapping[str, _PowerLevel]]] ) -> Dict[str, Union[int, Dict[str, int]]]: - """Copy the content of a power_levels event, unfreezing frozendicts along the way + """Copy the content of a power_levels event, unfreezing frozendicts along the way. + + We accept as input power level values which are strings, provided they represent an + integer, e.g. `"`100"` instead of 100. Such strings are converted to integers + in the returned dictionary (hence "fixup" in the function name). + + Note that future room versions will outlaw such stringy power levels (see + https://github.com/matrix-org/matrix-spec/issues/853). Raises: TypeError if the input does not look like a valid power levels event content @@ -592,29 +603,47 @@ def copy_power_levels_contents( raise TypeError("Not a valid power-levels content: %r" % (old_power_levels,)) power_levels: Dict[str, Union[int, Dict[str, int]]] = {} - for k, v in old_power_levels.items(): - - if isinstance(v, int): - power_levels[k] = v - continue + for k, v in old_power_levels.items(): if isinstance(v, collections.abc.Mapping): h: Dict[str, int] = {} power_levels[k] = h for k1, v1 in v.items(): - # we should only have one level of nesting - if not isinstance(v1, int): - raise TypeError( - "Invalid power_levels value for %s.%s: %r" % (k, k1, v1) - ) - h[k1] = v1 - continue + _copy_power_level_value_as_integer(v1, h, k1) - raise TypeError("Invalid power_levels value for %s: %r" % (k, v)) + else: + _copy_power_level_value_as_integer(v, power_levels, k) return power_levels +def _copy_power_level_value_as_integer( + old_value: object, + power_levels: MutableMapping[str, Any], + key: str, +) -> None: + """Set `power_levels[key]` to the integer represented by `old_value`. + + :raises TypeError: if `old_value` is not an integer, nor a base-10 string + representation of an integer. + """ + if isinstance(old_value, int): + power_levels[key] = old_value + return + + if isinstance(old_value, str): + try: + parsed_value = int(old_value, base=10) + except ValueError: + # Fall through to the final TypeError. + pass + else: + power_levels[key] = parsed_value + return + + raise TypeError(f"Invalid power_levels value for {key}: {old_value}") + + def validate_canonicaljson(value: Any) -> None: """ Ensure that the JSON object is valid according to the rules of canonical JSON. diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index b31f00b517..604eb6ec15 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -57,7 +57,7 @@ from synapse.api.filtering import Filter from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersion from synapse.event_auth import validate_event_for_room_version from synapse.events import EventBase -from synapse.events.utils import copy_power_levels_contents +from synapse.events.utils import copy_and_fixup_power_levels_contents from synapse.federation.federation_client import InvalidResponseError from synapse.handlers.federation import get_domains_from_state from synapse.handlers.relations import BundledAggregations @@ -337,13 +337,13 @@ class RoomCreationHandler: # 50, but if the default PL in a room is 50 or more, then we set the # required PL above that. - pl_content = dict(old_room_pl_state.content) - users_default = int(pl_content.get("users_default", 0)) + pl_content = copy_and_fixup_power_levels_contents(old_room_pl_state.content) + users_default: int = pl_content.get("users_default", 0) # type: ignore[assignment] restricted_level = max(users_default + 1, 50) updated = False for v in ("invite", "events_default"): - current = int(pl_content.get(v, 0)) + current: int = pl_content.get(v, 0) # type: ignore[assignment] if current < restricted_level: logger.debug( "Setting level for %s in %s to %i (was %i)", @@ -380,7 +380,9 @@ class RoomCreationHandler: "state_key": "", "room_id": new_room_id, "sender": requester.user.to_string(), - "content": old_room_pl_state.content, + "content": copy_and_fixup_power_levels_contents( + old_room_pl_state.content + ), }, ratelimit=False, ) @@ -471,7 +473,7 @@ class RoomCreationHandler: # dict so we can't just copy.deepcopy it. initial_state[ (EventTypes.PowerLevels, "") - ] = power_levels = copy_power_levels_contents( + ] = power_levels = copy_and_fixup_power_levels_contents( initial_state[(EventTypes.PowerLevels, "")] ) diff --git a/tests/events/test_utils.py b/tests/events/test_utils.py index 00ad19e446..b1c47efac7 100644 --- a/tests/events/test_utils.py +++ b/tests/events/test_utils.py @@ -17,7 +17,7 @@ from synapse.api.room_versions import RoomVersions from synapse.events import make_event_from_dict from synapse.events.utils import ( SerializeEventConfig, - copy_power_levels_contents, + copy_and_fixup_power_levels_contents, prune_event, serialize_event, ) @@ -529,7 +529,7 @@ class CopyPowerLevelsContentTestCase(unittest.TestCase): } def _test(self, input): - a = copy_power_levels_contents(input) + a = copy_and_fixup_power_levels_contents(input) self.assertEqual(a["ban"], 50) self.assertEqual(a["events"]["m.room.name"], 100) @@ -547,3 +547,40 @@ class CopyPowerLevelsContentTestCase(unittest.TestCase): def test_frozen(self): input = freeze(self.test_content) self._test(input) + + def test_stringy_integers(self): + """String representations of decimal integers are converted to integers.""" + input = { + "a": "100", + "b": { + "foo": 99, + "bar": "-98", + }, + "d": "0999", + } + output = copy_and_fixup_power_levels_contents(input) + expected_output = { + "a": 100, + "b": { + "foo": 99, + "bar": -98, + }, + "d": 999, + } + + self.assertEqual(output, expected_output) + + def test_strings_that_dont_represent_decimal_integers(self) -> None: + """Should raise when given inputs `s` for which `int(s, base=10)` raises.""" + for invalid_string in ["0x123", "123.0", "123.45", "hello", "0b1", "0o777"]: + with self.assertRaises(TypeError): + copy_and_fixup_power_levels_contents({"a": invalid_string}) + + def test_invalid_types_raise_type_error(self) -> None: + with self.assertRaises(TypeError): + copy_and_fixup_power_levels_contents({"a": ["hello", "grandma"]}) # type: ignore[arg-type] + copy_and_fixup_power_levels_contents({"a": None}) # type: ignore[arg-type] + + def test_invalid_nesting_raises_type_error(self) -> None: + with self.assertRaises(TypeError): + copy_and_fixup_power_levels_contents({"a": {"b": {"c": 1}}}) diff --git a/tests/rest/client/test_upgrade_room.py b/tests/rest/client/test_upgrade_room.py index b7d0f42daf..c86fc5df0d 100644 --- a/tests/rest/client/test_upgrade_room.py +++ b/tests/rest/client/test_upgrade_room.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. from typing import Optional +from unittest.mock import patch from twisted.test.proto_helpers import MemoryReactor @@ -167,6 +168,49 @@ class UpgradeRoomTest(unittest.HomeserverTestCase): ) self.assertNotIn(self.other, power_levels["users"]) + def test_stringy_power_levels(self) -> None: + """The room upgrade converts stringy power levels to proper integers.""" + # Retrieve the room's current power levels. + power_levels = self.helper.get_state( + self.room_id, + "m.room.power_levels", + tok=self.creator_token, + ) + + # Set creator's power level to the string "100" instead of the integer `100`. + power_levels["users"][self.creator] = "100" + + # Synapse refuses to accept new stringy power level events. Bypass this by + # neutering the validation. + with patch("synapse.events.validator.jsonschema.validate"): + # Note: https://github.com/matrix-org/matrix-spec/issues/853 plans to forbid + # string power levels in new rooms. For this test to have a clean + # conscience, we ought to ensure it's upgrading from a sufficiently old + # version of room. + self.helper.send_state( + self.room_id, + "m.room.power_levels", + body=power_levels, + tok=self.creator_token, + ) + + # Upgrade the room. Check the homeserver reports success. + channel = self._upgrade_room() + self.assertEqual(200, channel.code, channel.result) + + # Extract the new room ID. + new_room_id = channel.json_body["replacement_room"] + + # Fetch the new room's power level event. + new_power_levels = self.helper.get_state( + new_room_id, + "m.room.power_levels", + tok=self.creator_token, + ) + + # We should now have an integer power level. + self.assertEqual(new_power_levels["users"][self.creator], 100, new_power_levels) + def test_space(self) -> None: """Test upgrading a space.""" |