Matrix-Mirrors/anonymousland-synapse
1
0
Fork 0
mirror of https://git.anonymousland.org/anonymousland/synapse.git synced 2024-10-01 11:49:51 -04:00
Code Issues Packages Projects Releases Wiki Activity

Don't remember enabled of deleted push rules and properly return 404 for missing push rules in .../actions and .../enabled (#7796)

Browse Source 
Signed-off-by: Olivier Wilkinson (reivilibre) <olivier@librepush.net>

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
This commit is contained in:
reivilibre 2020-09-09 11:39:39 +01:00 committed by GitHub
parent e45b834119
commit a5370072b5
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 610 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
changelog.d/7796.bugfix Normal file
View File

@ -0,0 +1 @@
Fix inconsistent handling of non-existent push rules, and stop tracking the `enabled` state of removed push rules.

15
synapse/rest/client/v1/push_rule.py
View File

@ -163,6 +163,18 @@ class PushRuleRestServlet(RestServlet):
self.notifier.on_new_event("push_rules_key", stream_id, users=[user_id])
async 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 NotFoundError("Unknown rule %s" % (namespaced_rule_id,))
if spec["attr"] == "enabled":
if isinstance(val, dict) and "enabled" in val:
val = val["enabled"]
@ -171,9 +183,8 @@ class PushRuleRestServlet(RestServlet):
# 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 await self.store.set_push_rule_enabled(
user_id, namespaced_rule_id, val
user_id, namespaced_rule_id, val, is_default_rule
)
elif spec["attr"] == "actions":
actions = val.get("actions")

119
synapse/storage/databases/main/push_rule.py
View File

@ -13,11 +13,11 @@
# 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
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, db_to_json
@ -27,6 +27,7 @@ from synapse.storage.databases.main.events_worker import EventsWorkerStore
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, Sqlite3Engine
from synapse.storage.push_rule import InconsistentRuleException, RuleNotFoundException
from synapse.storage.util.id_generators import StreamIdGenerator
from synapse.util import json_encoder
@ -540,6 +541,25 @@ class PushRuleStore(PushRulesWorkerStore):
},
)
# ensure we have a push_rules_enable row
# enabledness defaults to true
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))
async def delete_push_rule(self, user_id: str, rule_id: str) -> None:
"""
Delete a push rule. Args specify the row to be deleted and can be
@ -552,6 +572,12 @@ class PushRuleStore(PushRulesWorkerStore):
"""
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_pool.simple_delete_txn(
txn, "push_rules_enable", {"user_name": user_id, "rule_id": rule_id}
)
self.db_pool.simple_delete_one_txn(
txn, "push_rules", {"user_name": user_id, "rule_id": rule_id}
)
@ -570,10 +596,29 @@ class PushRuleStore(PushRulesWorkerStore):
event_stream_ordering,
)
async def set_push_rule_enabled(self, user_id, rule_id, enabled) -> None:
async def set_push_rule_enabled(
self, user_id: str, rule_id: str, enabled: bool, is_default_rule: bool
) -> None:
"""
Sets the `enabled` state of a push rule.
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).
Raises:
NotFoundError if the rule does not exist.
"""
with await self._push_rules_stream_id_gen.get_next() as stream_id:
event_stream_ordering = self._stream_id_gen.get_current_token()
await self.db_pool.runInteraction(
"_set_push_rule_enabled_txn",
self._set_push_rule_enabled_txn,
@ -582,12 +627,47 @@ class PushRuleStore(PushRulesWorkerStore):
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:
# 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
for_key_share = ""
sql = (
"""
SELECT 1 FROM push_rules
WHERE user_name = ? AND rule_id = ?
%s
"""
% for_key_share
)
txn.execute(sql, (user_id, rule_id))
if txn.fetchone() is None:
# needed to set NOT_FOUND code.
raise NotFoundError("Push rule does not exist.")
self.db_pool.simple_upsert_txn(
txn,
"push_rules_enable",
@ -606,8 +686,30 @@ class PushRuleStore(PushRulesWorkerStore):
)
async def set_push_rule_actions(
self, user_id, rule_id, actions, is_default_rule
self,
user_id: str,
rule_id: str,
actions: List[Union[dict, str]],
is_default_rule: bool,
) -> None:
"""
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_encoder.encode(actions)
def set_push_rule_actions_txn(txn, stream_id, event_stream_ordering):
@ -629,12 +731,19 @@ class PushRuleStore(PushRulesWorkerStore):
update_stream=False,
)
else:
try:
self.db_pool.simple_update_one_txn(
txn,
"push_rules",
{"user_name": user_id, "rule_id": rule_id},
{"actions": actions_json},
)
except StoreError as serr:
if serr.code == 404:
# this sets the NOT_FOUND error Code
raise NotFoundError("Push rule does not exist")
else:
raise
self._insert_push_rules_update_txn(
txn,

28
synapse/storage/databases/main/schema/delta/58/10_pushrules_enabled_delete_obsolete.sql Normal file
View File

@ -0,0 +1,28 @@
/* 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 or non-existent 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 WHERE
rule_id NOT LIKE 'global/%/.m.rule.%'
AND NOT EXISTS (
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
);

448
tests/rest/client/v1/test_push_rule_attrs.py Normal file
View File

@ -0,0 +1,448 @@
# -*- 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 login, push_rule, room
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_non_existent(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_non_existent_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_non_existent_rule(self):
"""
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")
# 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_non_existent_server_rule(self):
"""
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")
# 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_non_existent(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_non_existent_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_non_existent_rule(self):
"""
Tests that `actions` gives 404 when putting to a rule that 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_non_existent_server_rule(self):
"""
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")
# enable & check 404 for never-heard-of rule
request, channel = self.make_request(
"PUT",
"/pushrules/global/override/.m.muahahah/actions",
{"actions": ["dont_notify"]},
access_token=token,
)
self.render(request)
self.assertEqual(channel.code, 404)
self.assertEqual(channel.json_body["errcode"], Codes.NOT_FOUND)