From 4b4e0dc3cecbe9ad65c4728c1ec461321d15789f Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 9 May 2023 10:34:10 -0400 Subject: Error if attempting to set m.push_rules account data, per MSC4010. (#15555) m.push_rules, like m.fully_read, is a special account data type that cannot be set using the normal /account_data endpoint. Return an error instead of allowing data that will not be used to be stored. --- changelog.d/15554.bugfix | 1 + changelog.d/15554.misc | 1 - changelog.d/15555.bugfix | 1 + synapse/config/experimental.py | 5 +++ synapse/handlers/push_rules.py | 16 ++++++- synapse/handlers/sync.py | 12 ++---- synapse/push/clientformat.py | 2 +- synapse/rest/client/account_data.py | 85 +++++++++++++++++++++++++++++-------- synapse/rest/client/push_rule.py | 7 +-- 9 files changed, 95 insertions(+), 35 deletions(-) create mode 100644 changelog.d/15554.bugfix delete mode 100644 changelog.d/15554.misc create mode 100644 changelog.d/15555.bugfix diff --git a/changelog.d/15554.bugfix b/changelog.d/15554.bugfix new file mode 100644 index 0000000000..0fd9de8c65 --- /dev/null +++ b/changelog.d/15554.bugfix @@ -0,0 +1 @@ +Experimental support for [MSC4010](https://github.com/matrix-org/matrix-spec-proposals/pull/4010) which rejects setting the `"m.push_rules"` via account data. diff --git a/changelog.d/15554.misc b/changelog.d/15554.misc deleted file mode 100644 index 002e3f5315..0000000000 --- a/changelog.d/15554.misc +++ /dev/null @@ -1 +0,0 @@ -Use account data constants in more places. diff --git a/changelog.d/15555.bugfix b/changelog.d/15555.bugfix new file mode 100644 index 0000000000..0fd9de8c65 --- /dev/null +++ b/changelog.d/15555.bugfix @@ -0,0 +1 @@ +Experimental support for [MSC4010](https://github.com/matrix-org/matrix-spec-proposals/pull/4010) which rejects setting the `"m.push_rules"` via account data. diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index 514d87cb2c..7af6dbcd09 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -202,3 +202,8 @@ class ExperimentalConfig(Config): # MSC4009: E.164 Matrix IDs self.msc4009_e164_mxids = experimental.get("msc4009_e164_mxids", False) + + # MSC4010: Do not allow setting m.push_rules account data. + self.msc4010_push_rules_account_data = experimental.get( + "msc4010_push_rules_account_data", False + ) diff --git a/synapse/handlers/push_rules.py b/synapse/handlers/push_rules.py index 813f3aa2d5..7ed88a3611 100644 --- a/synapse/handlers/push_rules.py +++ b/synapse/handlers/push_rules.py @@ -11,14 +11,15 @@ # 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 +from typing import TYPE_CHECKING, Any, Dict, List, Optional, Union import attr from synapse.api.errors import SynapseError, UnrecognizedRequestError +from synapse.push.clientformat import format_push_rules_for_user from synapse.storage.push_rule import RuleNotFoundException from synapse.synapse_rust.push import get_base_rule_ids -from synapse.types import JsonDict +from synapse.types import JsonDict, UserID if TYPE_CHECKING: from synapse.server import HomeServer @@ -115,6 +116,17 @@ class PushRulesHandler: stream_id = self._main_store.get_max_push_rules_stream_id() self._notifier.on_new_event("push_rules_key", stream_id, users=[user_id]) + async def push_rules_for_user( + self, user: UserID + ) -> Dict[str, Dict[str, List[Dict[str, Any]]]]: + """ + Push rules aren't really account data, but get formatted as such for /sync. + """ + user_id = user.to_string() + rules_raw = await self._main_store.get_push_rules_for_user(user_id) + rules = format_push_rules_for_user(user, rules_raw) + return rules + def check_actions(actions: List[Union[str, JsonDict]]) -> None: """Check if the given actions are spec compliant. diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index cc05b0afa0..c010405be6 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -50,7 +50,6 @@ from synapse.logging.opentracing import ( start_active_span, trace, ) -from synapse.push.clientformat import format_push_rules_for_user from synapse.storage.databases.main.event_push_actions import RoomNotifCounts from synapse.storage.databases.main.roommember import extract_heroes_from_room_summary from synapse.storage.roommember import MemberSummary @@ -261,6 +260,7 @@ class SyncHandler: self.notifier = hs.get_notifier() self.presence_handler = hs.get_presence_handler() self._relations_handler = hs.get_relations_handler() + self._push_rules_handler = hs.get_push_rules_handler() self.event_sources = hs.get_event_sources() self.clock = hs.get_clock() self.state = hs.get_state_handler() @@ -428,12 +428,6 @@ class SyncHandler: set_tag(SynapseTags.SYNC_RESULT, bool(sync_result)) return sync_result - async def push_rules_for_user(self, user: UserID) -> Dict[str, Dict[str, list]]: - user_id = user.to_string() - rules_raw = await self.store.get_push_rules_for_user(user_id) - rules = format_push_rules_for_user(user, rules_raw) - return rules - async def ephemeral_by_room( self, sync_result_builder: "SyncResultBuilder", @@ -1779,7 +1773,7 @@ class SyncHandler: global_account_data = dict(global_account_data) global_account_data[ AccountDataTypes.PUSH_RULES - ] = await self.push_rules_for_user(sync_config.user) + ] = await self._push_rules_handler.push_rules_for_user(sync_config.user) else: all_global_account_data = await self.store.get_global_account_data_for_user( user_id @@ -1788,7 +1782,7 @@ class SyncHandler: global_account_data = dict(all_global_account_data) global_account_data[ AccountDataTypes.PUSH_RULES - ] = await self.push_rules_for_user(sync_config.user) + ] = await self._push_rules_handler.push_rules_for_user(sync_config.user) account_data_for_user = ( await sync_config.filter_collection.filter_global_account_data( diff --git a/synapse/push/clientformat.py b/synapse/push/clientformat.py index 222afbdcc8..88b52c26a0 100644 --- a/synapse/push/clientformat.py +++ b/synapse/push/clientformat.py @@ -22,7 +22,7 @@ from synapse.types import UserID def format_push_rules_for_user( user: UserID, ruleslist: FilteredPushRules -) -> Dict[str, Dict[str, list]]: +) -> Dict[str, Dict[str, List[Dict[str, Any]]]]: """Converts a list of rawrules and a enabled map into nested dictionaries to match the Matrix client-server format for push rules""" diff --git a/synapse/rest/client/account_data.py b/synapse/rest/client/account_data.py index 8eebb21c76..b1f9e9dc9b 100644 --- a/synapse/rest/client/account_data.py +++ b/synapse/rest/client/account_data.py @@ -13,9 +13,9 @@ # limitations under the License. import logging -from typing import TYPE_CHECKING, Tuple +from typing import TYPE_CHECKING, Optional, Tuple -from synapse.api.constants import ReceiptTypes +from synapse.api.constants import AccountDataTypes, ReceiptTypes from synapse.api.errors import AuthError, Codes, NotFoundError, SynapseError from synapse.http.server import HttpServer from synapse.http.servlet import RestServlet, parse_json_object_from_request @@ -30,6 +30,23 @@ if TYPE_CHECKING: logger = logging.getLogger(__name__) +def _check_can_set_account_data_type(account_data_type: str) -> None: + """The fully read marker and push rules cannot be directly set via /account_data.""" + if account_data_type == ReceiptTypes.FULLY_READ: + raise SynapseError( + 405, + "Cannot set m.fully_read through this API." + " Use /rooms/!roomId:server.name/read_markers", + Codes.BAD_JSON, + ) + elif account_data_type == AccountDataTypes.PUSH_RULES: + raise SynapseError( + 405, + "Cannot set m.push_rules through this API. Use /pushrules", + Codes.BAD_JSON, + ) + + class AccountDataServlet(RestServlet): """ PUT /user/{user_id}/account_data/{account_dataType} HTTP/1.1 @@ -47,6 +64,7 @@ class AccountDataServlet(RestServlet): self.auth = hs.get_auth() self.store = hs.get_datastores().main self.handler = hs.get_account_data_handler() + self._push_rules_handler = hs.get_push_rules_handler() async def on_PUT( self, request: SynapseRequest, user_id: str, account_data_type: str @@ -55,6 +73,10 @@ class AccountDataServlet(RestServlet): if user_id != requester.user.to_string(): raise AuthError(403, "Cannot add account data for other users.") + # Raise an error if the account data type cannot be set directly. + if self._hs.config.experimental.msc4010_push_rules_account_data: + _check_can_set_account_data_type(account_data_type) + body = parse_json_object_from_request(request) # If experimental support for MSC3391 is enabled, then providing an empty dict @@ -78,19 +100,28 @@ class AccountDataServlet(RestServlet): if user_id != requester.user.to_string(): raise AuthError(403, "Cannot get account data for other users.") - event = await self.store.get_global_account_data_by_type_for_user( - user_id, account_data_type - ) + # Push rules are stored in a separate table and must be queried separately. + if ( + self._hs.config.experimental.msc4010_push_rules_account_data + and account_data_type == AccountDataTypes.PUSH_RULES + ): + account_data: Optional[ + JsonDict + ] = await self._push_rules_handler.push_rules_for_user(requester.user) + else: + account_data = await self.store.get_global_account_data_by_type_for_user( + user_id, account_data_type + ) - if event is None: + if account_data is None: raise NotFoundError("Account data not found") # If experimental support for MSC3391 is enabled, then this endpoint should # return a 404 if the content for an account data type is an empty dict. - if self._hs.config.experimental.msc3391_enabled and event == {}: + if self._hs.config.experimental.msc3391_enabled and account_data == {}: raise NotFoundError("Account data not found") - return 200, event + return 200, account_data class UnstableAccountDataServlet(RestServlet): @@ -109,6 +140,7 @@ class UnstableAccountDataServlet(RestServlet): def __init__(self, hs: "HomeServer"): super().__init__() + self._hs = hs self.auth = hs.get_auth() self.handler = hs.get_account_data_handler() @@ -122,6 +154,10 @@ class UnstableAccountDataServlet(RestServlet): if user_id != requester.user.to_string(): raise AuthError(403, "Cannot delete account data for other users.") + # Raise an error if the account data type cannot be set directly. + if self._hs.config.experimental.msc4010_push_rules_account_data: + _check_can_set_account_data_type(account_data_type) + await self.handler.remove_account_data_for_user(user_id, account_data_type) return 200, {} @@ -165,9 +201,10 @@ class RoomAccountDataServlet(RestServlet): Codes.INVALID_PARAM, ) - body = parse_json_object_from_request(request) - - if account_data_type == ReceiptTypes.FULLY_READ: + # Raise an error if the account data type cannot be set directly. + if self._hs.config.experimental.msc4010_push_rules_account_data: + _check_can_set_account_data_type(account_data_type) + elif account_data_type == ReceiptTypes.FULLY_READ: raise SynapseError( 405, "Cannot set m.fully_read through this API." @@ -175,6 +212,8 @@ class RoomAccountDataServlet(RestServlet): Codes.BAD_JSON, ) + body = parse_json_object_from_request(request) + # If experimental support for MSC3391 is enabled, then providing an empty dict # as the value for an account data type should be functionally equivalent to # calling the DELETE method on the same type. @@ -209,19 +248,26 @@ class RoomAccountDataServlet(RestServlet): Codes.INVALID_PARAM, ) - event = await self.store.get_account_data_for_room_and_type( - user_id, room_id, account_data_type - ) + # Room-specific push rules are not currently supported. + if ( + self._hs.config.experimental.msc4010_push_rules_account_data + and account_data_type == AccountDataTypes.PUSH_RULES + ): + account_data: Optional[JsonDict] = {} + else: + account_data = await self.store.get_account_data_for_room_and_type( + user_id, room_id, account_data_type + ) - if event is None: + if account_data is None: raise NotFoundError("Room account data not found") # If experimental support for MSC3391 is enabled, then this endpoint should # return a 404 if the content for an account data type is an empty dict. - if self._hs.config.experimental.msc3391_enabled and event == {}: + if self._hs.config.experimental.msc3391_enabled and account_data == {}: raise NotFoundError("Room account data not found") - return 200, event + return 200, account_data class UnstableRoomAccountDataServlet(RestServlet): @@ -241,6 +287,7 @@ class UnstableRoomAccountDataServlet(RestServlet): def __init__(self, hs: "HomeServer"): super().__init__() + self._hs = hs self.auth = hs.get_auth() self.handler = hs.get_account_data_handler() @@ -262,6 +309,10 @@ class UnstableRoomAccountDataServlet(RestServlet): Codes.INVALID_PARAM, ) + # Raise an error if the account data type cannot be set directly. + if self._hs.config.experimental.msc4010_push_rules_account_data: + _check_can_set_account_data_type(account_data_type) + await self.handler.remove_account_data_for_room( user_id, room_id, account_data_type ) diff --git a/synapse/rest/client/push_rule.py b/synapse/rest/client/push_rule.py index 1147b6f8ec..5c9fece3ba 100644 --- a/synapse/rest/client/push_rule.py +++ b/synapse/rest/client/push_rule.py @@ -28,7 +28,6 @@ from synapse.http.servlet import ( parse_string, ) from synapse.http.site import SynapseRequest -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 from synapse.storage.push_rule import InconsistentRuleException, RuleNotFoundException @@ -146,14 +145,12 @@ class PushRuleRestServlet(RestServlet): async def on_GET(self, request: SynapseRequest, path: str) -> Tuple[int, JsonDict]: requester = await self.auth.get_user_by_req(request) - user_id = requester.user.to_string() + requester.user.to_string() # we build up the full structure and then decide which bits of it # to send which means doing unnecessary work sometimes but is # is probably not going to make a whole lot of difference - rules_raw = await self.store.get_push_rules_for_user(user_id) - - rules = format_push_rules_for_user(requester.user, rules_raw) + rules = await self._push_rules_handler.push_rules_for_user(requester.user) path_parts = path.split("/")[1:] -- cgit 1.4.1