Add a module API to allow modules to edit push rule actions

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,19 +48,26 @@ 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
             # 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)
+        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])
 
@@ -110,19 +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")
-
-
-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)
+            raise InvalidRuleException("Unrecognised action %s" % a)
 
 
 class InvalidRuleException(Exception):

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,
@@ -1391,9 +1386,9 @@ 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.
-            synapse.module_api.errors.NotFoundError if the rule being modified can't be
-                found.
+            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.
         """
         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

synapse/module_api/errors.py

@@ -16,18 +16,18 @@
 
 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",
     "RedirectException",
     "SynapseError",
     "ConfigError",
     "InvalidRuleException",
-    "NotFoundError",
+    "RuleNotFoundException",
 ]

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)

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)

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
 

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,16 @@ 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"})
+
+        self.module_api.check_push_rule_actions(["notify"])
+
+        self.module_api.check_push_rule_actions(
+            [{"set_tweak": "sound", "value": "default"}]
         )