From 6688341e3057f7b1f59c66e2d3a02c1e8498d902 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 7 Apr 2022 11:31:58 +0100 Subject: [PATCH 01/14] Add a module API to allow modules to add push rules for local users --- synapse/module_api/__init__.py | 94 ++++++++++++++++++++++++++++++++++ synapse/module_api/errors.py | 5 ++ tests/module_api/test_api.py | 71 ++++++++++++++++++++++++- 3 files changed, 169 insertions(+), 1 deletion(-) diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 9a61593ff511..31daa2c487ba 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -97,6 +97,14 @@ ) from synapse.metrics.background_process_metrics import run_as_background_process from synapse.rest.client.login import LoginResponse +from synapse.rest.client.push_rule import ( + InvalidRuleException, + RuleSpec, + _check_actions, + _namespaced_rule_id, + _namespaced_rule_id_from_spec, + _priority_class_from_spec, +) from synapse.storage import DataStore from synapse.storage.background_updates import ( DEFAULT_BATCH_SIZE_CALLBACK, @@ -192,6 +200,7 @@ def __init__(self, hs: "HomeServer", auth_handler: AuthHandler) -> None: self._clock: Clock = hs.get_clock() self._registration_handler = hs.get_registration_handler() self._send_email_handler = hs.get_send_email_handler() + self._notifier = hs.get_notifier() self.custom_template_dir = hs.config.server.custom_template_directory try: @@ -1340,6 +1349,91 @@ async def store_remote_3pid_association( """ await self._store.add_user_bound_threepid(user_id, medium, address, id_server) + async def add_push_rule_for_user( + self, + user_id: str, + scope: str, + kind: str, + rule_id: str, + conditions: List[Dict[str, Any]], + actions: List[Union[str, Dict[str, str]]], + before: Optional[str] = None, + after: Optional[str] = None, + ) -> None: + """Adds a new push rule for the given user. + + See https://spec.matrix.org/latest/client-server-api/#push-rules for more + information about the push rules syntax. + + This method can only be called on the main process. + + Added in Synapse v1.58.0. + + Args: + user_id: The ID of the user to add the push rule for. + scope: The push rule's scope. Currently, the only supported scope is "global". + kind: The rule's kind. + rule_id: The rule's identifier. + conditions: The conditions to match to trigger the actions. + actions: The actions to trigger if all conditions are met. + before: If set, the new rule will be the next most important rule relative to + the given rule. Must be a user-defined rule (as opposed to a server-defined + one). + after: If set, the new rule will be the next least important rule relative to + the given rule. Must be a user-defined rule (as opposed to a server-defined + one). + + Raises: + synapse.module_api.errors.SynapseError if the module attempts to add a push + rule on a worker, or to add a push rule for a remote user. + synapse.module_api.errors.InvalidRuleException if the rule is not compliant + with the Matrix spec. + synapse.module_api.errors.RuleNotFoundException if the rule referred to by + before or after can't be found. + synapse.module_api.errors.InconsistentRuleException if the priority of the + rule's kind is not compatible with those of the rules referred to by + before or after. + """ + if not self.is_mine(user_id): + raise SynapseError(500, "Can only create push rules for local users") + + if self.worker_app is not None: + raise SynapseError(500, "Can't create push rules on a worker") + + # At this point we know we're on the main process so the store should not be a + # GenericWorkerSlavedStore. + assert isinstance(self._store, DataStore) + + spec = RuleSpec(scope, kind, rule_id, None) + priority_class = _priority_class_from_spec(spec) + + if spec.rule_id.startswith("."): + # Rule ids starting with '.' are reserved for server default rules. + raise InvalidRuleException( + 400, "cannot add new rule_ids that start with '.'" + ) + + if before: + before = _namespaced_rule_id(spec, before) + + if after: + after = _namespaced_rule_id(spec, after) + + _check_actions(actions) + + await self._store.add_push_rule( + user_id=user_id, + rule_id=_namespaced_rule_id_from_spec(spec), + priority_class=priority_class, + conditions=conditions, + actions=actions, + before=before, + after=after, + ) + + stream_id = self._store.get_max_push_rules_stream_id() + self._notifier.on_new_event("push_rules_key", stream_id, users=[user_id]) + class PublicRoomListManager: """Contains methods for adding to, removing from and querying whether a room diff --git a/synapse/module_api/errors.py b/synapse/module_api/errors.py index 1db900e41f64..a71e4b1ae3ed 100644 --- a/synapse/module_api/errors.py +++ b/synapse/module_api/errors.py @@ -20,10 +20,15 @@ SynapseError, ) from synapse.config._base import ConfigError +from synapse.rest.client.push_rule import InvalidRuleException +from synapse.storage.push_rule import InconsistentRuleException, RuleNotFoundException __all__ = [ "InvalidClientCredentialsError", "RedirectException", "SynapseError", "ConfigError", + "InvalidRuleException", + "RuleNotFoundException", + "InconsistentRuleException", ] diff --git a/tests/module_api/test_api.py b/tests/module_api/test_api.py index 9fd5d59c55b2..93c9d3a2432c 100644 --- a/tests/module_api/test_api.py +++ b/tests/module_api/test_api.py @@ -20,7 +20,7 @@ from synapse.federation.units import Transaction from synapse.handlers.presence import UserPresenceState from synapse.rest import admin -from synapse.rest.client import login, presence, profile, room +from synapse.rest.client import login, notifications, presence, profile, room from synapse.types import create_requester from tests.events.test_presence_router import send_presence_update, sync_presence @@ -38,6 +38,7 @@ class ModuleApiTestCase(HomeserverTestCase): room.register_servlets, presence.register_servlets, profile.register_servlets, + notifications.register_servlets, ] def prepare(self, reactor, clock, homeserver): @@ -553,6 +554,74 @@ def test_get_room_state(self): self.assertEqual(state[("org.matrix.test", "")].state_key, "") self.assertEqual(state[("org.matrix.test", "")].content, {}) + def test_add_push_rule(self) -> None: + """Test that a module can set a custom push rule for a user.""" + + # Create a room with 2 users in it. Push rules must not match if the user is the + # event's sender, so we need one user to send messages and one user to receive + # notifications. + user_id = self.register_user("user", "password") + tok = self.login("user", "password") + + room_id = self.helper.create_room_as(user_id, is_public=True, tok=tok) + + user_id2 = self.register_user("user2", "password") + tok2 = self.login("user2", "password") + self.helper.join(room_id, user_id2, tok=tok2) + + # Set a push rule that doesn't notify for events with the word "testword" in it. + # This makes sense because we notify for every message by default, so to test + # that our change had any impact the easiest way is to not notify on something. + self.get_success( + defer.ensureDeferred( + self.module_api.add_push_rule_for_user( + user_id=user_id, + scope="global", + kind="content", + rule_id="test", + conditions=[ + { + "kind": "event_match", + "key": "content.body", + "pattern": "testword", + } + ], + actions=["dont_notify"], + ) + ) + ) + + # Send a message as the second user containing this word, and check that it + # didn't notify. + self.helper.send(room_id=room_id, body="here's a testword", tok=tok2) + + channel = self.make_request( + "GET", + "/notifications", + access_token=tok, + ) + self.assertEqual(channel.code, 200, channel.result) + self.assertEqual(len(channel.json_body["notifications"]), 0) + + # Send a message as the second user not containing "testword" and check that it + # still notifies. + res = self.helper.send(room_id=room_id, body="here's a word", tok=tok2) + event_id = res["event_id"] + + channel = self.make_request( + "GET", + "/notifications", + access_token=tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + self.assertEqual(len(channel.json_body["notifications"]), 1, channel.json_body) + self.assertEqual( + channel.json_body["notifications"][0]["event"]["event_id"], + event_id, + channel.json_body, + ) + class ModuleApiWorkerTestCase(BaseMultiWorkerStreamTestCase): """For testing ModuleApi functionality in a multi-worker setup""" From 9b15307e691417fd958126c7daccddba46c49b6e Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 7 Apr 2022 13:32:32 +0100 Subject: [PATCH 02/14] Changelog --- changelog.d/12406.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/12406.feature diff --git a/changelog.d/12406.feature b/changelog.d/12406.feature new file mode 100644 index 000000000000..dcdcbad50d7c --- /dev/null +++ b/changelog.d/12406.feature @@ -0,0 +1 @@ +Add a module API to allow modules to create new push rules for local users. From 40a0fed2641b4870018220b5763b4f2bc0ecd7dc Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 8 Apr 2022 14:54:22 +0100 Subject: [PATCH 03/14] Create a push rules handler --- synapse/handlers/push_rules.py | 129 +++++++++++++++++++++++++++++++ synapse/rest/client/push_rule.py | 117 ++++++---------------------- synapse/server.py | 5 ++ 3 files changed, 156 insertions(+), 95 deletions(-) create mode 100644 synapse/handlers/push_rules.py diff --git a/synapse/handlers/push_rules.py b/synapse/handlers/push_rules.py new file mode 100644 index 000000000000..4a94ade2699b --- /dev/null +++ b/synapse/handlers/push_rules.py @@ -0,0 +1,129 @@ +# 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 typing import TYPE_CHECKING, List, Optional, Union + +import attr + +from synapse.api.errors import NotFoundError, SynapseError, UnrecognizedRequestError +from synapse.push.baserules import BASE_RULE_IDS +from synapse.types import JsonDict + +if TYPE_CHECKING: + from synapse.server import HomeServer + + +@attr.s(slots=True, frozen=True, auto_attribs=True) +class RuleSpec: + scope: str + template: str + rule_id: str + attr: Optional[str] + + +class PushRulesHandler: + def __init__(self, hs: "HomeServer"): + self._notifier = hs.get_notifier() + self._store = hs.get_datastores().main + + async def set_rule_attr( + self, user_id: str, spec: RuleSpec, val: Union[bool, JsonDict] + ) -> None: + """Set an attribute (enabled or actions) on an existing push rule. + + Args: + user_id: the user for which to modify the push rule. + spec: the spec of the push rule to modify. + val: the value to change the attribute to. + """ + if spec.attr not in ("enabled", "actions"): + # for the sake of potential future expansion, shouldn't report + # 404 in the case of an unknown request so check it corresponds to + # a known attribute first. + raise UnrecognizedRequestError() + + namespaced_rule_id = namespaced_rule_id_from_spec(spec) + rule_id = spec.rule_id + is_default_rule = rule_id.startswith(".") + if is_default_rule: + if namespaced_rule_id not in BASE_RULE_IDS: + raise NotFoundError("Unknown rule %s" % (namespaced_rule_id,)) + if spec.attr == "enabled": + if isinstance(val, dict) and "enabled" in val: + val = val["enabled"] + if not isinstance(val, bool): + # Legacy fallback + # This should *actually* take a dict, but many clients pass + # bools directly, so let's not break them. + raise SynapseError(400, "Value for 'enabled' must be boolean") + await self._store.set_push_rule_enabled( + user_id, namespaced_rule_id, val, is_default_rule + ) + elif spec.attr == "actions": + if not isinstance(val, dict): + raise SynapseError(400, "Value must be a dict") + actions = val.get("actions") + if not isinstance(actions, list): + raise SynapseError(400, "Value for 'actions' must be dict") + check_actions(actions) + rule_id = spec.rule_id + is_default_rule = rule_id.startswith(".") + if is_default_rule: + if namespaced_rule_id not in BASE_RULE_IDS: + raise SynapseError(404, "Unknown rule %r" % (namespaced_rule_id,)) + await self._store.set_push_rule_actions( + user_id, namespaced_rule_id, actions, is_default_rule + ) + else: + raise UnrecognizedRequestError() + + def notify_user(self, user_id: str) -> None: + stream_id = self._store.get_max_push_rules_stream_id() + self._notifier.on_new_event("push_rules_key", stream_id, users=[user_id]) + + +def check_actions(actions: List[Union[str, JsonDict]]) -> None: + """Check if the given actions are spec compliant. + + Args: + actions: the actions to check. + + Raises: + InvalidRuleException if the rules aren't compliant with the spec. + """ + if not isinstance(actions, list): + raise InvalidRuleException("No actions found") + + for a in actions: + if a in ["notify", "dont_notify", "coalesce"]: + pass + elif isinstance(a, dict) and "set_tweak" in a: + pass + else: + raise InvalidRuleException("Unrecognised action") + + +def namespaced_rule_id_from_spec(spec: RuleSpec) -> str: + """Generates a scope/kind/rule_id representation of a rule using only its spec.""" + return namespaced_rule_id(spec, spec.rule_id) + + +def namespaced_rule_id(spec: RuleSpec, rule_id: str) -> str: + """Generates a scope/kind/rule_id representation of a rule based on another rule's + spec and a rule ID. + """ + return "global/%s/%s" % (spec.template, rule_id) + + +class InvalidRuleException(Exception): + pass diff --git a/synapse/rest/client/push_rule.py b/synapse/rest/client/push_rule.py index a93f6fd5e0a8..5c7fe99b9052 100644 --- a/synapse/rest/client/push_rule.py +++ b/synapse/rest/client/push_rule.py @@ -12,9 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -from typing import TYPE_CHECKING, List, Optional, Sequence, Tuple, Union - -import attr +from typing import TYPE_CHECKING, List, Sequence, Tuple, Union from synapse.api.errors import ( NotFoundError, @@ -22,6 +20,13 @@ SynapseError, UnrecognizedRequestError, ) +from synapse.handlers.push_rules import ( + InvalidRuleException, + RuleSpec, + check_actions, + namespaced_rule_id, + namespaced_rule_id_from_spec, +) from synapse.http.server import HttpServer from synapse.http.servlet import ( RestServlet, @@ -29,7 +34,6 @@ parse_string, ) from synapse.http.site import SynapseRequest -from synapse.push.baserules import BASE_RULE_IDS from synapse.push.clientformat import format_push_rules_for_user from synapse.push.rulekinds import PRIORITY_CLASS_MAP from synapse.rest.client._base import client_patterns @@ -40,14 +44,6 @@ from synapse.server import HomeServer -@attr.s(slots=True, frozen=True, auto_attribs=True) -class RuleSpec: - scope: str - template: str - rule_id: str - attr: Optional[str] - - class PushRuleRestServlet(RestServlet): PATTERNS = client_patterns("/(?Ppushrules/.*)$", v1=True) SLIGHTLY_PEDANTIC_TRAILING_SLASH_ERROR = ( @@ -60,6 +56,7 @@ def __init__(self, hs: "HomeServer"): self.store = hs.get_datastores().main self.notifier = hs.get_notifier() self._is_worker = hs.config.worker.worker_app is not None + self._push_rules_handler = hs.get_push_rules_handler() async def on_PUT(self, request: SynapseRequest, path: str) -> Tuple[int, JsonDict]: if self._is_worker: @@ -81,8 +78,12 @@ async def on_PUT(self, request: SynapseRequest, path: str) -> Tuple[int, JsonDic user_id = requester.user.to_string() if spec.attr: - await self.set_rule_attr(user_id, spec, content) - self.notify_user(user_id) + try: + await self._push_rules_handler.set_rule_attr(user_id, spec, content) + except InvalidRuleException as e: + raise SynapseError(400, "Invalid actions: %s" % e) + + self._push_rules_handler.notify_user(user_id) return 200, {} if spec.rule_id.startswith("."): @@ -98,23 +99,23 @@ async def on_PUT(self, request: SynapseRequest, path: str) -> Tuple[int, JsonDic before = parse_string(request, "before") if before: - before = _namespaced_rule_id(spec, before) + before = namespaced_rule_id(spec, before) after = parse_string(request, "after") if after: - after = _namespaced_rule_id(spec, after) + after = namespaced_rule_id(spec, after) try: await self.store.add_push_rule( user_id=user_id, - rule_id=_namespaced_rule_id_from_spec(spec), + rule_id=namespaced_rule_id_from_spec(spec), priority_class=priority_class, conditions=conditions, actions=actions, before=before, after=after, ) - self.notify_user(user_id) + self._push_rules_handler.notify_user(user_id) except InconsistentRuleException as e: raise SynapseError(400, str(e)) except RuleNotFoundException as e: @@ -133,11 +134,11 @@ async def on_DELETE( requester = await self.auth.get_user_by_req(request) user_id = requester.user.to_string() - namespaced_rule_id = _namespaced_rule_id_from_spec(spec) + namespaced_rule_id = namespaced_rule_id_from_spec(spec) try: await self.store.delete_push_rule(user_id, namespaced_rule_id) - self.notify_user(user_id) + self._push_rules_handler.notify_user(user_id) return 200, {} except StoreError as e: if e.code == 404: @@ -172,55 +173,6 @@ async def on_GET(self, request: SynapseRequest, path: str) -> Tuple[int, JsonDic else: raise UnrecognizedRequestError() - def notify_user(self, user_id: str) -> None: - stream_id = self.store.get_max_push_rules_stream_id() - self.notifier.on_new_event("push_rules_key", stream_id, users=[user_id]) - - async def set_rule_attr( - self, user_id: str, spec: RuleSpec, val: Union[bool, JsonDict] - ) -> None: - if spec.attr not in ("enabled", "actions"): - # for the sake of potential future expansion, shouldn't report - # 404 in the case of an unknown request so check it corresponds to - # a known attribute first. - raise UnrecognizedRequestError() - - namespaced_rule_id = _namespaced_rule_id_from_spec(spec) - rule_id = spec.rule_id - is_default_rule = rule_id.startswith(".") - if is_default_rule: - if namespaced_rule_id not in BASE_RULE_IDS: - raise NotFoundError("Unknown rule %s" % (namespaced_rule_id,)) - if spec.attr == "enabled": - if isinstance(val, dict) and "enabled" in val: - val = val["enabled"] - if not isinstance(val, bool): - # Legacy fallback - # This should *actually* take a dict, but many clients pass - # bools directly, so let's not break them. - raise SynapseError(400, "Value for 'enabled' must be boolean") - await self.store.set_push_rule_enabled( - user_id, namespaced_rule_id, val, is_default_rule - ) - elif spec.attr == "actions": - if not isinstance(val, dict): - raise SynapseError(400, "Value must be a dict") - actions = val.get("actions") - if not isinstance(actions, list): - raise SynapseError(400, "Value for 'actions' must be dict") - _check_actions(actions) - namespaced_rule_id = _namespaced_rule_id_from_spec(spec) - rule_id = spec.rule_id - is_default_rule = rule_id.startswith(".") - if is_default_rule: - if namespaced_rule_id not in BASE_RULE_IDS: - raise SynapseError(404, "Unknown rule %r" % (namespaced_rule_id,)) - await self.store.set_push_rule_actions( - user_id, namespaced_rule_id, actions, is_default_rule - ) - else: - raise UnrecognizedRequestError() - def _rule_spec_from_path(path: Sequence[str]) -> RuleSpec: """Turn a sequence of path components into a rule spec @@ -291,24 +243,11 @@ def _rule_tuple_from_request_object( raise InvalidRuleException("No actions found") actions = req_obj["actions"] - _check_actions(actions) + check_actions(actions) return conditions, actions -def _check_actions(actions: List[Union[str, JsonDict]]) -> None: - if not isinstance(actions, list): - raise InvalidRuleException("No actions found") - - for a in actions: - if a in ["notify", "dont_notify", "coalesce"]: - pass - elif isinstance(a, dict) and "set_tweak" in a: - pass - else: - raise InvalidRuleException("Unrecognised action") - - def _filter_ruleset_with_path(ruleset: JsonDict, path: List[str]) -> JsonDict: if path == []: raise UnrecognizedRequestError( @@ -357,17 +296,5 @@ def _priority_class_from_spec(spec: RuleSpec) -> int: return pc -def _namespaced_rule_id_from_spec(spec: RuleSpec) -> str: - return _namespaced_rule_id(spec, spec.rule_id) - - -def _namespaced_rule_id(spec: RuleSpec, rule_id: str) -> str: - return "global/%s/%s" % (spec.template, rule_id) - - -class InvalidRuleException(Exception): - pass - - def register_servlets(hs: "HomeServer", http_server: HttpServer) -> None: PushRuleRestServlet(hs).register(http_server) diff --git a/synapse/server.py b/synapse/server.py index 380369db923e..49c4ada263ee 100644 --- a/synapse/server.py +++ b/synapse/server.py @@ -91,6 +91,7 @@ WorkerPresenceHandler, ) from synapse.handlers.profile import ProfileHandler +from synapse.handlers.push_rules import PushRulesHandler from synapse.handlers.read_marker import ReadMarkerHandler from synapse.handlers.receipts import ReceiptsHandler from synapse.handlers.register import RegistrationHandler @@ -850,3 +851,7 @@ def get_request_ratelimiter(self) -> RequestRatelimiter: self.config.ratelimiting.rc_message, self.config.ratelimiting.rc_admin_redaction, ) + + @cache_in_self + def get_push_rules_handler(self) -> PushRulesHandler: + return PushRulesHandler(self) From 7a0b2cc018b800acfadfcfde1e890f78558b261f Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 8 Apr 2022 14:54:54 +0100 Subject: [PATCH 04/14] Replace the callback with one to set actions --- synapse/module_api/__init__.py | 119 +++++++++++++-------------------- synapse/module_api/errors.py | 7 +- tests/module_api/test_api.py | 72 ++++++++++---------- 3 files changed, 85 insertions(+), 113 deletions(-) diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 31daa2c487ba..985e6798876d 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -82,6 +82,7 @@ ON_LOGGED_OUT_CALLBACK, AuthHandler, ) +from synapse.handlers.push_rules import InvalidRuleException, RuleSpec, check_actions from synapse.http.client import SimpleHttpClient from synapse.http.server import ( DirectServeHtmlResource, @@ -97,14 +98,6 @@ ) from synapse.metrics.background_process_metrics import run_as_background_process from synapse.rest.client.login import LoginResponse -from synapse.rest.client.push_rule import ( - InvalidRuleException, - RuleSpec, - _check_actions, - _namespaced_rule_id, - _namespaced_rule_id_from_spec, - _priority_class_from_spec, -) from synapse.storage import DataStore from synapse.storage.background_updates import ( DEFAULT_BATCH_SIZE_CALLBACK, @@ -200,7 +193,7 @@ def __init__(self, hs: "HomeServer", auth_handler: AuthHandler) -> None: self._clock: Clock = hs.get_clock() self._registration_handler = hs.get_registration_handler() self._send_email_handler = hs.get_send_email_handler() - self._notifier = hs.get_notifier() + self._push_rules_handler = hs.get_push_rules_handler() self.custom_template_dir = hs.config.server.custom_template_directory try: @@ -1349,90 +1342,74 @@ async def store_remote_3pid_association( """ await self._store.add_user_bound_threepid(user_id, medium, address, id_server) - async def add_push_rule_for_user( + async def check_push_rule_actions( + self, actions: List[Union[str, Dict[str, str]]] + ) -> bool: + """Checks if the given push rule actions are valid according to the Matrix + specification. + + See https://spec.matrix.org/v1.2/client-server-api/#actions for the list of valid + actions. + + Added in Synapse v1.58.0. + + Args: + actions: the actions to check. + + Returns: + True if the actions are valid, False otherwise. + """ + try: + check_actions(actions) + except InvalidRuleException: + return False + + return True + + async def set_push_rule_action( self, user_id: str, scope: str, kind: str, rule_id: str, - conditions: List[Dict[str, Any]], actions: List[Union[str, Dict[str, str]]], - before: Optional[str] = None, - after: Optional[str] = None, ) -> None: - """Adds a new push rule for the given user. + """Changes the actions of an existing push rule for the given user. - See https://spec.matrix.org/latest/client-server-api/#push-rules for more - information about the push rules syntax. + See https://spec.matrix.org/v1.2/client-server-api/#push-rules for more + information about push rules and their syntax. - This method can only be called on the main process. + Can only be called on the main process. Added in Synapse v1.58.0. Args: - user_id: The ID of the user to add the push rule for. - scope: The push rule's scope. Currently, the only supported scope is "global". - kind: The rule's kind. - rule_id: The rule's identifier. - conditions: The conditions to match to trigger the actions. - actions: The actions to trigger if all conditions are met. - before: If set, the new rule will be the next most important rule relative to - the given rule. Must be a user-defined rule (as opposed to a server-defined - one). - after: If set, the new rule will be the next least important rule relative to - the given rule. Must be a user-defined rule (as opposed to a server-defined - one). + user_id: the user for which to change the push rule's actions. + scope: the push rule's scope, currently only "global" is allowed. + kind: the push rule's kind. + rule_id: the push rule's identifier. + actions: the actions to run when the rule's conditions match. Raises: - synapse.module_api.errors.SynapseError if the module attempts to add a push - rule on a worker, or to add a push rule for a remote user. - synapse.module_api.errors.InvalidRuleException if the rule is not compliant - with the Matrix spec. - synapse.module_api.errors.RuleNotFoundException if the rule referred to by - before or after can't be found. - synapse.module_api.errors.InconsistentRuleException if the priority of the - rule's kind is not compatible with those of the rules referred to by - before or after. + RuntimeError if this method is called on a worker. + synapse.module_api.errors.NotFoundError if the rule being modified can't be + found. + synapse.module_api.errors.InvalidRuleException if the actions are invalid. """ - if not self.is_mine(user_id): - raise SynapseError(500, "Can only create push rules for local users") - if self.worker_app is not None: - raise SynapseError(500, "Can't create push rules on a worker") - - # At this point we know we're on the main process so the store should not be a - # GenericWorkerSlavedStore. - assert isinstance(self._store, DataStore) + raise RuntimeError("module tried to change push rule actions on a worker") - spec = RuleSpec(scope, kind, rule_id, None) - priority_class = _priority_class_from_spec(spec) - - if spec.rule_id.startswith("."): - # Rule ids starting with '.' are reserved for server default rules. - raise InvalidRuleException( - 400, "cannot add new rule_ids that start with '.'" + if scope != "global": + raise RuntimeError( + "invalid scope %s, only 'global' is currently allowed" % scope ) - if before: - before = _namespaced_rule_id(spec, before) - - if after: - after = _namespaced_rule_id(spec, after) - - _check_actions(actions) - - await self._store.add_push_rule( - user_id=user_id, - rule_id=_namespaced_rule_id_from_spec(spec), - priority_class=priority_class, - conditions=conditions, - actions=actions, - before=before, - after=after, + spec = RuleSpec(scope, kind, rule_id, "actions") + await self._push_rules_handler.set_rule_attr( + user_id, spec, {"actions": actions} ) - stream_id = self._store.get_max_push_rules_stream_id() - self._notifier.on_new_event("push_rules_key", stream_id, users=[user_id]) + self._push_rules_handler.notify_user(user_id) class PublicRoomListManager: diff --git a/synapse/module_api/errors.py b/synapse/module_api/errors.py index a71e4b1ae3ed..51a4782d9260 100644 --- a/synapse/module_api/errors.py +++ b/synapse/module_api/errors.py @@ -16,12 +16,12 @@ from synapse.api.errors import ( InvalidClientCredentialsError, + NotFoundError, RedirectException, SynapseError, ) from synapse.config._base import ConfigError -from synapse.rest.client.push_rule import InvalidRuleException -from synapse.storage.push_rule import InconsistentRuleException, RuleNotFoundException +from synapse.handlers.push_rules import InvalidRuleException __all__ = [ "InvalidClientCredentialsError", @@ -29,6 +29,5 @@ "SynapseError", "ConfigError", "InvalidRuleException", - "RuleNotFoundException", - "InconsistentRuleException", + "NotFoundError", ] diff --git a/tests/module_api/test_api.py b/tests/module_api/test_api.py index 93c9d3a2432c..dc0a16569e94 100644 --- a/tests/module_api/test_api.py +++ b/tests/module_api/test_api.py @@ -554,8 +554,8 @@ def test_get_room_state(self): self.assertEqual(state[("org.matrix.test", "")].state_key, "") self.assertEqual(state[("org.matrix.test", "")].content, {}) - def test_add_push_rule(self) -> None: - """Test that a module can set a custom push rule for a user.""" + def test_set_push_rules_action(self) -> None: + """Test that a module can change the actions of an existing push rule for a user.""" # Create a room with 2 users in it. Push rules must not match if the user is the # event's sender, so we need one user to send messages and one user to receive @@ -569,43 +569,14 @@ def test_add_push_rule(self) -> None: tok2 = self.login("user2", "password") self.helper.join(room_id, user_id2, tok=tok2) - # Set a push rule that doesn't notify for events with the word "testword" in it. - # This makes sense because we notify for every message by default, so to test - # that our change had any impact the easiest way is to not notify on something. - self.get_success( - defer.ensureDeferred( - self.module_api.add_push_rule_for_user( - user_id=user_id, - scope="global", - kind="content", - rule_id="test", - conditions=[ - { - "kind": "event_match", - "key": "content.body", - "pattern": "testword", - } - ], - actions=["dont_notify"], - ) - ) - ) - - # Send a message as the second user containing this word, and check that it - # didn't notify. - self.helper.send(room_id=room_id, body="here's a testword", tok=tok2) + # Register a 3rd user and join them to the room, so that we don't accidentally + # trigger 1:1 push rules. + user_id3 = self.register_user("user3", "password") + tok3 = self.login("user3", "password") + self.helper.join(room_id, user_id3, tok=tok3) - channel = self.make_request( - "GET", - "/notifications", - access_token=tok, - ) - self.assertEqual(channel.code, 200, channel.result) - self.assertEqual(len(channel.json_body["notifications"]), 0) - - # Send a message as the second user not containing "testword" and check that it - # still notifies. - res = self.helper.send(room_id=room_id, body="here's a word", tok=tok2) + # Send a message as the second user and check that it notifies. + res = self.helper.send(room_id=room_id, body="here's a message", tok=tok2) event_id = res["event_id"] channel = self.make_request( @@ -622,6 +593,31 @@ def test_add_push_rule(self) -> None: channel.json_body, ) + # Change the .m.rule.message actions to not notify on new messages. + self.get_success( + defer.ensureDeferred( + self.module_api.set_push_rule_action( + user_id=user_id, + scope="global", + kind="underride", + rule_id=".m.rule.message", + actions=["dont_notify"], + ) + ) + ) + + # Send another message as the second user and check that the number of + # notifications didn't change. + self.helper.send(room_id=room_id, body="here's another message", tok=tok2) + + channel = self.make_request( + "GET", + "/notifications?from=", + access_token=tok, + ) + self.assertEqual(channel.code, 200, channel.result) + self.assertEqual(len(channel.json_body["notifications"]), 1, channel.json_body) + class ModuleApiWorkerTestCase(BaseMultiWorkerStreamTestCase): """For testing ModuleApi functionality in a multi-worker setup""" From 38aa1e01832f3aa55286fad7257657eafebb2157 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 8 Apr 2022 14:56:12 +0100 Subject: [PATCH 05/14] Amend changelog --- changelog.d/12406.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/12406.feature b/changelog.d/12406.feature index dcdcbad50d7c..e345afdee729 100644 --- a/changelog.d/12406.feature +++ b/changelog.d/12406.feature @@ -1 +1 @@ -Add a module API to allow modules to create new push rules for local users. +Add a module API to allow modules to change actions for existing push rules of local users. From 36a3be4099c2f17f897a8c82b0e673a8fcdc59a2 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 8 Apr 2022 15:06:02 +0100 Subject: [PATCH 06/14] Add a test to the actions check --- synapse/module_api/__init__.py | 2 +- tests/module_api/test_api.py | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 985e6798876d..9844e6b7417c 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -1342,7 +1342,7 @@ async def store_remote_3pid_association( """ await self._store.add_user_bound_threepid(user_id, medium, address, id_server) - async def check_push_rule_actions( + def check_push_rule_actions( self, actions: List[Union[str, Dict[str, str]]] ) -> bool: """Checks if the given push rule actions are valid according to the Matrix diff --git a/tests/module_api/test_api.py b/tests/module_api/test_api.py index dc0a16569e94..4c6c209164a7 100644 --- a/tests/module_api/test_api.py +++ b/tests/module_api/test_api.py @@ -618,6 +618,19 @@ def test_set_push_rules_action(self) -> None: self.assertEqual(channel.code, 200, channel.result) self.assertEqual(len(channel.json_body["notifications"]), 1, channel.json_body) + def test_check_push_rules_actions(self) -> None: + """Test that modules can check whether a list of push rules actions are spec + compliant. + """ + self.assertFalse(self.module_api.check_push_rule_actions(["foo"])) + self.assertFalse(self.module_api.check_push_rule_actions({"foo": "bar"})) + self.assertTrue(self.module_api.check_push_rule_actions(["notify"])) + self.assertTrue( + self.module_api.check_push_rule_actions( + [{"set_tweak": "sound", "value": "default"}] + ) + ) + class ModuleApiWorkerTestCase(BaseMultiWorkerStreamTestCase): """For testing ModuleApi functionality in a multi-worker setup""" From 24a71200d40749afefe4fec1a4c3f7b2230a0e4e Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 25 Apr 2022 17:13:41 +0100 Subject: [PATCH 07/14] Incorporate review --- synapse/handlers/push_rules.py | 39 ++++++++++++--------- synapse/module_api/__init__.py | 21 ++++------- synapse/module_api/errors.py | 4 +-- synapse/rest/client/push_rule.py | 19 ++++------ synapse/server.py | 8 ++--- synapse/storage/databases/main/push_rule.py | 7 ++-- 6 files changed, 46 insertions(+), 52 deletions(-) diff --git a/synapse/handlers/push_rules.py b/synapse/handlers/push_rules.py index 4a94ade2699b..5f37a7bccd3a 100644 --- a/synapse/handlers/push_rules.py +++ b/synapse/handlers/push_rules.py @@ -15,8 +15,9 @@ import attr -from synapse.api.errors import NotFoundError, SynapseError, UnrecognizedRequestError +from synapse.api.errors import SynapseError, UnrecognizedRequestError from synapse.push.baserules import BASE_RULE_IDS +from synapse.storage.push_rule import RuleNotFoundException from synapse.types import JsonDict if TYPE_CHECKING: @@ -32,6 +33,8 @@ class RuleSpec: class PushRulesHandler: + """A class to handle changes in push rules for users.""" + def __init__(self, hs: "HomeServer"): self._notifier = hs.get_notifier() self._store = hs.get_datastores().main @@ -45,6 +48,13 @@ async def set_rule_attr( user_id: the user for which to modify the push rule. spec: the spec of the push rule to modify. val: the value to change the attribute to. + + Raises: + RuleNotFoundException if the rule being modified doesn't exist. + SynapseError(400) if the value is malformed. + UnrecognizedRequestError if the attribute to change is unknown. + InvalidRuleException if we're trying to change the actions on a rule but + the provided actions aren't compliant with the spec. """ if spec.attr not in ("enabled", "actions"): # for the sake of potential future expansion, shouldn't report @@ -52,12 +62,12 @@ async def set_rule_attr( # a known attribute first. raise UnrecognizedRequestError() - namespaced_rule_id = namespaced_rule_id_from_spec(spec) + namespaced_rule_id = f"global/{spec.template}/{spec.rule_id}" rule_id = spec.rule_id is_default_rule = rule_id.startswith(".") if is_default_rule: if namespaced_rule_id not in BASE_RULE_IDS: - raise NotFoundError("Unknown rule %s" % (namespaced_rule_id,)) + raise RuleNotFoundException("Unknown rule %r" % (namespaced_rule_id,)) if spec.attr == "enabled": if isinstance(val, dict) and "enabled" in val: val = val["enabled"] @@ -80,14 +90,23 @@ async def set_rule_attr( is_default_rule = rule_id.startswith(".") if is_default_rule: if namespaced_rule_id not in BASE_RULE_IDS: - raise SynapseError(404, "Unknown rule %r" % (namespaced_rule_id,)) + raise RuleNotFoundException( + "Unknown rule %r" % (namespaced_rule_id,) + ) await self._store.set_push_rule_actions( user_id, namespaced_rule_id, actions, is_default_rule ) else: raise UnrecognizedRequestError() + self.notify_user(user_id) + def notify_user(self, user_id: str) -> None: + """Notify listeners about a push rule change. + + Args: + user_id: the user ID the change is for. + """ stream_id = self._store.get_max_push_rules_stream_id() self._notifier.on_new_event("push_rules_key", stream_id, users=[user_id]) @@ -113,17 +132,5 @@ def check_actions(actions: List[Union[str, JsonDict]]) -> None: raise InvalidRuleException("Unrecognised action") -def namespaced_rule_id_from_spec(spec: RuleSpec) -> str: - """Generates a scope/kind/rule_id representation of a rule using only its spec.""" - return namespaced_rule_id(spec, spec.rule_id) - - -def namespaced_rule_id(spec: RuleSpec, rule_id: str) -> str: - """Generates a scope/kind/rule_id representation of a rule based on another rule's - spec and a rule ID. - """ - return "global/%s/%s" % (spec.template, rule_id) - - class InvalidRuleException(Exception): pass diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 9844e6b7417c..2287c9760514 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -82,7 +82,7 @@ ON_LOGGED_OUT_CALLBACK, AuthHandler, ) -from synapse.handlers.push_rules import InvalidRuleException, RuleSpec, check_actions +from synapse.handlers.push_rules import RuleSpec, check_actions from synapse.http.client import SimpleHttpClient from synapse.http.server import ( DirectServeHtmlResource, @@ -1344,7 +1344,7 @@ async def store_remote_3pid_association( def check_push_rule_actions( self, actions: List[Union[str, Dict[str, str]]] - ) -> bool: + ) -> None: """Checks if the given push rule actions are valid according to the Matrix specification. @@ -1356,15 +1356,10 @@ def check_push_rule_actions( Args: actions: the actions to check. - Returns: - True if the actions are valid, False otherwise. + Raises: + synapse.module_api.errors.InvalidRuleException if the actions are invalid. """ - try: - check_actions(actions) - except InvalidRuleException: - return False - - return True + check_actions(actions) async def set_push_rule_action( self, @@ -1392,8 +1387,8 @@ async def set_push_rule_action( Raises: RuntimeError if this method is called on a worker. - synapse.module_api.errors.NotFoundError if the rule being modified can't be - found. + synapse.module_api.errors.RuleNotFoundException if the rule being modified + can't be found. synapse.module_api.errors.InvalidRuleException if the actions are invalid. """ if self.worker_app is not None: @@ -1409,8 +1404,6 @@ async def set_push_rule_action( user_id, spec, {"actions": actions} ) - self._push_rules_handler.notify_user(user_id) - class PublicRoomListManager: """Contains methods for adding to, removing from and querying whether a room diff --git a/synapse/module_api/errors.py b/synapse/module_api/errors.py index 51a4782d9260..e58e0e60feab 100644 --- a/synapse/module_api/errors.py +++ b/synapse/module_api/errors.py @@ -16,12 +16,12 @@ from synapse.api.errors import ( InvalidClientCredentialsError, - NotFoundError, RedirectException, SynapseError, ) from synapse.config._base import ConfigError from synapse.handlers.push_rules import InvalidRuleException +from synapse.storage.push_rule import RuleNotFoundException __all__ = [ "InvalidClientCredentialsError", @@ -29,5 +29,5 @@ "SynapseError", "ConfigError", "InvalidRuleException", - "NotFoundError", + "RuleNotFoundException", ] diff --git a/synapse/rest/client/push_rule.py b/synapse/rest/client/push_rule.py index 5c7fe99b9052..b98640b14ac5 100644 --- a/synapse/rest/client/push_rule.py +++ b/synapse/rest/client/push_rule.py @@ -20,13 +20,7 @@ SynapseError, UnrecognizedRequestError, ) -from synapse.handlers.push_rules import ( - InvalidRuleException, - RuleSpec, - check_actions, - namespaced_rule_id, - namespaced_rule_id_from_spec, -) +from synapse.handlers.push_rules import InvalidRuleException, RuleSpec, check_actions from synapse.http.server import HttpServer from synapse.http.servlet import ( RestServlet, @@ -82,8 +76,9 @@ async def on_PUT(self, request: SynapseRequest, path: str) -> Tuple[int, JsonDic await self._push_rules_handler.set_rule_attr(user_id, spec, content) except InvalidRuleException as e: raise SynapseError(400, "Invalid actions: %s" % e) + except RuleNotFoundException: + raise NotFoundError("Unknown rule") - self._push_rules_handler.notify_user(user_id) return 200, {} if spec.rule_id.startswith("."): @@ -99,16 +94,16 @@ async def on_PUT(self, request: SynapseRequest, path: str) -> Tuple[int, JsonDic before = parse_string(request, "before") if before: - before = namespaced_rule_id(spec, before) + before = f"global/{spec.template}/{before}" after = parse_string(request, "after") if after: - after = namespaced_rule_id(spec, after) + after = f"global/{spec.template}/{after}" try: await self.store.add_push_rule( user_id=user_id, - rule_id=namespaced_rule_id_from_spec(spec), + rule_id=f"global/{spec.template}/{spec.rule_id}", priority_class=priority_class, conditions=conditions, actions=actions, @@ -134,7 +129,7 @@ async def on_DELETE( requester = await self.auth.get_user_by_req(request) user_id = requester.user.to_string() - namespaced_rule_id = namespaced_rule_id_from_spec(spec) + namespaced_rule_id = f"global/{spec.template}/{spec.rule_id}" try: await self.store.delete_push_rule(user_id, namespaced_rule_id) diff --git a/synapse/server.py b/synapse/server.py index 49c4ada263ee..fe578809ec73 100644 --- a/synapse/server.py +++ b/synapse/server.py @@ -811,6 +811,10 @@ def get_external_cache(self) -> ExternalCache: def get_account_handler(self) -> AccountHandler: return AccountHandler(self) + @cache_in_self + def get_push_rules_handler(self) -> PushRulesHandler: + return PushRulesHandler(self) + @cache_in_self def get_outbound_redis_connection(self) -> "ConnectionHandler": """ @@ -851,7 +855,3 @@ def get_request_ratelimiter(self) -> RequestRatelimiter: self.config.ratelimiting.rc_message, self.config.ratelimiting.rc_admin_redaction, ) - - @cache_in_self - def get_push_rules_handler(self) -> PushRulesHandler: - return PushRulesHandler(self) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index 92539f5d41b1..5a686201347e 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -16,7 +16,7 @@ import logging from typing import TYPE_CHECKING, Dict, List, Tuple, Union -from synapse.api.errors import NotFoundError, StoreError +from synapse.api.errors import StoreError 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 @@ -668,8 +668,7 @@ def _set_push_rule_enabled_txn( ) txn.execute(sql, (user_id, rule_id)) if txn.fetchone() is None: - # needed to set NOT_FOUND code. - raise NotFoundError("Push rule does not exist.") + raise RuleNotFoundException("Push rule does not exist.") self.db_pool.simple_upsert_txn( txn, @@ -744,7 +743,7 @@ def set_push_rule_actions_txn(txn, stream_id, event_stream_ordering): except StoreError as serr: if serr.code == 404: # this sets the NOT_FOUND error Code - raise NotFoundError("Push rule does not exist") + raise RuleNotFoundException("Push rule does not exist") else: raise From 023edf75ecfe7793b3e7db3c2a4d91bfd8139d40 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 25 Apr 2022 17:15:40 +0100 Subject: [PATCH 08/14] Fix test --- tests/module_api/test_api.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/tests/module_api/test_api.py b/tests/module_api/test_api.py index 4c6c209164a7..12b964b498c5 100644 --- a/tests/module_api/test_api.py +++ b/tests/module_api/test_api.py @@ -19,6 +19,7 @@ from synapse.events import EventBase from synapse.federation.units import Transaction from synapse.handlers.presence import UserPresenceState +from synapse.handlers.push_rules import InvalidRuleException from synapse.rest import admin from synapse.rest.client import login, notifications, presence, profile, room from synapse.types import create_requester @@ -622,13 +623,17 @@ def test_check_push_rules_actions(self) -> None: """Test that modules can check whether a list of push rules actions are spec compliant. """ - self.assertFalse(self.module_api.check_push_rule_actions(["foo"])) - self.assertFalse(self.module_api.check_push_rule_actions({"foo": "bar"})) - self.assertTrue(self.module_api.check_push_rule_actions(["notify"])) - self.assertTrue( - self.module_api.check_push_rule_actions( - [{"set_tweak": "sound", "value": "default"}] - ) + with self.assertRaises(InvalidRuleException): + self.module_api.check_push_rule_actions(["foo"]) + + with self.assertRaises(InvalidRuleException): + self.module_api.check_push_rule_actions({"foo": "bar"}) + + with self.assertRaises(InvalidRuleException): + self.module_api.check_push_rule_actions(["notify"]) + + self.module_api.check_push_rule_actions( + [{"set_tweak": "sound", "value": "default"}] ) From 464f8ace89463fb5137d1db21a749fd79371ac49 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 25 Apr 2022 17:17:24 +0100 Subject: [PATCH 09/14] Fix omission in docstring --- synapse/module_api/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 2287c9760514..20add5b33520 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -1386,7 +1386,7 @@ async def set_push_rule_action( actions: the actions to run when the rule's conditions match. Raises: - RuntimeError if this method is called on a worker. + RuntimeError if this method is called on a worker or `scope` is invalid. synapse.module_api.errors.RuleNotFoundException if the rule being modified can't be found. synapse.module_api.errors.InvalidRuleException if the actions are invalid. From c34dd5e61cc703551dc4b4f0513d42e32e54c29e Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 25 Apr 2022 17:20:11 +0100 Subject: [PATCH 10/14] Provide more info on unrecognised action --- synapse/handlers/push_rules.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/push_rules.py b/synapse/handlers/push_rules.py index 5f37a7bccd3a..33cf8900d748 100644 --- a/synapse/handlers/push_rules.py +++ b/synapse/handlers/push_rules.py @@ -129,7 +129,7 @@ def check_actions(actions: List[Union[str, JsonDict]]) -> None: elif isinstance(a, dict) and "set_tweak" in a: pass else: - raise InvalidRuleException("Unrecognised action") + raise InvalidRuleException("Unrecognised action %s" % a) class InvalidRuleException(Exception): From 1ceede3b2666cbf1317bfef7eefe3c24badab8c6 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 25 Apr 2022 17:39:05 +0100 Subject: [PATCH 11/14] Fix test --- tests/module_api/test_api.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/module_api/test_api.py b/tests/module_api/test_api.py index 12b964b498c5..8bc84aaaca44 100644 --- a/tests/module_api/test_api.py +++ b/tests/module_api/test_api.py @@ -629,8 +629,7 @@ def test_check_push_rules_actions(self) -> None: with self.assertRaises(InvalidRuleException): self.module_api.check_push_rule_actions({"foo": "bar"}) - with self.assertRaises(InvalidRuleException): - self.module_api.check_push_rule_actions(["notify"]) + self.module_api.check_push_rule_actions(["notify"]) self.module_api.check_push_rule_actions( [{"set_tweak": "sound", "value": "default"}] From dab7bf000d08d7ec14f507962ccfac90d6507a05 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 27 Apr 2022 14:18:05 +0100 Subject: [PATCH 12/14] Incorporate review --- synapse/handlers/push_rules.py | 11 ++++++----- synapse/storage/databases/main/push_rule.py | 8 ++++---- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/push_rules.py b/synapse/handlers/push_rules.py index 33cf8900d748..486c2a41a181 100644 --- a/synapse/handlers/push_rules.py +++ b/synapse/handlers/push_rules.py @@ -37,12 +37,13 @@ class PushRulesHandler: def __init__(self, hs: "HomeServer"): self._notifier = hs.get_notifier() - self._store = hs.get_datastores().main + self._main_store = hs.get_datastores().main async def set_rule_attr( self, user_id: str, spec: RuleSpec, val: Union[bool, JsonDict] ) -> None: - """Set an attribute (enabled or actions) on an existing push rule. + """Set an attribute (enabled or actions) on an existing push rule, and notify + listeners (e.g. sync handler) of the change. Args: user_id: the user for which to modify the push rule. @@ -76,7 +77,7 @@ async def set_rule_attr( # This should *actually* take a dict, but many clients pass # bools directly, so let's not break them. raise SynapseError(400, "Value for 'enabled' must be boolean") - await self._store.set_push_rule_enabled( + await self._main_store.set_push_rule_enabled( user_id, namespaced_rule_id, val, is_default_rule ) elif spec.attr == "actions": @@ -93,7 +94,7 @@ async def set_rule_attr( raise RuleNotFoundException( "Unknown rule %r" % (namespaced_rule_id,) ) - await self._store.set_push_rule_actions( + await self._main_store.set_push_rule_actions( user_id, namespaced_rule_id, actions, is_default_rule ) else: @@ -107,7 +108,7 @@ def notify_user(self, user_id: str) -> None: Args: user_id: the user ID the change is for. """ - stream_id = self._store.get_max_push_rules_stream_id() + stream_id = self._main_store.get_max_push_rules_stream_id() self._notifier.on_new_event("push_rules_key", stream_id, users=[user_id]) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index 5a686201347e..eb85bbd39243 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -618,7 +618,7 @@ async def set_push_rule_enabled( are always stored in the database `push_rules` table). Raises: - NotFoundError if the rule does not exist. + RuleNotFoundException if the rule does not exist. """ async with self._push_rules_stream_id_gen.get_next() as stream_id: event_stream_ordering = self._stream_id_gen.get_current_token() @@ -697,9 +697,6 @@ async def set_push_rule_actions( """ Sets the `actions` state of a push rule. - Will throw NotFoundError if the rule does not exist; the Code for this - is NOT_FOUND. - Args: user_id: the user ID of the user who wishes to enable/disable the rule e.g. '@tina:example.org' @@ -711,6 +708,9 @@ async def set_push_rule_actions( is_default_rule: True if and only if this is a server-default rule. This skips the check for existence (as only user-created rules are always stored in the database `push_rules` table). + + Raises: + RuleNotFoundException if the rule does not exist. """ actions_json = json_encoder.encode(actions) From 18c23958a1ddc463df4782d274bdbf5be3e5dc24 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 27 Apr 2022 15:25:52 +0200 Subject: [PATCH 13/14] Update synapse/handlers/push_rules.py Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- synapse/handlers/push_rules.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/push_rules.py b/synapse/handlers/push_rules.py index 486c2a41a181..5d46bcfd991b 100644 --- a/synapse/handlers/push_rules.py +++ b/synapse/handlers/push_rules.py @@ -42,8 +42,9 @@ def __init__(self, hs: "HomeServer"): async def set_rule_attr( self, user_id: str, spec: RuleSpec, val: Union[bool, JsonDict] ) -> None: - """Set an attribute (enabled or actions) on an existing push rule, and notify - listeners (e.g. sync handler) of the change. + """Set an attribute (enabled or actions) on an existing push rule. + + Notifies listeners (e.g. sync handler) of the change. Args: user_id: the user for which to modify the push rule. From 1e18a780fcca9d4876aae2078ce6917fab658166 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 27 Apr 2022 14:32:37 +0100 Subject: [PATCH 14/14] Lint --- synapse/handlers/push_rules.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/push_rules.py b/synapse/handlers/push_rules.py index 5d46bcfd991b..2599160bcc00 100644 --- a/synapse/handlers/push_rules.py +++ b/synapse/handlers/push_rules.py @@ -43,7 +43,7 @@ async def set_rule_attr( self, user_id: str, spec: RuleSpec, val: Union[bool, JsonDict] ) -> None: """Set an attribute (enabled or actions) on an existing push rule. - + Notifies listeners (e.g. sync handler) of the change. Args: