From bc3f8927bd69bc6879aee21ea4ca41d53f18758b Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 11 Jan 2023 10:11:40 -0500 Subject: [PATCH 1/5] Implement MSC3952: Intentional mentions --- changelog.d/14823.feature | 1 + rust/src/push/base_rules.rs | 16 ++++++ rust/src/push/evaluator.rs | 17 +++++- rust/src/push/mod.rs | 26 +++++++++ stubs/synapse/synapse_rust/push.pyi | 3 +- synapse/api/constants.py | 3 + synapse/push/bulk_push_rule_evaluator.py | 21 ++++++- tests/push/test_bulk_push_rule_evaluator.py | 61 ++++++++++++++++++++ tests/push/test_push_rule_evaluator.py | 63 ++++++++++++++++++--- 9 files changed, 201 insertions(+), 10 deletions(-) create mode 100644 changelog.d/14823.feature diff --git a/changelog.d/14823.feature b/changelog.d/14823.feature new file mode 100644 index 000000000000..8293e99effbc --- /dev/null +++ b/changelog.d/14823.feature @@ -0,0 +1 @@ +Experimental support for [MSC3952](https://github.com/matrix-org/matrix-spec-proposals/pull/3952): intentional mentions. diff --git a/rust/src/push/base_rules.rs b/rust/src/push/base_rules.rs index 9140a69bb654..d3bdb1d24ddf 100644 --- a/rust/src/push/base_rules.rs +++ b/rust/src/push/base_rules.rs @@ -131,6 +131,14 @@ pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[ default: true, default_enabled: true, }, + PushRule { + rule_id: Cow::Borrowed(".org.matrix.msc3952.is_user_mentioned"), + priority_class: 5, + conditions: Cow::Borrowed(&[Condition::Known(KnownCondition::IsUserMention)]), + actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_ACTION, SOUND_ACTION]), + default: true, + default_enabled: true, + }, PushRule { rule_id: Cow::Borrowed("global/override/.m.rule.contains_display_name"), priority_class: 5, @@ -139,6 +147,14 @@ pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[ default: true, default_enabled: true, }, + PushRule { + rule_id: Cow::Borrowed(".org.matrix.msc3952.is_room_mentioned"), + priority_class: 5, + conditions: Cow::Borrowed(&[Condition::Known(KnownCondition::IsRoomMention)]), + actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_ACTION, SOUND_ACTION]), + default: true, + default_enabled: true, + }, PushRule { rule_id: Cow::Borrowed("global/override/.m.rule.roomnotif"), priority_class: 5, diff --git a/rust/src/push/evaluator.rs b/rust/src/push/evaluator.rs index 0242ee1c5ffc..4af402eb6a9e 100644 --- a/rust/src/push/evaluator.rs +++ b/rust/src/push/evaluator.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; use anyhow::{Context, Error}; use lazy_static::lazy_static; @@ -68,6 +68,9 @@ pub struct PushRuleEvaluator { /// The "content.body", if any. body: String, + /// The mentions that were part of the message, note this has + mentions: BTreeSet, + /// The number of users in the room. room_member_count: u64, @@ -100,6 +103,7 @@ impl PushRuleEvaluator { #[new] pub fn py_new( flattened_keys: BTreeMap, + mentions: BTreeSet, room_member_count: u64, sender_power_level: Option, notification_power_levels: BTreeMap, @@ -116,6 +120,7 @@ impl PushRuleEvaluator { Ok(PushRuleEvaluator { flattened_keys, body, + mentions, room_member_count, notification_power_levels, sender_power_level, @@ -229,6 +234,14 @@ impl PushRuleEvaluator { KnownCondition::RelatedEventMatch(event_match) => { self.match_related_event_match(event_match, user_id)? } + KnownCondition::IsUserMention => { + if let Some(uid) = user_id { + self.mentions.contains(uid) + } else { + false + } + } + KnownCondition::IsRoomMention => self.mentions.contains("@room"), KnownCondition::ContainsDisplayName => { if let Some(dn) = display_name { if !dn.is_empty() { @@ -424,6 +437,7 @@ fn push_rule_evaluator() { flattened_keys.insert("content.body".to_string(), "foo bar bob hello".to_string()); let evaluator = PushRuleEvaluator::py_new( flattened_keys, + BTreeSet::new(), 10, Some(0), BTreeMap::new(), @@ -449,6 +463,7 @@ fn test_requires_room_version_supports_condition() { let flags = vec![RoomVersionFeatures::ExtensibleEvents.as_str().to_string()]; let evaluator = PushRuleEvaluator::py_new( flattened_keys, + BTreeSet::new(), 10, Some(0), BTreeMap::new(), diff --git a/rust/src/push/mod.rs b/rust/src/push/mod.rs index 842b13c88b2c..8e5cfed2187d 100644 --- a/rust/src/push/mod.rs +++ b/rust/src/push/mod.rs @@ -269,6 +269,10 @@ pub enum KnownCondition { EventMatch(EventMatchCondition), #[serde(rename = "im.nheko.msc3664.related_event_match")] RelatedEventMatch(RelatedEventMatchCondition), + #[serde(rename = "org.matrix.msc3952.is_user_mention")] + IsUserMention, + #[serde(rename = "org.matrix.msc3952.is_room_mention")] + IsRoomMention, ContainsDisplayName, RoomMemberCount { #[serde(skip_serializing_if = "Option::is_none")] @@ -522,6 +526,28 @@ fn test_deserialize_unstable_msc3931_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_unstable_msc3952_room_condition() { + let json = r#"{"kind":"org.matrix.msc3952.is_room_mention"}"#; + + let condition: Condition = serde_json::from_str(json).unwrap(); + assert!(matches!( + condition, + Condition::Known(KnownCondition::IsRoomMention) + )); +} + #[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 304ed7111c20..6496db378a82 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, Tuple, Union +from typing import Any, Collection, Dict, Mapping, Optional, Sequence, Set, Tuple, Union from synapse.types import JsonDict @@ -55,6 +55,7 @@ class PushRuleEvaluator: def __init__( self, flattened_keys: Mapping[str, str], + mentions: Set[str], room_member_count: int, sender_power_level: Optional[int], notification_power_levels: Mapping[str, int], diff --git a/synapse/api/constants.py b/synapse/api/constants.py index 6432d32d8376..6776e2c1ff5b 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -231,6 +231,9 @@ class EventContentFields: # The authorising user for joining a restricted room. AUTHORISING_USER: Final = "join_authorised_via_users_server" + # Use for mentioning users. + MSC3952_MENTIONS: Final = "mentions" + # an unspecced field added to to-device messages to identify them uniquely-ish TO_DEVICE_MSGID: Final = "org.matrix.msgid" diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index f27ba64d5365..6bd3db3e860b 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -28,7 +28,13 @@ from prometheus_client import Counter -from synapse.api.constants import MAIN_TIMELINE, EventTypes, Membership, RelationTypes +from synapse.api.constants import ( + MAIN_TIMELINE, + EventContentFields, + EventTypes, + Membership, + RelationTypes, +) from synapse.api.room_versions import PushRuleRoomFlag, RoomVersion from synapse.event_auth import auth_types_for_event, get_user_power_level from synapse.events import EventBase, relation_from_event @@ -342,8 +348,21 @@ async def _action_for_event_by_user( for user_id, level in notification_levels.items(): notification_levels[user_id] = int(level) + # Pull out the mentions field if it exists and trim the values to things + # that might be valid. + mentions_raw = event.content.get(EventContentFields.MSC3952_MENTIONS) + if isinstance(mentions_raw, list): + # Take the first 10 items, then strip out any non-string ones and convert + # to a tuple. + mentions = set( + filter(lambda item: isinstance(item, str), mentions_raw[:10]) + ) + else: + mentions = set() + evaluator = PushRuleEvaluator( _flatten_dict(event, room_version=event.room_version), + mentions, room_member_count, sender_power_level, notification_levels, diff --git a/tests/push/test_bulk_push_rule_evaluator.py b/tests/push/test_bulk_push_rule_evaluator.py index 9c17a42b650f..4e029de95fab 100644 --- a/tests/push/test_bulk_push_rule_evaluator.py +++ b/tests/push/test_bulk_push_rule_evaluator.py @@ -12,10 +12,12 @@ # See the License for the specific language governing permissions and # limitations under the License. +from typing import Any from unittest.mock import patch from twisted.test.proto_helpers import MemoryReactor +from synapse.api.constants import EventContentFields from synapse.api.room_versions import RoomVersions from synapse.push.bulk_push_rule_evaluator import BulkPushRuleEvaluator from synapse.rest import admin @@ -126,3 +128,62 @@ def test_action_for_event_by_user_disabled_by_config(self) -> None: # Ensure no actions are generated! self.get_success(bulk_evaluator.action_for_events_by_user([(event, context)])) bulk_evaluator._action_for_event_by_user.assert_not_called() + + def test_mentions(self) -> None: + """Test the behavior of an event which includes invalid mentions.""" + bulk_evaluator = BulkPushRuleEvaluator(self.hs) + + sentinel = object() + + def create_and_process(mentions: Any = sentinel) -> bool: + content = {} + if mentions is not sentinel: + content[EventContentFields.MSC3952_MENTIONS] = mentions + + # Create a new message event which should cause a notification. + event, context = self.get_success( + self.event_creation_handler.create_event( + self.requester, + { + "type": "test", + "room_id": self.room_id, + "content": content, + "sender": f"@bob:{self.hs.hostname}", + }, + ) + ) + + # Ensure no actions are generated! + self.get_success( + bulk_evaluator.action_for_events_by_user([(event, context)]) + ) + + # If any actions are generated for this event, return true. + result = self.get_success( + self.hs.get_datastores().main.db_pool.simple_select_list( + table="event_push_actions_staging", + keyvalues={"event_id": event.event_id}, + retcols=("*",), + desc="get_event_push_actions_staging", + ) + ) + return len(result) > 0 + + # Not including the mentions field should result in no notifications. + self.assertFalse(create_and_process()) + + # Invalid data should be ignored. + mentions: Any + for mentions in (None, True, False, "foo", {}): + self.assertFalse(create_and_process(mentions)) + + # The Matrix ID appearing anywhere in the mentions list should match + self.assertTrue(create_and_process([self.alice])) + self.assertTrue(create_and_process(["@another:test", self.alice])) + + # The Matrix ID appearing > 10 entries into the list should be ignored. + self.assertFalse(create_and_process(["@another:test"] * 10 + [self.alice])) + + # Invalid entries in the list are ignored, but count towards the limit. + self.assertTrue(create_and_process([None, True, False, {}, [], self.alice])) + self.assertFalse(create_and_process([None] * 10 + [self.alice])) diff --git a/tests/push/test_push_rule_evaluator.py b/tests/push/test_push_rule_evaluator.py index 1b87756b751a..0441dd76a796 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 Dict, List, Optional, Union, cast +from typing import Dict, List, Optional, Set, Union, cast import frozendict @@ -39,7 +39,11 @@ class PushRuleEvaluatorTestCase(unittest.TestCase): def _get_evaluator( - self, content: JsonMapping, related_events: Optional[JsonDict] = None + self, + content: JsonMapping, + *, + mentions: Optional[Set[str]] = None, + related_events: Optional[JsonDict] = None, ) -> PushRuleEvaluator: event = FrozenEvent( { @@ -57,13 +61,14 @@ def _get_evaluator( power_levels: Dict[str, Union[int, Dict[str, int]]] = {} return PushRuleEvaluator( _flatten_dict(event), + mentions or set(), room_member_count, sender_power_level, cast(Dict[str, int], power_levels.get("notifications", {})), {} if related_events is None else related_events, - True, - event.room_version.msc3931_push_features, - True, + related_event_match_enabled=True, + room_version_feature_flags=event.room_version.msc3931_push_features, + msc3931_enabled=True, ) def test_display_name(self) -> None: @@ -90,6 +95,50 @@ def test_display_name(self) -> None: # 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({}) + self.assertFalse(evaluator.matches(condition, "@user:test", None)) + + # An empty set shouldn't match + evaluator = self._get_evaluator({}, 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({}, mentions={"@user:test"}) + self.assertTrue(evaluator.matches(condition, "@user:test", None)) + + evaluator = self._get_evaluator({}, 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 test_room_mentions(self) -> None: + """Check for room mentions.""" + condition = {"kind": "org.matrix.msc3952.is_room_mention"} + + # No mentions shouldn't match. + evaluator = self._get_evaluator({}) + self.assertFalse(evaluator.matches(condition, None, None)) + + # An empty set shouldn't match + evaluator = self._get_evaluator({}, mentions=set()) + self.assertFalse(evaluator.matches(condition, None, None)) + + # The @room appearing anywhere in the mentions list should match + evaluator = self._get_evaluator({}, mentions={"@room"}) + self.assertTrue(evaluator.matches(condition, None, None)) + + evaluator = self._get_evaluator({}, mentions={"@another:test", "@room"}) + self.assertTrue(evaluator.matches(condition, None, 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: @@ -308,7 +357,7 @@ def test_related_event_match(self) -> None: }, } }, - { + related_events={ "m.in_reply_to": { "event_id": "$parent_event_id", "type": "m.room.message", @@ -408,7 +457,7 @@ def test_related_event_match_with_fallback(self) -> None: }, } }, - { + related_events={ "m.in_reply_to": { "event_id": "$parent_event_id", "type": "m.room.message", From 3c88356404c396983b318baf54ca3e62a4226876 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 12 Jan 2023 13:23:23 -0500 Subject: [PATCH 2/5] Add an experimental config option. --- rust/src/push/evaluator.rs | 2 +- rust/src/push/mod.rs | 8 ++++++++ stubs/synapse/synapse_rust/push.pyi | 1 + synapse/config/experimental.py | 5 +++++ synapse/storage/databases/main/push_rule.py | 1 + tests/push/test_bulk_push_rule_evaluator.py | 1 + 6 files changed, 17 insertions(+), 1 deletion(-) diff --git a/rust/src/push/evaluator.rs b/rust/src/push/evaluator.rs index 4af402eb6a9e..133958c330f6 100644 --- a/rust/src/push/evaluator.rs +++ b/rust/src/push/evaluator.rs @@ -498,7 +498,7 @@ fn test_requires_room_version_supports_condition() { }; let rules = PushRules::new(vec![custom_rule]); result = evaluator.run( - &FilteredPushRules::py_new(rules, BTreeMap::new(), true, false, true), + &FilteredPushRules::py_new(rules, BTreeMap::new(), true, false, true, false), None, None, ); diff --git a/rust/src/push/mod.rs b/rust/src/push/mod.rs index 8e5cfed2187d..7e449f243371 100644 --- a/rust/src/push/mod.rs +++ b/rust/src/push/mod.rs @@ -418,6 +418,7 @@ pub struct FilteredPushRules { msc1767_enabled: bool, msc3381_polls_enabled: bool, msc3664_enabled: bool, + msc3952_intentional_mentions: bool, } #[pymethods] @@ -429,6 +430,7 @@ impl FilteredPushRules { msc1767_enabled: bool, msc3381_polls_enabled: bool, msc3664_enabled: bool, + msc3952_intentional_mentions: bool, ) -> Self { Self { push_rules, @@ -436,6 +438,7 @@ impl FilteredPushRules { msc1767_enabled, msc3381_polls_enabled, msc3664_enabled, + msc3952_intentional_mentions, } } @@ -469,6 +472,11 @@ impl FilteredPushRules { return false; } + if !self.msc3952_intentional_mentions && rule.rule_id.contains("org.matrix.msc3952") + { + return false; + } + true }) .map(|r| { diff --git a/stubs/synapse/synapse_rust/push.pyi b/stubs/synapse/synapse_rust/push.pyi index 6496db378a82..2770256f0712 100644 --- a/stubs/synapse/synapse_rust/push.pyi +++ b/stubs/synapse/synapse_rust/push.pyi @@ -46,6 +46,7 @@ class FilteredPushRules: msc1767_enabled: bool, msc3381_polls_enabled: bool, msc3664_enabled: bool, + msc3952_intentional_mentions: bool, ): ... def rules(self) -> Collection[Tuple[PushRule, bool]]: ... diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index 89586db76313..e22dd345467d 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -168,3 +168,8 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: # MSC3925: do not replace events with their edits self.msc3925_inhibit_edit = experimental.get("msc3925_inhibit_edit", False) + + # MSC3952: Intentional mentions + self.msc3952_intentional_mentions = experimental.get( + "msc3952_intentional_mentions", False + ) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index 14ca167b34b4..466a1145b78c 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -89,6 +89,7 @@ def _load_rules( msc1767_enabled=experimental_config.msc1767_enabled, msc3664_enabled=experimental_config.msc3664_enabled, msc3381_polls_enabled=experimental_config.msc3381_polls_enabled, + msc3952_intentional_mentions=experimental_config.msc3952_intentional_mentions, ) return filtered_rules diff --git a/tests/push/test_bulk_push_rule_evaluator.py b/tests/push/test_bulk_push_rule_evaluator.py index 4e029de95fab..0d343a45fcd8 100644 --- a/tests/push/test_bulk_push_rule_evaluator.py +++ b/tests/push/test_bulk_push_rule_evaluator.py @@ -129,6 +129,7 @@ def test_action_for_event_by_user_disabled_by_config(self) -> None: self.get_success(bulk_evaluator.action_for_events_by_user([(event, context)])) bulk_evaluator._action_for_event_by_user.assert_not_called() + @override_config({"experimental_features": {"msc3952_intentional_mentions": True}}) def test_mentions(self) -> None: """Test the behavior of an event which includes invalid mentions.""" bulk_evaluator = BulkPushRuleEvaluator(self.hs) From 34a187e9323c61dcec1597c6935410bb5050b370 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 24 Jan 2023 13:50:54 -0500 Subject: [PATCH 3/5] Update for changes to the MSC. --- rust/src/push/evaluator.rs | 18 +++++++---- stubs/synapse/synapse_rust/push.pyi | 3 +- synapse/api/constants.py | 2 +- synapse/push/bulk_push_rule_evaluator.py | 28 +++++++++------- tests/push/test_bulk_push_rule_evaluator.py | 36 ++++++++++++++------- tests/push/test_push_rule_evaluator.py | 29 +++++++++-------- 6 files changed, 72 insertions(+), 44 deletions(-) diff --git a/rust/src/push/evaluator.rs b/rust/src/push/evaluator.rs index 133958c330f6..aa71202e4379 100644 --- a/rust/src/push/evaluator.rs +++ b/rust/src/push/evaluator.rs @@ -68,8 +68,10 @@ pub struct PushRuleEvaluator { /// The "content.body", if any. body: String, - /// The mentions that were part of the message, note this has - mentions: BTreeSet, + /// The user mentions that were part of the message. + user_mentions: BTreeSet, + /// True if the message is a room message. + room_mention: bool, /// The number of users in the room. room_member_count: u64, @@ -103,7 +105,8 @@ impl PushRuleEvaluator { #[new] pub fn py_new( flattened_keys: BTreeMap, - mentions: BTreeSet, + user_mentions: BTreeSet, + room_mention: bool, room_member_count: u64, sender_power_level: Option, notification_power_levels: BTreeMap, @@ -120,7 +123,8 @@ impl PushRuleEvaluator { Ok(PushRuleEvaluator { flattened_keys, body, - mentions, + user_mentions, + room_mention, room_member_count, notification_power_levels, sender_power_level, @@ -236,12 +240,12 @@ impl PushRuleEvaluator { } KnownCondition::IsUserMention => { if let Some(uid) = user_id { - self.mentions.contains(uid) + self.user_mentions.contains(uid) } else { false } } - KnownCondition::IsRoomMention => self.mentions.contains("@room"), + KnownCondition::IsRoomMention => self.room_mention, KnownCondition::ContainsDisplayName => { if let Some(dn) = display_name { if !dn.is_empty() { @@ -438,6 +442,7 @@ fn push_rule_evaluator() { let evaluator = PushRuleEvaluator::py_new( flattened_keys, BTreeSet::new(), + false, 10, Some(0), BTreeMap::new(), @@ -464,6 +469,7 @@ fn test_requires_room_version_supports_condition() { let evaluator = PushRuleEvaluator::py_new( flattened_keys, BTreeSet::new(), + false, 10, Some(0), BTreeMap::new(), diff --git a/stubs/synapse/synapse_rust/push.pyi b/stubs/synapse/synapse_rust/push.pyi index 2770256f0712..588d90c25a82 100644 --- a/stubs/synapse/synapse_rust/push.pyi +++ b/stubs/synapse/synapse_rust/push.pyi @@ -56,7 +56,8 @@ class PushRuleEvaluator: def __init__( self, flattened_keys: Mapping[str, str], - mentions: Set[str], + user_mentions: Set[str], + room_mention: bool, room_member_count: int, sender_power_level: Optional[int], notification_power_levels: Mapping[str, int], diff --git a/synapse/api/constants.py b/synapse/api/constants.py index 6776e2c1ff5b..9fa6be7ec3a3 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -232,7 +232,7 @@ class EventContentFields: AUTHORISING_USER: Final = "join_authorised_via_users_server" # Use for mentioning users. - MSC3952_MENTIONS: Final = "mentions" + MSC3952_MENTIONS: Final = "org.matrix.msc3952.mentions" # an unspecced field added to to-device messages to identify them uniquely-ish TO_DEVICE_MSGID: Final = "org.matrix.msgid" diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index 6bd3db3e860b..deaec1956472 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -22,6 +22,7 @@ List, Mapping, Optional, + Set, Tuple, Union, ) @@ -348,21 +349,24 @@ async def _action_for_event_by_user( for user_id, level in notification_levels.items(): notification_levels[user_id] = int(level) - # Pull out the mentions field if it exists and trim the values to things - # that might be valid. - mentions_raw = event.content.get(EventContentFields.MSC3952_MENTIONS) - if isinstance(mentions_raw, list): - # Take the first 10 items, then strip out any non-string ones and convert - # to a tuple. - mentions = set( - filter(lambda item: isinstance(item, str), mentions_raw[:10]) - ) - else: - mentions = set() + # Pull out any user and room mentions. + mentions = event.content.get(EventContentFields.MSC3952_MENTIONS) + user_mentions: Set[str] = set() + room_mention = False + if 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) + ) + # Room mention is only true if the value is exactly true. + room_mention = mentions.get("room") is True evaluator = PushRuleEvaluator( _flatten_dict(event, room_version=event.room_version), - mentions, + user_mentions, + room_mention, room_member_count, sender_power_level, notification_levels, diff --git a/tests/push/test_bulk_push_rule_evaluator.py b/tests/push/test_bulk_push_rule_evaluator.py index 0d343a45fcd8..8b8d4e680bc4 100644 --- a/tests/push/test_bulk_push_rule_evaluator.py +++ b/tests/push/test_bulk_push_rule_evaluator.py @@ -170,21 +170,35 @@ def create_and_process(mentions: Any = sentinel) -> bool: ) return len(result) > 0 - # Not including the mentions field should result in no notifications. + # Not including the mentions field should not notify. self.assertFalse(create_and_process()) + # An empty mentions field should not notify. + self.assertFalse(create_and_process({})) - # Invalid data should be ignored. + # Non-dict mentions should be ignored. mentions: Any - for mentions in (None, True, False, "foo", {}): + for mentions in (None, True, False, 1, "foo", []): self.assertFalse(create_and_process(mentions)) - # The Matrix ID appearing anywhere in the mentions list should match - self.assertTrue(create_and_process([self.alice])) - self.assertTrue(create_and_process(["@another:test", self.alice])) + # A non-list should be ignored. + for mentions in (None, True, False, 1, "foo", {}): + self.assertFalse(create_and_process({"user_ids": mentions})) - # The Matrix ID appearing > 10 entries into the list should be ignored. - self.assertFalse(create_and_process(["@another:test"] * 10 + [self.alice])) + # The Matrix ID appearing anywhere in the list should notify. + self.assertTrue(create_and_process({"user_ids": [self.alice]})) + self.assertTrue(create_and_process({"user_ids": ["@another:test", self.alice]})) - # Invalid entries in the list are ignored, but count towards the limit. - self.assertTrue(create_and_process([None, True, False, {}, [], self.alice])) - self.assertFalse(create_and_process([None] * 10 + [self.alice])) + # Duplicate user IDs should notify. + self.assertTrue(create_and_process({"user_ids": [self.alice, self.alice]})) + + # Invalid entries in the list are ignored. + self.assertFalse(create_and_process({"user_ids": [None, True, False, {}, []]})) + self.assertTrue( + create_and_process({"user_ids": [None, True, False, {}, [], self.alice]}) + ) + + # Room mentions should notify. + self.assertTrue(create_and_process({"room": True})) + # A non-True should not notify. + for mentions in (None, False, 1, "foo", [], {}): + self.assertFalse(create_and_process({"room": mentions})) diff --git a/tests/push/test_push_rule_evaluator.py b/tests/push/test_push_rule_evaluator.py index 0441dd76a796..9d01c989d408 100644 --- a/tests/push/test_push_rule_evaluator.py +++ b/tests/push/test_push_rule_evaluator.py @@ -42,7 +42,8 @@ def _get_evaluator( self, content: JsonMapping, *, - mentions: Optional[Set[str]] = None, + user_mentions: Optional[Set[str]] = None, + room_mention: bool = False, related_events: Optional[JsonDict] = None, ) -> PushRuleEvaluator: event = FrozenEvent( @@ -61,7 +62,8 @@ def _get_evaluator( power_levels: Dict[str, Union[int, Dict[str, int]]] = {} return PushRuleEvaluator( _flatten_dict(event), - mentions or set(), + user_mentions or set(), + room_mention, room_member_count, sender_power_level, cast(Dict[str, int], power_levels.get("notifications", {})), @@ -104,14 +106,16 @@ def test_user_mentions(self) -> None: self.assertFalse(evaluator.matches(condition, "@user:test", None)) # An empty set shouldn't match - evaluator = self._get_evaluator({}, mentions=set()) + evaluator = self._get_evaluator({}, 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({}, mentions={"@user:test"}) + evaluator = self._get_evaluator({}, user_mentions={"@user:test"}) self.assertTrue(evaluator.matches(condition, "@user:test", None)) - evaluator = self._get_evaluator({}, mentions={"@another:test", "@user:test"}) + evaluator = self._get_evaluator( + {}, 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 @@ -121,19 +125,18 @@ def test_room_mentions(self) -> None: """Check for room mentions.""" condition = {"kind": "org.matrix.msc3952.is_room_mention"} - # No mentions shouldn't match. + # No room mention shouldn't match. evaluator = self._get_evaluator({}) self.assertFalse(evaluator.matches(condition, None, None)) - # An empty set shouldn't match - evaluator = self._get_evaluator({}, mentions=set()) - self.assertFalse(evaluator.matches(condition, None, None)) - - # The @room appearing anywhere in the mentions list should match - evaluator = self._get_evaluator({}, mentions={"@room"}) + # Room mention should match. + evaluator = self._get_evaluator({}, room_mention=True) self.assertTrue(evaluator.matches(condition, None, None)) - evaluator = self._get_evaluator({}, mentions={"@another:test", "@room"}) + # A room mention and user mention is valid. + evaluator = self._get_evaluator( + {}, user_mentions={"@another:test"}, room_mention=True + ) self.assertTrue(evaluator.matches(condition, None, None)) # Note that invalid data is tested at tests.push.test_bulk_push_rule_evaluator.TestBulkPushRuleEvaluator.test_mentions From dbc73d704a6d0349f3c5305f84d9858621aa34cb Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 26 Jan 2023 16:09:48 -0500 Subject: [PATCH 4/5] Clarify comments. Co-authored-by: David Robertson --- tests/push/test_bulk_push_rule_evaluator.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/push/test_bulk_push_rule_evaluator.py b/tests/push/test_bulk_push_rule_evaluator.py index 8b8d4e680bc4..8583bd205912 100644 --- a/tests/push/test_bulk_push_rule_evaluator.py +++ b/tests/push/test_bulk_push_rule_evaluator.py @@ -137,6 +137,7 @@ def test_mentions(self) -> None: sentinel = object() def create_and_process(mentions: Any = sentinel) -> bool: + """Returns true iff the `mentions` trigger an event push action.""" content = {} if mentions is not sentinel: content[EventContentFields.MSC3952_MENTIONS] = mentions From 8af1763913ad6593bf6f14faba49497b19015086 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 27 Jan 2023 08:10:58 -0500 Subject: [PATCH 5/5] Add missing power level condition for room mentions. --- rust/src/push/base_rules.rs | 7 ++++++- tests/push/test_bulk_push_rule_evaluator.py | 15 +++++++++++++-- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/rust/src/push/base_rules.rs b/rust/src/push/base_rules.rs index d3bdb1d24ddf..880eed0ef405 100644 --- a/rust/src/push/base_rules.rs +++ b/rust/src/push/base_rules.rs @@ -150,7 +150,12 @@ pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[ PushRule { rule_id: Cow::Borrowed(".org.matrix.msc3952.is_room_mentioned"), priority_class: 5, - conditions: Cow::Borrowed(&[Condition::Known(KnownCondition::IsRoomMention)]), + conditions: Cow::Borrowed(&[ + Condition::Known(KnownCondition::IsRoomMention), + Condition::Known(KnownCondition::SenderNotificationPermission { + key: Cow::Borrowed("room"), + }), + ]), actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_ACTION, SOUND_ACTION]), default: true, default_enabled: true, diff --git a/tests/push/test_bulk_push_rule_evaluator.py b/tests/push/test_bulk_push_rule_evaluator.py index 8583bd205912..aba62b5dc8fe 100644 --- a/tests/push/test_bulk_push_rule_evaluator.py +++ b/tests/push/test_bulk_push_rule_evaluator.py @@ -198,8 +198,19 @@ def create_and_process(mentions: Any = sentinel) -> bool: create_and_process({"user_ids": [None, True, False, {}, [], self.alice]}) ) - # Room mentions should notify. + # Room mentions from those without power should not notify. + self.assertFalse(create_and_process({"room": True})) + + # Room mentions from those with power should notify. + self.helper.send_state( + self.room_id, + "m.room.power_levels", + {"notifications": {"room": 0}}, + self.token, + state_key="", + ) self.assertTrue(create_and_process({"room": True})) - # A non-True should not notify. + + # Invalid data should not notify. for mentions in (None, False, 1, "foo", [], {}): self.assertFalse(create_and_process({"room": mentions}))