From ab3c6ba5b3d7bd61952c9c487de640311a0914ad Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Mon, 6 Jul 2020 16:17:17 +0100 Subject: [PATCH 01/28] Write some relevant tests (some failing) Currently failing: test_actions_404_when_put_inexistent_rule test_actions_404_when_put_inexistent_server_rule test_enabled_404_when_put_inexistent_rule test_enabled_404_when_put_inexistent_server_rule test_enabled_on_recreation --- tests/rest/client/v1/test_push_rule_attrs.py | 447 +++++++++++++++++++ 1 file changed, 447 insertions(+) create mode 100644 tests/rest/client/v1/test_push_rule_attrs.py diff --git a/tests/rest/client/v1/test_push_rule_attrs.py b/tests/rest/client/v1/test_push_rule_attrs.py new file mode 100644 index 000000000000..09d5c7454008 --- /dev/null +++ b/tests/rest/client/v1/test_push_rule_attrs.py @@ -0,0 +1,447 @@ +# -*- coding: utf-8 -*- +# Copyright 2020 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. +import synapse +from synapse.api.errors import Codes +from synapse.rest.client.v1 import room, login, push_rule +from tests.unittest import HomeserverTestCase + + +class PushRuleAttributesTestCase(HomeserverTestCase): + servlets = [ + synapse.rest.admin.register_servlets_for_client_rest_resource, + room.register_servlets, + login.register_servlets, + push_rule.register_servlets, + ] + hijack_auth = False + + def test_enabled_on_creation(self): + """ + Tests the GET and PUT of push rules' `enabled` endpoints. + Tests that a rule is enabled upon creation, even though a rule with that + ruleId existed previously and was disabled. + """ + self.register_user("user", "pass") + token = self.login("user", "pass") + + body = { + "conditions": [ + {"kind": "event_match", "key": "sender", "pattern": "@user2:hs"} + ], + "actions": ["notify", {"set_tweak": "highlight"}], + } + + # PUT a new rule + request, channel = self.make_request( + "PUT", "/pushrules/global/override/best.friend", body, access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 200) + + # GET enabled for that new rule + request, channel = self.make_request( + "GET", "/pushrules/global/override/best.friend/enabled", access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 200) + self.assertEqual(channel.json_body["enabled"], True) + + def test_enabled_on_recreation(self): + """ + Tests the GET and PUT of push rules' `enabled` endpoints. + Tests that a rule is enabled upon creation, even if a rule with that + ruleId existed previously and was disabled. + """ + self.register_user("user", "pass") + token = self.login("user", "pass") + + body = { + "conditions": [ + {"kind": "event_match", "key": "sender", "pattern": "@user2:hs"} + ], + "actions": ["notify", {"set_tweak": "highlight"}], + } + + # PUT a new rule + request, channel = self.make_request( + "PUT", "/pushrules/global/override/best.friend", body, access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 200) + + # disable the rule + request, channel = self.make_request( + "PUT", + "/pushrules/global/override/best.friend/enabled", + {"enabled": False}, + access_token=token, + ) + self.render(request) + self.assertEqual(channel.code, 200) + + # check rule disabled + request, channel = self.make_request( + "GET", "/pushrules/global/override/best.friend/enabled", access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 200) + self.assertEqual(channel.json_body["enabled"], False) + + # DELETE the rule + request, channel = self.make_request( + "DELETE", "/pushrules/global/override/best.friend", access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 200) + + # PUT a new rule + request, channel = self.make_request( + "PUT", "/pushrules/global/override/best.friend", body, access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 200) + + # GET enabled for that new rule + request, channel = self.make_request( + "GET", "/pushrules/global/override/best.friend/enabled", access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 200) + self.assertEqual(channel.json_body["enabled"], True) + + def test_enabled_disable(self): + """ + Tests the GET and PUT of push rules' `enabled` endpoints. + Tests that a rule is disabled and enabled when we ask for it. + """ + self.register_user("user", "pass") + token = self.login("user", "pass") + + body = { + "conditions": [ + {"kind": "event_match", "key": "sender", "pattern": "@user2:hs"} + ], + "actions": ["notify", {"set_tweak": "highlight"}], + } + + # PUT a new rule + request, channel = self.make_request( + "PUT", "/pushrules/global/override/best.friend", body, access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 200) + + # disable the rule + request, channel = self.make_request( + "PUT", + "/pushrules/global/override/best.friend/enabled", + {"enabled": False}, + access_token=token, + ) + self.render(request) + self.assertEqual(channel.code, 200) + + # check rule disabled + request, channel = self.make_request( + "GET", "/pushrules/global/override/best.friend/enabled", access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 200) + self.assertEqual(channel.json_body["enabled"], False) + + # re-enable the rule + request, channel = self.make_request( + "PUT", + "/pushrules/global/override/best.friend/enabled", + {"enabled": True}, + access_token=token, + ) + self.render(request) + self.assertEqual(channel.code, 200) + + # check rule enabled + request, channel = self.make_request( + "GET", "/pushrules/global/override/best.friend/enabled", access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 200) + self.assertEqual(channel.json_body["enabled"], True) + + def test_enabled_404_when_get_inexistent(self): + """ + Tests that `enabled` gives 404 when the rule doesn't exist. + """ + self.register_user("user", "pass") + token = self.login("user", "pass") + + body = { + "conditions": [ + {"kind": "event_match", "key": "sender", "pattern": "@user2:hs"} + ], + "actions": ["notify", {"set_tweak": "highlight"}], + } + + # check 404 for never-heard-of rule + request, channel = self.make_request( + "GET", "/pushrules/global/override/best.friend/enabled", access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 404) + self.assertEqual(channel.json_body["errcode"], Codes.NOT_FOUND) + + # PUT a new rule + request, channel = self.make_request( + "PUT", "/pushrules/global/override/best.friend", body, access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 200) + + # GET enabled for that new rule + request, channel = self.make_request( + "GET", "/pushrules/global/override/best.friend/enabled", access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 200) + + # DELETE the rule + request, channel = self.make_request( + "DELETE", "/pushrules/global/override/best.friend", access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 200) + + # check 404 for deleted rule + request, channel = self.make_request( + "GET", "/pushrules/global/override/best.friend/enabled", access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 404) + self.assertEqual(channel.json_body["errcode"], Codes.NOT_FOUND) + + def test_enabled_404_when_get_inexistent_server_rule(self): + """ + Tests that `enabled` gives 404 when the server-default rule doesn't exist. + """ + self.register_user("user", "pass") + token = self.login("user", "pass") + + # check 404 for never-heard-of rule + request, channel = self.make_request( + "GET", "/pushrules/global/override/.m.muahahaha/enabled", access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 404) + self.assertEqual(channel.json_body["errcode"], Codes.NOT_FOUND) + + def test_enabled_404_when_put_inexistent_rule(self): + """ + Tests that `enabled` gives 404 when the server-default rule doesn't exist. + """ + self.register_user("user", "pass") + token = self.login("user", "pass") + + # enable & check 404 for never-heard-of rule + request, channel = self.make_request( + "PUT", + "/pushrules/global/override/best.friend/enabled", + {"enabled": True}, + access_token=token, + ) + self.render(request) + self.assertEqual(channel.code, 404) + self.assertEqual(channel.json_body["errcode"], Codes.NOT_FOUND) + + def test_enabled_404_when_put_inexistent_server_rule(self): + """ + Tests that `enabled` gives 404 when the server-default rule doesn't exist. + """ + self.register_user("user", "pass") + token = self.login("user", "pass") + + # enable & check 404 for never-heard-of rule + request, channel = self.make_request( + "PUT", + "/pushrules/global/override/.m.muahahah/enabled", + {"enabled": True}, + access_token=token, + ) + self.render(request) + self.assertEqual(channel.code, 404) + self.assertEqual(channel.json_body["errcode"], Codes.NOT_FOUND) + + def test_actions_get(self): + """ + Tests that `actions` gives you what you expect on a fresh rule. + """ + self.register_user("user", "pass") + token = self.login("user", "pass") + + body = { + "conditions": [ + {"kind": "event_match", "key": "sender", "pattern": "@user2:hs"} + ], + "actions": ["notify", {"set_tweak": "highlight"}], + } + + # PUT a new rule + request, channel = self.make_request( + "PUT", "/pushrules/global/override/best.friend", body, access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 200) + + # GET actions for that new rule + request, channel = self.make_request( + "GET", "/pushrules/global/override/best.friend/actions", access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 200) + self.assertEqual( + channel.json_body["actions"], ["notify", {"set_tweak": "highlight"}] + ) + + def test_actions_put(self): + """ + Tests that PUT on actions updates the value you'd get from GET. + """ + self.register_user("user", "pass") + token = self.login("user", "pass") + + body = { + "conditions": [ + {"kind": "event_match", "key": "sender", "pattern": "@user2:hs"} + ], + "actions": ["notify", {"set_tweak": "highlight"}], + } + + # PUT a new rule + request, channel = self.make_request( + "PUT", "/pushrules/global/override/best.friend", body, access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 200) + + # change the rule actions + request, channel = self.make_request( + "PUT", + "/pushrules/global/override/best.friend/actions", + {"actions": ["dont_notify"]}, + access_token=token, + ) + self.render(request) + self.assertEqual(channel.code, 200) + + # GET actions for that new rule + request, channel = self.make_request( + "GET", "/pushrules/global/override/best.friend/actions", access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 200) + self.assertEqual(channel.json_body["actions"], ["dont_notify"]) + + def test_actions_404_when_get_inexistent(self): + """ + Tests that `actions` gives 404 when the rule doesn't exist. + """ + self.register_user("user", "pass") + token = self.login("user", "pass") + + body = { + "conditions": [ + {"kind": "event_match", "key": "sender", "pattern": "@user2:hs"} + ], + "actions": ["notify", {"set_tweak": "highlight"}], + } + + # check 404 for never-heard-of rule + request, channel = self.make_request( + "GET", "/pushrules/global/override/best.friend/enabled", access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 404) + self.assertEqual(channel.json_body["errcode"], Codes.NOT_FOUND) + + # PUT a new rule + request, channel = self.make_request( + "PUT", "/pushrules/global/override/best.friend", body, access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 200) + + # DELETE the rule + request, channel = self.make_request( + "DELETE", "/pushrules/global/override/best.friend", access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 200) + + # check 404 for deleted rule + request, channel = self.make_request( + "GET", "/pushrules/global/override/best.friend/enabled", access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 404) + self.assertEqual(channel.json_body["errcode"], Codes.NOT_FOUND) + + def test_actions_404_when_get_inexistent_server_rule(self): + """ + Tests that `actions` gives 404 when the server-default rule doesn't exist. + """ + self.register_user("user", "pass") + token = self.login("user", "pass") + + # check 404 for never-heard-of rule + request, channel = self.make_request( + "GET", "/pushrules/global/override/.m.muahahaha/actions", access_token=token + ) + self.render(request) + self.assertEqual(channel.code, 404) + self.assertEqual(channel.json_body["errcode"], Codes.NOT_FOUND) + + def test_actions_404_when_put_inexistent_rule(self): + """ + Tests that `actions` gives 404 when the default rule doesn't exist. + """ + self.register_user("user", "pass") + token = self.login("user", "pass") + + # enable & check 404 for never-heard-of rule + request, channel = self.make_request( + "PUT", + "/pushrules/global/override/best.friend/actions", + {"actions": ["dont_notify"]}, + access_token=token, + ) + self.render(request) + self.assertEqual(channel.code, 404) + self.assertEqual(channel.json_body["errcode"], Codes.NOT_FOUND) + + def test_actions_404_when_put_inexistent_server_rule(self): + """ + Tests that `enabled` gives 404 when the server-default rule doesn't exist. + """ + self.register_user("user", "pass") + token = self.login("user", "pass") + + # enable & check 404 for never-heard-of rule + request, channel = self.make_request( + "PUT", + "/pushrules/global/override/.m.muahahah/enabled", + {"actions": ["dont_notify"]}, + access_token=token, + ) + self.render(request) + self.assertEqual(channel.code, 404) + self.assertEqual(channel.json_body["errcode"], Codes.NOT_FOUND) From 4b5bfc7741a5c2b2a3857657d576baeb4cf7dcd2 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 7 Jul 2020 11:48:50 +0100 Subject: [PATCH 02/28] Fix test bug --- tests/rest/client/v1/test_push_rule_attrs.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/rest/client/v1/test_push_rule_attrs.py b/tests/rest/client/v1/test_push_rule_attrs.py index 09d5c7454008..f357a0ac6172 100644 --- a/tests/rest/client/v1/test_push_rule_attrs.py +++ b/tests/rest/client/v1/test_push_rule_attrs.py @@ -438,7 +438,7 @@ def test_actions_404_when_put_inexistent_server_rule(self): # enable & check 404 for never-heard-of rule request, channel = self.make_request( "PUT", - "/pushrules/global/override/.m.muahahah/enabled", + "/pushrules/global/override/.m.muahahah/actions", {"actions": ["dont_notify"]}, access_token=token, ) From 7f6791f109b981b282bc69a76e0a8b6c961f68b5 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 7 Jul 2020 11:50:07 +0100 Subject: [PATCH 03/28] Make test_actions_404_when_put_inexistent_server_rule pass --- synapse/rest/client/v1/push_rule.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/rest/client/v1/push_rule.py b/synapse/rest/client/v1/push_rule.py index 9fd490813693..518075ef3555 100644 --- a/synapse/rest/client/v1/push_rule.py +++ b/synapse/rest/client/v1/push_rule.py @@ -18,7 +18,7 @@ NotFoundError, StoreError, SynapseError, - UnrecognizedRequestError, + UnrecognizedRequestError, Codes, ) from synapse.http.servlet import ( RestServlet, @@ -180,7 +180,7 @@ def set_rule_attr(self, user_id, spec, val): 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 SynapseError(404, "Unknown rule %r" % (namespaced_rule_id,), Codes.NOT_FOUND) return self.store.set_push_rule_actions( user_id, namespaced_rule_id, actions, is_default_rule ) From d01e5369aa40d0031ac4c31dcfee595f1ed9a63a Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 7 Jul 2020 11:57:58 +0100 Subject: [PATCH 04/28] Delete push_rule_enable row on rule deletion Make test_enabled_on_recreation pass --- synapse/storage/data_stores/main/push_rule.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/synapse/storage/data_stores/main/push_rule.py b/synapse/storage/data_stores/main/push_rule.py index f6e78ca5903f..3d72af397867 100644 --- a/synapse/storage/data_stores/main/push_rule.py +++ b/synapse/storage/data_stores/main/push_rule.py @@ -627,6 +627,12 @@ def delete_push_rule(self, user_id, rule_id): """ def delete_push_rule_txn(txn, stream_id, event_stream_ordering): + # we don't use simple_delete_one_txn because that would fail if the + # user did not have a push_rule_enable row. + self.db.simple_delete_txn( + txn, "push_rules_enable", {"user_name": user_id, "rule_id": rule_id} + ) + self.db.simple_delete_one_txn( txn, "push_rules", {"user_name": user_id, "rule_id": rule_id} ) From bdfef699196a166fde28702312bc65c5c0199fa8 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 7 Jul 2020 12:02:38 +0100 Subject: [PATCH 05/28] Use M_NOTFOUND code for missing push rules Makes test_actions_404_when_put_inexistent_rule pass --- synapse/storage/data_stores/main/push_rule.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/synapse/storage/data_stores/main/push_rule.py b/synapse/storage/data_stores/main/push_rule.py index 3d72af397867..b91212894f0d 100644 --- a/synapse/storage/data_stores/main/push_rule.py +++ b/synapse/storage/data_stores/main/push_rule.py @@ -22,6 +22,7 @@ from twisted.internet import defer +from synapse.api.errors import SynapseError, Codes 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 @@ -708,12 +709,18 @@ def set_push_rule_actions_txn(txn, stream_id, event_stream_ordering): update_stream=False, ) else: - self.db.simple_update_one_txn( - txn, - "push_rules", - {"user_name": user_id, "rule_id": rule_id}, - {"actions": actions_json}, - ) + try: + self.db.simple_update_one_txn( + txn, + "push_rules", + {"user_name": user_id, "rule_id": rule_id}, + {"actions": actions_json}, + ) + except SynapseError as serr: + if serr.code == 404: + raise SynapseError(404, "Push rule does not exist", Codes.NOT_FOUND) + else: + raise serr self._insert_push_rules_update_txn( txn, From 3911200b9ec576e32d89c09683f7841133f4802e Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 7 Jul 2020 12:08:43 +0100 Subject: [PATCH 06/28] Check for nonexistant rules first Makes test_enabled_404_when_put_inexistent_server_rule pass --- synapse/rest/client/v1/push_rule.py | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/synapse/rest/client/v1/push_rule.py b/synapse/rest/client/v1/push_rule.py index 518075ef3555..4f8f2fca9680 100644 --- a/synapse/rest/client/v1/push_rule.py +++ b/synapse/rest/client/v1/push_rule.py @@ -162,6 +162,19 @@ def notify_user(self, user_id): self.notifier.on_new_event("push_rules_key", stream_id, users=[user_id]) def set_rule_attr(self, user_id, spec, val): + 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 SynapseError(404, "Unknown rule %r" % (namespaced_rule_id,), Codes.NOT_FOUND) + if spec["attr"] == "enabled": if isinstance(val, dict) and "enabled" in val: val = val["enabled"] @@ -170,17 +183,10 @@ def set_rule_attr(self, user_id, spec, val): # 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") - namespaced_rule_id = _namespaced_rule_id_from_spec(spec) return self.store.set_push_rule_enabled(user_id, namespaced_rule_id, val) elif spec["attr"] == "actions": actions = val.get("actions") _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,), Codes.NOT_FOUND) return self.store.set_push_rule_actions( user_id, namespaced_rule_id, actions, is_default_rule ) From 271a9371837633b0304e1de4ee253a0421e85637 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 7 Jul 2020 12:21:40 +0100 Subject: [PATCH 07/28] Check non-server-default push rules exist before enabling/disabling Makes test_enabled_404_when_put_inexistent_rule pass. --- synapse/rest/client/v1/push_rule.py | 2 +- synapse/storage/data_stores/main/push_rule.py | 21 ++++++++++++++++--- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/synapse/rest/client/v1/push_rule.py b/synapse/rest/client/v1/push_rule.py index 4f8f2fca9680..cfdf9dc00eb4 100644 --- a/synapse/rest/client/v1/push_rule.py +++ b/synapse/rest/client/v1/push_rule.py @@ -183,7 +183,7 @@ def set_rule_attr(self, user_id, spec, val): # 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") - return self.store.set_push_rule_enabled(user_id, namespaced_rule_id, val) + return self.store.set_push_rule_enabled(user_id, namespaced_rule_id, val, is_default_rule) elif spec["attr"] == "actions": actions = val.get("actions") _check_actions(actions) diff --git a/synapse/storage/data_stores/main/push_rule.py b/synapse/storage/data_stores/main/push_rule.py index b91212894f0d..78fcb9ed4500 100644 --- a/synapse/storage/data_stores/main/push_rule.py +++ b/synapse/storage/data_stores/main/push_rule.py @@ -22,7 +22,7 @@ from twisted.internet import defer -from synapse.api.errors import SynapseError, Codes +from synapse.api.errors import SynapseError, Codes, 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 @@ -652,7 +652,7 @@ def delete_push_rule_txn(txn, stream_id, event_stream_ordering): ) @defer.inlineCallbacks - def set_push_rule_enabled(self, user_id, rule_id, enabled): + def set_push_rule_enabled(self, user_id, rule_id, enabled, is_default_rule): with self._push_rules_stream_id_gen.get_next() as ids: stream_id, event_stream_ordering = ids yield self.db.runInteraction( @@ -663,12 +663,27 @@ def set_push_rule_enabled(self, user_id, rule_id, enabled): user_id, rule_id, enabled, + is_default_rule, ) def _set_push_rule_enabled_txn( - self, txn, stream_id, event_stream_ordering, user_id, rule_id, enabled + self, txn, stream_id, event_stream_ordering, user_id, rule_id, enabled, is_default_rule ): new_id = self._push_rules_enable_id_gen.get_next() + + if not is_default_rule: + try: + # first check it exists + self.db.simple_select_one_onecol_txn( + txn, + "push_rules", + {"user_name": user_id, "rule_id": rule_id}, + "id" + ) + except StoreError as serr: + if serr.code == 404: + raise StoreError(404, "Push rule does not exist.", Codes.NOT_FOUND) + self.db.simple_upsert_txn( txn, "push_rules_enable", From cc3dfa7ac400655e0773d5b8ca493a9324874f3b Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 7 Jul 2020 12:34:38 +0100 Subject: [PATCH 08/28] Add SQL migration to remove dangling push_rules_enabled rows. --- .../59/pushrules_enabled_delete_obsolete.sql | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 synapse/storage/data_stores/main/schema/delta/59/pushrules_enabled_delete_obsolete.sql diff --git a/synapse/storage/data_stores/main/schema/delta/59/pushrules_enabled_delete_obsolete.sql b/synapse/storage/data_stores/main/schema/delta/59/pushrules_enabled_delete_obsolete.sql new file mode 100644 index 000000000000..8d58fa5e647f --- /dev/null +++ b/synapse/storage/data_stores/main/schema/delta/59/pushrules_enabled_delete_obsolete.sql @@ -0,0 +1,32 @@ +/* Copyright 2020 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. + */ + +/** + Delete stuck 'enabled' bits that correspond to deleted push rules. + We ignore rules that are server-default rules because they are not defined + in the `push_rules` table. +**/ + +DELETE FROM push_rules_enable pre WHERE + pre.rule_id NOT LIKE 'global/override/.%' + AND pre.rule_id NOT LIKE 'global/underride/.%' + AND pre.rule_id NOT LIKE 'global/sender/.%' + AND pre.rule_id NOT LIKE 'global/room/.%' + AND pre.rule_id NOT LIKE 'global/content/.%' + AND NOT EXISTS ( + SELECT 1 FROM push_rules pr + WHERE pr.user_name = pre.user_name + AND pr.rule_id = pre.rule_id + ); From e0cb3d2c5fd90c79b57affc88c25f42443205028 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 7 Jul 2020 12:36:04 +0100 Subject: [PATCH 09/28] Antilint --- synapse/rest/client/v1/push_rule.py | 11 ++++++++--- synapse/storage/data_stores/main/push_rule.py | 18 ++++++++++++------ 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/synapse/rest/client/v1/push_rule.py b/synapse/rest/client/v1/push_rule.py index cfdf9dc00eb4..4ec6b71a895a 100644 --- a/synapse/rest/client/v1/push_rule.py +++ b/synapse/rest/client/v1/push_rule.py @@ -18,7 +18,8 @@ NotFoundError, StoreError, SynapseError, - UnrecognizedRequestError, Codes, + UnrecognizedRequestError, + Codes, ) from synapse.http.servlet import ( RestServlet, @@ -173,7 +174,9 @@ def set_rule_attr(self, user_id, spec, val): 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,), Codes.NOT_FOUND) + raise SynapseError( + 404, "Unknown rule %r" % (namespaced_rule_id,), Codes.NOT_FOUND + ) if spec["attr"] == "enabled": if isinstance(val, dict) and "enabled" in val: @@ -183,7 +186,9 @@ def set_rule_attr(self, user_id, spec, val): # 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") - return self.store.set_push_rule_enabled(user_id, namespaced_rule_id, val, is_default_rule) + return self.store.set_push_rule_enabled( + user_id, namespaced_rule_id, val, is_default_rule + ) elif spec["attr"] == "actions": actions = val.get("actions") _check_actions(actions) diff --git a/synapse/storage/data_stores/main/push_rule.py b/synapse/storage/data_stores/main/push_rule.py index 78fcb9ed4500..8ff4eb94fd05 100644 --- a/synapse/storage/data_stores/main/push_rule.py +++ b/synapse/storage/data_stores/main/push_rule.py @@ -667,7 +667,14 @@ def set_push_rule_enabled(self, user_id, rule_id, enabled, is_default_rule): ) def _set_push_rule_enabled_txn( - self, txn, stream_id, event_stream_ordering, user_id, rule_id, enabled, is_default_rule + self, + txn, + stream_id, + event_stream_ordering, + user_id, + rule_id, + enabled, + is_default_rule, ): new_id = self._push_rules_enable_id_gen.get_next() @@ -675,10 +682,7 @@ def _set_push_rule_enabled_txn( try: # first check it exists self.db.simple_select_one_onecol_txn( - txn, - "push_rules", - {"user_name": user_id, "rule_id": rule_id}, - "id" + txn, "push_rules", {"user_name": user_id, "rule_id": rule_id}, "id" ) except StoreError as serr: if serr.code == 404: @@ -733,7 +737,9 @@ def set_push_rule_actions_txn(txn, stream_id, event_stream_ordering): ) except SynapseError as serr: if serr.code == 404: - raise SynapseError(404, "Push rule does not exist", Codes.NOT_FOUND) + raise SynapseError( + 404, "Push rule does not exist", Codes.NOT_FOUND + ) else: raise serr From 9e664629a781a17b5fae201baffab26ad68b91f4 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 7 Jul 2020 12:38:03 +0100 Subject: [PATCH 10/28] Newsfile --- changelog.d/7796.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/7796.bugfix diff --git a/changelog.d/7796.bugfix b/changelog.d/7796.bugfix new file mode 100644 index 000000000000..9343636ff04c --- /dev/null +++ b/changelog.d/7796.bugfix @@ -0,0 +1 @@ +Fix inconsistent handling of nonexistant push rules' and no longer latently track the 'enabled' bit of removed push rules. From 7987fb90e8783dee33bfebb7ca1267d9a624906d Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 7 Jul 2020 12:44:02 +0100 Subject: [PATCH 11/28] I sort, you sort, we all sort! --- synapse/rest/client/v1/push_rule.py | 4 +--- synapse/storage/data_stores/main/push_rule.py | 3 +-- tests/rest/client/v1/test_push_rule_attrs.py | 3 ++- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/synapse/rest/client/v1/push_rule.py b/synapse/rest/client/v1/push_rule.py index 4ec6b71a895a..6c70ce6f00d9 100644 --- a/synapse/rest/client/v1/push_rule.py +++ b/synapse/rest/client/v1/push_rule.py @@ -12,14 +12,12 @@ # 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 synapse.api.errors import ( + Codes, NotFoundError, StoreError, SynapseError, UnrecognizedRequestError, - Codes, ) from synapse.http.servlet import ( RestServlet, diff --git a/synapse/storage/data_stores/main/push_rule.py b/synapse/storage/data_stores/main/push_rule.py index 8ff4eb94fd05..2bcdc62ba971 100644 --- a/synapse/storage/data_stores/main/push_rule.py +++ b/synapse/storage/data_stores/main/push_rule.py @@ -13,7 +13,6 @@ # 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. - import abc import logging from typing import List, Tuple, Union @@ -22,7 +21,7 @@ from twisted.internet import defer -from synapse.api.errors import SynapseError, Codes, StoreError +from synapse.api.errors import Codes, StoreError, SynapseError 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 diff --git a/tests/rest/client/v1/test_push_rule_attrs.py b/tests/rest/client/v1/test_push_rule_attrs.py index f357a0ac6172..7dafad22d2d5 100644 --- a/tests/rest/client/v1/test_push_rule_attrs.py +++ b/tests/rest/client/v1/test_push_rule_attrs.py @@ -14,7 +14,8 @@ # limitations under the License. import synapse from synapse.api.errors import Codes -from synapse.rest.client.v1 import room, login, push_rule +from synapse.rest.client.v1 import login, push_rule, room + from tests.unittest import HomeserverTestCase From aebeb9f63eba990b769973c13d2b9f8829468fb1 Mon Sep 17 00:00:00 2001 From: reivilibre <38398653+reivilibre@users.noreply.github.com> Date: Fri, 17 Jul 2020 13:15:24 +0100 Subject: [PATCH 12/28] Apply magic from Rich Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- changelog.d/7796.bugfix | 2 +- synapse/rest/client/v1/push_rule.py | 2 +- synapse/storage/data_stores/main/push_rule.py | 2 +- .../main/schema/delta/59/pushrules_enabled_delete_obsolete.sql | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/changelog.d/7796.bugfix b/changelog.d/7796.bugfix index 9343636ff04c..65e5eb42a248 100644 --- a/changelog.d/7796.bugfix +++ b/changelog.d/7796.bugfix @@ -1 +1 @@ -Fix inconsistent handling of nonexistant push rules' and no longer latently track the 'enabled' bit of removed push rules. +Fix inconsistent handling of non-existent push rules, and stop tracking the `enabled` state of removed push rules. diff --git a/synapse/rest/client/v1/push_rule.py b/synapse/rest/client/v1/push_rule.py index 6c70ce6f00d9..01773096198f 100644 --- a/synapse/rest/client/v1/push_rule.py +++ b/synapse/rest/client/v1/push_rule.py @@ -173,7 +173,7 @@ def set_rule_attr(self, user_id, spec, val): if is_default_rule: if namespaced_rule_id not in BASE_RULE_IDS: raise SynapseError( - 404, "Unknown rule %r" % (namespaced_rule_id,), Codes.NOT_FOUND + 404, "Unknown rule %s" % (namespaced_rule_id,), Codes.NOT_FOUND ) if spec["attr"] == "enabled": diff --git a/synapse/storage/data_stores/main/push_rule.py b/synapse/storage/data_stores/main/push_rule.py index 2bcdc62ba971..1f0a621fda59 100644 --- a/synapse/storage/data_stores/main/push_rule.py +++ b/synapse/storage/data_stores/main/push_rule.py @@ -740,7 +740,7 @@ def set_push_rule_actions_txn(txn, stream_id, event_stream_ordering): 404, "Push rule does not exist", Codes.NOT_FOUND ) else: - raise serr + raise self._insert_push_rules_update_txn( txn, diff --git a/synapse/storage/data_stores/main/schema/delta/59/pushrules_enabled_delete_obsolete.sql b/synapse/storage/data_stores/main/schema/delta/59/pushrules_enabled_delete_obsolete.sql index 8d58fa5e647f..5fb8118f52d9 100644 --- a/synapse/storage/data_stores/main/schema/delta/59/pushrules_enabled_delete_obsolete.sql +++ b/synapse/storage/data_stores/main/schema/delta/59/pushrules_enabled_delete_obsolete.sql @@ -14,7 +14,7 @@ */ /** - Delete stuck 'enabled' bits that correspond to deleted push rules. + Delete stuck 'enabled' bits that correspond to deleted or non-existent push rules. We ignore rules that are server-default rules because they are not defined in the `push_rules` table. **/ From 0cc3f0294d9bff3f6f5fdb375d0d303e3bea7273 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 17 Jul 2020 13:19:23 +0100 Subject: [PATCH 13/28] Move delta to correct place and simplify. --- .../10_pushrules_enabled_delete_obsolete.sql} | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) rename synapse/storage/data_stores/main/schema/delta/{59/pushrules_enabled_delete_obsolete.sql => 58/10_pushrules_enabled_delete_obsolete.sql} (81%) diff --git a/synapse/storage/data_stores/main/schema/delta/59/pushrules_enabled_delete_obsolete.sql b/synapse/storage/data_stores/main/schema/delta/58/10_pushrules_enabled_delete_obsolete.sql similarity index 81% rename from synapse/storage/data_stores/main/schema/delta/59/pushrules_enabled_delete_obsolete.sql rename to synapse/storage/data_stores/main/schema/delta/58/10_pushrules_enabled_delete_obsolete.sql index 5fb8118f52d9..afd45c732bf3 100644 --- a/synapse/storage/data_stores/main/schema/delta/59/pushrules_enabled_delete_obsolete.sql +++ b/synapse/storage/data_stores/main/schema/delta/58/10_pushrules_enabled_delete_obsolete.sql @@ -20,11 +20,7 @@ **/ DELETE FROM push_rules_enable pre WHERE - pre.rule_id NOT LIKE 'global/override/.%' - AND pre.rule_id NOT LIKE 'global/underride/.%' - AND pre.rule_id NOT LIKE 'global/sender/.%' - AND pre.rule_id NOT LIKE 'global/room/.%' - AND pre.rule_id NOT LIKE 'global/content/.%' + pre.rule_id NOT LIKE 'global/%/.m.rule.%' AND NOT EXISTS ( SELECT 1 FROM push_rules pr WHERE pr.user_name = pre.user_name From afea1e5fb0a40c3183e46e52b2bf602ed5d652a1 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 17 Jul 2020 13:20:58 +0100 Subject: [PATCH 14/28] inexistent could be said to be non-existent --- tests/rest/client/v1/test_push_rule_attrs.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/rest/client/v1/test_push_rule_attrs.py b/tests/rest/client/v1/test_push_rule_attrs.py index 7dafad22d2d5..2d4201fd2208 100644 --- a/tests/rest/client/v1/test_push_rule_attrs.py +++ b/tests/rest/client/v1/test_push_rule_attrs.py @@ -180,7 +180,7 @@ def test_enabled_disable(self): self.assertEqual(channel.code, 200) self.assertEqual(channel.json_body["enabled"], True) - def test_enabled_404_when_get_inexistent(self): + def test_enabled_404_when_get_non_existent(self): """ Tests that `enabled` gives 404 when the rule doesn't exist. """ @@ -231,7 +231,7 @@ def test_enabled_404_when_get_inexistent(self): self.assertEqual(channel.code, 404) self.assertEqual(channel.json_body["errcode"], Codes.NOT_FOUND) - def test_enabled_404_when_get_inexistent_server_rule(self): + def test_enabled_404_when_get_non_existent_server_rule(self): """ Tests that `enabled` gives 404 when the server-default rule doesn't exist. """ @@ -246,7 +246,7 @@ def test_enabled_404_when_get_inexistent_server_rule(self): self.assertEqual(channel.code, 404) self.assertEqual(channel.json_body["errcode"], Codes.NOT_FOUND) - def test_enabled_404_when_put_inexistent_rule(self): + def test_enabled_404_when_put_non_existent_rule(self): """ Tests that `enabled` gives 404 when the server-default rule doesn't exist. """ @@ -264,7 +264,7 @@ def test_enabled_404_when_put_inexistent_rule(self): self.assertEqual(channel.code, 404) self.assertEqual(channel.json_body["errcode"], Codes.NOT_FOUND) - def test_enabled_404_when_put_inexistent_server_rule(self): + def test_enabled_404_when_put_non_existent_server_rule(self): """ Tests that `enabled` gives 404 when the server-default rule doesn't exist. """ @@ -352,7 +352,7 @@ def test_actions_put(self): self.assertEqual(channel.code, 200) self.assertEqual(channel.json_body["actions"], ["dont_notify"]) - def test_actions_404_when_get_inexistent(self): + def test_actions_404_when_get_non_existent(self): """ Tests that `actions` gives 404 when the rule doesn't exist. """ @@ -396,7 +396,7 @@ def test_actions_404_when_get_inexistent(self): self.assertEqual(channel.code, 404) self.assertEqual(channel.json_body["errcode"], Codes.NOT_FOUND) - def test_actions_404_when_get_inexistent_server_rule(self): + def test_actions_404_when_get_non_existent_server_rule(self): """ Tests that `actions` gives 404 when the server-default rule doesn't exist. """ @@ -411,7 +411,7 @@ def test_actions_404_when_get_inexistent_server_rule(self): self.assertEqual(channel.code, 404) self.assertEqual(channel.json_body["errcode"], Codes.NOT_FOUND) - def test_actions_404_when_put_inexistent_rule(self): + def test_actions_404_when_put_non_existent_rule(self): """ Tests that `actions` gives 404 when the default rule doesn't exist. """ @@ -429,7 +429,7 @@ def test_actions_404_when_put_inexistent_rule(self): self.assertEqual(channel.code, 404) self.assertEqual(channel.json_body["errcode"], Codes.NOT_FOUND) - def test_actions_404_when_put_inexistent_server_rule(self): + def test_actions_404_when_put_non_existent_server_rule(self): """ Tests that `enabled` gives 404 when the server-default rule doesn't exist. """ From 0118ac5a5b2829e283fdd0927317dcc6912e6c64 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 17 Jul 2020 13:22:26 +0100 Subject: [PATCH 15/28] Tighten to StoreError --- synapse/storage/data_stores/main/push_rule.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/data_stores/main/push_rule.py b/synapse/storage/data_stores/main/push_rule.py index 1f0a621fda59..d8a156be7d7f 100644 --- a/synapse/storage/data_stores/main/push_rule.py +++ b/synapse/storage/data_stores/main/push_rule.py @@ -734,9 +734,9 @@ def set_push_rule_actions_txn(txn, stream_id, event_stream_ordering): {"user_name": user_id, "rule_id": rule_id}, {"actions": actions_json}, ) - except SynapseError as serr: + except StoreError as serr: if serr.code == 404: - raise SynapseError( + raise StoreError( 404, "Push rule does not exist", Codes.NOT_FOUND ) else: From 872a39c6e5fa8364c7bb0a9ae73dc1f905c3c634 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 17 Jul 2020 13:29:22 +0100 Subject: [PATCH 16/28] Wow, I did not find the NotFoundError :) --- synapse/rest/client/v1/push_rule.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/rest/client/v1/push_rule.py b/synapse/rest/client/v1/push_rule.py index 01773096198f..6d5c9ed3dee6 100644 --- a/synapse/rest/client/v1/push_rule.py +++ b/synapse/rest/client/v1/push_rule.py @@ -172,8 +172,8 @@ def set_rule_attr(self, user_id, spec, val): is_default_rule = rule_id.startswith(".") if is_default_rule: if namespaced_rule_id not in BASE_RULE_IDS: - raise SynapseError( - 404, "Unknown rule %s" % (namespaced_rule_id,), Codes.NOT_FOUND + raise NotFoundError( + "Unknown rule %s" % (namespaced_rule_id,) ) if spec["attr"] == "enabled": From e8ea6a8a37876b2c920ba7c658cb778ff54a324b Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 17 Jul 2020 13:31:59 +0100 Subject: [PATCH 17/28] Antilint --- synapse/rest/client/v1/push_rule.py | 5 +---- synapse/storage/data_stores/main/push_rule.py | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/synapse/rest/client/v1/push_rule.py b/synapse/rest/client/v1/push_rule.py index 6d5c9ed3dee6..d18c4fea74db 100644 --- a/synapse/rest/client/v1/push_rule.py +++ b/synapse/rest/client/v1/push_rule.py @@ -13,7 +13,6 @@ # See the License for the specific language governing permissions and # limitations under the License. from synapse.api.errors import ( - Codes, NotFoundError, StoreError, SynapseError, @@ -172,9 +171,7 @@ def set_rule_attr(self, user_id, spec, val): 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 NotFoundError("Unknown rule %s" % (namespaced_rule_id,)) if spec["attr"] == "enabled": if isinstance(val, dict) and "enabled" in val: diff --git a/synapse/storage/data_stores/main/push_rule.py b/synapse/storage/data_stores/main/push_rule.py index d8a156be7d7f..7be46eec3b40 100644 --- a/synapse/storage/data_stores/main/push_rule.py +++ b/synapse/storage/data_stores/main/push_rule.py @@ -21,7 +21,7 @@ from twisted.internet import defer -from synapse.api.errors import Codes, StoreError, SynapseError +from synapse.api.errors import Codes, 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 From 6667e271feb44002f90d89dcdccfdfa03290d370 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 17 Jul 2020 14:06:28 +0100 Subject: [PATCH 18/28] Change syntax to support old SQLite3s --- .../delta/58/10_pushrules_enabled_delete_obsolete.sql | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/synapse/storage/data_stores/main/schema/delta/58/10_pushrules_enabled_delete_obsolete.sql b/synapse/storage/data_stores/main/schema/delta/58/10_pushrules_enabled_delete_obsolete.sql index afd45c732bf3..847aebd85eae 100644 --- a/synapse/storage/data_stores/main/schema/delta/58/10_pushrules_enabled_delete_obsolete.sql +++ b/synapse/storage/data_stores/main/schema/delta/58/10_pushrules_enabled_delete_obsolete.sql @@ -19,10 +19,10 @@ in the `push_rules` table. **/ -DELETE FROM push_rules_enable pre WHERE - pre.rule_id NOT LIKE 'global/%/.m.rule.%' +DELETE FROM push_rules_enable WHERE + rule_id NOT LIKE 'global/%/.m.rule.%' AND NOT EXISTS ( - SELECT 1 FROM push_rules pr - WHERE pr.user_name = pre.user_name - AND pr.rule_id = pre.rule_id + SELECT 1 FROM push_rules + WHERE push_rules.user_name = push_rules_enable.user_name + AND push_rules.rule_id = push_rules_enable.rule_id ); From 7565a445c946f8f74ec8a7cbb31ff61b10bc7b75 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 4 Aug 2020 10:58:04 +0100 Subject: [PATCH 19/28] Docstrings, type annotations and minor changes for readability --- synapse/storage/data_stores/main/push_rule.py | 62 ++++++++++++++++--- 1 file changed, 54 insertions(+), 8 deletions(-) diff --git a/synapse/storage/data_stores/main/push_rule.py b/synapse/storage/data_stores/main/push_rule.py index 7be46eec3b40..21f169f94c34 100644 --- a/synapse/storage/data_stores/main/push_rule.py +++ b/synapse/storage/data_stores/main/push_rule.py @@ -21,7 +21,7 @@ from twisted.internet import defer -from synapse.api.errors import Codes, StoreError +from synapse.api.errors import NotFoundError, 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 @@ -651,7 +651,27 @@ def delete_push_rule_txn(txn, stream_id, event_stream_ordering): ) @defer.inlineCallbacks - def set_push_rule_enabled(self, user_id, rule_id, enabled, is_default_rule): + def set_push_rule_enabled( + self, user_id: str, rule_id: str, enabled: bool, is_default_rule: bool + ): + """ + Sets the `enabled` 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' + rule_id: the full rule ID of the rule to be enabled/disabled + e.g. 'global/override/.m.rule.roomnotif' + or 'global/override/myCustomRule' + enabled: True if the rule is to be enabled, False if it is to be + disabled + 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). + """ with self._push_rules_stream_id_gen.get_next() as ids: stream_id, event_stream_ordering = ids yield self.db.runInteraction( @@ -679,13 +699,16 @@ def _set_push_rule_enabled_txn( if not is_default_rule: try: - # first check it exists + # first check it exists XXX need SELECT FOR KEY SHARE self.db.simple_select_one_onecol_txn( txn, "push_rules", {"user_name": user_id, "rule_id": rule_id}, "id" ) except StoreError as serr: if serr.code == 404: - raise StoreError(404, "Push rule does not exist.", Codes.NOT_FOUND) + # needed to set NOT_FOUND code. + raise NotFoundError("Push rule does not exist.") + else: + raise self.db.simple_upsert_txn( txn, @@ -705,7 +728,31 @@ def _set_push_rule_enabled_txn( ) @defer.inlineCallbacks - def set_push_rule_actions(self, user_id, rule_id, actions, is_default_rule): + def set_push_rule_actions( + self, + user_id: str, + rule_id: str, + actions: List[Union[dict, str]], + is_default_rule: bool, + ): + """ + 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' + rule_id: the full rule ID of the rule to be enabled/disabled + e.g. 'global/override/.m.rule.roomnotif' + or 'global/override/myCustomRule' + actions: A list of actions (each action being a dict or string), + e.g. ["notify", {"set_tweak": "highlight", "value": false}] + 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). + """ actions_json = json.dumps(actions) def set_push_rule_actions_txn(txn, stream_id, event_stream_ordering): @@ -736,9 +783,8 @@ def set_push_rule_actions_txn(txn, stream_id, event_stream_ordering): ) except StoreError as serr: if serr.code == 404: - raise StoreError( - 404, "Push rule does not exist", Codes.NOT_FOUND - ) + # this sets the NOT_FOUND error Code + raise NotFoundError("Push rule does not exist") else: raise From ee165dc078aefcdcff37a9ac242176005818eb65 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 18 Aug 2020 18:04:44 +0100 Subject: [PATCH 20/28] Always create an enabled row for push rules. --- synapse/storage/data_stores/main/push_rule.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/synapse/storage/data_stores/main/push_rule.py b/synapse/storage/data_stores/main/push_rule.py index 21f169f94c34..bbe04ecfde72 100644 --- a/synapse/storage/data_stores/main/push_rule.py +++ b/synapse/storage/data_stores/main/push_rule.py @@ -614,6 +614,16 @@ def _upsert_push_rule_txn( }, ) + # ensure we have a push_rules_enable row + # enabledness defaults to true + sql = """ + INSERT INTO push_rules_enable (id, user_name, rule_id, enabled) + VALUES (?, ?, ?, ?) + ON CONFLICT DO NOTHING + """ + new_enable_id = self._push_rules_enable_id_gen.get_next() + txn.execute(sql, (new_enable_id, user_id, rule_id, True)) + @defer.inlineCallbacks def delete_push_rule(self, user_id, rule_id): """ From 32c1c2af78bda225b67e844b81a376ce091c40cc Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 18 Aug 2020 18:25:00 +0100 Subject: [PATCH 21/28] Use FOR KEY SHARE --- synapse/storage/databases/main/push_rule.py | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index 62a15c5f2924..f611e33583a0 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -631,17 +631,16 @@ def _set_push_rule_enabled_txn( new_id = self._push_rules_enable_id_gen.get_next() if not is_default_rule: - try: - # first check it exists XXX need SELECT FOR KEY SHARE - self.db.simple_select_one_onecol_txn( - txn, "push_rules", {"user_name": user_id, "rule_id": rule_id}, "id" - ) - except StoreError as serr: - if serr.code == 404: - # needed to set NOT_FOUND code. - raise NotFoundError("Push rule does not exist.") - else: - raise + # first check it exists — need FOR KEY SHARE + sql = """ + SELECT 1 FROM push_rules + WHERE user_name = ? AND rule_id = ? + FOR KEY SHARE + """ + txn.execute(sql, (user_id, rule_id)) + if txn.rowcount() < 1: + # needed to set NOT_FOUND code. + raise NotFoundError("Push rule does not exist.") self.db_pool.simple_upsert_txn( txn, From d5b63b95e4a1a8e798e3d4f273ce59e2b3bbc9d9 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 18 Aug 2020 18:41:33 +0100 Subject: [PATCH 22/28] rowcount is not what I thought --- synapse/storage/databases/main/push_rule.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index f611e33583a0..9ee5efb706d7 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -638,7 +638,7 @@ def _set_push_rule_enabled_txn( FOR KEY SHARE """ txn.execute(sql, (user_id, rule_id)) - if txn.rowcount() < 1: + if txn.fetchone(): # needed to set NOT_FOUND code. raise NotFoundError("Push rule does not exist.") From 5f211a6d89bf9ce6df654b7398695421bf2ceff8 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Wed, 19 Aug 2020 08:02:01 +0100 Subject: [PATCH 23/28] Oops, 1 is truthy --- synapse/storage/databases/main/push_rule.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index 9ee5efb706d7..178b7acd1068 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -547,7 +547,7 @@ def _upsert_push_rule_txn( ON CONFLICT DO NOTHING """ new_enable_id = self._push_rules_enable_id_gen.get_next() - txn.execute(sql, (new_enable_id, user_id, rule_id, True)) + txn.execute(sql, (new_enable_id, user_id, rule_id, 1)) async def delete_push_rule(self, user_id: str, rule_id: str) -> None: """ From cca039dfe9a50edc55f49748d636d35b5651ee3e Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Wed, 19 Aug 2020 11:08:56 +0100 Subject: [PATCH 24/28] For key share is a Postgresqlism --- synapse/storage/databases/main/push_rule.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index 178b7acd1068..ac5ba8a089e0 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -29,6 +29,7 @@ from synapse.storage.databases.main.pusher import PusherWorkerStore from synapse.storage.databases.main.receipts import ReceiptsWorkerStore from synapse.storage.databases.main.roommember import RoomMemberWorkerStore +from synapse.storage.engines import PostgresEngine from synapse.storage.push_rule import InconsistentRuleException, RuleNotFoundException from synapse.storage.util.id_generators import ChainedIdGenerator from synapse.util import json_encoder @@ -632,13 +633,20 @@ def _set_push_rule_enabled_txn( if not is_default_rule: # first check it exists — need FOR KEY SHARE - sql = """ + for_key_share = "FOR KEY SHARE" + if not isinstance(self.database_engine, PostgresEngine): + # For key share is not applicable/available on SQLite + for_key_share = "" + sql = ( + """ SELECT 1 FROM push_rules WHERE user_name = ? AND rule_id = ? - FOR KEY SHARE + %s """ + % for_key_share + ) txn.execute(sql, (user_id, rule_id)) - if txn.fetchone(): + if txn.fetchone() is None: # needed to set NOT_FOUND code. raise NotFoundError("Push rule does not exist.") From bd1fad6cfe2df75c2c1aa8586268b2edd2567835 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Wed, 19 Aug 2020 11:09:04 +0100 Subject: [PATCH 25/28] Fix test docstrings --- tests/rest/client/v1/test_push_rule_attrs.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/rest/client/v1/test_push_rule_attrs.py b/tests/rest/client/v1/test_push_rule_attrs.py index 2d4201fd2208..081052f6a65e 100644 --- a/tests/rest/client/v1/test_push_rule_attrs.py +++ b/tests/rest/client/v1/test_push_rule_attrs.py @@ -248,7 +248,7 @@ def test_enabled_404_when_get_non_existent_server_rule(self): def test_enabled_404_when_put_non_existent_rule(self): """ - Tests that `enabled` gives 404 when the server-default rule doesn't exist. + Tests that `enabled` gives 404 when we put to a rule that doesn't exist. """ self.register_user("user", "pass") token = self.login("user", "pass") @@ -266,7 +266,7 @@ def test_enabled_404_when_put_non_existent_rule(self): def test_enabled_404_when_put_non_existent_server_rule(self): """ - Tests that `enabled` gives 404 when the server-default rule doesn't exist. + Tests that `enabled` gives 404 when we put to a server-default rule that doesn't exist. """ self.register_user("user", "pass") token = self.login("user", "pass") @@ -413,7 +413,7 @@ def test_actions_404_when_get_non_existent_server_rule(self): def test_actions_404_when_put_non_existent_rule(self): """ - Tests that `actions` gives 404 when the default rule doesn't exist. + Tests that `actions` gives 404 when putting to a rule that doesn't exist. """ self.register_user("user", "pass") token = self.login("user", "pass") @@ -431,7 +431,7 @@ def test_actions_404_when_put_non_existent_rule(self): def test_actions_404_when_put_non_existent_server_rule(self): """ - Tests that `enabled` gives 404 when the server-default rule doesn't exist. + Tests that `actions` gives 404 when putting to a server-default rule that doesn't exist. """ self.register_user("user", "pass") token = self.login("user", "pass") From 52035dfbd7166d27abb98cf4bd923ff091541342 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Wed, 19 Aug 2020 12:31:57 +0100 Subject: [PATCH 26/28] Fix old SQLite3 support --- synapse/storage/databases/main/push_rule.py | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index ac5ba8a089e0..5f6e03739546 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -29,7 +29,7 @@ from synapse.storage.databases.main.pusher import PusherWorkerStore from synapse.storage.databases.main.receipts import ReceiptsWorkerStore from synapse.storage.databases.main.roommember import RoomMemberWorkerStore -from synapse.storage.engines import PostgresEngine +from synapse.storage.engines import PostgresEngine, Sqlite3Engine from synapse.storage.push_rule import InconsistentRuleException, RuleNotFoundException from synapse.storage.util.id_generators import ChainedIdGenerator from synapse.util import json_encoder @@ -542,11 +542,20 @@ def _upsert_push_rule_txn( # ensure we have a push_rules_enable row # enabledness defaults to true - sql = """ - INSERT INTO push_rules_enable (id, user_name, rule_id, enabled) - VALUES (?, ?, ?, ?) - ON CONFLICT DO NOTHING - """ + if isinstance(self.database_engine, PostgresEngine): + sql = """ + INSERT INTO push_rules_enable (id, user_name, rule_id, enabled) + VALUES (?, ?, ?, ?) + ON CONFLICT DO NOTHING + """ + elif isinstance(self.database_engine, Sqlite3Engine): + sql = """ + INSERT OR IGNORE INTO push_rules_enable (id, user_name, rule_id, enabled) + VALUES (?, ?, ?, ?) + """ + else: + raise RuntimeError("Unknown database engine") + new_enable_id = self._push_rules_enable_id_gen.get_next() txn.execute(sql, (new_enable_id, user_id, rule_id, 1)) From ce94cc8b63234fc5b68ee62a2258d0f1e41bafe7 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 21 Aug 2020 17:48:45 +0100 Subject: [PATCH 27/28] Cosmetic changes --- synapse/storage/databases/main/push_rule.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index 70315dd42ac1..a3f415b4bca5 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -602,9 +602,6 @@ async def set_push_rule_enabled( """ Sets the `enabled` 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' @@ -616,6 +613,9 @@ async def set_push_rule_enabled( 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: + NotFoundError if the rule does not exist. """ with self._push_rules_stream_id_gen.get_next() as stream_id: event_stream_ordering = self._stream_id_gen.get_current_token() @@ -643,7 +643,14 @@ def _set_push_rule_enabled_txn( new_id = self._push_rules_enable_id_gen.get_next() if not is_default_rule: - # first check it exists — need FOR KEY SHARE + # first check it exists; we need to lock for key share so that a + # transaction that deletes the push rule will conflict with this one. + # We also need a push_rule_enable row to exist for every push_rules + # row, otherwise it is possible to simultaneously delete a push rule + # (that has no _enable row) and enable it, resulting in a dangling + # _enable row. To solve this: we either need to use SERIALISABLE or + # ensure we always have a push_rule_enable row for every push_rule + # row. We chose the latter. for_key_share = "FOR KEY SHARE" if not isinstance(self.database_engine, PostgresEngine): # For key share is not applicable/available on SQLite @@ -661,6 +668,7 @@ def _set_push_rule_enabled_txn( # needed to set NOT_FOUND code. raise NotFoundError("Push rule does not exist.") + self.db_pool.simple_upsert_txn( txn, "push_rules_enable", From 114b01fe7848fbe4c33009ad0504210e54655860 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Thu, 27 Aug 2020 08:40:40 +0100 Subject: [PATCH 28/28] Antilint Signed-off-by: Olivier Wilkinson (reivilibre) --- synapse/storage/databases/main/push_rule.py | 1 - 1 file changed, 1 deletion(-) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index a3f415b4bca5..4c875610c56c 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -668,7 +668,6 @@ def _set_push_rule_enabled_txn( # needed to set NOT_FOUND code. raise NotFoundError("Push rule does not exist.") - self.db_pool.simple_upsert_txn( txn, "push_rules_enable",