From 8ef324ea6f1390876940989eacc8734fe0d15582 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 2 Mar 2023 08:30:51 -0500 Subject: [PATCH] Update intentional mentions (MSC3952) to depend on `exact_event_property_contains` (MSC3966). (#15051) This replaces the specific `is_user_mention` push rule condition used in MSC3952 with the generic `exact_event_property_contains` push rule condition from MSC3966. --- changelog.d/15051.misc | 1 + rust/benches/evaluator.rs | 4 -- rust/src/push/base_rules.rs | 9 +++- rust/src/push/evaluator.rs | 50 ++++++++++++--------- rust/src/push/mod.rs | 28 ++++++------ stubs/synapse/synapse_rust/push.pyi | 3 +- synapse/config/experimental.py | 8 +++- synapse/push/bulk_push_rule_evaluator.py | 18 ++------ synapse/push/clientformat.py | 11 ++--- tests/push/test_bulk_push_rule_evaluator.py | 2 + tests/push/test_push_rule_evaluator.py | 33 +------------- 11 files changed, 73 insertions(+), 94 deletions(-) create mode 100644 changelog.d/15051.misc diff --git a/changelog.d/15051.misc b/changelog.d/15051.misc new file mode 100644 index 0000000000..fabfe77d35 --- /dev/null +++ b/changelog.d/15051.misc @@ -0,0 +1 @@ +Update [MSC3952](https://github.com/matrix-org/matrix-spec-proposals/pull/3952) support based on changes to the MSC. diff --git a/rust/benches/evaluator.rs b/rust/benches/evaluator.rs index 9a871f5693..7c987d4948 100644 --- a/rust/benches/evaluator.rs +++ b/rust/benches/evaluator.rs @@ -44,7 +44,6 @@ fn bench_match_exact(b: &mut Bencher) { let eval = PushRuleEvaluator::py_new( flattened_keys, false, - BTreeSet::new(), 10, Some(0), Default::default(), @@ -92,7 +91,6 @@ fn bench_match_word(b: &mut Bencher) { let eval = PushRuleEvaluator::py_new( flattened_keys, false, - BTreeSet::new(), 10, Some(0), Default::default(), @@ -140,7 +138,6 @@ fn bench_match_word_miss(b: &mut Bencher) { let eval = PushRuleEvaluator::py_new( flattened_keys, false, - BTreeSet::new(), 10, Some(0), Default::default(), @@ -188,7 +185,6 @@ fn bench_eval_message(b: &mut Bencher) { let eval = PushRuleEvaluator::py_new( flattened_keys, false, - BTreeSet::new(), 10, Some(0), Default::default(), diff --git a/rust/src/push/base_rules.rs b/rust/src/push/base_rules.rs index 62de51d915..3d72a4a4c3 100644 --- a/rust/src/push/base_rules.rs +++ b/rust/src/push/base_rules.rs @@ -21,13 +21,13 @@ use lazy_static::lazy_static; use serde_json::Value; use super::KnownCondition; -use crate::push::PushRule; use crate::push::RelatedEventMatchTypeCondition; use crate::push::SetTweak; use crate::push::TweakValue; use crate::push::{Action, ExactEventMatchCondition, SimpleJsonValue}; use crate::push::{Condition, EventMatchTypeCondition}; use crate::push::{EventMatchCondition, EventMatchPatternType}; +use crate::push::{ExactEventMatchTypeCondition, PushRule}; const HIGHLIGHT_ACTION: Action = Action::SetTweak(SetTweak { set_tweak: Cow::Borrowed("highlight"), @@ -144,7 +144,12 @@ pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[ PushRule { rule_id: Cow::Borrowed(".org.matrix.msc3952.is_user_mention"), priority_class: 5, - conditions: Cow::Borrowed(&[Condition::Known(KnownCondition::IsUserMention)]), + conditions: Cow::Borrowed(&[Condition::Known( + KnownCondition::ExactEventPropertyContainsType(ExactEventMatchTypeCondition { + key: Cow::Borrowed("content.org.matrix.msc3952.mentions.user_ids"), + value_type: Cow::Borrowed(&EventMatchPatternType::UserId), + }), + )]), actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_ACTION, SOUND_ACTION]), default: true, default_enabled: true, diff --git a/rust/src/push/evaluator.rs b/rust/src/push/evaluator.rs index a65c645caf..55846627cc 100644 --- a/rust/src/push/evaluator.rs +++ b/rust/src/push/evaluator.rs @@ -13,7 +13,7 @@ // limitations under the License. use std::borrow::Cow; -use std::collections::{BTreeMap, BTreeSet}; +use std::collections::BTreeMap; use crate::push::{EventMatchPatternType, JsonValue}; use anyhow::{Context, Error}; @@ -72,8 +72,6 @@ pub struct PushRuleEvaluator { /// True if the event has a mentions property and MSC3952 support is enabled. has_mentions: bool, - /// The user mentions that were part of the message. - user_mentions: BTreeSet, /// The number of users in the room. room_member_count: u64, @@ -114,7 +112,6 @@ impl PushRuleEvaluator { pub fn py_new( flattened_keys: BTreeMap, has_mentions: bool, - user_mentions: BTreeSet, room_member_count: u64, sender_power_level: Option, notification_power_levels: BTreeMap, @@ -134,7 +131,6 @@ impl PushRuleEvaluator { flattened_keys, body, has_mentions, - user_mentions, room_member_count, notification_power_levels, sender_power_level, @@ -310,15 +306,30 @@ impl PushRuleEvaluator { Some(Cow::Borrowed(pattern)), )? } - KnownCondition::ExactEventPropertyContains(exact_event_match) => { - self.match_exact_event_property_contains(exact_event_match)? - } - KnownCondition::IsUserMention => { - if let Some(uid) = user_id { - self.user_mentions.contains(uid) + KnownCondition::ExactEventPropertyContains(exact_event_match) => self + .match_exact_event_property_contains( + exact_event_match.key.clone(), + exact_event_match.value.clone(), + )?, + KnownCondition::ExactEventPropertyContainsType(exact_event_match) => { + // The `pattern_type` can either be "user_id" or "user_localpart", + // either way if we don't have a `user_id` then the condition can't + // match. + let user_id = if let Some(user_id) = user_id { + user_id } else { - false - } + return Ok(false); + }; + + let pattern = match &*exact_event_match.value_type { + EventMatchPatternType::UserId => user_id, + EventMatchPatternType::UserLocalpart => get_localpart_from_id(user_id)?, + }; + + self.match_exact_event_property_contains( + exact_event_match.key.clone(), + Cow::Borrowed(&SimpleJsonValue::Str(pattern.to_string())), + )? } KnownCondition::ContainsDisplayName => { if let Some(dn) = display_name { @@ -456,24 +467,21 @@ impl PushRuleEvaluator { /// Evaluates a `exact_event_property_contains` condition. (MSC3758) fn match_exact_event_property_contains( &self, - exact_event_match: &ExactEventMatchCondition, + key: Cow, + value: Cow, ) -> Result { // First check if the feature is enabled. if !self.msc3966_exact_event_property_contains { return Ok(false); } - let value = &exact_event_match.value; - - let haystack = if let Some(JsonValue::Array(haystack)) = - self.flattened_keys.get(&*exact_event_match.key) - { + let haystack = if let Some(JsonValue::Array(haystack)) = self.flattened_keys.get(&*key) { haystack } else { return Ok(false); }; - Ok(haystack.contains(&**value)) + Ok(haystack.contains(&value)) } /// Match the member count against an 'is' condition @@ -510,7 +518,6 @@ fn push_rule_evaluator() { let evaluator = PushRuleEvaluator::py_new( flattened_keys, false, - BTreeSet::new(), 10, Some(0), BTreeMap::new(), @@ -542,7 +549,6 @@ fn test_requires_room_version_supports_condition() { let evaluator = PushRuleEvaluator::py_new( flattened_keys, false, - BTreeSet::new(), 10, Some(0), BTreeMap::new(), diff --git a/rust/src/push/mod.rs b/rust/src/push/mod.rs index 97feb6efc9..6391d2ed47 100644 --- a/rust/src/push/mod.rs +++ b/rust/src/push/mod.rs @@ -340,8 +340,12 @@ pub enum KnownCondition { RelatedEventMatchType(RelatedEventMatchTypeCondition), #[serde(rename = "org.matrix.msc3966.exact_event_property_contains")] ExactEventPropertyContains(ExactEventMatchCondition), - #[serde(rename = "org.matrix.msc3952.is_user_mention")] - IsUserMention, + // Identical to exact_event_property_contains but gives predefined patterns. Cannot be added by users. + #[serde( + skip_deserializing, + rename = "org.matrix.msc3966.exact_event_property_contains" + )] + ExactEventPropertyContainsType(ExactEventMatchTypeCondition), ContainsDisplayName, RoomMemberCount { #[serde(skip_serializing_if = "Option::is_none")] @@ -398,6 +402,15 @@ pub struct ExactEventMatchCondition { pub value: Cow<'static, SimpleJsonValue>, } +/// The body of a [`Condition::ExactEventMatch`] that uses user_id or user_localpart as a pattern. +#[derive(Serialize, Debug, Clone)] +pub struct ExactEventMatchTypeCondition { + pub key: Cow<'static, str>, + // During serialization, the pattern_type property gets replaced with a + // pattern property of the correct value in synapse.push.clientformat.format_push_rules_for_user. + pub value_type: Cow<'static, EventMatchPatternType>, +} + /// The body of a [`Condition::RelatedEventMatch`] #[derive(Serialize, Deserialize, Debug, Clone)] pub struct RelatedEventMatchCondition { @@ -739,17 +752,6 @@ fn test_deserialize_unstable_msc3758_condition() { )); } -#[test] -fn test_deserialize_unstable_msc3952_user_condition() { - let json = r#"{"kind":"org.matrix.msc3952.is_user_mention"}"#; - - let condition: Condition = serde_json::from_str(json).unwrap(); - assert!(matches!( - condition, - Condition::Known(KnownCondition::IsUserMention) - )); -} - #[test] fn test_deserialize_custom_condition() { let json = r#"{"kind":"custom_tag"}"#; diff --git a/stubs/synapse/synapse_rust/push.pyi b/stubs/synapse/synapse_rust/push.pyi index a8f0ed2435..c17796ffbd 100644 --- a/stubs/synapse/synapse_rust/push.pyi +++ b/stubs/synapse/synapse_rust/push.pyi @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -from typing import Any, Collection, Dict, Mapping, Optional, Sequence, Set, Tuple, Union +from typing import Any, Collection, Dict, Mapping, Optional, Sequence, Tuple, Union from synapse.types import JsonDict, JsonValue @@ -58,7 +58,6 @@ class PushRuleEvaluator: self, flattened_keys: Mapping[str, JsonValue], has_mentions: bool, - user_mentions: Set[str], room_member_count: int, sender_power_level: Optional[int], notification_power_levels: Mapping[str, int], diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index 7c81f055b6..fc64f2bda1 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -179,10 +179,16 @@ class ExperimentalConfig(Config): "msc3873_escape_event_match_key", False ) - # MSC3952: Intentional mentions, this depends on MSC3758. + # MSC3966: exact_event_property_contains push rule condition. + self.msc3966_exact_event_property_contains = experimental.get( + "msc3966_exact_event_property_contains", False + ) + + # MSC3952: Intentional mentions, this depends on MSC3758 and MSC3966. self.msc3952_intentional_mentions = ( experimental.get("msc3952_intentional_mentions", False) and self.msc3758_exact_event_match + and self.msc3966_exact_event_property_contains ) # MSC3959: Do not generate notifications for edits. diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index 3c4a152d6b..abcf687f05 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -23,7 +23,6 @@ from typing import ( Mapping, Optional, Sequence, - Set, Tuple, Union, ) @@ -396,18 +395,10 @@ class BulkPushRuleEvaluator: del notification_levels[key] # Pull out any user and room mentions. - mentions = event.content.get(EventContentFields.MSC3952_MENTIONS) - has_mentions = self._intentional_mentions_enabled and isinstance(mentions, dict) - user_mentions: Set[str] = set() - if has_mentions: - # mypy seems to have lost the type even though it must be a dict here. - assert isinstance(mentions, dict) - # Remove out any non-string items and convert to a set. - user_mentions_raw = mentions.get("user_ids") - if isinstance(user_mentions_raw, list): - user_mentions = set( - filter(lambda item: isinstance(item, str), user_mentions_raw) - ) + has_mentions = ( + self._intentional_mentions_enabled + and EventContentFields.MSC3952_MENTIONS in event.content + ) evaluator = PushRuleEvaluator( _flatten_dict( @@ -415,7 +406,6 @@ class BulkPushRuleEvaluator: msc3873_escape_event_match_key=self.hs.config.experimental.msc3873_escape_event_match_key, ), has_mentions, - user_mentions, room_member_count, sender_power_level, notification_levels, diff --git a/synapse/push/clientformat.py b/synapse/push/clientformat.py index bb76c169c6..222afbdcc8 100644 --- a/synapse/push/clientformat.py +++ b/synapse/push/clientformat.py @@ -41,11 +41,12 @@ def format_push_rules_for_user( rulearray.append(template_rule) - pattern_type = template_rule.pop("pattern_type", None) - if pattern_type == "user_id": - template_rule["pattern"] = user.to_string() - elif pattern_type == "user_localpart": - template_rule["pattern"] = user.localpart + 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 diff --git a/tests/push/test_bulk_push_rule_evaluator.py b/tests/push/test_bulk_push_rule_evaluator.py index 1458076a90..73fecfd4ad 100644 --- a/tests/push/test_bulk_push_rule_evaluator.py +++ b/tests/push/test_bulk_push_rule_evaluator.py @@ -233,6 +233,7 @@ class TestBulkPushRuleEvaluator(HomeserverTestCase): "experimental_features": { "msc3758_exact_event_match": True, "msc3952_intentional_mentions": True, + "msc3966_exact_event_property_contains": True, } } ) @@ -336,6 +337,7 @@ class TestBulkPushRuleEvaluator(HomeserverTestCase): "experimental_features": { "msc3758_exact_event_match": True, "msc3952_intentional_mentions": True, + "msc3966_exact_event_property_contains": True, } } ) diff --git a/tests/push/test_push_rule_evaluator.py b/tests/push/test_push_rule_evaluator.py index 1d30e3c3e4..d4a4bc4d93 100644 --- a/tests/push/test_push_rule_evaluator.py +++ b/tests/push/test_push_rule_evaluator.py @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -from typing import Any, Dict, List, Optional, Set, Union, cast +from typing import Any, Dict, List, Optional, Union, cast import frozendict @@ -147,8 +147,6 @@ class PushRuleEvaluatorTestCase(unittest.TestCase): self, content: JsonMapping, *, - has_mentions: bool = False, - user_mentions: Optional[Set[str]] = None, related_events: Optional[JsonDict] = None, ) -> PushRuleEvaluator: event = FrozenEvent( @@ -167,8 +165,7 @@ class PushRuleEvaluatorTestCase(unittest.TestCase): power_levels: Dict[str, Union[int, Dict[str, int]]] = {} return PushRuleEvaluator( _flatten_dict(event), - has_mentions, - user_mentions or set(), + False, room_member_count, sender_power_level, cast(Dict[str, int], power_levels.get("notifications", {})), @@ -204,32 +201,6 @@ class PushRuleEvaluatorTestCase(unittest.TestCase): # A display name with spaces should work fine. self.assertTrue(evaluator.matches(condition, "@user:test", "foo bar")) - def test_user_mentions(self) -> None: - """Check for user mentions.""" - condition = {"kind": "org.matrix.msc3952.is_user_mention"} - - # No mentions shouldn't match. - evaluator = self._get_evaluator({}, has_mentions=True) - self.assertFalse(evaluator.matches(condition, "@user:test", None)) - - # An empty set shouldn't match - evaluator = self._get_evaluator({}, has_mentions=True, user_mentions=set()) - self.assertFalse(evaluator.matches(condition, "@user:test", None)) - - # The Matrix ID appearing anywhere in the mentions list should match - evaluator = self._get_evaluator( - {}, has_mentions=True, user_mentions={"@user:test"} - ) - self.assertTrue(evaluator.matches(condition, "@user:test", None)) - - evaluator = self._get_evaluator( - {}, has_mentions=True, user_mentions={"@another:test", "@user:test"} - ) - self.assertTrue(evaluator.matches(condition, "@user:test", None)) - - # Note that invalid data is tested at tests.push.test_bulk_push_rule_evaluator.TestBulkPushRuleEvaluator.test_mentions - # since the BulkPushRuleEvaluator is what handles data sanitisation. - def _assert_matches( self, condition: JsonDict, content: JsonMapping, msg: Optional[str] = None ) -> None: