From 57feeab364325374b14ff67ac97c288983cc5cde Mon Sep 17 00:00:00 2001
From: reivilibre <38398653+reivilibre@users.noreply.github.com>
Date: Mon, 6 Jul 2020 11:43:41 +0100
Subject: [PATCH] Don't ignore `set_tweak` actions with no explicit `value`.
 (#7766)

* Fix spec compliance; tweaks without values are valid

(default to True, which is only concretely specified for
`highlight`, but it seems only reasonable to generalise)

* Changelog for 7766.

* Add documentation to `tweaks_for_actions`

May as well tidy up when I'm here.

* Add a test for `tweaks_for_actions`
---
 changelog.d/7766.bugfix                |  1 +
 synapse/push/push_rule_evaluator.py    | 31 ++++++++++++++++++++++----
 tests/push/test_push_rule_evaluator.py | 17 ++++++++++++++
 3 files changed, 45 insertions(+), 4 deletions(-)
 create mode 100644 changelog.d/7766.bugfix

diff --git a/changelog.d/7766.bugfix b/changelog.d/7766.bugfix
new file mode 100644
index 0000000000..ec5ecd8055
--- /dev/null
+++ b/changelog.d/7766.bugfix
@@ -0,0 +1 @@
+Fix to not ignore `set_tweak` actions in Push Rules that have no `value`, as permitted by the specification.
diff --git a/synapse/push/push_rule_evaluator.py b/synapse/push/push_rule_evaluator.py
index 8e0d3a416d..2d79ada189 100644
--- a/synapse/push/push_rule_evaluator.py
+++ b/synapse/push/push_rule_evaluator.py
@@ -16,7 +16,7 @@
 
 import logging
 import re
-from typing import Pattern
+from typing import Any, Dict, List, Pattern, Union
 
 from synapse.events import EventBase
 from synapse.types import UserID
@@ -72,13 +72,36 @@ def _test_ineq_condition(condition, number):
         return False
 
 
-def tweaks_for_actions(actions):
+def tweaks_for_actions(actions: List[Union[str, Dict]]) -> Dict[str, Any]:
+    """
+    Converts a list of actions into a `tweaks` dict (which can then be passed to
+        the push gateway).
+
+    This function ignores all actions other than `set_tweak` actions, and treats
+    absent `value`s as `True`, which agrees with the only spec-defined treatment
+    of absent `value`s (namely, for `highlight` tweaks).
+
+    Args:
+        actions: list of actions
+            e.g. [
+                {"set_tweak": "a", "value": "AAA"},
+                {"set_tweak": "b", "value": "BBB"},
+                {"set_tweak": "highlight"},
+                "notify"
+            ]
+
+    Returns:
+        dictionary of tweaks for those actions
+            e.g. {"a": "AAA", "b": "BBB", "highlight": True}
+    """
     tweaks = {}
     for a in actions:
         if not isinstance(a, dict):
             continue
-        if "set_tweak" in a and "value" in a:
-            tweaks[a["set_tweak"]] = a["value"]
+        if "set_tweak" in a:
+            # value is allowed to be absent in which case the value assumed
+            # should be True.
+            tweaks[a["set_tweak"]] = a.get("value", True)
     return tweaks
 
 
diff --git a/tests/push/test_push_rule_evaluator.py b/tests/push/test_push_rule_evaluator.py
index af35d23aea..1f4b5ca2ac 100644
--- a/tests/push/test_push_rule_evaluator.py
+++ b/tests/push/test_push_rule_evaluator.py
@@ -15,6 +15,7 @@
 
 from synapse.api.room_versions import RoomVersions
 from synapse.events import FrozenEvent
+from synapse.push import push_rule_evaluator
 from synapse.push.push_rule_evaluator import PushRuleEvaluatorForEvent
 
 from tests import unittest
@@ -84,3 +85,19 @@ class PushRuleEvaluatorTestCase(unittest.TestCase):
         for body in (1, True, {"foo": "bar"}):
             evaluator = self._get_evaluator({"body": body})
             self.assertFalse(evaluator.matches(condition, "@user:test", "foo"))
+
+    def test_tweaks_for_actions(self):
+        """
+        This tests the behaviour of tweaks_for_actions.
+        """
+
+        actions = [
+            {"set_tweak": "sound", "value": "default"},
+            {"set_tweak": "highlight"},
+            "notify",
+        ]
+
+        self.assertEqual(
+            push_rule_evaluator.tweaks_for_actions(actions),
+            {"sound": "default", "highlight": True},
+        )