Reject modifying server-default push rules that are not served (#20202)

TLDR: `PUT` on a server-default push rule now applies the same existence
check that `GET` already does. A rule that is not served cannot be
modified.

Synapse ships some server-default push rules behind experimental feature
flags, so the set of server-default rules a user has depends on the
server's configuration. Most flags add rules that only exist while the
flag is on.
[MSC4210](https://github.com/matrix-org/matrix-spec-proposals/pull/4210)
works the other way: it removes the three legacy mention rules while its
flag is on. The flags that change the set are:

- `msc1767_enabled`:
[MSC1767](https://github.com/matrix-org/matrix-spec-proposals/pull/1767)
extensible events rules
- `msc3381_polls_enabled`:
[MSC3381](https://github.com/matrix-org/matrix-spec-proposals/pull/3381)
poll rules
- `msc3664_enabled`:
[MSC3664](https://github.com/matrix-org/matrix-spec-proposals/pull/3664)
reply rule
- `msc4028_push_encrypted_events`:
[MSC4028](https://github.com/matrix-org/matrix-spec-proposals/pull/4028)
encrypted event rule
- `msc4306_enabled`:
[MSC4306](https://github.com/matrix-org/matrix-spec-proposals/pull/4306)
thread subscription rules
- `msc4210_enabled`: removes `.m.rule.contains_display_name`,
`.m.rule.contains_user_name` and `.m.rule.roomnotif`

`GET /pushrules/` only lists the rules that exist for that
configuration. `GET` on a single rule that is not in the set returns
`404 M_NOT_FOUND`. The same set is used to evaluate notifications, and
to build the `m.push_rules` account data in `/sync`.

`PUT .../enabled` and `PUT .../actions` did not use that set. They
accepted any rule ID that Synapse knows about, whether or not the server
currently serves it. Clients could modify a rule the server had just
said did not exist. The request returned `200`, the change was stored,
and it was then ignored. Both endpoints now return `404 M_NOT_FOUND` for
such a rule, as [the spec
requires](https://spec.matrix.org/v1.19/client-server-api/#put_matrixclientv3pushrulesglobalkindruleidenabled)
for a rule that does not exist.

Before (`msc4210_enabled: true`):

```
GET /_matrix/client/v3/pushrules/global/override/.m.rule.roomnotif
  404 {"errcode": "M_NOT_FOUND", "error": "..."}

PUT /_matrix/client/v3/pushrules/global/override/.m.rule.roomnotif/enabled
  {"enabled": false}
  200 {}
```

After:

```
GET /_matrix/client/v3/pushrules/global/override/.m.rule.roomnotif
  404 {"errcode": "M_NOT_FOUND", "error": "..."}

PUT /_matrix/client/v3/pushrules/global/override/.m.rule.roomnotif/enabled
  {"enabled": false}
  404 {"errcode": "M_NOT_FOUND", "error": "Unknown rule"}
```

This is a no-op for default configurations. All of these flags are off
by default, and rules that are served are still modifiable as before.

changelog.d/20202.bugfix

@@ -0,0 +1 @@
+Fix a bug where server-default push rules that are disabled by the server's configuration, and thus not served to clients, could still be modified through the client API instead of returning a 404.

rust/src/push/mod.rs

@@ -59,7 +59,7 @@
 //! kind, etc, etc.
 
 use std::borrow::Cow;
-use std::collections::{BTreeMap, HashMap, HashSet};
+use std::collections::{BTreeMap, HashMap};
 
 use anyhow::{Context, Error};
 use log::warn;
@@ -84,7 +84,6 @@ pub fn register_module(py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()>
     child_module.add_class::<PushRules>()?;
     child_module.add_class::<FilteredPushRules>()?;
     child_module.add_class::<PushRuleEvaluator>()?;
-    child_module.add_function(wrap_pyfunction!(get_base_rule_ids, m)?)?;
 
     m.add_submodule(&child_module)?;
 
@@ -97,11 +96,6 @@ pub fn register_module(py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()>
     Ok(())
 }
 
-#[pyfunction]
-fn get_base_rule_ids() -> HashSet<&'static str> {
-    base_rules::BASE_RULES_BY_ID.keys().copied().collect()
-}
-
 /// A single push rule for a user.
 #[derive(Debug, Clone)]
 #[pyclass(frozen, from_py_object)]

synapse/handlers/push_rules.py

@@ -24,17 +24,15 @@ import attr
 
 from synapse.api.errors import SynapseError, UnrecognizedRequestError
 from synapse.push.clientformat import format_push_rules_for_user
+from synapse.storage.databases.main.push_rule import filter_push_rules
 from synapse.storage.push_rule import RuleNotFoundException
-from synapse.synapse_rust.push import get_base_rule_ids
+from synapse.synapse_rust.push import PushRules
 from synapse.types import JsonDict, StreamKeyType, UserID
 
 if TYPE_CHECKING:
     from synapse.server import HomeServer
 
 
-BASE_RULE_IDS = get_base_rule_ids()
-
-
 @attr.s(slots=True, frozen=True, auto_attribs=True)
 class RuleSpec:
     scope: str
@@ -50,6 +48,13 @@ class PushRulesHandler:
         self._notifier = hs.get_notifier()
         self._main_store = hs.get_datastores().main
 
+        self._served_default_rule_ids: frozenset[str] = frozenset(
+            rule.rule_id
+            for rule, _ in filter_push_rules(
+                PushRules([]), {}, hs.config.experimental
+            ).rules()
+        )
+
     async def set_rule_attr(
         self, user_id: str, spec: RuleSpec, val: bool | JsonDict
     ) -> None:
@@ -63,7 +68,9 @@ class PushRulesHandler:
             val: the value to change the attribute to.
 
         Raises:
-            RuleNotFoundException if the rule being modified doesn't exist.
+            RuleNotFoundException if the rule being modified doesn't exist, or is a
+                server-default rule that is not served to the user (e.g. one gated
+                behind a disabled experimental feature).
             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
@@ -79,7 +86,7 @@ class PushRulesHandler:
         rule_id = spec.rule_id
         is_default_rule = rule_id.startswith(".")
         if is_default_rule:
-            if namespaced_rule_id not in BASE_RULE_IDS:
+            if namespaced_rule_id not in self._served_default_rule_ids:
                 raise RuleNotFoundException("Unknown rule %r" % (namespaced_rule_id,))
         if spec.attr == "enabled":
             if isinstance(val, dict) and "enabled" in val:
@@ -99,13 +106,6 @@ class PushRulesHandler:
             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 RuleNotFoundException(
-                        "Unknown rule %r" % (namespaced_rule_id,)
-                    )
             await self._main_store.set_push_rule_actions(
                 user_id, namespaced_rule_id, actions, is_default_rule
             )

synapse/storage/databases/main/push_rule.py

@@ -97,9 +97,28 @@ def _load_rules(
         for rawrule in rawrules
     ]
 
-    push_rules = PushRules(ruleslist)
+    return filter_push_rules(PushRules(ruleslist), enabled_map, experimental_config)
 
-    filtered_rules = FilteredPushRules(
+
+def filter_push_rules(
+    push_rules: PushRules,
+    enabled_map: dict[str, bool],
+    experimental_config: ExperimentalConfig,
+) -> FilteredPushRules:
+    """Combine a user's rules with the server-default rules, keeping only the
+    server-default rules enabled by the server's configuration.
+
+    Args:
+        push_rules: The user's own rules.
+        enabled_map: A dictionary of rule ID to a boolean of whether the rule is
+            enabled. This might not include all rule IDs from push_rules.
+        experimental_config: The `experimental_features` section of the Synapse
+            config. (Used to check if various features are enabled.)
+
+    Returns:
+        A new FilteredPushRules object.
+    """
+    return FilteredPushRules(
         push_rules,
         enabled_map,
         msc1767_enabled=experimental_config.msc1767_enabled,
@@ -111,8 +130,6 @@ def _load_rules(
         msc4306_enabled=experimental_config.msc4306_enabled,
     )
 
-    return filtered_rules
-
 
 def _push_rule_size_for_limits(*, conditions_json: str, actions_json: str) -> int:
     """

synapse/synapse_rust/push.pyi

@@ -54,8 +54,6 @@ class FilteredPushRules:
     ): ...
     def rules(self) -> Collection[tuple[PushRule, bool]]: ...
 
-def get_base_rule_ids() -> Collection[str]: ...
-
 class PushRuleEvaluator:
     def __init__(
         self,

tests/rest/client/test_push_rule_attrs.py

@@ -29,7 +29,7 @@ from synapse.api.errors import Codes
 from synapse.rest.client import login, push_rule, room
 from synapse.types import JsonDict
 
-from tests.unittest import HomeserverTestCase
+from tests.unittest import HomeserverTestCase, override_config
 
 
 class PushRuleAttributesTestCase(HomeserverTestCase):
@@ -427,6 +427,99 @@ class PushRuleAttributesTestCase(HomeserverTestCase):
         self.assertEqual(channel.code, 404)
         self.assertEqual(channel.json_body["errcode"], Codes.NOT_FOUND)
 
+    def _assert_default_rule_absent(self, token: str, rule_path: str) -> None:
+        """Checks that a server-default rule is neither served nor modifiable."""
+        channel = self.make_request(
+            "GET", f"/pushrules/{rule_path}", access_token=token
+        )
+        self.assertEqual(channel.code, 404)
+        self.assertEqual(channel.json_body["errcode"], Codes.NOT_FOUND)
+
+        channel = self.make_request(
+            "PUT",
+            f"/pushrules/{rule_path}/enabled",
+            {"enabled": False},
+            access_token=token,
+        )
+        self.assertEqual(channel.code, 404)
+        self.assertEqual(channel.json_body["errcode"], Codes.NOT_FOUND)
+
+        channel = self.make_request(
+            "PUT",
+            f"/pushrules/{rule_path}/actions",
+            {"actions": ["notify"]},
+            access_token=token,
+        )
+        self.assertEqual(channel.code, 404)
+        self.assertEqual(channel.json_body["errcode"], Codes.NOT_FOUND)
+
+        _, kind, rule_id = rule_path.split("/")
+        channel = self.make_request("GET", "/pushrules/", access_token=token)
+        self.assertEqual(channel.code, 200)
+        self.assertNotIn(
+            rule_id,
+            [rule["rule_id"] for rule in channel.json_body["global"].get(kind, [])],
+        )
+
+    def _assert_default_rule_modifiable(self, token: str, rule_path: str) -> None:
+        """Checks that a server-default rule is served and can be modified."""
+        channel = self.make_request(
+            "GET", f"/pushrules/{rule_path}", access_token=token
+        )
+        self.assertEqual(channel.code, 200)
+
+        channel = self.make_request(
+            "PUT",
+            f"/pushrules/{rule_path}/actions",
+            {"actions": ["notify"]},
+            access_token=token,
+        )
+        self.assertEqual(channel.code, 200)
+
+        channel = self.make_request(
+            "PUT",
+            f"/pushrules/{rule_path}/enabled",
+            {"enabled": False},
+            access_token=token,
+        )
+        self.assertEqual(channel.code, 200)
+
+        channel = self.make_request(
+            "GET", f"/pushrules/{rule_path}", access_token=token
+        )
+        self.assertEqual(channel.code, 200)
+        self.assertEqual(channel.json_body["actions"], ["notify"])
+        self.assertEqual(channel.json_body["enabled"], False)
+
+    # A server-default rule gated behind an experimental feature is only served,
+    # and thus only modifiable, while the feature is enabled. One such rule is
+    # enough to check this; the MSC3664 reply rule is used below. When MSC3664
+    # stabilises, swap in another gated rule, or drop these two tests if none
+    # remain.
+
+    def test_gated_default_rule_disabled(self) -> None:
+        """
+        Tests that a server-default rule gated behind an experimental feature
+        is neither served nor modifiable while the feature is disabled.
+        """
+        self.register_user("bob", "pass")
+        token = self.login("bob", "pass")
+        self._assert_default_rule_absent(
+            token, "global/override/.im.nheko.msc3664.reply"
+        )
+
+    @override_config({"experimental_features": {"msc3664_enabled": True}})
+    def test_gated_default_rule_enabled(self) -> None:
+        """
+        Tests that a server-default rule gated behind an experimental feature
+        is served and modifiable once the feature is enabled.
+        """
+        self.register_user("bob", "pass")
+        token = self.login("bob", "pass")
+        self._assert_default_rule_modifiable(
+            token, "global/override/.im.nheko.msc3664.reply"
+        )
+
     def test_contains_user_name(self) -> None:
         """
         Tests that `contains_user_name` rule is present and have proper value in `pattern`.