Merge commit '453dfe210' into anoa/dinsic_release_1_21_x

* commit '453dfe210':
  blacklist MSC2753 sytests until it's implemented in synapse (#8285)
  Don't remember `enabled` of deleted push rules and properly return 404 for missing push rules in `.../actions` and `.../enabled` (#7796)

changelog.d/7796.bugfix

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

changelog.d/8285.misc

@@ -0,0 +1 @@
+Blacklist [MSC2753](https://github.com/matrix-org/matrix-doc/pull/2753) SyTests until it is implemented.

synapse/rest/client/v1/push_rule.py

@@ -163,6 +163,18 @@ def notify_user(self, user_id):
         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 @@ async 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 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")

synapse/rest/client/v2_alpha/account.py

@@ -362,20 +362,19 @@ async def on_POST(self, request):
             shadow_user = UserID(
                 requester.user.localpart, self.hs.config.shadow_server.get("hs")
             )
-            self.shadow_password(params, shadow_user.to_string())
+            await self.shadow_password(params, shadow_user.to_string())
 
         return 200, {}
 
     def on_OPTIONS(self, _):
         return 200, {}
 
-    @defer.inlineCallbacks
-    def shadow_password(self, body, user_id):
+    async def shadow_password(self, body, user_id):
         # TODO: retries
         shadow_hs_url = self.hs.config.shadow_server.get("hs_url")
         as_token = self.hs.config.shadow_server.get("as_token")
 
-        yield self.http_client.post_json_get_json(
+        await self.http_client.post_json_get_json(
             "%s/_matrix/client/r0/account/password?access_token=%s&user_id=%s"
             % (shadow_hs_url, as_token, user_id),
             body,
@@ -756,7 +755,7 @@ async def on_POST(self, request):
                 shadow_user = UserID(
                     requester.user.localpart, self.hs.config.shadow_server.get("hs")
                 )
-                self.shadow_3pid({"threepid": threepid}, shadow_user.to_string())
+                await self.shadow_3pid({"threepid": threepid}, shadow_user.to_string())
 
             return 200, {}
 
@@ -791,21 +790,20 @@ async def on_POST(self, request):
                     "address": validation_session["address"],
                     "validated_at": validation_session["validated_at"],
                 }
-                self.shadow_3pid({"threepid": threepid}, shadow_user.to_string())
+                await self.shadow_3pid({"threepid": threepid}, shadow_user.to_string())
 
             return 200, {}
 
         raise SynapseError(
             400, "No validated 3pid session found", Codes.THREEPID_AUTH_FAILED
         )
 
-    @defer.inlineCallbacks
-    def shadow_3pid(self, body, user_id):
+    async def shadow_3pid(self, body, user_id):
         # TODO: retries
         shadow_hs_url = self.hs.config.shadow_server.get("hs_url")
         as_token = self.hs.config.shadow_server.get("as_token")
 
-        yield self.http_client.post_json_get_json(
+        await self.http_client.post_json_get_json(
             "%s/_matrix/client/r0/account/3pid?access_token=%s&user_id=%s"
             % (shadow_hs_url, as_token, user_id),
             body,
@@ -866,20 +864,19 @@ async def on_POST(self, request):
                     "address": validation_session["address"],
                     "validated_at": validation_session["validated_at"],
                 }
-                self.shadow_3pid({"threepid": threepid}, shadow_user.to_string())
+                await self.shadow_3pid({"threepid": threepid}, shadow_user.to_string())
             return 200, {}
 
         raise SynapseError(
             400, "No validated 3pid session found", Codes.THREEPID_AUTH_FAILED
         )
 
-    @defer.inlineCallbacks
-    def shadow_3pid(self, body, user_id):
+    async def shadow_3pid(self, body, user_id):
         # TODO: retries
         shadow_hs_url = self.hs.config.shadow_server.get("hs_url")
         as_token = self.hs.config.shadow_server.get("as_token")
 
-        yield self.http_client.post_json_get_json(
+        await self.http_client.post_json_get_json(
             "%s/_matrix/client/r0/account/3pid?access_token=%s&user_id=%s"
             % (shadow_hs_url, as_token, user_id),
             body,
@@ -983,7 +980,7 @@ async def on_POST(self, request):
             shadow_user = UserID(
                 requester.user.localpart, self.hs.config.shadow_server.get("hs")
             )
-            self.shadow_3pid_delete(body, shadow_user.to_string())
+            await self.shadow_3pid_delete(body, shadow_user.to_string())
 
         if ret:
             id_server_unbind_result = "success"
@@ -992,13 +989,12 @@ async def on_POST(self, request):
 
         return 200, {"id_server_unbind_result": id_server_unbind_result}
 
-    @defer.inlineCallbacks
-    def shadow_3pid_delete(self, body, user_id):
+    async def shadow_3pid_delete(self, body, user_id):
         # TODO: retries
         shadow_hs_url = self.hs.config.shadow_server.get("hs_url")
         as_token = self.hs.config.shadow_server.get("as_token")
 
-        yield self.http_client.post_json_get_json(
+        await self.http_client.post_json_get_json(
             "%s/_matrix/client/r0/account/3pid/delete?access_token=%s&user_id=%s"
             % (shadow_hs_url, as_token, user_id),
             body,
@@ -1101,45 +1097,6 @@ def assert_valid_next_link(hs: "HomeServer", next_link: str):
         )
 
 
-def assert_valid_next_link(hs: "HomeServer", next_link: str):
-    """
-    Raises a SynapseError if a given next_link value is invalid
-
-    next_link is valid if the scheme is http(s) and the next_link.domain_whitelist config
-    option is either empty or contains a domain that matches the one in the given next_link
-
-    Args:
-        hs: The homeserver object
-        next_link: The next_link value given by the client
-
-    Raises:
-        SynapseError: If the next_link is invalid
-    """
-    valid = True
-
-    # Parse the contents of the URL
-    next_link_parsed = urlparse(next_link)
-
-    # Scheme must not point to the local drive
-    if next_link_parsed.scheme == "file":
-        valid = False
-
-    # If the domain whitelist is set, the domain must be in it
-    if (
-        valid
-        and hs.config.next_link_domain_whitelist is not None
-        and next_link_parsed.hostname not in hs.config.next_link_domain_whitelist
-    ):
-        valid = False
-
-    if not valid:
-        raise SynapseError(
-            400,
-            "'next_link' domain not included in whitelist, or not http(s)",
-            errcode=Codes.INVALID_PARAM,
-        )
-
-
 class WhoamiRestServlet(RestServlet):
     PATTERNS = client_patterns("/account/whoami$")
 

synapse/storage/databases/main/push_rule.py

@@ -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.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 @@ def _upsert_push_rule_txn(
                 },
             )
 
+        # 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 @@ async def delete_push_rule(self, user_id: str, rule_id: str) -> None:
         """
 
         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 @@ def delete_push_rule_txn(txn, stream_id, event_stream_ordering):
                 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 @@ async def set_push_rule_enabled(self, user_id, rule_id, enabled) -> None:
                 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 @@ def _set_push_rule_enabled_txn(
         )
 
     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 @@ def set_push_rule_actions_txn(txn, stream_id, event_stream_ordering):
                     update_stream=False,
                 )
             else:
-                self.db_pool.simple_update_one_txn(
-                    txn,
-                    "push_rules",
-                    {"user_name": user_id, "rule_id": rule_id},
-                    {"actions": actions_json},
-                )
+                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,