From 878370a2ec7030886948362267168293b8dbc82a Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 7 Apr 2022 17:39:10 +0100 Subject: [PATCH 1/9] Implement a module that creates new push rules for new users --- synapse_patch_push_rules/__init__.py | 81 ++++++++++++++++++++++++++++ synapse_patch_push_rules/py.typed | 0 2 files changed, 81 insertions(+) create mode 100644 synapse_patch_push_rules/__init__.py create mode 100644 synapse_patch_push_rules/py.typed diff --git a/synapse_patch_push_rules/__init__.py b/synapse_patch_push_rules/__init__.py new file mode 100644 index 0000000..b53b03e --- /dev/null +++ b/synapse_patch_push_rules/__init__.py @@ -0,0 +1,81 @@ +# Copyright 2022 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. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +import logging +from typing import Any, Dict, List, Union + +import attr +from synapse.module_api import ModuleApi +from synapse.module_api.errors import ConfigError + +logger = logging.getLogger(__name__) + + +@attr.s(auto_attribs=True, frozen=True) +class PushRule: + kind: str + conditions: List[Dict[str, Any]] + actions: List[Union[str, Dict[str, str]]] + + +class PushRulesPatcherConfig: + def __init__(self, config: Dict[str, Any]) -> None: + self.rules: Dict[str, PushRule] = {} + + rules = config["rules"] + for rule_id, rule in rules.items(): + self.rules[rule_id] = PushRule(**rule) + + +class PushRulesPatcher: + def __init__(self, config: PushRulesPatcherConfig, api: ModuleApi): + # Keep a reference to the config and Module API + self._api = api + self._config = config + + self._api.register_account_validity_callbacks( + on_user_registration=self.set_push_rules_for_user, + ) + + @staticmethod + def parse_config(config: Dict[str, Any]) -> PushRulesPatcherConfig: + if "rules" not in config: + raise ConfigError("Missing 'rules' in module configuration") + + if not isinstance(config["rules"], dict): + raise ConfigError("'rules' must be a dictionary") + + return PushRulesPatcherConfig(config) + + async def set_push_rules_for_user(self, user_id: str) -> None: + """Create new push rules for the given user. + Implements the on_user_registration account validity callback. + """ + # If we're running on a worker, make this a noop. on_user_registration callbacks + # should only be called on the main process, so this is mostly a stopgap in case + # something changes in the future. + if self._api.worker_app: + logger.warning( + "Attempted to run callback 'set_push_rules_for_user' on a worker, aborting" + ) + return + + for rule_id, rule in self._config.rules.items(): + await self._api.add_push_rule_for_user( + user_id=user_id, + scope="global", + kind=rule.kind, + rule_id=rule_id, + conditions=rule.conditions, + actions=rule.actions, + ) diff --git a/synapse_patch_push_rules/py.typed b/synapse_patch_push_rules/py.typed new file mode 100644 index 0000000..e69de29 From 79af57d750bfe401cfca2bf68c304f26a60a2834 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 7 Apr 2022 17:39:22 +0100 Subject: [PATCH 2/9] Add tests --- tests/__init__.py | 47 ++++++++++++++++ tests/test_push_rules_patcher.py | 93 ++++++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+) create mode 100644 tests/__init__.py create mode 100644 tests/test_push_rules_patcher.py diff --git a/tests/__init__.py b/tests/__init__.py new file mode 100644 index 0000000..c0de0c8 --- /dev/null +++ b/tests/__init__.py @@ -0,0 +1,47 @@ +# Copyright 2022 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. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +from asyncio import Future +from typing import Any, Awaitable, Dict, Optional, TypeVar +from unittest.mock import Mock + +from synapse.module_api import ModuleApi + +from synapse_patch_push_rules import PushRulesPatcher + +TV = TypeVar("TV") + + +def make_awaitable(result: TV) -> Awaitable[TV]: + """ + Makes an awaitable, suitable for mocking an `async` function. + This uses Futures as they can be awaited multiple times so can be returned + to multiple callers. + """ + future = Future() # type: ignore + future.set_result(result) + return future + + +def create_module(config: Optional[Dict[str, Any]] = None) -> PushRulesPatcher: + # Create a mock based on the ModuleApi spec, but override some mocked functions + # because some capabilities are needed for running the tests. + module_api = Mock(spec=ModuleApi) + module_api.add_push_rule_for_user = Mock(return_value=make_awaitable(None)) + + # If necessary, give parse_config some configuration to parse. + if config is None: + config = {} + parsed_config = PushRulesPatcher.parse_config(config) + + return PushRulesPatcher(parsed_config, module_api) diff --git a/tests/test_push_rules_patcher.py b/tests/test_push_rules_patcher.py new file mode 100644 index 0000000..d75af82 --- /dev/null +++ b/tests/test_push_rules_patcher.py @@ -0,0 +1,93 @@ +# Copyright 2022 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. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +from unittest.mock import Mock + +import aiounittest +from synapse.module_api.errors import ConfigError + +from tests import create_module + + +class PushRulesPatcherTestCase(aiounittest.AsyncTestCase): + async def test_no_rules(self) -> None: + """Tests that the module can't be created if 'rules' is missing from the + configuration. + """ + with self.assertRaises(ConfigError): + create_module() + + async def test_rules_no_dict(self) -> None: + """Tests that the module can't be created if the configured set of rules isn't a + dictionary. + """ + with self.assertRaises(ConfigError): + create_module({"rules": "hello"}) + + async def test_create_rules(self) -> None: + """Tests that, when configuring the module with multiple rules and calling the + callback, each rule is created for the newly registered user. + """ + rule_id1 = "foo" + rule_id2 = "bar" + conditions = [ + {"kind": "event_match", "key": "content.body", "pattern": "testword"} + ] + actions1 = ["dont_notify"] + actions2 = ["notify"] + kind = "content" + user_id = "@user:example.com" + + # Configure the module with 2 different rules. + module = create_module( + { + "rules": { + rule_id1: { + "kind": kind, + "conditions": conditions, + "actions": actions1, + }, + rule_id2: { + "kind": kind, + "conditions": conditions, + "actions": actions2, + }, + } + } + ) + + # Run the callback. + await module.set_push_rules_for_user(user_id) + + # Check the module API got called twice, and with the right arguments. + add_push_rule_mock: Mock = module._api.add_push_rule_for_user # type: ignore[assignment] + + self.assertEqual(add_push_rule_mock.call_count, 2) + + add_push_rule_mock.assert_any_call( + user_id=user_id, + scope="global", + kind=kind, + rule_id=rule_id1, + conditions=conditions, + actions=actions1, + ) + + add_push_rule_mock.assert_any_call( + user_id=user_id, + scope="global", + kind=kind, + rule_id=rule_id2, + conditions=conditions, + actions=actions2, + ) From 54c8db55f7cb4607ee76db326f89af6d0eb3e97d Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 7 Apr 2022 17:44:19 +0100 Subject: [PATCH 3/9] Doc --- README.md | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 57086bc..f9a0f15 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,22 @@ Then alter your homeserver configuration, adding to your `modules` configuration modules: - module: synapse_patch_push_rules.PushRulesPatcher config: - # TODO: Complete this section with an example for your module + # Rules to set when new users register. + # Required. + rules: + # The rule ID. Must be unique within rules of the same kind. + my_rule: + # See https://spec.matrix.org/latest/client-server-api/#push-rules for a + # reference on the allowed values and format for 'kind', 'conditions' and + # 'actions'. + # 'kind', 'conditions' and 'actions' are all required. + kind: "content" + conditions: + - kind: "event_match" + key: "content.body" + pattern: "testword" + actions: + - "notify" ``` @@ -75,11 +90,7 @@ Synapse developers (assuming a Unix-like shell): git push origin tag v$version ``` - 7. If applicable: - Create a *release*, based on the tag you just pushed, on GitHub or GitLab. - - 8. If applicable: - Create a source distribution and upload it to PyPI: + 7. Create a source distribution and upload it to PyPI: ```shell python -m build twine upload dist/synapse_patch_push_rules-$version* From 262b855ab5d00c6ab32f3a7604d5eda78dc8fe64 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 7 Apr 2022 17:49:17 +0100 Subject: [PATCH 4/9] Fix lint and tests --- setup.cfg | 4 ++-- tests/__init__.py | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/setup.cfg b/setup.cfg index 17ea243..d810bd8 100644 --- a/setup.cfg +++ b/setup.cfg @@ -29,9 +29,9 @@ dev = twisted aiounittest # for type checking - mypy == 0.910 + mypy == 0.931 # for linting - black == 21.9b0 + black == 22.3.0 flake8 == 4.0.1 isort == 5.9.3 diff --git a/tests/__init__.py b/tests/__init__.py index c0de0c8..18c2f38 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -38,6 +38,7 @@ def create_module(config: Optional[Dict[str, Any]] = None) -> PushRulesPatcher: # because some capabilities are needed for running the tests. module_api = Mock(spec=ModuleApi) module_api.add_push_rule_for_user = Mock(return_value=make_awaitable(None)) + module_api.worker_app = None # If necessary, give parse_config some configuration to parse. if config is None: From 3d1fdee98e8be6d1d7c1292ae7858191826c6182 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 8 Apr 2022 15:25:07 +0100 Subject: [PATCH 5/9] Edit the actions of existing rules rather than creating new ones --- README.md | 17 +++++++---------- synapse_patch_push_rules/__init__.py | 22 +++++++++++++++------- tests/__init__.py | 2 +- tests/test_push_rules_patcher.py | 19 ++++++------------- 4 files changed, 29 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index f9a0f15..dfd69e3 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # Push rules patcher -Synapse module to create specific push rules when a new user registers +Synapse module to change the actions of specific push rules when a new user registers. ## Installation @@ -17,20 +17,17 @@ Then alter your homeserver configuration, adding to your `modules` configuration modules: - module: synapse_patch_push_rules.PushRulesPatcher config: - # Rules to set when new users register. + # Rules to change with new actions when new users register. # Required. rules: - # The rule ID. Must be unique within rules of the same kind. + # The rule ID. Must be one of the predefined rules defined in the Matrix + # specification. See https://spec.matrix.org/latest/client-server-api/#predefined-rules + # for a complete list. my_rule: # See https://spec.matrix.org/latest/client-server-api/#push-rules for a - # reference on the allowed values and format for 'kind', 'conditions' and - # 'actions'. - # 'kind', 'conditions' and 'actions' are all required. + # reference on the allowed values and format for 'kind' and 'actions'. + # 'kind' and 'actions' are both required. kind: "content" - conditions: - - kind: "event_match" - key: "content.body" - pattern: "testword" actions: - "notify" ``` diff --git a/synapse_patch_push_rules/__init__.py b/synapse_patch_push_rules/__init__.py index b53b03e..f04677d 100644 --- a/synapse_patch_push_rules/__init__.py +++ b/synapse_patch_push_rules/__init__.py @@ -24,16 +24,13 @@ @attr.s(auto_attribs=True, frozen=True) class PushRule: kind: str - conditions: List[Dict[str, Any]] - actions: List[Union[str, Dict[str, str]]] + actions: Dict[str, List[Union[str, Dict[str, str]]]] class PushRulesPatcherConfig: def __init__(self, config: Dict[str, Any]) -> None: self.rules: Dict[str, PushRule] = {} - - rules = config["rules"] - for rule_id, rule in rules.items(): + for rule_id, rule in config["rules"].items(): self.rules[rule_id] = PushRule(**rule) @@ -43,6 +40,18 @@ def __init__(self, config: PushRulesPatcherConfig, api: ModuleApi): self._api = api self._config = config + # Check that the actions are valid. + for rule_id, rule in self._config.rules.items(): + if not rule_id.startswith("."): + raise ConfigError( + "Only the rules predefined in the Matrix specification are supported" + " by this module. See https://spec.matrix.org/latest/client-server-api/#predefined-rules" + " for a complete list." + ) + + if self._api.check_push_rule_actions(rule.actions) is False: + raise ConfigError("Invalid actions for rule %s" % rule_id) + self._api.register_account_validity_callbacks( on_user_registration=self.set_push_rules_for_user, ) @@ -71,11 +80,10 @@ async def set_push_rules_for_user(self, user_id: str) -> None: return for rule_id, rule in self._config.rules.items(): - await self._api.add_push_rule_for_user( + await self._api.set_push_rule_action( user_id=user_id, scope="global", kind=rule.kind, rule_id=rule_id, - conditions=rule.conditions, actions=rule.actions, ) diff --git a/tests/__init__.py b/tests/__init__.py index 18c2f38..c955f1c 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -37,7 +37,7 @@ def create_module(config: Optional[Dict[str, Any]] = None) -> PushRulesPatcher: # Create a mock based on the ModuleApi spec, but override some mocked functions # because some capabilities are needed for running the tests. module_api = Mock(spec=ModuleApi) - module_api.add_push_rule_for_user = Mock(return_value=make_awaitable(None)) + module_api.set_push_rule_action = Mock(return_value=make_awaitable(None)) module_api.worker_app = None # If necessary, give parse_config some configuration to parse. diff --git a/tests/test_push_rules_patcher.py b/tests/test_push_rules_patcher.py index d75af82..b80d8db 100644 --- a/tests/test_push_rules_patcher.py +++ b/tests/test_push_rules_patcher.py @@ -34,15 +34,12 @@ async def test_rules_no_dict(self) -> None: with self.assertRaises(ConfigError): create_module({"rules": "hello"}) - async def test_create_rules(self) -> None: + async def test_set_rule_actions(self) -> None: """Tests that, when configuring the module with multiple rules and calling the - callback, each rule is created for the newly registered user. + callback, each rule is changed for the newly registered user. """ rule_id1 = "foo" rule_id2 = "bar" - conditions = [ - {"kind": "event_match", "key": "content.body", "pattern": "testword"} - ] actions1 = ["dont_notify"] actions2 = ["notify"] kind = "content" @@ -54,12 +51,10 @@ async def test_create_rules(self) -> None: "rules": { rule_id1: { "kind": kind, - "conditions": conditions, "actions": actions1, }, rule_id2: { "kind": kind, - "conditions": conditions, "actions": actions2, }, } @@ -70,24 +65,22 @@ async def test_create_rules(self) -> None: await module.set_push_rules_for_user(user_id) # Check the module API got called twice, and with the right arguments. - add_push_rule_mock: Mock = module._api.add_push_rule_for_user # type: ignore[assignment] + set_push_rule_action_mock: Mock = module._api.set_push_rule_action # type: ignore[assignment] - self.assertEqual(add_push_rule_mock.call_count, 2) + self.assertEqual(set_push_rule_action_mock.call_count, 2) - add_push_rule_mock.assert_any_call( + set_push_rule_action_mock.assert_any_call( user_id=user_id, scope="global", kind=kind, rule_id=rule_id1, - conditions=conditions, actions=actions1, ) - add_push_rule_mock.assert_any_call( + set_push_rule_action_mock.assert_any_call( user_id=user_id, scope="global", kind=kind, rule_id=rule_id2, - conditions=conditions, actions=actions2, ) From db2b9cd864a259d91d3a09d085d3c14b3de46ce8 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 8 Apr 2022 15:31:12 +0100 Subject: [PATCH 6/9] Fix test --- tests/test_push_rules_patcher.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_push_rules_patcher.py b/tests/test_push_rules_patcher.py index b80d8db..8a89ab4 100644 --- a/tests/test_push_rules_patcher.py +++ b/tests/test_push_rules_patcher.py @@ -38,11 +38,11 @@ async def test_set_rule_actions(self) -> None: """Tests that, when configuring the module with multiple rules and calling the callback, each rule is changed for the newly registered user. """ - rule_id1 = "foo" - rule_id2 = "bar" + rule_id1 = ".m.rule.message" + rule_id2 = ".m.rule.encrypted" actions1 = ["dont_notify"] actions2 = ["notify"] - kind = "content" + kind = "underride" user_id = "@user:example.com" # Configure the module with 2 different rules. From 64dd9c59cd0c611613509b16bcd5e07ea713560c Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 8 Apr 2022 15:34:00 +0100 Subject: [PATCH 7/9] Fix doc --- README.md | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index dfd69e3..81c7651 100644 --- a/README.md +++ b/README.md @@ -23,13 +23,16 @@ modules: # The rule ID. Must be one of the predefined rules defined in the Matrix # specification. See https://spec.matrix.org/latest/client-server-api/#predefined-rules # for a complete list. - my_rule: + ".m.rule.message": + # The kind (override, underride or content) of the rule being modified. + # Required. + kind: "underride" + # The new actions for this rule. # See https://spec.matrix.org/latest/client-server-api/#push-rules for a - # reference on the allowed values and format for 'kind' and 'actions'. - # 'kind' and 'actions' are both required. - kind: "content" + # reference on the allowed values and format. + # Required. actions: - - "notify" + - "dont_notify" ``` From 199794768ceddb45e8a591aad34d5e2225261ca8 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 8 Apr 2022 15:36:00 +0100 Subject: [PATCH 8/9] Add a mock for check_push_rule_actions --- tests/__init__.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/__init__.py b/tests/__init__.py index c955f1c..f32da25 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -39,6 +39,7 @@ def create_module(config: Optional[Dict[str, Any]] = None) -> PushRulesPatcher: module_api = Mock(spec=ModuleApi) module_api.set_push_rule_action = Mock(return_value=make_awaitable(None)) module_api.worker_app = None + module_api.check_push_rule_actions = Mock(return_value=True) # If necessary, give parse_config some configuration to parse. if config is None: From b5b9d8b93487f0fe9235763d38cdd32b4044f309 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 25 Apr 2022 17:44:56 +0100 Subject: [PATCH 9/9] Incorporate review --- synapse_patch_push_rules/__init__.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/synapse_patch_push_rules/__init__.py b/synapse_patch_push_rules/__init__.py index f04677d..7469fa9 100644 --- a/synapse_patch_push_rules/__init__.py +++ b/synapse_patch_push_rules/__init__.py @@ -16,7 +16,7 @@ import attr from synapse.module_api import ModuleApi -from synapse.module_api.errors import ConfigError +from synapse.module_api.errors import ConfigError, InvalidRuleException logger = logging.getLogger(__name__) @@ -24,7 +24,7 @@ @attr.s(auto_attribs=True, frozen=True) class PushRule: kind: str - actions: Dict[str, List[Union[str, Dict[str, str]]]] + actions: List[Union[str, Dict[str, str]]] class PushRulesPatcherConfig: @@ -49,8 +49,10 @@ def __init__(self, config: PushRulesPatcherConfig, api: ModuleApi): " for a complete list." ) - if self._api.check_push_rule_actions(rule.actions) is False: - raise ConfigError("Invalid actions for rule %s" % rule_id) + try: + self._api.check_push_rule_actions(rule.actions) + except InvalidRuleException as e: + raise ConfigError("Invalid actions for rule %s: %s" % (rule_id, e)) self._api.register_account_validity_callbacks( on_user_registration=self.set_push_rules_for_user,