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."""
|
