Convert stringy power levels to integers on room upgrade (#12657)

This commit is contained in:
David Robertson 2022-05-07 13:37:29 +01:00 committed by GitHub
parent 4337d33a73
commit 051a1c3f22
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 137 additions and 24 deletions

1
changelog.d/12657.bugfix Normal file
View File

@ -0,0 +1 @@
Fix a long-standing bug where rooms containing power levels with string values could not be upgraded.

View File

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

View File

@ -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, "")]
)

View File

@ -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}}})

View File

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