diff --git a/changelog.d/19955.feature b/changelog.d/19955.feature new file mode 100644 index 00000000000..534f4943ec3 --- /dev/null +++ b/changelog.d/19955.feature @@ -0,0 +1 @@ +Push relevant users if there's a knock on a room (MSC4506). diff --git a/rust/benches/evaluator.rs b/rust/benches/evaluator.rs index 96169fd45d9..44059327503 100644 --- a/rust/benches/evaluator.rs +++ b/rust/benches/evaluator.rs @@ -57,6 +57,7 @@ fn bench_match_exact(b: &mut Bencher) { Some(0), Default::default(), Default::default(), + Default::default(), true, vec![], false, @@ -72,10 +73,15 @@ fn bench_match_exact(b: &mut Bencher) { }, )); - let matched = eval.match_condition(&condition, None, None, None).unwrap(); + let matched = eval + .match_condition(&condition, None, None, None, None) + .unwrap(); assert!(matched, "Didn't match"); - b.iter(|| eval.match_condition(&condition, None, None, None).unwrap()); + b.iter(|| { + eval.match_condition(&condition, None, None, None, None) + .unwrap() + }); } #[bench] @@ -104,6 +110,7 @@ fn bench_match_word(b: &mut Bencher) { Some(0), Default::default(), Default::default(), + Default::default(), true, vec![], false, @@ -119,10 +126,15 @@ fn bench_match_word(b: &mut Bencher) { }, )); - let matched = eval.match_condition(&condition, None, None, None).unwrap(); + let matched = eval + .match_condition(&condition, None, None, None, None) + .unwrap(); assert!(matched, "Didn't match"); - b.iter(|| eval.match_condition(&condition, None, None, None).unwrap()); + b.iter(|| { + eval.match_condition(&condition, None, None, None, None) + .unwrap() + }); } #[bench] @@ -151,6 +163,7 @@ fn bench_match_word_miss(b: &mut Bencher) { Some(0), Default::default(), Default::default(), + Default::default(), true, vec![], false, @@ -166,10 +179,15 @@ fn bench_match_word_miss(b: &mut Bencher) { }, )); - let matched = eval.match_condition(&condition, None, None, None).unwrap(); + let matched = eval + .match_condition(&condition, None, None, None, None) + .unwrap(); assert!(!matched, "Didn't match"); - b.iter(|| eval.match_condition(&condition, None, None, None).unwrap()); + b.iter(|| { + eval.match_condition(&condition, None, None, None, None) + .unwrap() + }); } #[bench] @@ -198,6 +216,7 @@ fn bench_eval_message(b: &mut Bencher) { Some(0), Default::default(), Default::default(), + Default::default(), true, vec![], false, @@ -215,7 +234,8 @@ fn bench_eval_message(b: &mut Bencher) { false, false, false, + false, ); - b.iter(|| eval.run(&rules, Some("bob"), Some("person"), None)); + b.iter(|| eval.run(&rules, Some("bob"), Some("person"), None, None)); } diff --git a/rust/src/push/base_rules.rs b/rust/src/push/base_rules.rs index 47d5289006b..3db3a287d9e 100644 --- a/rust/src/push/base_rules.rs +++ b/rust/src/push/base_rules.rs @@ -117,6 +117,30 @@ pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[ default: true, default_enabled: true, }, + // MSC4506: notify the members of a room who are able to act on a knock + // (i.e. those with a power level sufficient to invite the knocker). Must + // come before `.m.rule.member_event`, which suppresses all other member + // events. + PushRule { + rule_id: Cow::Borrowed("global/override/.org.matrix.msc4506.rule.knock"), + priority_class: 5, + conditions: Cow::Borrowed(&[ + Condition::Known(KnownCondition::EventMatch(EventMatchCondition { + key: Cow::Borrowed("type"), + pattern: Cow::Borrowed("m.room.member"), + })), + Condition::Known(KnownCondition::EventMatch(EventMatchCondition { + key: Cow::Borrowed("content.membership"), + pattern: Cow::Borrowed("knock"), + })), + Condition::Known(KnownCondition::RecipientPermission { + key: Cow::Borrowed("invite"), + }), + ]), + actions: Cow::Borrowed(&[Action::Notify, HIGHLIGHT_FALSE_ACTION, SOUND_ACTION]), + default: true, + default_enabled: true, + }, PushRule { rule_id: Cow::Borrowed("global/override/.m.rule.member_event"), priority_class: 5, diff --git a/rust/src/push/evaluator.rs b/rust/src/push/evaluator.rs index 1cbca4c6355..b47033b8015 100644 --- a/rust/src/push/evaluator.rs +++ b/rust/src/push/evaluator.rs @@ -92,6 +92,12 @@ pub struct PushRuleEvaluator { /// outlier. sender_power_level: Option, + /// MSC4506: the power level required to perform each of the room's + /// power-levels actions (e.g. "invite", "kick"), for the + /// `recipient_permission` condition. Includes the spec defaults for + /// actions absent from the `m.room.power_levels` content. + action_power_levels: BTreeMap, + /// The related events, indexed by relation type. Flattened in the same manner as /// `flattened_keys`. related_events_flattened: BTreeMap>, @@ -124,6 +130,7 @@ impl PushRuleEvaluator { room_member_count, sender_power_level, notification_power_levels, + action_power_levels, related_events_flattened, related_event_match_enabled, room_version_feature_flags, @@ -137,6 +144,7 @@ impl PushRuleEvaluator { room_member_count: u64, sender_power_level: Option, notification_power_levels: BTreeMap, + action_power_levels: BTreeMap, related_events_flattened: BTreeMap>, related_event_match_enabled: bool, room_version_feature_flags: Vec, @@ -156,6 +164,7 @@ impl PushRuleEvaluator { room_member_count, notification_power_levels, sender_power_level, + action_power_levels, related_events_flattened, related_event_match_enabled, room_version_feature_flags, @@ -179,13 +188,17 @@ impl PushRuleEvaluator { /// - `None` if the event is not in a thread, or if MSC4306 is disabled. /// - `Some(true)` if the event is in a thread and the user has a subscription for that thread /// - `Some(false)` if the event is in a thread and the user does NOT have a subscription for that thread - #[pyo3(signature = (push_rules, user_id=None, display_name=None, msc4306_thread_subscription_state=None))] + /// + /// recipient_power_level: the power level of the user the rules are being + /// evaluated for, used by the MSC4506 `recipient_permission` condition. + #[pyo3(signature = (push_rules, user_id=None, display_name=None, msc4306_thread_subscription_state=None, recipient_power_level=None))] pub fn run( &self, push_rules: &FilteredPushRules, user_id: Option<&str>, display_name: Option<&str>, msc4306_thread_subscription_state: Option, + recipient_power_level: Option, ) -> Vec { 'outer: for (push_rule, enabled) in push_rules.iter() { if !enabled { @@ -222,6 +235,7 @@ impl PushRuleEvaluator { user_id, display_name, msc4306_thread_subscription_state, + recipient_power_level, ) { Ok(true) => {} Ok(false) => continue 'outer, @@ -255,19 +269,21 @@ impl PushRuleEvaluator { } /// Check if the given condition matches. - #[pyo3(signature = (condition, user_id=None, display_name=None, msc4306_thread_subscription_state=None))] + #[pyo3(signature = (condition, user_id=None, display_name=None, msc4306_thread_subscription_state=None, recipient_power_level=None))] fn matches( &self, condition: Condition, user_id: Option<&str>, display_name: Option<&str>, msc4306_thread_subscription_state: Option, + recipient_power_level: Option, ) -> bool { match self.match_condition( &condition, user_id, display_name, msc4306_thread_subscription_state, + recipient_power_level, ) { Ok(true) => true, Ok(false) => false, @@ -287,6 +303,7 @@ impl PushRuleEvaluator { user_id: Option<&str>, display_name: Option<&str>, msc4306_thread_subscription_state: Option, + recipient_power_level: Option, ) -> Result { let known_condition = match condition { Condition::Known(known) => known, @@ -409,6 +426,18 @@ impl PushRuleEvaluator { false } } + KnownCondition::RecipientPermission { key } => { + if let Some(recipient_power_level) = recipient_power_level { + if let Some(required_level) = self.action_power_levels.get(key.as_ref()) { + recipient_power_level >= *required_level + } else { + // Unknown action keys never match. + false + } + } else { + false + } + } KnownCondition::RoomVersionSupports { feature } => { if !self.msc3931_enabled { false @@ -564,6 +593,7 @@ fn push_rule_evaluator() { Some(0), BTreeMap::new(), BTreeMap::new(), + BTreeMap::new(), true, vec![], true, @@ -572,10 +602,68 @@ fn push_rule_evaluator() { ) .unwrap(); - let result = evaluator.run(&FilteredPushRules::default(), None, Some("bob"), None); + let result = evaluator.run(&FilteredPushRules::default(), None, Some("bob"), None, None); assert_eq!(result.len(), 3); } +#[test] +fn test_recipient_permission_condition() { + let mut action_power_levels = BTreeMap::new(); + action_power_levels.insert("invite".to_string(), 50); + + let evaluator = PushRuleEvaluator::py_new( + BTreeMap::new(), + false, + 10, + Some(0), + BTreeMap::new(), + action_power_levels, + BTreeMap::new(), + true, + vec![], + true, + false, + false, + ) + .unwrap(); + + let condition = Condition::Known(KnownCondition::RecipientPermission { + key: Cow::Borrowed("invite"), + }); + + // A recipient at or above the required level matches. + assert!(evaluator + .match_condition(&condition, Some("@bob:example.org"), None, None, Some(50)) + .unwrap()); + assert!(evaluator + .match_condition(&condition, Some("@bob:example.org"), None, None, Some(100)) + .unwrap()); + + // A recipient below the required level does not match. + assert!(!evaluator + .match_condition(&condition, Some("@bob:example.org"), None, None, Some(0)) + .unwrap()); + + // No recipient power level provided: never matches. + assert!(!evaluator + .match_condition(&condition, Some("@bob:example.org"), None, None, None) + .unwrap()); + + // An action key we don't have a level for never matches. + let unknown_key = Condition::Known(KnownCondition::RecipientPermission { + key: Cow::Borrowed("frobnicate"), + }); + assert!(!evaluator + .match_condition( + &unknown_key, + Some("@bob:example.org"), + None, + None, + Some(100) + ) + .unwrap()); +} + #[test] fn test_requires_room_version_supports_condition() { use std::borrow::Cow; @@ -595,6 +683,7 @@ fn test_requires_room_version_supports_condition() { Some(0), BTreeMap::new(), BTreeMap::new(), + BTreeMap::new(), false, flags, true, @@ -610,6 +699,7 @@ fn test_requires_room_version_supports_condition() { Some("@bob:example.org"), None, None, + None, ); assert_eq!(result.len(), 3); @@ -637,10 +727,12 @@ fn test_requires_room_version_supports_condition() { false, false, false, + false, ), None, None, None, + None, ); assert_eq!(result.len(), 1); } diff --git a/rust/src/push/mod.rs b/rust/src/push/mod.rs index 780d7a8cbd8..19873a0bf8f 100644 --- a/rust/src/push/mod.rs +++ b/rust/src/push/mod.rs @@ -369,6 +369,13 @@ pub enum KnownCondition { SenderNotificationPermission { key: Cow<'static, str>, }, + // MSC4506 (knock push rules): matches if the user the rules are being + // evaluated for has a power level at least that required to perform the + // action named by `key` (e.g. "invite"). + #[serde(rename = "org.matrix.msc4506.recipient_permission")] + RecipientPermission { + key: Cow<'static, str>, + }, #[serde(rename = "org.matrix.msc3931.room_version_supports")] RoomVersionSupports { feature: Cow<'static, str>, @@ -559,6 +566,7 @@ pub struct FilteredPushRules { msc4028_push_encrypted_events: bool, msc4210_enabled: bool, msc4306_enabled: bool, + msc4506_enabled: bool, } #[pymethods] @@ -574,6 +582,7 @@ impl FilteredPushRules { msc4028_push_encrypted_events: bool, msc4210_enabled: bool, msc4306_enabled: bool, + msc4506_enabled: bool, ) -> Self { Self { push_rules, @@ -584,6 +593,7 @@ impl FilteredPushRules { msc4028_push_encrypted_events, msc4210_enabled, msc4306_enabled, + msc4506_enabled, } } @@ -620,6 +630,11 @@ impl FilteredPushRules { return false; } + if !self.msc4506_enabled && rule.rule_id.contains("/.org.matrix.msc4506.rule.knock") + { + return false; + } + if !self.msc4028_push_encrypted_events && rule.rule_id == "global/override/.org.matrix.msc4028.encrypted_event" { diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index f99f7b139ec..146b49a9ce5 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -156,6 +156,12 @@ def read_config( "msc3381_polls_enabled", False ) + # MSC4506: default push rule notifying room members + # who can act on a knock (MSC2403), via the new `recipient_permission` + # push rule condition. Unstable ids: .org.matrix.msc4506.rule.knock / + # org.matrix.msc4506.recipient_permission. + self.msc4506_enabled: bool = experimental.get("msc4506_enabled", False) + # MSC3912: Relation-based redactions. self.msc3912_enabled: bool = experimental.get("msc3912_enabled", False) diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index 03dd3417440..e6c1555c0b0 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -89,6 +89,19 @@ SENTINEL = object() +def _coerce_power_level(level: object, default: int) -> int: + """Interpret a power level from `m.room.power_levels` content as an + integer, tolerating the non-integer levels (floats, strings, nulls) that + old room versions permit. Falls back to `default` if uninterpretable. + """ + if type(level) is int: # noqa: E721 + return level + try: + return int(level) # type: ignore[call-overload] + except (TypeError, ValueError): + return default + + 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(): @@ -177,6 +190,15 @@ async def _get_rules_for_event( ) if target_already_in_room: local_users = [event.state_key] + elif ( + self.hs.config.experimental.msc4506_enabled + and event.membership == Membership.KNOCK + ): + # MSC4506: a knock (sender == state_key, so not caught above) + # should notify the members of the room who can act on it, so + # we can't take the membership-events-only-notify-their-target + # fast path. + local_users = await self.store.get_local_users_in_room(event.room_id) else: # We get the users who may need to be notified by first fetching the # local users currently in the room, finding those that have push rules, @@ -458,6 +480,29 @@ async def _action_for_event_by_user( except (TypeError, ValueError): del notification_levels[key] + # MSC4506 (knock push rules): the level required to perform each of the + # room's power-levels actions, and the data to compute each recipient's + # own level, for the `recipient_permission` condition. As above, + # non-integer levels in old room versions are interpreted as integers + # where possible and otherwise dropped. + action_power_levels = { + "invite": power_levels.get("invite", 0), + "kick": power_levels.get("kick", 50), + "ban": power_levels.get("ban", 50), + "redact": power_levels.get("redact", 50), + } + for key in list(action_power_levels.keys()): + level = action_power_levels[key] + if type(level) is not int: # noqa: E721 + try: + action_power_levels[key] = int(level) + except (TypeError, ValueError): + del action_power_levels[key] + users_default_level = _coerce_power_level( + power_levels.get("users_default", 0), 0 + ) + user_power_levels = power_levels.get("users", {}) + # Pull out any user and room mentions. has_mentions = EventContentFields.MENTIONS in event.content @@ -467,6 +512,7 @@ async def _action_for_event_by_user( room_member_count, sender_power_level, notification_levels, + action_power_levels, related_events, self._related_event_match_enabled, event.room_version.msc3931_push_features, @@ -512,8 +558,18 @@ async def _action_for_event_by_user( if msc4306_thread_subscribers is not None: msc4306_thread_subscription_state = uid in msc4306_thread_subscribers + # MSC4506: this user's own power level, for the + # `recipient_permission` condition. + recipient_power_level = _coerce_power_level( + user_power_levels.get(uid, users_default_level), users_default_level + ) + actions = evaluator.run( - rules, uid, display_name, msc4306_thread_subscription_state + rules, + uid, + display_name, + msc4306_thread_subscription_state, + recipient_power_level, ) if "notify" in actions: # Push rules say we should notify the user of this event diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index d361166cec4..2c06d73cbb3 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -107,6 +107,7 @@ def _load_rules( msc4028_push_encrypted_events=experimental_config.msc4028_push_encrypted_events, msc4210_enabled=experimental_config.msc4210_enabled, msc4306_enabled=experimental_config.msc4306_enabled, + msc4506_enabled=experimental_config.msc4506_enabled, ) return filtered_rules diff --git a/synapse/synapse_rust/push.pyi b/synapse/synapse_rust/push.pyi index ef0d5f94f4f..344e4d3def2 100644 --- a/synapse/synapse_rust/push.pyi +++ b/synapse/synapse_rust/push.pyi @@ -50,6 +50,7 @@ class FilteredPushRules: msc4028_push_encrypted_events: bool, msc4210_enabled: bool, msc4306_enabled: bool, + msc4506_enabled: bool, ): ... def rules(self) -> Collection[tuple[PushRule, bool]]: ... @@ -63,6 +64,7 @@ class PushRuleEvaluator: room_member_count: int, sender_power_level: int | None, notification_power_levels: Mapping[str, int], + action_power_levels: Mapping[str, int], related_events_flattened: Mapping[str, Mapping[str, JsonValue]], related_event_match_enabled: bool, room_version_feature_flags: list[str], @@ -76,6 +78,7 @@ class PushRuleEvaluator: user_id: str | None, display_name: str | None, msc4306_thread_subscription_state: bool | None, + recipient_power_level: int | None = None, ) -> Collection[Mapping | str]: ... def matches( self, @@ -83,4 +86,5 @@ class PushRuleEvaluator: user_id: str | None, display_name: str | None, msc4306_thread_subscription_state: bool | None = None, + recipient_power_level: int | None = None, ) -> bool: ... diff --git a/tests/push/test_bulk_push_rule_evaluator.py b/tests/push/test_bulk_push_rule_evaluator.py index 137bbe24b28..7da0215917d 100644 --- a/tests/push/test_bulk_push_rule_evaluator.py +++ b/tests/push/test_bulk_push_rule_evaluator.py @@ -31,7 +31,7 @@ from synapse.api.room_versions import RoomVersions from synapse.push.bulk_push_rule_evaluator import BulkPushRuleEvaluator from synapse.rest import admin -from synapse.rest.client import login, push_rule, register, room +from synapse.rest.client import knock, login, push_rule, register, room from synapse.server import HomeServer from synapse.types import JsonDict, create_requester from synapse.util.clock import Clock @@ -650,3 +650,82 @@ def test_with_disabled_thread_subscriptions(self) -> None: type="m.room.message", ) ) + + +class TestKnockPushRules(HomeserverTestCase): + """Tests for the MSC4506 knock push rule (`.org.matrix.msc4506.rule.knock` + with the `recipient_permission` condition): a knock should notify exactly + the members of the room whose power level lets them act on it. + """ + + servlets = [ + admin.register_servlets_for_client_rest_resource, + room.register_servlets, + knock.register_servlets, + login.register_servlets, + register.register_servlets, + push_rule.register_servlets, + ] + + def _knock_and_get_event_id(self) -> str: + """Set up a knockable room and knock on it. + + Alice (PL 100, can invite) and Bob (PL 0, cannot invite: the room + requires PL 50) are joined; Charlie knocks. Returns the knock event id. + """ + self.alice = self.register_user("alice", "pass") + alice_token = self.login(self.alice, "pass") + self.bob = self.register_user("bob", "pass") + bob_token = self.login(self.bob, "pass") + self.charlie = self.register_user("charlie", "pass") + charlie_token = self.login(self.charlie, "pass") + + room_id = self.helper.create_room_as( + self.alice, is_public=True, tok=alice_token + ) + self.helper.join(room_id, self.bob, tok=bob_token) + self.helper.send_state( + room_id, + "m.room.power_levels", + {"users": {self.alice: 100}, "invite": 50}, + alice_token, + ) + self.helper.send_state( + room_id, + "m.room.join_rules", + {"join_rule": "knock"}, + alice_token, + ) + self.helper.knock(room=room_id, user=self.charlie, tok=charlie_token) + + state = self.get_success( + self.hs.get_storage_controllers().state.get_current_state(room_id) + ) + return state[("m.room.member", self.charlie)].event_id + + def _push_actions_for(self, event_id: str, user_id: str) -> list: + return self.get_success( + self.hs.get_datastores().main.db_pool.simple_select_list( + table="event_push_actions", + keyvalues={"event_id": event_id, "user_id": user_id, "notif": 1}, + retcols=("*",), + desc="get_event_push_actions", + ) + ) + + @override_config({"experimental_features": {"msc4506_enabled": True}}) + def test_knock_notifies_only_users_who_can_act(self) -> None: + knock_event_id = self._knock_and_get_event_id() + + # Alice can invite the knocker, so she is notified. + self.assertEqual(len(self._push_actions_for(knock_event_id, self.alice)), 1) + # Bob cannot, so he is not. + self.assertEqual(len(self._push_actions_for(knock_event_id, self.bob)), 0) + # The knocker is never notified of their own knock. + self.assertEqual(len(self._push_actions_for(knock_event_id, self.charlie)), 0) + + def test_knock_notifies_nobody_when_disabled(self) -> None: + knock_event_id = self._knock_and_get_event_id() + + for user_id in (self.alice, self.bob, self.charlie): + self.assertEqual(len(self._push_actions_for(knock_event_id, user_id)), 0) diff --git a/tests/push/test_push_rule_evaluator.py b/tests/push/test_push_rule_evaluator.py index 2e389710b95..c3951bde266 100644 --- a/tests/push/test_push_rule_evaluator.py +++ b/tests/push/test_push_rule_evaluator.py @@ -161,6 +161,7 @@ def _get_evaluator( content: JsonMapping, *, related_events: JsonDict | None = None, + action_power_levels: dict[str, int] | None = None, msc4210: bool = False, msc4306: bool = False, ) -> PushRuleEvaluator: @@ -184,6 +185,7 @@ def _get_evaluator( room_member_count, sender_power_level, cast(dict[str, int], power_levels.get("notifications", {})), + {} if action_power_levels is None else action_power_levels, {} if related_events is None else related_events, related_event_match_enabled=True, room_version_feature_flags=event.room_version.msc3931_push_features, @@ -820,6 +822,45 @@ def test_related_event_match_no_related_event(self) -> None: ) ) + def test_recipient_permission(self) -> None: + """ + Test the MSC4506 `recipient_permission` condition (knock push rules). + """ + evaluator = self._get_evaluator( + {"membership": "knock"}, + action_power_levels={"invite": 50}, + ) + condition = { + "kind": "org.matrix.msc4506.recipient_permission", + "key": "invite", + } + + # A recipient at or above the required level matches. + self.assertTrue( + evaluator.matches(condition, "@user:test", None, recipient_power_level=50) + ) + self.assertTrue( + evaluator.matches(condition, "@user:test", None, recipient_power_level=100) + ) + + # A recipient below the required level does not match. + self.assertFalse( + evaluator.matches(condition, "@user:test", None, recipient_power_level=0) + ) + + # No recipient power level: never matches. + self.assertFalse(evaluator.matches(condition, "@user:test", None)) + + # An action key with no known level never matches. + self.assertFalse( + evaluator.matches( + {"kind": "org.matrix.msc4506.recipient_permission", "key": "nonsense"}, + "@user:test", + None, + recipient_power_level=100, + ) + ) + def test_thread_subscription_subscribed(self) -> None: """ Test MSC4306 thread subscription push rules against an event in a subscribed thread.