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.
This commit is contained in:
Patrick Cloke 2023-03-02 08:30:51 -05:00 committed by GitHub
parent 33a85cf08c
commit 8ef324ea6f
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
11 changed files with 73 additions and 94 deletions

1
changelog.d/15051.misc Normal file
View File

@ -0,0 +1 @@
Update [MSC3952](https://github.com/matrix-org/matrix-spec-proposals/pull/3952) support based on changes to the MSC.

View File

@ -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(),

View File

@ -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,

View File

@ -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<String>,
/// The number of users in the room.
room_member_count: u64,
@ -114,7 +112,6 @@ impl PushRuleEvaluator {
pub fn py_new(
flattened_keys: BTreeMap<String, JsonValue>,
has_mentions: bool,
user_mentions: BTreeSet<String>,
room_member_count: u64,
sender_power_level: Option<i64>,
notification_power_levels: BTreeMap<String, i64>,
@ -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<str>,
value: Cow<SimpleJsonValue>,
) -> Result<bool, Error> {
// 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(),

View File

@ -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"}"#;

View File

@ -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],

View File

@ -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.

View File

@ -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,

View File

@ -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

View File

@ -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,
}
}
)

View File

@ -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: