From f90782a6589ee567d6dc9ead19451cc8a0aabdd3 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 5 Feb 2015 09:35:21 +0000 Subject: [PATCH 1/3] namespace rule IDs to be unique within their scope and rule type. --- synapse/rest/client/v1/push_rule.py | 252 ++++++++++++++-------------- 1 file changed, 129 insertions(+), 123 deletions(-) diff --git a/synapse/rest/client/v1/push_rule.py b/synapse/rest/client/v1/push_rule.py index 5582f33c8e..eaef55cc1e 100644 --- a/synapse/rest/client/v1/push_rule.py +++ b/synapse/rest/client/v1/push_rule.py @@ -38,100 +38,9 @@ class PushRuleRestServlet(ClientV1RestServlet): SLIGHTLY_PEDANTIC_TRAILING_SLASH_ERROR = ( "Unrecognised request: You probably wanted a trailing slash") - def rule_spec_from_path(self, path): - if len(path) < 2: - raise UnrecognizedRequestError() - if path[0] != 'pushrules': - raise UnrecognizedRequestError() - - scope = path[1] - path = path[2:] - if scope not in ['global', 'device']: - raise UnrecognizedRequestError() - - device = None - if scope == 'device': - if len(path) == 0: - raise UnrecognizedRequestError() - device = path[0] - path = path[1:] - - if len(path) == 0: - raise UnrecognizedRequestError() - - template = path[0] - path = path[1:] - - if len(path) == 0: - raise UnrecognizedRequestError() - - rule_id = path[0] - - spec = { - 'scope': scope, - 'template': template, - 'rule_id': rule_id - } - if device: - spec['profile_tag'] = device - return spec - - def rule_tuple_from_request_object(self, rule_template, rule_id, req_obj, device=None): - if rule_template in ['override', 'underride']: - if 'conditions' not in req_obj: - raise InvalidRuleException("Missing 'conditions'") - conditions = req_obj['conditions'] - for c in conditions: - if 'kind' not in c: - raise InvalidRuleException("Condition without 'kind'") - elif rule_template == 'room': - conditions = [{ - 'kind': 'event_match', - 'key': 'room_id', - 'pattern': rule_id - }] - elif rule_template == 'sender': - conditions = [{ - 'kind': 'event_match', - 'key': 'user_id', - 'pattern': rule_id - }] - elif rule_template == 'content': - if 'pattern' not in req_obj: - raise InvalidRuleException("Content rule missing 'pattern'") - pat = req_obj['pattern'] - - conditions = [{ - 'kind': 'event_match', - 'key': 'content.body', - 'pattern': pat - }] - else: - raise InvalidRuleException("Unknown rule template: %s" % (rule_template,)) - - if device: - conditions.append({ - 'kind': 'device', - 'profile_tag': device - }) - - if 'actions' not in req_obj: - raise InvalidRuleException("No actions found") - actions = req_obj['actions'] - - for a in actions: - if a in ['notify', 'dont_notify', 'coalesce']: - pass - elif isinstance(a, dict) and 'set_sound' in a: - pass - else: - raise InvalidRuleException("Unrecognised action") - - return conditions, actions - @defer.inlineCallbacks def on_PUT(self, request): - spec = self.rule_spec_from_path(request.postpath) + spec = _rule_spec_from_path(request.postpath) try: priority_class = _priority_class_from_spec(spec) except InvalidRuleException as e: @@ -142,10 +51,13 @@ class PushRuleRestServlet(ClientV1RestServlet): if spec['template'] == 'default': raise SynapseError(403, "The default rules are immutable.") + if not spec['rule_id'].isalnum(): + raise SynapseError(400, "rule_id may only contain alphanumeric characters") + content = _parse_json(request) try: - (conditions, actions) = self.rule_tuple_from_request_object( + (conditions, actions) = _rule_tuple_from_request_object( spec['template'], spec['rule_id'], content, @@ -164,7 +76,7 @@ class PushRuleRestServlet(ClientV1RestServlet): try: yield self.hs.get_datastore().add_push_rule( user_name=user.to_string(), - rule_id=spec['rule_id'], + rule_id=_namespaced_rule_id_from_spec(spec), priority_class=priority_class, conditions=conditions, actions=actions, @@ -180,7 +92,7 @@ class PushRuleRestServlet(ClientV1RestServlet): @defer.inlineCallbacks def on_DELETE(self, request): - spec = self.rule_spec_from_path(request.postpath) + spec = _rule_spec_from_path(request.postpath) try: priority_class = _priority_class_from_spec(spec) except InvalidRuleException as e: @@ -188,32 +100,18 @@ class PushRuleRestServlet(ClientV1RestServlet): user, _ = yield self.auth.get_user_by_req(request) - if 'profile_tag' in spec: - rules = yield self.hs.get_datastore().get_push_rules_for_user_name( - user.to_string() - ) + namespaced_rule_id = _namespaced_rule_id_from_spec(spec) - for r in rules: - conditions = json.loads(r['conditions']) - pt = _profile_tag_from_conditions(conditions) - if pt == spec['profile_tag'] and r['priority_class'] == priority_class: - yield self.hs.get_datastore().delete_push_rule( - user.to_string(), spec['rule_id'] - ) - defer.returnValue((200, {})) - raise NotFoundError() - else: - try: - yield self.hs.get_datastore().delete_push_rule( - user.to_string(), spec['rule_id'], - priority_class=priority_class - ) - defer.returnValue((200, {})) - except StoreError as e: - if e.code == 404: - raise NotFoundError() - else: - raise + try: + yield self.hs.get_datastore().delete_push_rule( + user.to_string(), namespaced_rule_id + ) + defer.returnValue((200, {})) + except StoreError as e: + if e.code == 404: + raise NotFoundError() + else: + raise @defer.inlineCallbacks def on_GET(self, request): @@ -298,6 +196,99 @@ class PushRuleRestServlet(ClientV1RestServlet): return 200, {} +def _rule_spec_from_path(path): + if len(path) < 2: + raise UnrecognizedRequestError() + if path[0] != 'pushrules': + raise UnrecognizedRequestError() + + scope = path[1] + path = path[2:] + if scope not in ['global', 'device']: + raise UnrecognizedRequestError() + + device = None + if scope == 'device': + if len(path) == 0: + raise UnrecognizedRequestError() + device = path[0] + path = path[1:] + + if len(path) == 0: + raise UnrecognizedRequestError() + + template = path[0] + path = path[1:] + + if len(path) == 0: + raise UnrecognizedRequestError() + + rule_id = path[0] + + spec = { + 'scope': scope, + 'template': template, + 'rule_id': rule_id + } + if device: + spec['profile_tag'] = device + return spec + + +def _rule_tuple_from_request_object(rule_template, rule_id, req_obj, device=None): + if rule_template in ['override', 'underride']: + if 'conditions' not in req_obj: + raise InvalidRuleException("Missing 'conditions'") + conditions = req_obj['conditions'] + for c in conditions: + if 'kind' not in c: + raise InvalidRuleException("Condition without 'kind'") + elif rule_template == 'room': + conditions = [{ + 'kind': 'event_match', + 'key': 'room_id', + 'pattern': rule_id + }] + elif rule_template == 'sender': + conditions = [{ + 'kind': 'event_match', + 'key': 'user_id', + 'pattern': rule_id + }] + elif rule_template == 'content': + if 'pattern' not in req_obj: + raise InvalidRuleException("Content rule missing 'pattern'") + pat = req_obj['pattern'] + + conditions = [{ + 'kind': 'event_match', + 'key': 'content.body', + 'pattern': pat + }] + else: + raise InvalidRuleException("Unknown rule template: %s" % (rule_template,)) + + if device: + conditions.append({ + 'kind': 'device', + 'profile_tag': device + }) + + if 'actions' not in req_obj: + raise InvalidRuleException("No actions found") + actions = req_obj['actions'] + + for a in actions: + if a in ['notify', 'dont_notify', 'coalesce']: + pass + elif isinstance(a, dict) and 'set_sound' in a: + pass + else: + raise InvalidRuleException("Unrecognised action") + + return conditions, actions + + def _add_empty_priority_class_arrays(d): for pc in PushRuleRestServlet.PRIORITY_CLASS_MAP.keys(): d[pc] = [] @@ -361,20 +352,24 @@ def _priority_class_to_template_name(pc): def _rule_to_template(rule): + unscoped_rule_id = _rule_id_from_namespaced(rule['rule_id']) + template_name = _priority_class_to_template_name(rule['priority_class']) if template_name in ['default']: return {k: rule[k] for k in ["conditions", "actions"]} elif template_name in ['override', 'underride']: - return {k: rule[k] for k in ["rule_id", "conditions", "actions"]} + ret = {k: rule[k] for k in ["conditions", "actions"]} + ret['rule_id'] = unscoped_rule_id + return ret elif template_name in ["sender", "room"]: - return {k: rule[k] for k in ["rule_id", "actions"]} + return {'rule_id': unscoped_rule_id, 'actions': rule['actions']} elif template_name == 'content': if len(rule["conditions"]) != 1: return None thecond = rule["conditions"][0] if "pattern" not in thecond: return None - ret = {k: rule[k] for k in ["rule_id", "actions"]} + ret = {'rule_id': unscoped_rule_id, 'actions': rule['actions']} ret["pattern"] = thecond["pattern"] return ret @@ -386,6 +381,17 @@ def _strip_device_condition(rule): return rule +def _namespaced_rule_id_from_spec(spec): + if spec['scope'] == 'global': + scope = 'global' + else: + scope = 'device.%s' % (spec['profile_tag']) + return "%s.%s.%s" % (scope, spec['template'], spec['rule_id']) + + +def _rule_id_from_namespaced(in_rule_id, spec): + return in_rule_id.split('.')[-1] + class InvalidRuleException(Exception): pass From 2df41aa1386545f4237c0141c19db1fef85e7161 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 5 Feb 2015 14:46:06 +0000 Subject: [PATCH 2/3] Server default rules now of all kinds rather than all being at lowest prio. --- synapse/push/__init__.py | 8 ++-- synapse/push/baserules.py | 62 +++++++++++++++++++++---- synapse/push/rulekinds.py | 8 ++++ synapse/rest/client/v1/push_rule.py | 71 +++++++++++++---------------- 4 files changed, 98 insertions(+), 51 deletions(-) create mode 100644 synapse/push/rulekinds.py diff --git a/synapse/push/__init__.py b/synapse/push/__init__.py index 8c6f0a6571..7293715293 100644 --- a/synapse/push/__init__.py +++ b/synapse/push/__init__.py @@ -77,15 +77,15 @@ class Pusher(object): if ev['state_key'] != self.user_name: defer.returnValue(['dont_notify']) - rules = yield self.store.get_push_rules_for_user_name(self.user_name) + rawrules = yield self.store.get_push_rules_for_user_name(self.user_name) - for r in rules: + for r in rawrules: r['conditions'] = json.loads(r['conditions']) r['actions'] = json.loads(r['actions']) - user_name_localpart = UserID.from_string(self.user_name).localpart + user = UserID.from_string(self.user_name) - rules.extend(baserules.make_base_rules(user_name_localpart)) + rules = baserules.list_with_base_rules(rawrules, user) # get *our* member event for display name matching member_events_for_room = yield self.store.get_current_state( diff --git a/synapse/push/baserules.py b/synapse/push/baserules.py index 376d1d4d33..191909ad4d 100644 --- a/synapse/push/baserules.py +++ b/synapse/push/baserules.py @@ -1,20 +1,68 @@ -def make_base_rules(user_name): - rules = [ +from synapse.push.rulekinds import PRIORITY_CLASS_MAP, PRIORITY_CLASS_INVERSE_MAP + +def list_with_base_rules(rawrules, user_name): + ruleslist = [] + + # shove the server default rules for each kind onto the end of each + current_prio_class = 1 + for r in rawrules: + if r['priority_class'] > current_prio_class: + while current_prio_class < r['priority_class']: + ruleslist.extend(make_base_rules( + user_name, + PRIORITY_CLASS_INVERSE_MAP[current_prio_class]) + ) + current_prio_class += 1 + + ruleslist.append(r) + + while current_prio_class <= PRIORITY_CLASS_INVERSE_MAP.keys()[-1]: + ruleslist.extend(make_base_rules( + user_name, + PRIORITY_CLASS_INVERSE_MAP[current_prio_class]) + ) + current_prio_class += 1 + + return ruleslist + + +def make_base_rules(user, kind): + rules = [] + + if kind == 'override': + rules = make_base_override_rules() + elif kind == 'content': + rules = make_base_content_rules(user) + + for r in rules: + r['priority_class'] = PRIORITY_CLASS_MAP[kind] + + return rules + + +def make_base_content_rules(user): + return [ { 'conditions': [ { 'kind': 'event_match', 'key': 'content.body', - 'pattern': '*%s*' % (user_name,), # Matrix ID match + 'pattern': user.localpart, # Matrix ID match } ], 'actions': [ 'notify', { - 'set_sound': 'default' + 'set_tweak': 'sound', + 'value': 'default', } ] }, + ] + + +def make_base_override_rules(): + return [ { 'conditions': [ { @@ -24,7 +72,8 @@ def make_base_rules(user_name): 'actions': [ 'notify', { - 'set_sound': 'default' + 'set_tweak': 'sound', + 'value': 'default' } ] }, @@ -44,6 +93,3 @@ def make_base_rules(user_name): ] } ] - for r in rules: - r['priority_class'] = 0 - return rules \ No newline at end of file diff --git a/synapse/push/rulekinds.py b/synapse/push/rulekinds.py new file mode 100644 index 0000000000..763bdee58e --- /dev/null +++ b/synapse/push/rulekinds.py @@ -0,0 +1,8 @@ +PRIORITY_CLASS_MAP = { + 'underride': 1, + 'sender': 2, + 'room': 3, + 'content': 4, + 'override': 5, + } +PRIORITY_CLASS_INVERSE_MAP = {v: k for k, v in PRIORITY_CLASS_MAP.items()} diff --git a/synapse/rest/client/v1/push_rule.py b/synapse/rest/client/v1/push_rule.py index eaef55cc1e..7ab167ce03 100644 --- a/synapse/rest/client/v1/push_rule.py +++ b/synapse/rest/client/v1/push_rule.py @@ -20,21 +20,13 @@ from synapse.api.errors import SynapseError, Codes, UnrecognizedRequestError, No from .base import ClientV1RestServlet, client_path_pattern from synapse.storage.push_rule import InconsistentRuleException, RuleNotFoundException import synapse.push.baserules as baserules +from synapse.push.rulekinds import PRIORITY_CLASS_MAP, PRIORITY_CLASS_INVERSE_MAP import json class PushRuleRestServlet(ClientV1RestServlet): PATTERN = client_path_pattern("/pushrules/.*$") - PRIORITY_CLASS_MAP = { - 'default': 0, - 'underride': 1, - 'sender': 2, - 'room': 3, - 'content': 4, - 'override': 5, - } - PRIORITY_CLASS_INVERSE_MAP = {v: k for k, v in PRIORITY_CLASS_MAP.items()} SLIGHTLY_PEDANTIC_TRAILING_SLASH_ERROR = ( "Unrecognised request: You probably wanted a trailing slash") @@ -48,11 +40,8 @@ class PushRuleRestServlet(ClientV1RestServlet): user, _ = yield self.auth.get_user_by_req(request) - if spec['template'] == 'default': - raise SynapseError(403, "The default rules are immutable.") - - if not spec['rule_id'].isalnum(): - raise SynapseError(400, "rule_id may only contain alphanumeric characters") + if '/' in spec['rule_id'] or '\\' in spec['rule_id']: + raise SynapseError(400, "rule_id may not contain slashes") content = _parse_json(request) @@ -121,21 +110,23 @@ class PushRuleRestServlet(ClientV1RestServlet): # to send which means doing unnecessary work sometimes but is # is probably not going to make a whole lot of difference rawrules = yield self.hs.get_datastore().get_push_rules_for_user_name(user.to_string()) + for r in rawrules: r["conditions"] = json.loads(r["conditions"]) r["actions"] = json.loads(r["actions"]) - rawrules.extend(baserules.make_base_rules(user.to_string())) + + ruleslist = baserules.list_with_base_rules(rawrules, user) rules = {'global': {}, 'device': {}} rules['global'] = _add_empty_priority_class_arrays(rules['global']) - for r in rawrules: + for r in ruleslist: rulearray = None template_name = _priority_class_to_template_name(r['priority_class']) - if r['priority_class'] > PushRuleRestServlet.PRIORITY_CLASS_MAP['override']: + if r['priority_class'] > PRIORITY_CLASS_MAP['override']: # per-device rule profile_tag = _profile_tag_from_conditions(r["conditions"]) r = _strip_device_condition(r) @@ -290,7 +281,7 @@ def _rule_tuple_from_request_object(rule_template, rule_id, req_obj, device=None def _add_empty_priority_class_arrays(d): - for pc in PushRuleRestServlet.PRIORITY_CLASS_MAP.keys(): + for pc in PRIORITY_CLASS_MAP.keys(): d[pc] = [] return d @@ -332,46 +323,48 @@ def _filter_ruleset_with_path(ruleset, path): def _priority_class_from_spec(spec): - if spec['template'] not in PushRuleRestServlet.PRIORITY_CLASS_MAP.keys(): + if spec['template'] not in PRIORITY_CLASS_MAP.keys(): raise InvalidRuleException("Unknown template: %s" % (spec['kind'])) - pc = PushRuleRestServlet.PRIORITY_CLASS_MAP[spec['template']] + pc = PRIORITY_CLASS_MAP[spec['template']] if spec['scope'] == 'device': - pc += len(PushRuleRestServlet.PRIORITY_CLASS_MAP) + pc += len(PRIORITY_CLASS_MAP) return pc def _priority_class_to_template_name(pc): - if pc > PushRuleRestServlet.PRIORITY_CLASS_MAP['override']: + if pc > PRIORITY_CLASS_MAP['override']: # per-device prio_class_index = pc - len(PushRuleRestServlet.PRIORITY_CLASS_MAP) - return PushRuleRestServlet.PRIORITY_CLASS_INVERSE_MAP[prio_class_index] + return PRIORITY_CLASS_INVERSE_MAP[prio_class_index] else: - return PushRuleRestServlet.PRIORITY_CLASS_INVERSE_MAP[pc] + return PRIORITY_CLASS_INVERSE_MAP[pc] def _rule_to_template(rule): - unscoped_rule_id = _rule_id_from_namespaced(rule['rule_id']) + unscoped_rule_id = None + if 'rule_id' in rule: + _rule_id_from_namespaced(rule['rule_id']) template_name = _priority_class_to_template_name(rule['priority_class']) - if template_name in ['default']: - return {k: rule[k] for k in ["conditions", "actions"]} - elif template_name in ['override', 'underride']: - ret = {k: rule[k] for k in ["conditions", "actions"]} - ret['rule_id'] = unscoped_rule_id - return ret + if template_name in ['override', 'underride']: + templaterule = {k: rule[k] for k in ["conditions", "actions"]} elif template_name in ["sender", "room"]: - return {'rule_id': unscoped_rule_id, 'actions': rule['actions']} + templaterule = {'actions': rule['actions']} + unscoped_rule_id = rule['conditions'][0]['pattern'] elif template_name == 'content': if len(rule["conditions"]) != 1: return None thecond = rule["conditions"][0] if "pattern" not in thecond: return None - ret = {'rule_id': unscoped_rule_id, 'actions': rule['actions']} - ret["pattern"] = thecond["pattern"] - return ret + templaterule = {'actions': rule['actions']} + templaterule["pattern"] = thecond["pattern"] + + if unscoped_rule_id: + templaterule['rule_id'] = unscoped_rule_id + return templaterule def _strip_device_condition(rule): @@ -385,12 +378,12 @@ def _namespaced_rule_id_from_spec(spec): if spec['scope'] == 'global': scope = 'global' else: - scope = 'device.%s' % (spec['profile_tag']) - return "%s.%s.%s" % (scope, spec['template'], spec['rule_id']) + scope = 'device/%s' % (spec['profile_tag']) + return "%s/%s/%s" % (scope, spec['template'], spec['rule_id']) -def _rule_id_from_namespaced(in_rule_id, spec): - return in_rule_id.split('.')[-1] +def _rule_id_from_namespaced(in_rule_id): + return in_rule_id.split('/')[-1] class InvalidRuleException(Exception): pass From aaf50bf6f3d6adee92fa4d5cb55dbf3c5a13dbe3 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 5 Feb 2015 15:11:38 +0000 Subject: [PATCH 3/3] Give server default rules the 'default' attribute and fix various brokenness. --- synapse/push/baserules.py | 1 + synapse/rest/client/v1/push_rule.py | 4 +++- synapse/storage/push_rule.py | 7 +++++-- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/synapse/push/baserules.py b/synapse/push/baserules.py index 191909ad4d..8d4b806da6 100644 --- a/synapse/push/baserules.py +++ b/synapse/push/baserules.py @@ -36,6 +36,7 @@ def make_base_rules(user, kind): for r in rules: r['priority_class'] = PRIORITY_CLASS_MAP[kind] + r['default'] = True return rules diff --git a/synapse/rest/client/v1/push_rule.py b/synapse/rest/client/v1/push_rule.py index 7ab167ce03..80f116b1ed 100644 --- a/synapse/rest/client/v1/push_rule.py +++ b/synapse/rest/client/v1/push_rule.py @@ -345,7 +345,7 @@ def _priority_class_to_template_name(pc): def _rule_to_template(rule): unscoped_rule_id = None if 'rule_id' in rule: - _rule_id_from_namespaced(rule['rule_id']) + unscoped_rule_id = _rule_id_from_namespaced(rule['rule_id']) template_name = _priority_class_to_template_name(rule['priority_class']) if template_name in ['override', 'underride']: @@ -364,6 +364,8 @@ def _rule_to_template(rule): if unscoped_rule_id: templaterule['rule_id'] = unscoped_rule_id + if 'default' in rule: + templaterule['default'] = rule['default'] return templaterule diff --git a/synapse/storage/push_rule.py b/synapse/storage/push_rule.py index 27502d2399..30e23445d9 100644 --- a/synapse/storage/push_rule.py +++ b/synapse/storage/push_rule.py @@ -176,7 +176,7 @@ class PushRuleStore(SQLBaseStore): txn.execute(sql, new_rule.values()) @defer.inlineCallbacks - def delete_push_rule(self, user_name, rule_id, **kwargs): + def delete_push_rule(self, user_name, rule_id): """ Delete a push rule. Args specify the row to be deleted and can be any of the columns in the push_rule table, but below are the @@ -186,7 +186,10 @@ class PushRuleStore(SQLBaseStore): user_name (str): The matrix ID of the push rule owner rule_id (str): The rule_id of the rule to be deleted """ - yield self._simple_delete_one(PushRuleTable.table_name, kwargs) + yield self._simple_delete_one( + PushRuleTable.table_name, + {'user_name': user_name, 'rule_id': rule_id} + ) class RuleNotFoundException(Exception):