Add tests that justify the postcontent section

Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>

changelog.d/18846.feature

@@ -0,0 +1 @@
+Update push rules for experimental [MSC4306: Thread Subscriptions](https://github.com/matrix-org/matrix-doc/issues/4306) to follow newer draft.

rust/src/push/base_rules.rs

@@ -289,10 +289,10 @@ pub const BASE_APPEND_CONTENT_RULES: &[PushRule] = &[PushRule {
     default_enabled: true,
 }];
 
-pub const BASE_APPEND_UNDERRIDE_RULES: &[PushRule] = &[
+pub const BASE_APPEND_POSTCONTENT_RULES: &[PushRule] = &[
     PushRule {
-        rule_id: Cow::Borrowed("global/content/.io.element.msc4306.rule.unsubscribed_thread"),
-        priority_class: 1,
+        rule_id: Cow::Borrowed("global/postcontent/.io.element.msc4306.rule.unsubscribed_thread"),
+        priority_class: 6,
         conditions: Cow::Borrowed(&[Condition::Known(
             KnownCondition::Msc4306ThreadSubscription { subscribed: false },
         )]),
@@ -301,8 +301,8 @@ pub const BASE_APPEND_UNDERRIDE_RULES: &[PushRule] = &[
         default_enabled: true,
     },
     PushRule {
-        rule_id: Cow::Borrowed("global/content/.io.element.msc4306.rule.subscribed_thread"),
-        priority_class: 1,
+        rule_id: Cow::Borrowed("global/postcontent/.io.element.msc4306.rule.subscribed_thread"),
+        priority_class: 6,
         conditions: Cow::Borrowed(&[Condition::Known(
             KnownCondition::Msc4306ThreadSubscription { subscribed: true },
         )]),
@@ -310,6 +310,9 @@ pub const BASE_APPEND_UNDERRIDE_RULES: &[PushRule] = &[
         default: true,
         default_enabled: true,
     },
+];
+
+pub const BASE_APPEND_UNDERRIDE_RULES: &[PushRule] = &[
     PushRule {
         rule_id: Cow::Borrowed("global/underride/.m.rule.call"),
         priority_class: 1,
@@ -726,6 +729,7 @@ lazy_static! {
             .iter()
             .chain(BASE_APPEND_OVERRIDE_RULES.iter())
             .chain(BASE_APPEND_CONTENT_RULES.iter())
+            .chain(BASE_APPEND_POSTCONTENT_RULES.iter())
             .chain(BASE_APPEND_UNDERRIDE_RULES.iter())
             .map(|rule| { (&*rule.rule_id, rule) })
             .collect();

rust/src/push/mod.rs

@@ -527,6 +527,7 @@ impl PushRules {
             .chain(base_rules::BASE_APPEND_OVERRIDE_RULES.iter())
             .chain(self.content.iter())
             .chain(base_rules::BASE_APPEND_CONTENT_RULES.iter())
+            .chain(base_rules::BASE_APPEND_POSTCONTENT_RULES.iter())
             .chain(self.room.iter())
             .chain(self.sender.iter())
             .chain(self.underride.iter())

synapse/push/clientformat.py

@@ -91,7 +91,7 @@ def _rule_to_template(rule: PushRule) -> Optional[Dict[str, Any]]:
     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"]:
+    if template_name in ["override", "underride", "postcontent"]:
         templaterule = {"conditions": rule.conditions, "actions": rule.actions}
     elif template_name in ["sender", "room"]:
         templaterule = {"actions": rule.actions}

synapse/push/rulekinds.py

@@ -19,10 +19,14 @@
 #
 #
 
+# Integer literals for push rule `kind`s
+# This is used to store them in the database.
 PRIORITY_CLASS_MAP = {
     "underride": 1,
     "sender": 2,
     "room": 3,
+    # MSC4306
+    "postcontent": 6,
     "content": 4,
     "override": 5,
 }

synapse/rest/client/push_rule.py

@@ -19,9 +19,11 @@
 #
 #
 
+from http import HTTPStatus
 from typing import TYPE_CHECKING, List, Tuple, Union
 
 from synapse.api.errors import (
+    Codes,
     NotFoundError,
     StoreError,
     SynapseError,
@@ -239,6 +241,15 @@ def _rule_spec_from_path(path: List[str]) -> RuleSpec:
 def _rule_tuple_from_request_object(
     rule_template: str, rule_id: str, req_obj: JsonDict
 ) -> Tuple[List[JsonDict], List[Union[str, JsonDict]]]:
+    if rule_template == "postcontent":
+        # postcontent is from MSC4306, which says that clients
+        # cannot create their own postcontent rules right now.
+        raise SynapseError(
+            HTTPStatus.BAD_REQUEST,
+            "user-defined rules using `postcontent` are not accepted",
+            errcode=Codes.INVALID_PARAM,
+        )
+
     if rule_template in ["override", "underride"]:
         if "conditions" not in req_obj:
             raise InvalidRuleException("Missing 'conditions'")

tests/push/test_bulk_push_rule_evaluator.py

@@ -19,6 +19,7 @@
 #
 #
 
+from http import HTTPStatus
 from typing import Any, Optional
 from unittest.mock import AsyncMock, patch
 
@@ -30,7 +31,7 @@ from synapse.api.constants import EventContentFields, EventTypes, RelationTypes
 from synapse.api.room_versions import RoomVersions
 from synapse.push.bulk_push_rule_evaluator import BulkPushRuleEvaluator
 from synapse.rest import admin
-from synapse.rest.client import login, register, room
+from synapse.rest.client import login, push_rule, register, room
 from synapse.server import HomeServer
 from synapse.types import JsonDict, create_requester
 from synapse.util import Clock
@@ -44,6 +45,7 @@ class TestBulkPushRuleEvaluator(HomeserverTestCase):
         room.register_servlets,
         login.register_servlets,
         register.register_servlets,
+        push_rule.register_servlets,
     ]
 
     def prepare(
@@ -494,6 +496,135 @@ class TestBulkPushRuleEvaluator(HomeserverTestCase):
             )
         )
 
+    @override_config({"experimental_features": {"msc4306_enabled": True}})
+    def test_thread_subscriptions_suppression_after_keyword_mention_overrides(
+        self,
+    ) -> None:
+        """
+        Tests one of the purposes of the `postcontent` push rule section:
+        When a keyword mention is configured (in the `content` section),
+        it does not get suppressed by the thread being unsubscribed.
+        """
+        # add a keyword mention to alice's push rules
+        channel = self.make_request(
+            "PUT",
+            "/_matrix/client/v3/pushrules/global/content/biscuits",
+            {"pattern": "biscuits", "actions": ["notify"]},
+            access_token=self.token,
+        )
+        self.assertEqual(channel.code, HTTPStatus.OK)
+
+        bulk_evaluator = BulkPushRuleEvaluator(self.hs)
+        (thread_root_id,) = self.helper.send_messages(self.room_id, 1, tok=self.token)
+
+        self.assertFalse(
+            self._create_and_process(
+                bulk_evaluator,
+                {
+                    "msgtype": "m.text",
+                    "body": "do you want some cookies?",
+                    "m.relates_to": {
+                        "rel_type": RelationTypes.THREAD,
+                        "event_id": thread_root_id,
+                    },
+                },
+                type="m.room.message",
+            ),
+            "alice is not subscribed to thread and does not have a mention on 'cookies' so should not be notified",
+        )
+
+        self.assertTrue(
+            self._create_and_process(
+                bulk_evaluator,
+                {
+                    "msgtype": "m.text",
+                    "body": "biscuits are available in the kitchen",
+                    "m.relates_to": {
+                        "rel_type": RelationTypes.THREAD,
+                        "event_id": thread_root_id,
+                    },
+                },
+                type="m.room.message",
+            ),
+            "alice is not subscribed to thread but DOES have a mention on 'biscuits' so should be notified",
+        )
+
+    @override_config({"experimental_features": {"msc4306_enabled": True}})
+    def test_thread_subscriptions_notification_before_keywords_and_mentions(
+        self,
+    ) -> None:
+        """
+        Tests one of the purposes of the `postcontent` push rule section:
+        When a room is set to (what is commonly known as) 'keywords & mentions', we still receive notifications
+        for messages in threads that we are subscribed to.
+        Effectively making this 'keywords, mentions & subscriptions'
+        """
+        # add a 'keywords & mentions' setting to the room alice's push rules
+        # In case this rule isn't clear: by adding a rule in the `room` section that does nothing,
+        # it stops execution of the push rules before we fall through to the `underride` section,
+        # where intuitively many kinds of messages will ambiently generate notifications.
+        # Mentions and keywords are triggered before the `room` block, so this doesn't suppress those.
+        channel = self.make_request(
+            "PUT",
+            f"/_matrix/client/v3/pushrules/global/room/{self.room_id}",
+            {"actions": []},
+            access_token=self.token,
+        )
+        self.assertEqual(channel.code, HTTPStatus.OK)
+
+        bulk_evaluator = BulkPushRuleEvaluator(self.hs)
+        (thread_root_id,) = self.helper.send_messages(self.room_id, 1, tok=self.token)
+
+        # sanity check that our mentions still work
+        self.assertFalse(
+            self._create_and_process(
+                bulk_evaluator,
+                {
+                    "msgtype": "m.text",
+                    "body": "this is a plain message with no mention",
+                },
+                type="m.room.message",
+            ),
+            "alice should not be notified (mentions & keywords room setting)",
+        )
+        self.assertTrue(
+            self._create_and_process(
+                bulk_evaluator,
+                {
+                    "msgtype": "m.text",
+                    "body": "this is a message that mentions alice",
+                },
+                type="m.room.message",
+            ),
+            "alice should be notified (mentioned)",
+        )
+
+        # let's have alice subscribe to the thread
+        self.get_success(
+            self.hs.get_datastores().main.subscribe_user_to_thread(
+                self.alice,
+                self.room_id,
+                thread_root_id,
+                automatic_event_orderings=None,
+            )
+        )
+
+        self.assertTrue(
+            self._create_and_process(
+                bulk_evaluator,
+                {
+                    "msgtype": "m.text",
+                    "body": "some message in the thread",
+                    "m.relates_to": {
+                        "rel_type": RelationTypes.THREAD,
+                        "event_id": thread_root_id,
+                    },
+                },
+                type="m.room.message",
+            ),
+            "alice is subscribed to thread so should be notified",
+        )
+
     def test_with_disabled_thread_subscriptions(self) -> None:
         """
         Test what happens with threaded events when MSC4306 is disabled.

tests/rest/client/test_push_rule_attrs.py

@@ -18,6 +18,8 @@
 # [This file includes modifications made by New Vector Limited]
 #
 #
+from http import HTTPStatus
+
 import synapse
 from synapse.api.errors import Codes
 from synapse.rest.client import login, push_rule, room
@@ -486,3 +488,23 @@ class PushRuleAttributesTestCase(HomeserverTestCase):
             },
             channel.json_body,
         )
+
+    def test_no_user_defined_postcontent_rules(self) -> None:
+        """
+        Tests that clients are not permitted to create MSC4306 `postcontent` rules.
+        """
+        self.register_user("bob", "pass")
+        token = self.login("bob", "pass")
+
+        channel = self.make_request(
+            "PUT",
+            "/pushrules/global/postcontent/some.user.rule",
+            {},
+            access_token=token,
+        )
+
+        self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST)
+        self.assertEqual(
+            Codes.INVALID_PARAM,
+            channel.json_body["errcode"],
+        )