diff --git a/changelog.d/14942.bugfix b/changelog.d/14942.bugfix
new file mode 100644
index 0000000000..a3ca3eb7e9
--- /dev/null
+++ b/changelog.d/14942.bugfix
@@ -0,0 +1 @@
+Fix a bug introduced in Synapse 1.68.0 where we were unable to service remote joins in rooms with `@room` notification levels set to `null` in their (malformed) power levels.
diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py
index deaec19564..88cfc05d05 100644
--- a/synapse/push/bulk_push_rule_evaluator.py
+++ b/synapse/push/bulk_push_rule_evaluator.py
@@ -69,6 +69,9 @@ STATE_EVENT_TYPES_TO_MARK_UNREAD = {
}
+SENTINEL = object()
+
+
def _should_count_as_unread(event: EventBase, context: EventContext) -> bool:
# Exclude rejected and soft-failed events.
if context.rejected or event.internal_metadata.is_soft_failed():
@@ -343,11 +346,21 @@ class BulkPushRuleEvaluator:
related_events = await self._related_events(event)
# It's possible that old room versions have non-integer power levels (floats or
- # strings). Workaround this by explicitly converting to int.
+ # strings; even the occasional `null`). For old rooms, we interpret these as if
+ # they were integers. Do this here for the `@room` power level threshold.
+ # Note that this is done automatically for the sender's power level by
+ # _get_power_levels_and_sender_level in its call to get_user_power_level
+ # (even for room V10.)
notification_levels = power_levels.get("notifications", {})
if not event.room_version.msc3667_int_only_power_levels:
- for user_id, level in notification_levels.items():
- notification_levels[user_id] = int(level)
+ keys = list(notification_levels.keys())
+ for key in keys:
+ level = notification_levels.get(key, SENTINEL)
+ if level is not SENTINEL and type(level) is not int:
+ try:
+ notification_levels[key] = int(level)
+ except (TypeError, ValueError):
+ del notification_levels[key]
# Pull out any user and room mentions.
mentions = event.content.get(EventContentFields.MSC3952_MENTIONS)
diff --git a/tests/push/test_bulk_push_rule_evaluator.py b/tests/push/test_bulk_push_rule_evaluator.py
index aba62b5dc8..fda48d9f61 100644
--- a/tests/push/test_bulk_push_rule_evaluator.py
+++ b/tests/push/test_bulk_push_rule_evaluator.py
@@ -15,6 +15,8 @@
from typing import Any
from unittest.mock import patch
+from parameterized import parameterized
+
from twisted.test.proto_helpers import MemoryReactor
from synapse.api.constants import EventContentFields
@@ -48,35 +50,84 @@ class TestBulkPushRuleEvaluator(HomeserverTestCase):
self.requester = create_requester(self.alice)
self.room_id = self.helper.create_room_as(
- self.alice, room_version=RoomVersions.V9.identifier, tok=self.token
+ # This is deliberately set to V9, because we want to test the logic which
+ # handles stringy power levels. Stringy power levels were outlawed in V10.
+ self.alice,
+ room_version=RoomVersions.V9.identifier,
+ tok=self.token,
)
self.event_creation_handler = self.hs.get_event_creation_handler()
- def test_action_for_event_by_user_handles_noninteger_power_levels(self) -> None:
- """We should convert floats and strings to integers before passing to Rust.
+ @parameterized.expand(
+ [
+ # The historically-permitted bad values. Alice's notification should be
+ # allowed if this threshold is at or below her power level (60)
+ ("100", False),
+ ("0", True),
+ (12.34, True),
+ (60.0, True),
+ (67.89, False),
+ # Values that int(...) would not successfully cast should be ignored.
+ # The room notification level should then default to 50, per the spec, so
+ # Alice's notification is allowed.
+ (None, True),
+ # We haven't seen `"room": []` or `"room": {}` in the wild (yet), but
+ # let's check them for paranoia's sake.
+ ([], True),
+ ({}, True),
+ ]
+ )
+ def test_action_for_event_by_user_handles_noninteger_room_power_levels(
+ self, bad_room_level: object, should_permit: bool
+ ) -> None:
+ """We should convert strings in `room` to integers before passing to Rust.
+
+ Test this as follows:
+ - Create a room as Alice and invite two other users Bob and Charlie.
+ - Set PLs so that Alice has PL 60 and `notifications.room` is set to a bad value.
+ - Have Alice create a message notifying @room.
+ - Evaluate notification actions for that message. This should not raise.
+ - Look in the DB to see if that message triggered a highlight for Bob.
+
+ The test is parameterised with two arguments:
+ - the bad power level value for "room", before JSON serisalistion
+ - whether Bob should expect the message to be highlighted
Reproduces #14060.
A lack of validation: the gift that keeps on giving.
"""
-
- # Alter the power levels in that room to include stringy and floaty levels.
- # We need to suppress the validation logic or else it will reject these dodgy
- # values. (Presumably this validation was not always present.)
+ # Join another user to the room, so that there is someone to see Alice's
+ # @room notification.
+ bob = self.register_user("bob", "pass")
+ bob_token = self.login(bob, "pass")
+ self.helper.join(self.room_id, bob, tok=bob_token)
+
+ # Alter the power levels in that room to include the bad @room notification
+ # level. We need to suppress
+ #
+ # - canonicaljson validation, because canonicaljson forbids floats;
+ # - the event jsonschema validation, because it will forbid bad values; and
+ # - the auth rules checks, because they stop us from creating power levels
+ # with `"room": null`. (We want to test this case, because we have seen it
+ # in the wild.)
+ #
+ # We have seen stringy and null values for "room" in the wild, so presumably
+ # some of this validation was missing in the past.
with patch("synapse.events.validator.validate_canonicaljson"), patch(
"synapse.events.validator.jsonschema.validate"
- ):
- self.helper.send_state(
+ ), patch("synapse.handlers.event_auth.check_state_dependent_auth_rules"):
+ pl_event_id = self.helper.send_state(
self.room_id,
"m.room.power_levels",
{
- "users": {self.alice: "100"}, # stringy
- "notifications": {"room": 100.0}, # float
+ "users": {self.alice: 60},
+ "notifications": {"room": bad_room_level},
},
self.token,
state_key="",
- )
+ )["event_id"]
# Create a new message event, and try to evaluate it under the dodgy
# power level event.
@@ -88,10 +139,11 @@ class TestBulkPushRuleEvaluator(HomeserverTestCase):
"room_id": self.room_id,
"content": {
"msgtype": "m.text",
- "body": "helo",
+ "body": "helo @room",
},
"sender": self.alice,
},
+ prev_event_ids=[pl_event_id],
)
)
@@ -99,6 +151,21 @@ class TestBulkPushRuleEvaluator(HomeserverTestCase):
# should not raise
self.get_success(bulk_evaluator.action_for_events_by_user([(event, context)]))
+ # Did Bob see Alice's @room notification?
+ highlighted_actions = self.get_success(
+ self.hs.get_datastores().main.db_pool.simple_select_list(
+ table="event_push_actions_staging",
+ keyvalues={
+ "event_id": event.event_id,
+ "user_id": bob,
+ "highlight": 1,
+ },
+ retcols=("*",),
+ desc="get_event_push_actions_staging",
+ )
+ )
+ self.assertEqual(len(highlighted_actions), int(should_permit))
+
@override_config({"push": {"enabled": False}})
def test_action_for_event_by_user_disabled_by_config(self) -> None:
"""Ensure that push rules are not calculated when disabled in the config"""
diff --git a/tests/test_event_auth.py b/tests/test_event_auth.py
index f4d9fba0a1..0a7937f1cc 100644
--- a/tests/test_event_auth.py
+++ b/tests/test_event_auth.py
@@ -13,7 +13,7 @@
# limitations under the License.
import unittest
-from typing import Collection, Dict, Iterable, List, Optional
+from typing import Any, Collection, Dict, Iterable, List, Optional
from parameterized import parameterized
@@ -728,6 +728,36 @@ class EventAuthTestCase(unittest.TestCase):
pl_event.room_version, pl_event2, {("fake_type", "fake_key"): pl_event}
)
+ def test_room_v10_rejects_other_non_integer_power_levels(self) -> None:
+ """We should reject PLs that are non-integer, non-string JSON values.
+
+ test_room_v10_rejects_string_power_levels above handles the string case.
+ """
+
+ def create_event(pl_event_content: Dict[str, Any]) -> EventBase:
+ return make_event_from_dict(
+ {
+ "room_id": TEST_ROOM_ID,
+ **_maybe_get_event_id_dict_for_room_version(RoomVersions.V10),
+ "type": "m.room.power_levels",
+ "sender": "@test:test.com",
+ "state_key": "",
+ "content": pl_event_content,
+ "signatures": {"test.com": {"ed25519:0": "some9signature"}},
+ },
+ room_version=RoomVersions.V10,
+ )
+
+ contents: Iterable[Dict[str, Any]] = [
+ {"notifications": {"room": None}},
+ {"users": {"@alice:wonderland": []}},
+ {"users_default": {}},
+ ]
+ for content in contents:
+ event = create_event(content)
+ with self.assertRaises(SynapseError):
+ event_auth._check_power_levels(event.room_version, event, {})
+
# helpers for making events
TEST_DOMAIN = "example.com"
|
