diff --git a/changelog.d/20220.removal b/changelog.d/20220.removal new file mode 100644 index 0000000000..245a1592ff --- /dev/null +++ b/changelog.d/20220.removal @@ -0,0 +1 @@ +Remove the legacy mention push rules by default, as per [MSC4210](https://github.com/matrix-org/matrix-spec-proposals/pull/4210). The `experimental_features.msc4210_enabled` flag now defaults to `true`. diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index c80b2740c3..ca67f1bf56 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -257,7 +257,8 @@ class ExperimentalConfig(Config): self.msc4169_enabled: bool = experimental.get("msc4169_enabled", False) # MSC4210: Remove legacy mentions - self.msc4210_enabled: bool = experimental.get("msc4210_enabled", False) + # TODO: Remove the flag and the legacy rules after 2026-12-01. + self.msc4210_enabled: bool = experimental.get("msc4210_enabled", True) # MSC4222: Adding `state_after` to sync v2 self.msc4222_enabled: bool = experimental.get("msc4222_enabled", False) diff --git a/tests/push/test_bulk_push_rule_evaluator.py b/tests/push/test_bulk_push_rule_evaluator.py index 55c38c010b..f4c4d2bdde 100644 --- a/tests/push/test_bulk_push_rule_evaluator.py +++ b/tests/push/test_bulk_push_rule_evaluator.py @@ -149,6 +149,7 @@ class TestBulkPushRuleEvaluator(HomeserverTestCase): "content": { "msgtype": "m.text", "body": "helo @room", + EventContentFields.MENTIONS: {"room": True}, }, "sender": self.alice, }, @@ -241,6 +242,44 @@ class TestBulkPushRuleEvaluator(HomeserverTestCase): ) return len(result) > 0 + def test_legacy_mention_rules_removed(self) -> None: + """ + Matrix v1.17 (MSC4210) removed the legacy mention rules from the base rule + set: an event which merely contains the user's name in its body, without + an `m.mentions` property, must no longer notify. + """ + bulk_evaluator = BulkPushRuleEvaluator(self.hs) + + # Note that the events are not `m.room.message`s, so that the generic + # underride rules do not fire and only the mention rules can notify. + self.assertFalse( + self._create_and_process(bulk_evaluator, {"body": "hello alice"}), + "alice should not be notified (legacy mention rules are removed)", + ) + self.assertTrue( + self._create_and_process( + bulk_evaluator, + { + "body": "hello alice", + EventContentFields.MENTIONS: {"user_ids": [self.alice]}, + }, + ), + "alice should be notified (intentional mention)", + ) + + @override_config({"experimental_features": {"msc4210_enabled": False}}) + def test_legacy_mention_rules_opt_out(self) -> None: + """ + The legacy mention rules can temporarily be restored, in which case an + event which contains the user's name in its body notifies again. + """ + bulk_evaluator = BulkPushRuleEvaluator(self.hs) + + self.assertTrue( + self._create_and_process(bulk_evaluator, {"body": "hello alice"}), + "alice should be notified (legacy mention)", + ) + def test_user_mentions(self) -> None: """Test the behavior of an event which includes invalid user mentions.""" bulk_evaluator = BulkPushRuleEvaluator(self.hs) @@ -593,6 +632,7 @@ class TestBulkPushRuleEvaluator(HomeserverTestCase): { "msgtype": "m.text", "body": "this is a message that mentions alice", + EventContentFields.MENTIONS: {"user_ids": [self.alice]}, }, type="m.room.message", ), diff --git a/tests/push/test_http.py b/tests/push/test_http.py index bd2ada9155..bf6d2ce084 100644 --- a/tests/push/test_http.py +++ b/tests/push/test_http.py @@ -25,6 +25,7 @@ from parameterized import parameterized from twisted.internet.defer import Deferred from twisted.internet.testing import MemoryReactor +from synapse.api.constants import EventContentFields from synapse.logging.context import make_deferred_yieldable from synapse.push import PusherConfig, PusherConfigException from synapse.rest import admin @@ -422,10 +423,10 @@ class HTTPPusherTests(HomeserverTestCase): # check that this is low-priority self.assertEqual(self.push_attempts[1][2]["notification"]["prio"], "low") - def test_sends_high_priority_for_mention(self) -> None: + def test_sends_high_priority_for_user_mention(self) -> None: """ The HTTP pusher will send pushes at high priority if they correspond - to a message containing the user's display name. + to a message that intentionally mentions the user via `m.mentions`. """ # Register the user who gets notified user_id = self.register_user("user", "pass") @@ -469,8 +470,18 @@ class HTTPPusherTests(HomeserverTestCase): ) ) - # Send a message - self.helper.send(room, body="Oh, user, hello!", tok=other_access_token) + # Send a message mentioning the user. As a client would, the body refers + # to the user as well, but it is `m.mentions` that makes it a mention. + self.helper.send_event( + room, + "m.room.message", + { + "msgtype": "m.text", + "body": "Oh, @user, hello!", + EventContentFields.MENTIONS: {"user_ids": [user_id]}, + }, + tok=other_access_token, + ) # Advance time a bit, so the pusher will register something has happened self.pump() @@ -547,10 +558,15 @@ class HTTPPusherTests(HomeserverTestCase): ) ) - # Send a message - self.helper.send( + # Send a message mentioning the room + self.helper.send_event( room, - body="@room eeek! There's a spider on the table!", + "m.room.message", + { + "msgtype": "m.text", + "body": "@room eeek! There's a spider on the table!", + EventContentFields.MENTIONS: {"room": True}, + }, tok=other_access_token, ) @@ -569,8 +585,15 @@ class HTTPPusherTests(HomeserverTestCase): self.assertEqual(self.push_attempts[0][2]["notification"]["prio"], "high") # Send another event, this time as someone without the power of @room - self.helper.send( - room, body="@room the spider is gone", tok=yet_another_access_token + self.helper.send_event( + room, + "m.room.message", + { + "msgtype": "m.text", + "body": "@room the spider is gone", + EventContentFields.MENTIONS: {"room": True}, + }, + tok=yet_another_access_token, ) # Advance time a bit, so the pusher will register something has happened diff --git a/tests/rest/client/test_push_rule_attrs.py b/tests/rest/client/test_push_rule_attrs.py index 687aec3b60..39179853af 100644 --- a/tests/rest/client/test_push_rule_attrs.py +++ b/tests/rest/client/test_push_rule_attrs.py @@ -520,9 +520,39 @@ class PushRuleAttributesTestCase(HomeserverTestCase): token, "global/override/.im.nheko.msc3664.reply" ) - def test_contains_user_name(self) -> None: + @parameterized.expand( + [ + ("global/override/.m.rule.contains_display_name",), + ("global/content/.m.rule.contains_user_name",), + ("global/override/.m.rule.roomnotif",), + ] + ) + def test_legacy_mention_rules_removed(self, rule_path: str) -> None: """ - Tests that `contains_user_name` rule is present and have proper value in `pattern`. + Tests that the legacy mention rules, which Matrix v1.17 (MSC4210) removed + from the base rule set, are not served by default. + """ + self.register_user("bob", "pass") + token = self.login("bob", "pass") + + channel = self.make_request( + "GET", f"/pushrules/{rule_path}", access_token=token + ) + self.assertEqual(channel.code, 404) + self.assertEqual(channel.json_body["errcode"], Codes.NOT_FOUND) + + _, kind, rule_id = rule_path.split("/") + channel = self.make_request("GET", "/pushrules/", access_token=token) + self.assertEqual(channel.code, 200) + self.assertNotIn( + rule_id, [rule["rule_id"] for rule in channel.json_body["global"][kind]] + ) + + @override_config({"experimental_features": {"msc4210_enabled": False}}) + def test_contains_user_name_with_legacy_mentions(self) -> None: + """ + Tests that, when the legacy mention rules are restored, the + `contains_user_name` rule is present and has the proper value in `pattern`. """ username = "bob" self.register_user(username, "pass") diff --git a/tests/storage/test_event_push_actions.py b/tests/storage/test_event_push_actions.py index 158426f83a..d6aaacb87a 100644 --- a/tests/storage/test_event_push_actions.py +++ b/tests/storage/test_event_push_actions.py @@ -22,7 +22,7 @@ from twisted.internet.testing import MemoryReactor -from synapse.api.constants import MAIN_TIMELINE, RelationTypes +from synapse.api.constants import MAIN_TIMELINE, EventContentFields, RelationTypes from synapse.rest import admin from synapse.rest.client import login, room from synapse.server import HomeServer @@ -84,6 +84,7 @@ class EventPushActionsStoreTestCase(HomeserverTestCase): content={ "msgtype": "m.text", "body": user_id, + EventContentFields.MENTIONS: {"user_ids": [user_id]}, "m.relates_to": { "rel_type": RelationTypes.THREAD, "event_id": first_event_id, @@ -194,7 +195,13 @@ class EventPushActionsStoreTestCase(HomeserverTestCase): result = self.helper.send_event( room_id, type="m.room.message", - content={"msgtype": "m.text", "body": user_id if highlight else "msg"}, + content={ + "msgtype": "m.text", + "body": "msg", + EventContentFields.MENTIONS: {"user_ids": [user_id]} + if highlight + else {}, + }, tok=other_token, ) nonlocal last_event_id @@ -388,7 +395,13 @@ class EventPushActionsStoreTestCase(HomeserverTestCase): return self.helper.send_event( room_id, type="m.room.message", - content={"msgtype": "m.text", "body": user_id if highlight else "msg"}, + content={ + "msgtype": "m.text", + "body": "msg", + EventContentFields.MENTIONS: {"user_ids": [user_id]} + if highlight + else {}, + }, tok=other_token, )["event_id"] @@ -435,7 +448,10 @@ class EventPushActionsStoreTestCase(HomeserverTestCase): def _send(thread_root: str | None = None, highlight: bool = False) -> str: content: JsonDict = { "msgtype": "m.text", - "body": user_id if highlight else "msg", + "body": "msg", + EventContentFields.MENTIONS: {"user_ids": [user_id]} + if highlight + else {}, } if thread_root is not None: content["m.relates_to"] = { @@ -609,7 +625,10 @@ class EventPushActionsStoreTestCase(HomeserverTestCase): def _create_event(highlight: bool = False, thread_id: str | None = None) -> str: content: JsonDict = { "msgtype": "m.text", - "body": user_id if highlight else "msg", + "body": "msg", + EventContentFields.MENTIONS: {"user_ids": [user_id]} + if highlight + else {}, } if thread_id: content["m.relates_to"] = { @@ -789,7 +808,10 @@ class EventPushActionsStoreTestCase(HomeserverTestCase): def _create_event(highlight: bool = False, thread_id: str | None = None) -> str: content: JsonDict = { "msgtype": "m.text", - "body": user_id if highlight else "msg", + "body": "msg", + EventContentFields.MENTIONS: {"user_ids": [user_id]} + if highlight + else {}, } if thread_id: content["m.relates_to"] = {