Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Implement MSC3930: polls push rules #14787

Merged
merged 12 commits into from
Jan 19, 2023
Merged

Implement MSC3930: polls push rules #14787

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
23c1dc0
Add an experimental config option for MSC3930
anoadragon453 Jan 6, 2023
f9f3ea1
Implement MSC3930: Polls push rules & notifications
anoadragon453 Jan 5, 2023
206349e
Enable support for msc3930 in synapse's complement runs
anoadragon453 Jan 6, 2023
ee1b3fa
changelog
anoadragon453 Jan 6, 2023
33e6e91
Fix up arguments to FilteredPushRules, PushRuleEvaluator
anoadragon453 Jan 11, 2023
b63b2ff
Organise MSC* arguments alphabetically
anoadragon453 Jan 11, 2023
5954833
Rename msc3930_enabled -> msc3381_polls_enabled
anoadragon453 Jan 11, 2023
f96f86c
Catch a behavioural change after argument order shuffling
anoadragon453 Jan 11, 2023
78bf8b0
Run cargo fmt
anoadragon453 Jan 11, 2023
0527397
Simplify experimental option checking when filtering push rules
anoadragon453 Jan 12, 2023
34ba120
Merge branch 'develop' of github.com:matrix-org/synapse into anoa/msc…
anoadragon453 Jan 17, 2023
263e825
Merge branch 'develop' of github.com:matrix-org/synapse into anoa/msc…
anoadragon453 Jan 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/14787.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Implement experimental support for MSC3930.
2 changes: 2 additions & 0 deletions docker/complement/conf/workers-shared-extra.yaml.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ experimental_features:
msc3874_enabled: true
# Enable removing account data support
msc3391_enabled: true
# Enable push rules for polls
msc3930_enabled: true

server_notices:
system_mxid_localpart: _server
Expand Down
78 changes: 77 additions & 1 deletion rust/src/push/base_rules.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2022 The Matrix.org Foundation C.I.C.
// Copyright 2022, 2023 The Matrix.org Foundation C.I.C.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -208,6 +208,20 @@ pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[
default: true,
default_enabled: true,
},
PushRule {
rule_id: Cow::Borrowed("global/override/.org.matrix.msc3930.rule.poll_response"),
priority_class: 5,
conditions: Cow::Borrowed(&[
Condition::Known(KnownCondition::EventMatch(EventMatchCondition {
key: Cow::Borrowed("type"),
pattern: Some(Cow::Borrowed("org.matrix.msc3381.poll.response")),
pattern_type: None,
})),
]),
actions: Cow::Borrowed(&[]),
default: true,
default_enabled: true,
},
];

