From c2473622e752a0b77f36d517f55d4f112872245c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Sat, 30 Apr 2022 17:20:59 +0200 Subject: [PATCH 1/9] Implement MSC3786: Add a default push rule to ignore m.room.server_acl events MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/push/baserules.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/synapse/push/baserules.py b/synapse/push/baserules.py index f42f605f2383..6dd10ca00a05 100644 --- a/synapse/push/baserules.py +++ b/synapse/push/baserules.py @@ -277,6 +277,18 @@ def make_base_prepend_rules( ], "actions": ["dont_notify"], }, + { + "rule_id": "global/override/org.matrix.msc3786.rule.room.server_acl", + "conditions": [ + { + "kind": "event_match", + "key": "type", + "pattern": "m.room.server_acl", + "_cache_key": "_room_server_acl", + } + ], + "actions": ["dont_notify"], + }, ] From d314905df0d552ba47debd51be018635c29af043 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Sat, 30 Apr 2022 17:24:59 +0200 Subject: [PATCH 2/9] Add changelog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- changelog.d/12601.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/12601.feature diff --git a/changelog.d/12601.feature b/changelog.d/12601.feature new file mode 100644 index 000000000000..c13360ff35c3 --- /dev/null +++ b/changelog.d/12601.feature @@ -0,0 +1 @@ +Implement MSC3786: Add a default push rule to ignore m.room.server_acl events. From 348bb1fc3aeb12e44430d0150c4bceffa2fede82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Sat, 30 Apr 2022 18:37:03 +0200 Subject: [PATCH 3/9] Add prefix `.` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/push/baserules.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/push/baserules.py b/synapse/push/baserules.py index 6dd10ca00a05..e99ff0ff470a 100644 --- a/synapse/push/baserules.py +++ b/synapse/push/baserules.py @@ -278,7 +278,7 @@ def make_base_prepend_rules( "actions": ["dont_notify"], }, { - "rule_id": "global/override/org.matrix.msc3786.rule.room.server_acl", + "rule_id": "global/override/.org.matrix.msc3786.rule.room.server_acl", "conditions": [ { "kind": "event_match", From cbce206c369082efd75c2985cd29180890a98d4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Fri, 6 May 2022 09:27:27 +0200 Subject: [PATCH 4/9] Hide MSC3786 behind a flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/config/experimental.py | 3 ++ synapse/push/baserules.py | 3 ++ synapse/storage/databases/main/push_rule.py | 39 ++++++++++++++++----- 3 files changed, 36 insertions(+), 9 deletions(-) diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index 421ed7481baf..0dbf63cdc3a8 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -81,3 +81,6 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: # MSC2815 (allow room moderators to view redacted event content) self.msc2815_enabled: bool = experimental.get("msc2815_enabled", False) + + # MSC3786 (Add a default push rule to ignore m.room.server_acl events) + self.msc3786_enabled: bool = experimental.get("msc3786_enabled", False) diff --git a/synapse/push/baserules.py b/synapse/push/baserules.py index e99ff0ff470a..a17b35a605fb 100644 --- a/synapse/push/baserules.py +++ b/synapse/push/baserules.py @@ -277,6 +277,9 @@ def make_base_prepend_rules( ], "actions": ["dont_notify"], }, + # XXX: This is an experimental rule that is only enabled if msc3786_enabled + # is enabled, if it is not the rule gets filtered out in _load_rules() in + # PushRulesWorkerStore { "rule_id": "global/override/.org.matrix.msc3786.rule.room.server_acl", "conditions": [ diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index eb85bbd39243..66963a7af918 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -17,6 +17,7 @@ from typing import TYPE_CHECKING, Dict, List, Tuple, Union from synapse.api.errors import StoreError +from synapse.config.homeserver import ExperimentalConfig from synapse.push.baserules import list_with_base_rules from synapse.replication.slave.storage._slaved_id_tracker import SlavedIdTracker from synapse.storage._base import SQLBaseStore, db_to_json @@ -42,7 +43,18 @@ logger = logging.getLogger(__name__) -def _load_rules(rawrules, enabled_map): +def _is_experimental_rule_enabled( + rule_id: str, experimental_config: ExperimentalConfig +) -> bool: + if ( + rule_id == "global/override/.org.matrix.msc3786.rule.room.server_acl" + and not experimental_config.msc3786_enabled + ): + return False + return True + + +def _load_rules(rawrules, enabled_map, experimental_config: ExperimentalConfig): ruleslist = [] for rawrule in rawrules: rule = dict(rawrule) @@ -56,12 +68,19 @@ def _load_rules(rawrules, enabled_map): for i, rule in enumerate(rules): rule_id = rule["rule_id"] - if rule_id in enabled_map: - if rule.get("enabled", True) != bool(enabled_map[rule_id]): - # Rules are cached across users. - rule = dict(rule) - rule["enabled"] = bool(enabled_map[rule_id]) - rules[i] = rule + + if not _is_experimental_rule_enabled(rule_id, experimental_config): + rules.remove(rules[i]) + continue + if rule_id not in enabled_map: + continue + if rule.get("enabled", True) == bool(enabled_map[rule_id]): + continue + + # Rules are cached across users. + rule = dict(rule) + rule["enabled"] = bool(enabled_map[rule_id]) + rules[i] = rule return rules @@ -141,7 +160,7 @@ async def get_push_rules_for_user(self, user_id): enabled_map = await self.get_push_rules_enabled_for_user(user_id) - return _load_rules(rows, enabled_map) + return _load_rules(rows, enabled_map, self.hs.config.experimental) @cached(max_entries=5000) async def get_push_rules_enabled_for_user(self, user_id) -> Dict[str, bool]: @@ -200,7 +219,9 @@ async def bulk_get_push_rules(self, user_ids): enabled_map_by_user = await self.bulk_get_push_rules_enabled(user_ids) for user_id, rules in results.items(): - results[user_id] = _load_rules(rules, enabled_map_by_user.get(user_id, {})) + results[user_id] = _load_rules( + rules, enabled_map_by_user.get(user_id, {}), self.hs.config.experimental + ) return results From 94b20931a9ffcb52c505a8ed8f093198964697c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Sat, 7 May 2022 08:56:07 +0200 Subject: [PATCH 5/9] Fix sytests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/storage/databases/main/push_rule.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index 66963a7af918..483e47f262e7 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -66,11 +66,12 @@ def _load_rules(rawrules, enabled_map, experimental_config: ExperimentalConfig): # We're going to be mutating this a lot, so do a deep copy rules = list(list_with_base_rules(ruleslist)) + rules_to_remove = [] for i, rule in enumerate(rules): rule_id = rule["rule_id"] if not _is_experimental_rule_enabled(rule_id, experimental_config): - rules.remove(rules[i]) + rules_to_remove.append(i) continue if rule_id not in enabled_map: continue @@ -82,6 +83,9 @@ def _load_rules(rawrules, enabled_map, experimental_config: ExperimentalConfig): rule["enabled"] = bool(enabled_map[rule_id]) rules[i] = rule + for rule_index in rules_to_remove: + rules.remove(rules[rule_index]) + return rules From a838f59c888fbff22b55d1a79b1a8a60476e8dfe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Mon, 9 May 2022 16:46:59 +0200 Subject: [PATCH 6/9] Add a comment Co-authored-by: Erik Johnston --- synapse/storage/databases/main/push_rule.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index 483e47f262e7..cf60a9558429 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -46,6 +46,9 @@ def _is_experimental_rule_enabled( rule_id: str, experimental_config: ExperimentalConfig ) -> bool: + """Used by `_load_rules` to filter out experimental rules when they + have not been enabled. + """ if ( rule_id == "global/override/.org.matrix.msc3786.rule.room.server_acl" and not experimental_config.msc3786_enabled From 91e1889acb3222b108e21efd4df7ca9af9a0a76a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Mon, 9 May 2022 16:57:20 +0200 Subject: [PATCH 7/9] Simplify code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/storage/databases/main/push_rule.py | 22 ++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index cf60a9558429..6319b1143d07 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -66,10 +66,13 @@ def _load_rules(rawrules, enabled_map, experimental_config: ExperimentalConfig): rule["default"] = False ruleslist.append(rule) - # We're going to be mutating this a lot, so do a deep copy - rules = list(list_with_base_rules(ruleslist)) + # We're going to be mutating this a lot, so copy it. We also filter out + # any experimental default push rules that aren't enabled. + rules = [ + rule for rule in list_with_base_rules(ruleslist) + if _is_experimental_rule_enabled(rule["rule_id"], experimental_config) + ] - rules_to_remove = [] for i, rule in enumerate(rules): rule_id = rule["rule_id"] @@ -86,9 +89,6 @@ def _load_rules(rawrules, enabled_map, experimental_config: ExperimentalConfig): rule["enabled"] = bool(enabled_map[rule_id]) rules[i] = rule - for rule_index in rules_to_remove: - rules.remove(rules[rule_index]) - return rules @@ -138,7 +138,7 @@ def __init__( prefilled_cache=push_rules_prefill, ) - @abc.abstractmethod + @ abc.abstractmethod def get_max_push_rules_stream_id(self): """Get the position of the push rules stream. @@ -147,7 +147,7 @@ def get_max_push_rules_stream_id(self): """ raise NotImplementedError() - @cached(max_entries=5000) + @ cached(max_entries=5000) async def get_push_rules_for_user(self, user_id): rows = await self.db_pool.simple_select_list( table="push_rules", @@ -169,7 +169,7 @@ async def get_push_rules_for_user(self, user_id): return _load_rules(rows, enabled_map, self.hs.config.experimental) - @cached(max_entries=5000) + @ cached(max_entries=5000) async def get_push_rules_enabled_for_user(self, user_id) -> Dict[str, bool]: results = await self.db_pool.simple_select_list( table="push_rules_enable", @@ -199,7 +199,7 @@ def have_push_rules_changed_txn(txn): "have_push_rules_changed", have_push_rules_changed_txn ) - @cachedList( + @ cachedList( cached_method_name="get_push_rules_for_user", list_name="user_ids", num_args=1, @@ -283,7 +283,7 @@ async def copy_push_rules_from_room_to_room_for_user( ): await self.copy_push_rule_from_room_to_room(new_room_id, user_id, rule) - @cachedList( + @ cachedList( cached_method_name="get_push_rules_enabled_for_user", list_name="user_ids", num_args=1, From 27ad91cfb965d9d1176d457c6f9478b4c62377da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Mon, 9 May 2022 17:02:59 +0200 Subject: [PATCH 8/9] Delint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/storage/databases/main/push_rule.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index 6319b1143d07..8e26c068308a 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -69,7 +69,8 @@ def _load_rules(rawrules, enabled_map, experimental_config: ExperimentalConfig): # We're going to be mutating this a lot, so copy it. We also filter out # any experimental default push rules that aren't enabled. rules = [ - rule for rule in list_with_base_rules(ruleslist) + rule + for rule in list_with_base_rules(ruleslist) if _is_experimental_rule_enabled(rule["rule_id"], experimental_config) ] @@ -138,7 +139,7 @@ def __init__( prefilled_cache=push_rules_prefill, ) - @ abc.abstractmethod + @abc.abstractmethod def get_max_push_rules_stream_id(self): """Get the position of the push rules stream. @@ -147,7 +148,7 @@ def get_max_push_rules_stream_id(self): """ raise NotImplementedError() - @ cached(max_entries=5000) + @cached(max_entries=5000) async def get_push_rules_for_user(self, user_id): rows = await self.db_pool.simple_select_list( table="push_rules", @@ -169,7 +170,7 @@ async def get_push_rules_for_user(self, user_id): return _load_rules(rows, enabled_map, self.hs.config.experimental) - @ cached(max_entries=5000) + @cached(max_entries=5000) async def get_push_rules_enabled_for_user(self, user_id) -> Dict[str, bool]: results = await self.db_pool.simple_select_list( table="push_rules_enable", @@ -199,7 +200,7 @@ def have_push_rules_changed_txn(txn): "have_push_rules_changed", have_push_rules_changed_txn ) - @ cachedList( + @cachedList( cached_method_name="get_push_rules_for_user", list_name="user_ids", num_args=1, @@ -283,7 +284,7 @@ async def copy_push_rules_from_room_to_room_for_user( ): await self.copy_push_rule_from_room_to_room(new_room_id, user_id, rule) - @ cachedList( + @cachedList( cached_method_name="get_push_rules_enabled_for_user", list_name="user_ids", num_args=1, From 46463d1e25a5aaac782379ba6354dbab52803a57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Mon, 9 May 2022 17:07:54 +0200 Subject: [PATCH 9/9] Don't be an idiot, Simon, think about what you're doing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- synapse/storage/databases/main/push_rule.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index 8e26c068308a..4ed913e24879 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -77,9 +77,6 @@ def _load_rules(rawrules, enabled_map, experimental_config: ExperimentalConfig): for i, rule in enumerate(rules): rule_id = rule["rule_id"] - if not _is_experimental_rule_enabled(rule_id, experimental_config): - rules_to_remove.append(i) - continue if rule_id not in enabled_map: continue if rule.get("enabled", True) == bool(enabled_map[rule_id]):