From b6e3c1071a84f22be49b8fb599700a671261647c Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Wed, 14 Jun 2023 15:19:56 +0200 Subject: [PATCH 1/7] push rules: fix internal conversion from _type to value --- synapse/push/clientformat.py | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/synapse/push/clientformat.py b/synapse/push/clientformat.py index 88b52c26a008..0f9ffad7e243 100644 --- a/synapse/push/clientformat.py +++ b/synapse/push/clientformat.py @@ -41,13 +41,6 @@ def format_push_rules_for_user( rulearray.append(template_rule) - for type_key in ("pattern", "value"): - type_value = template_rule.pop(f"{type_key}_type", None) - if type_value == "user_id": - template_rule[type_key] = user.to_string() - elif type_value == "user_localpart": - template_rule[type_key] = user.localpart - template_rule["enabled"] = enabled if "conditions" not in template_rule: @@ -63,15 +56,12 @@ def format_push_rules_for_user( for c in template_rule["conditions"]: c.pop("_cache_key", None) - pattern_type = c.pop("pattern_type", None) - if pattern_type == "user_id": - c["pattern"] = user.to_string() - elif pattern_type == "user_localpart": - c["pattern"] = user.localpart - - sender_type = c.pop("sender_type", None) - if sender_type == "user_id": - c["sender"] = user.to_string() + for type_key in ("pattern", "sender", "value"): + type_value = c.pop(f"{type_key}_type", None) + if type_value == "user_id": + c[type_key] = user.to_string() + elif type_value == "user_localpart": + c[type_key] = user.localpart return rules From e6830eddb90de4a3e463285d178d77250d88ba47 Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Wed, 14 Jun 2023 15:34:05 +0200 Subject: [PATCH 2/7] Add changelog --- changelog.d/15781.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/15781.bugfix diff --git a/changelog.d/15781.bugfix b/changelog.d/15781.bugfix new file mode 100644 index 000000000000..2de48b399ef3 --- /dev/null +++ b/changelog.d/15781.bugfix @@ -0,0 +1 @@ +Fix a bug in push rules handling leading to an invalid (per spec) `is_user_mention` rule sent to clients. From e8c1f4495fa5e55fac95f0cee211e55f266a03f9 Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Wed, 14 Jun 2023 16:32:58 +0200 Subject: [PATCH 3/7] Factor out function --- synapse/push/clientformat.py | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/synapse/push/clientformat.py b/synapse/push/clientformat.py index 0f9ffad7e243..735cef0aed76 100644 --- a/synapse/push/clientformat.py +++ b/synapse/push/clientformat.py @@ -41,6 +41,8 @@ def format_push_rules_for_user( rulearray.append(template_rule) + _convert_type_to_value(template_rule, user) + template_rule["enabled"] = enabled if "conditions" not in template_rule: @@ -56,16 +58,20 @@ def format_push_rules_for_user( for c in template_rule["conditions"]: c.pop("_cache_key", None) - for type_key in ("pattern", "sender", "value"): - type_value = c.pop(f"{type_key}_type", None) - if type_value == "user_id": - c[type_key] = user.to_string() - elif type_value == "user_localpart": - c[type_key] = user.localpart + _convert_type_to_value(c, user) return rules +def _convert_type_to_value(rule_or_cond: Dict[str, Any], user: UserID) -> None: + for type_key in ("pattern", "value"): + type_value = rule_or_cond.pop(f"{type_key}_type", None) + if type_value == "user_id": + rule_or_cond[type_key] = user.to_string() + elif type_value == "user_localpart": + rule_or_cond[type_key] = user.localpart + + def _add_empty_priority_class_arrays(d: Dict[str, list]) -> Dict[str, list]: for pc in PRIORITY_CLASS_MAP.keys(): d[pc] = [] From bdd9854f81fa5909b7ed59bed1bf30ef13c419c1 Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Wed, 14 Jun 2023 18:05:40 +0200 Subject: [PATCH 4/7] Add tests and fix rules names --- rust/src/push/base_rules.rs | 4 +-- tests/rest/client/test_push_rule_attrs.py | 38 +++++++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/rust/src/push/base_rules.rs b/rust/src/push/base_rules.rs index 9d6c304d9285..7eea9313f038 100644 --- a/rust/src/push/base_rules.rs +++ b/rust/src/push/base_rules.rs @@ -142,7 +142,7 @@ pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[ default_enabled: true, }, PushRule { - rule_id: Cow::Borrowed("global/override/.m.is_user_mention"), + rule_id: Cow::Borrowed("global/override/.m.rule.is_user_mention"), priority_class: 5, conditions: Cow::Borrowed(&[Condition::Known( KnownCondition::ExactEventPropertyContainsType(EventPropertyIsTypeCondition { @@ -163,7 +163,7 @@ pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[ default_enabled: true, }, PushRule { - rule_id: Cow::Borrowed("global/override/.m.is_room_mention"), + rule_id: Cow::Borrowed("global/override/.m.rule.is_room_mention"), priority_class: 5, conditions: Cow::Borrowed(&[ Condition::Known(KnownCondition::EventPropertyIs(EventPropertyIsCondition { diff --git a/tests/rest/client/test_push_rule_attrs.py b/tests/rest/client/test_push_rule_attrs.py index 4f875b92898e..851efad99278 100644 --- a/tests/rest/client/test_push_rule_attrs.py +++ b/tests/rest/client/test_push_rule_attrs.py @@ -412,3 +412,41 @@ def test_actions_404_when_put_non_existent_server_rule(self) -> None: ) self.assertEqual(channel.code, 404) self.assertEqual(channel.json_body["errcode"], Codes.NOT_FOUND) + + + def test_contains_user_name(self) -> None: + """ + Tests that `contains_user_name` rule is present and have proper value in `pattern`. + """ + username = "bob" + self.register_user(username, "pass") + token = self.login(username, "pass") + + channel = self.make_request( + "GET", + "/pushrules/global/content/.m.rule.contains_user_name", + access_token=token, + ) + + self.assertEqual(channel.code, 200) + self.assertIn("pattern", channel.json_body) + self.assertEqual(channel.json_body["pattern"], username) + + def test_is_user_mention(self) -> None: + """ + Tests that `is_user_mention` rule is present and have proper value in `value`. + """ + user = self.register_user("bob", "pass") + token = self.login("bob", "pass") + + channel = self.make_request( + "GET", + "/pushrules/global/override/.m.rule.is_user_mention", + access_token=token, + ) + + self.assertEqual(channel.code, 200) + self.assertIn("conditions", channel.json_body) + self.assertEqual(len(channel.json_body["conditions"]), 1) + self.assertIn("value", channel.json_body["conditions"][0]) + self.assertEqual(channel.json_body["conditions"][0]["value"], user) From 986ac839982903083d5f71e80f70bdb1c6ffe2ab Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Wed, 14 Jun 2023 18:13:19 +0200 Subject: [PATCH 5/7] Update changelog --- changelog.d/15781.bugfix | 2 +- tests/rest/client/test_push_rule_attrs.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/changelog.d/15781.bugfix b/changelog.d/15781.bugfix index 2de48b399ef3..5faf59afee0c 100644 --- a/changelog.d/15781.bugfix +++ b/changelog.d/15781.bugfix @@ -1 +1 @@ -Fix a bug in push rules handling leading to an invalid (per spec) `is_user_mention` rule sent to clients. +Fix a bug in push rules handling leading to an invalid (per spec) `is_user_mention` rule sent to clients. Also fix wrong rule names for `is_user_mention` and `is_room_mention`. \ No newline at end of file diff --git a/tests/rest/client/test_push_rule_attrs.py b/tests/rest/client/test_push_rule_attrs.py index 851efad99278..cfbd172b76b2 100644 --- a/tests/rest/client/test_push_rule_attrs.py +++ b/tests/rest/client/test_push_rule_attrs.py @@ -413,7 +413,6 @@ def test_actions_404_when_put_non_existent_server_rule(self) -> None: self.assertEqual(channel.code, 404) self.assertEqual(channel.json_body["errcode"], Codes.NOT_FOUND) - def test_contains_user_name(self) -> None: """ Tests that `contains_user_name` rule is present and have proper value in `pattern`. From f8c59adb171ea59240524ac939bb4c9b604d94a4 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 16 Jun 2023 11:43:47 +0100 Subject: [PATCH 6/7] Match against the full condition from the spec --- tests/rest/client/test_push_rule_attrs.py | 44 +++++++++++++++++++---- 1 file changed, 37 insertions(+), 7 deletions(-) diff --git a/tests/rest/client/test_push_rule_attrs.py b/tests/rest/client/test_push_rule_attrs.py index cfbd172b76b2..73fa573f69e6 100644 --- a/tests/rest/client/test_push_rule_attrs.py +++ b/tests/rest/client/test_push_rule_attrs.py @@ -428,14 +428,27 @@ def test_contains_user_name(self) -> None: ) self.assertEqual(channel.code, 200) - self.assertIn("pattern", channel.json_body) - self.assertEqual(channel.json_body["pattern"], username) + + self.assertEqual( + { + "rule_id": ".m.rule.contains_user_name", + "default": True, + "enabled": True, + "pattern": "bob", + "actions": [ + "notify", + {"set_tweak": "highlight"}, + {"set_tweak": "sound", "value": "default"}, + ], + }, + channel.json_body, + ) def test_is_user_mention(self) -> None: """ Tests that `is_user_mention` rule is present and have proper value in `value`. """ - user = self.register_user("bob", "pass") + self.register_user("bob", "pass") token = self.login("bob", "pass") channel = self.make_request( @@ -445,7 +458,24 @@ def test_is_user_mention(self) -> None: ) self.assertEqual(channel.code, 200) - self.assertIn("conditions", channel.json_body) - self.assertEqual(len(channel.json_body["conditions"]), 1) - self.assertIn("value", channel.json_body["conditions"][0]) - self.assertEqual(channel.json_body["conditions"][0]["value"], user) + + self.assertEqual( + { + "rule_id": ".m.rule.is_user_mention", + "default": True, + "enabled": True, + "conditions": [ + { + "kind": "event_property_contains", + "key": "content.m\\.mentions.user_ids", + "value": "@bob:test", + } + ], + "actions": [ + "notify", + {"set_tweak": "highlight"}, + {"set_tweak": "sound", "value": "default"}, + ], + }, + channel.json_body, + ) From 8e1cafc8e9abe9e740fc391cbdb77250b2a19df0 Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Fri, 16 Jun 2023 12:50:28 +0200 Subject: [PATCH 7/7] Use username var --- tests/rest/client/test_push_rule_attrs.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/rest/client/test_push_rule_attrs.py b/tests/rest/client/test_push_rule_attrs.py index 73fa573f69e6..5aca74475f6f 100644 --- a/tests/rest/client/test_push_rule_attrs.py +++ b/tests/rest/client/test_push_rule_attrs.py @@ -434,7 +434,7 @@ def test_contains_user_name(self) -> None: "rule_id": ".m.rule.contains_user_name", "default": True, "enabled": True, - "pattern": "bob", + "pattern": username, "actions": [ "notify", {"set_tweak": "highlight"}, @@ -448,7 +448,7 @@ def test_is_user_mention(self) -> None: """ Tests that `is_user_mention` rule is present and have proper value in `value`. """ - self.register_user("bob", "pass") + user = self.register_user("bob", "pass") token = self.login("bob", "pass") channel = self.make_request( @@ -468,7 +468,7 @@ def test_is_user_mention(self) -> None: { "kind": "event_property_contains", "key": "content.m\\.mentions.user_ids", - "value": "@bob:test", + "value": user, } ], "actions": [