Repository: matrix-org/synapse

Issue:
Transfer notification settings when joining upgraded rooms

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/matrix-org/synapse/pull/4838?src=pr&el=h1) Report
> Merging [#4838](https://codecov.io/gh/matrix-org/synapse/pull/4838?src=pr&el=desc) into [develop](https://codecov.io/gh/matrix-org/synapse/commit/39e57f97286fd0db108656ddf3bfdc9bae91945f?src=pr&el=desc) will **decrease** coverage by `7.62%`.
> The diff coverage is `40%`.

```diff
@@             Coverage Diff             @@
##           develop    #4838      +/-   ##
===========================================
- Coverage    75.34%   67.72%   -7.63%     
===========================================
  Files          340      340              
  Lines        34920    34940      +20     
  Branches      5717     5725       +8     
===========================================
- Hits         26312    23664    -2648     
- Misses        6999     9619    +2620     
- Partials      1609     1657      +48
```

codecov[bot]: # [Codecov](https://codecov.io/gh/matrix-org/synapse/pull/4838?src=pr&el=h1) Report
> Merging [#4838](https://codecov.io/gh/matrix-org/synapse/pull/4838?src=pr&el=desc) into [develop](https://codecov.io/gh/matrix-org/synapse/commit/39e57f97286fd0db108656ddf3bfdc9bae91945f?src=pr&el=desc) will **decrease** coverage by `7.62%`.
> The diff coverage is `40%`.

```diff
@@             Coverage Diff             @@
##           develop    #4838      +/-   ##
===========================================
- Coverage    75.34%   67.72%   -7.63%     
===========================================
  Files          340      340              
  Lines        34920    34940      +20     
  Branches      5717     5725       +8     
===========================================
- Hits         26312    23664    -2648     
- Misses        6999     9619    +2620     
- Partials      1609     1657      +48
```


Patch:
diff --git a/INSTALL.md b/INSTALL.md
index 2993f3a9e..5c67f14ed 100644
--- a/INSTALL.md
+++ b/INSTALL.md
@@ -402,8 +402,8 @@ This process uses a setting `registration_shared_secret` in
 `homeserver.yaml`, which is shared between Synapse itself and the
 `register_new_matrix_user` script. It doesn't matter what it is (a random
 value is generated by `--generate-config`), but it should be kept secret, as
-anyone with knowledge of it can register users on your server even if
-`enable_registration` is `false`.
+anyone with knowledge of it can register users, including admin accounts,
+on your server even if `enable_registration` is `false`.
 
 ## Setting up a TURN server
 
diff --git a/changelog.d/4793.feature b/changelog.d/4793.feature
new file mode 100644
index 000000000..90dba7d12
--- /dev/null
+++ b/changelog.d/4793.feature
@@ -0,0 +1,1 @@
+Synapse is now permissive about trailing slashes on some of its federation endpoints, allowing zero or more to be present.
\ No newline at end of file
diff --git a/changelog.d/4838.bugfix b/changelog.d/4838.bugfix
new file mode 100644
index 000000000..7f4fceabf
--- /dev/null
+++ b/changelog.d/4838.bugfix
@@ -0,0 +1,1 @@
+Transfer a user's notification settings (push rules) on room upgrade.
\ No newline at end of file
diff --git a/changelog.d/4844.misc b/changelog.d/4844.misc
new file mode 100644
index 000000000..eff6f1c43
--- /dev/null
+++ b/changelog.d/4844.misc
@@ -0,0 +1,1 @@
+Clarify what registration_shared_secret allows for.
diff --git a/changelog.d/4847.misc b/changelog.d/4847.misc
new file mode 100644
index 000000000..a001238e0
--- /dev/null
+++ b/changelog.d/4847.misc
@@ -0,0 +1,1 @@
+Correctly log expected errors when fetching server keys.
diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml
index b62745dd6..22d5e6b1d 100644
--- a/docs/sample_config.yaml
+++ b/docs/sample_config.yaml
@@ -624,8 +624,8 @@ enable_registration: False
 #  - medium: msisdn
 #    pattern: '\+44'
 
-# If set, allows registration by anyone who also has the shared
-# secret, even if registration is otherwise disabled.
+# If set, allows registration of standard or admin accounts by anyone who
+# has the shared secret, even if registration is otherwise disabled.
 #
 # registration_shared_secret: <PRIVATE STRING>
 
diff --git a/synapse/config/registration.py b/synapse/config/registration.py
index d34dc9e45..a123f25a6 100644
--- a/synapse/config/registration.py
+++ b/synapse/config/registration.py
@@ -92,8 +92,8 @@ class RegistrationConfig(Config):
         #  - medium: msisdn
         #    pattern: '\\+44'
 
-        # If set, allows registration by anyone who also has the shared
-        # secret, even if registration is otherwise disabled.
+        # If set, allows registration of standard or admin accounts by anyone who
+        # has the shared secret, even if registration is otherwise disabled.
         #
         %(registration_shared_secret)s
 
diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py
index 7474fd515..0207cd989 100644
--- a/synapse/crypto/keyring.py
+++ b/synapse/crypto/keyring.py
@@ -686,9 +686,9 @@ def _handle_key_deferred(verify_request):
     try:
         with PreserveLoggingContext():
             _, key_id, verify_key = yield verify_request.deferred
-    except (IOError, RequestSendFailed) as e:
+    except KeyLookupError as e:
         logger.warn(
-            "Got IOError when downloading keys for %s: %s %s",
+            "Failed to download keys for %s: %s %s",
             server_name, type(e).__name__, str(e),
         )
         raise SynapseError(
diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py
index 8e2be218e..4e8919d65 100644
--- a/synapse/federation/transport/client.py
+++ b/synapse/federation/transport/client.py
@@ -167,7 +167,7 @@ class TransportLayerClient(object):
         # generated by the json_data_callback.
         json_data = transaction.get_dict()
 
-        path = _create_v1_path("/send/%s/", transaction.transaction_id)
+        path = _create_v1_path("/send/%s", transaction.transaction_id)
 
         response = yield self.client.put_json(
             transaction.destination,
diff --git a/synapse/federation/transport/server.py b/synapse/federation/transport/server.py
index 96d680a5a..efb6bdca4 100644
--- a/synapse/federation/transport/server.py
+++ b/synapse/federation/transport/server.py
@@ -312,7 +312,7 @@ class BaseFederationServlet(object):
 
 
 class FederationSendServlet(BaseFederationServlet):
-    PATH = "/send/(?P<transaction_id>[^/]*)/"
+    PATH = "/send/(?P<transaction_id>[^/]*)/?"
 
     def __init__(self, handler, server_name, **kwargs):
         super(FederationSendServlet, self).__init__(
@@ -378,7 +378,7 @@ class FederationSendServlet(BaseFederationServlet):
 
 
 class FederationEventServlet(BaseFederationServlet):
-    PATH = "/event/(?P<event_id>[^/]*)/"
+    PATH = "/event/(?P<event_id>[^/]*)/?"
 
     # This is when someone asks for a data item for a given server data_id pair.
     def on_GET(self, origin, content, query, event_id):
@@ -386,7 +386,7 @@ class FederationEventServlet(BaseFederationServlet):
 
 
 class FederationStateServlet(BaseFederationServlet):
-    PATH = "/state/(?P<context>[^/]*)/"
+    PATH = "/state/(?P<context>[^/]*)/?"
 
     # This is when someone asks for all data for a given context.
     def on_GET(self, origin, content, query, context):
@@ -398,7 +398,7 @@ class FederationStateServlet(BaseFederationServlet):
 
 
 class FederationStateIdsServlet(BaseFederationServlet):
-    PATH = "/state_ids/(?P<room_id>[^/]*)/"
+    PATH = "/state_ids/(?P<room_id>[^/]*)/?"
 
     def on_GET(self, origin, content, query, room_id):
         return self.handler.on_state_ids_request(
@@ -409,7 +409,7 @@ class FederationStateIdsServlet(BaseFederationServlet):
 
 
 class FederationBackfillServlet(BaseFederationServlet):
-    PATH = "/backfill/(?P<context>[^/]*)/"
+    PATH = "/backfill/(?P<context>[^/]*)/?"
 
     def on_GET(self, origin, content, query, context):
         versions = [x.decode('ascii') for x in query[b"v"]]
@@ -1080,7 +1080,7 @@ class FederationGroupsCategoriesServlet(BaseFederationServlet):
     """Get all categories for a group
     """
     PATH = (
-        "/groups/(?P<group_id>[^/]*)/categories/"
+        "/groups/(?P<group_id>[^/]*)/categories/?"
     )
 
     @defer.inlineCallbacks
@@ -1150,7 +1150,7 @@ class FederationGroupsRolesServlet(BaseFederationServlet):
     """Get roles in a group
     """
     PATH = (
-        "/groups/(?P<group_id>[^/]*)/roles/"
+        "/groups/(?P<group_id>[^/]*)/roles/?"
     )
 
     @defer.inlineCallbacks
diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py
index 190ea2c7b..aead9e460 100644
--- a/synapse/handlers/room_member.py
+++ b/synapse/handlers/room_member.py
@@ -232,6 +232,10 @@ class RoomMemberHandler(object):
                 self.copy_room_tags_and_direct_to_room(
                     predecessor["room_id"], room_id, user_id,
                 )
+                # Move over old push rules
+                self.store.move_push_rules_from_room_to_room_for_user(
+                    predecessor["room_id"], room_id, user_id,
+                )
         elif event.membership == Membership.LEAVE:
             if prev_member_event_id:
                 prev_member_event = yield self.store.get_event(prev_member_event_id)
diff --git a/synapse/storage/push_rule.py b/synapse/storage/push_rule.py
index 6a5028961..4b8438c3e 100644
--- a/synapse/storage/push_rule.py
+++ b/synapse/storage/push_rule.py
@@ -185,6 +185,63 @@ class PushRulesWorkerStore(ApplicationServiceWorkerStore,
 
         defer.returnValue(results)
 
+    @defer.inlineCallbacks
+    def move_push_rule_from_room_to_room(
+        self, new_room_id, user_id, rule,
+    ):
+        """Move a single push rule from one room to another for a specific user.
+
+        Args:
+            new_room_id (str): ID of the new room.
+            user_id (str): ID of user the push rule belongs to.
+            rule (Dict): A push rule.
+        """
+        # Create new rule id
+        rule_id_scope = '/'.join(rule["rule_id"].split('/')[:-1])
+        new_rule_id = rule_id_scope + "/" + new_room_id
+
+        # Change room id in each condition
+        for condition in rule.get("conditions", []):
+            if condition.get("key") == "room_id":
+                condition["pattern"] = new_room_id
+
+        # Add the rule for the new room
+        yield self.add_push_rule(
+            user_id=user_id,
+            rule_id=new_rule_id,
+            priority_class=rule["priority_class"],
+            conditions=rule["conditions"],
+            actions=rule["actions"],
+        )
+
+        # Delete push rule for the old room
+        yield self.delete_push_rule(user_id, rule["rule_id"])
+
+    @defer.inlineCallbacks
+    def move_push_rules_from_room_to_room_for_user(
+        self, old_room_id, new_room_id, user_id,
+    ):
+        """Move all of the push rules from one room to another for a specific
+        user.
+
+        Args:
+            old_room_id (str): ID of the old room.
+            new_room_id (str): ID of the new room.
+            user_id (str): ID of user to copy push rules for.
+        """
+        # Retrieve push rules for this user
+        user_push_rules = yield self.get_push_rules_for_user(user_id)
+
+        # Get rules relating to the old room, move them to the new room, then
+        # delete them from the old room
+        for rule in user_push_rules:
+            conditions = rule.get("conditions", [])
+            if any((c.get("key") == "room_id" and
+                    c.get("pattern") == old_room_id) for c in conditions):
+                self.move_push_rule_from_room_to_room(
+                    new_room_id, user_id, rule,
+                )
+
     @defer.inlineCallbacks
     def bulk_get_push_rules_for_room(self, event, context):
         state_group = context.state_group


Test patch:
diff --git a/tests/handlers/test_typing.py b/tests/handlers/test_typing.py
index 13486930f..b8e97390d 100644
--- a/tests/handlers/test_typing.py
+++ b/tests/handlers/test_typing.py
@@ -180,7 +180,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase):
         put_json = self.hs.get_http_client().put_json
         put_json.assert_called_once_with(
             "farm",
-            path="/_matrix/federation/v1/send/1000000/",
+            path="/_matrix/federation/v1/send/1000000",
             data=_expect_edu_transaction(
                 "m.typing",
                 content={
@@ -201,7 +201,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase):
 
         (request, channel) = self.make_request(
             "PUT",
-            "/_matrix/federation/v1/send/1000000/",
+            "/_matrix/federation/v1/send/1000000",
             _make_edu_transaction_json(
                 "m.typing",
                 content={
@@ -257,7 +257,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase):
         put_json = self.hs.get_http_client().put_json
         put_json.assert_called_once_with(
             "farm",
-            path="/_matrix/federation/v1/send/1000000/",
+            path="/_matrix/federation/v1/send/1000000",
             data=_expect_edu_transaction(
                 "m.typing",
                 content={