pub const BASE_APPEND_CONTENT_RULES: &[PushRule] = &[PushRule {
Expand Down Expand Up @@ -596,6 +610,68 @@ pub const BASE_APPEND_UNDERRIDE_RULES: &[PushRule] = &[
default: true,
default_enabled: true,
},
PushRule {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the placement in the order here matter? The MSC doesn't state where it goes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, the ordering of these rules will matter yes. We want the *one_to_one rules to be tested in clients first before their group room counterparts. I'm a little confused on how priority works in push rules though. The spec seems to imply that rules are checked by kind, then an "ordering priority". I'm guessing the latter is the order that the rules are sent to clients in, rather than a field? It's a bit vague though.

We can also just sidestep the ordering requirement by placing a room_member_count>2 condition on the group room rules, but that shouldn't be necessary.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I have asserted the ordering in the Complement tests: https://github.com/matrix-org/complement/pull/578/files#diff-ce4102a82b595c7b72d6f945db418fbc7d34ff7973f5ca6e8935ad98a09273d2R161

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, the ordering of these rules will matter yes.

Sorry for being unclear! I meant how did you decide this went after global/underride/.im.vector.jitsi.

I think internally the ordering is reasonable and matches the MSC.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! I didn't consider ordering against other underride push rules. But perhaps this should go above global/underride/.m.rule.encrypted, such that clients match this push rule before the generic "encrypted message" one?

Though that does make me question all of the other non-state event-related rules in this file. Shouldn't they go above rules that match generic m.room.encrypted events? I suppose the client must be doing some mangling of the order here/filtering out of the m.rule.encrypted rule(s) already?

It turns out matrix-react-sdk is just sorting known rules client-side anyways into this order. Unknown rules appear at the end in the order they were received from the homeserver. Interestingly the generic EncryptedMessage and EncryptedDM rules are put in front. I'm not sure how this achieves the desired result unless there's additional logic I'm missing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding was the spec mandates an order (and thus MSCs must define an order).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have tried to get some clarity in matrix-org/matrix-spec-proposals#3930 (comment).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've been given the go ahead to define our own ordering for now: matrix-org/matrix-spec-proposals#3930 (comment)

rule_id: Cow::Borrowed("global/underride/.org.matrix.msc3930.rule.poll_start_one_to_one"),
priority_class: 1,
conditions: Cow::Borrowed(&[
Condition::Known(KnownCondition::RoomMemberCount {
is: Some(Cow::Borrowed("2")),
}),
Condition::Known(KnownCondition::EventMatch(EventMatchCondition {
key: Cow::Borrowed("type"),
pattern: Some(Cow::Borrowed("org.matrix.msc3381.poll.start")),
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved
pattern_type: None,
})),
]),
actions: Cow::Borrowed(&[Action::Notify, SOUND_ACTION]),
default: true,
default_enabled: true,
},
PushRule {
rule_id: Cow::Borrowed("global/underride/.org.matrix.msc3930.rule.poll_start"),
priority_class: 1,
conditions: Cow::Borrowed(&[
Condition::Known(KnownCondition::EventMatch(EventMatchCondition {
key: Cow::Borrowed("type"),
pattern: Some(Cow::Borrowed("org.matrix.msc3381.poll.start")),
pattern_type: None,
})),
]),
actions: Cow::Borrowed(&[Action::Notify]),
default: true,
default_enabled: true,
},
PushRule {
rule_id: Cow::Borrowed("global/underride/.org.matrix.msc3930.rule.poll_end_one_to_one"),
priority_class: 1,
conditions: Cow::Borrowed(&[
Condition::Known(KnownCondition::RoomMemberCount {
is: Some(Cow::Borrowed("2")),
}),
Condition::Known(KnownCondition::EventMatch(EventMatchCondition {
key: Cow::Borrowed("type"),
pattern: Some(Cow::Borrowed("org.matrix.msc3381.poll.end")),
pattern_type: None,
})),
]),
actions: Cow::Borrowed(&[Action::Notify, SOUND_ACTION]),
default: true,
default_enabled: true,
},
PushRule {
rule_id: Cow::Borrowed("global/underride/.org.matrix.msc3930.rule.poll_end"),
priority_class: 1,
conditions: Cow::Borrowed(&[
Condition::Known(KnownCondition::EventMatch(EventMatchCondition {
key: Cow::Borrowed("type"),
pattern: Some(Cow::Borrowed("org.matrix.msc3381.poll.end")),
pattern_type: None,
})),
]),
actions: Cow::Borrowed(&[Action::Notify]),
default: true,
default_enabled: true,
},
];

lazy_static! {
Expand Down
13 changes: 13 additions & 0 deletions rust/src/push/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,7 @@ impl PushRules {
pub struct FilteredPushRules {
push_rules: PushRules,
enabled_map: BTreeMap<String, bool>,
msc3930_enabled: bool,
msc3664_enabled: bool,
msc1767_enabled: bool,
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved
}
Expand All @@ -421,12 +422,14 @@ impl FilteredPushRules {
pub fn py_new(
push_rules: PushRules,
enabled_map: BTreeMap<String, bool>,
msc3930_enabled: bool,
msc3664_enabled: bool,
msc1767_enabled: bool,
) -> Self {
Self {
push_rules,
enabled_map,
msc3930_enabled,
msc3664_enabled,
msc1767_enabled,
}
Expand All @@ -447,6 +450,16 @@ impl FilteredPushRules {
.iter()
.filter(|rule| {
// Ignore disabled experimental push rules
if !self.msc3930_enabled
&& (rule.rule_id == "global/override/.org.matrix.msc3930.rule.poll_response"
|| rule.rule_id == "global/underride/.org.matrix.msc3930.rule.poll_start_one_to_one"
|| rule.rule_id == "global/underride/.org.matrix.msc3930.rule.poll_start"
|| rule.rule_id == "global/underride/.org.matrix.msc3930.rule.poll_end_one_to_one"
|| rule.rule_id == "global/underride/.org.matrix.msc3930.rule.poll_end")
{
return false;
}

if !self.msc3664_enabled
&& rule.rule_id == "global/override/.im.nheko.msc3664.reply"
{
Expand Down
2 changes: 1 addition & 1 deletion scripts-dev/complement.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ fi

extra_test_args=()

test_tags="synapse_blacklist,msc3787,msc3874,msc3391"
test_tags="synapse_blacklist,msc3787,msc3874,msc3391,msc3930"

# All environment variables starting with PASS_ will be shared.
# (The prefix is stripped off before reaching the container.)
Expand Down
1 change: 1 addition & 0 deletions stubs/synapse/synapse_rust/push.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class FilteredPushRules:
self,
push_rules: PushRules,
enabled_map: Dict[str, bool],
msc3930_enabled: bool,
msc3664_enabled: bool,
msc1767_enabled: bool,
): ...
Expand Down
3 changes: 3 additions & 0 deletions synapse/config/experimental.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,9 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
"msc3886_endpoint", None
)

# MSC3930: Push rules for MSC3391 polls
self.msc3930_enabled: bool = experimental.get("msc3930_enabled", False)
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved

# MSC3912: Relation-based redactions.
self.msc3912_enabled: bool = experimental.get("msc3912_enabled", False)

Expand Down
1 change: 1 addition & 0 deletions synapse/push/bulk_push_rule_evaluator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,7 @@ async def _action_for_event_by_user(
sender_power_level,
notification_levels,
related_events,
self.hs.config.experimental.msc3930_enabled,
self._related_event_match_enabled,
event.room_version.msc3931_push_features,
self.hs.config.experimental.msc1767_enabled, # MSC3931 flag
Expand Down
1 change: 1 addition & 0 deletions synapse/storage/databases/main/push_rule.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ def _load_rules(
filtered_rules = FilteredPushRules(
push_rules,
enabled_map,
msc3930_enabled=experimental_config.msc3930_enabled,
msc3664_enabled=experimental_config.msc3664_enabled,
msc1767_enabled=experimental_config.msc1767_enabled,
)
Expand Down