diff --git a/changelog.d/20202.bugfix b/changelog.d/20202.bugfix new file mode 100644 index 0000000000..3416987fc8 --- /dev/null +++ b/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. diff --git a/rust/src/push/mod.rs b/rust/src/push/mod.rs index 868a57d00f..54dc22ce89 100644 --- a/rust/src/push/mod.rs +++ b/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::()?; child_module.add_class::()?; child_module.add_class::()?; - 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)] diff --git a/synapse/handlers/push_rules.py b/synapse/handlers/push_rules.py index 746c712bac..7c6ece470e 100644 --- a/synapse/handlers/push_rules.py +++ b/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 ) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index b06e36149e..b65f5e8bfa 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/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: """ diff --git a/synapse/synapse_rust/push.pyi b/synapse/synapse_rust/push.pyi index eede2c5c37..1df9cee1f0 100644 --- a/synapse/synapse_rust/push.pyi +++ b/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, diff --git a/tests/rest/client/test_push_rule_attrs.py b/tests/rest/client/test_push_rule_attrs.py index f6bbc7496e..687aec3b60 100644 --- a/tests/rest/client/test_push_rule_attrs.py +++ b/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`.