Merge remote-tracking branch 'origin/release-v1.27.0' into social_login_hotfixes

changelog.d/9297.feature

@@ -0,0 +1 @@
+Further improvements to the user experience of registration via single sign-on.

changelog.d/9302.bugfix

@@ -0,0 +1 @@
+Fix new ratelimiting for invites to respect the `ratelimit` flag on application services. Introduced in v1.27.0rc1.

changelog.d/9310.doc

@@ -0,0 +1 @@
+Clarify the sample configuration for changes made to the template loading code.

docs/sample_config.yaml

@@ -1961,8 +1961,7 @@ sso:
     #
     #   When rendering, this template is given the following variables:
     #     * redirect_url: the URL that the user will be redirected to after
-    #       login. Needs manual escaping (see
+    #       login.
-    #       https://jinja.palletsprojects.com/en/2.11.x/templates/#html-escaping).
     #
     #     * server_name: the homeserver's name.
     #
@@ -2040,15 +2039,12 @@ sso:
     #
     #   When rendering, this template is given the following variables:
     #
-    #     * redirect_url: the URL the user is about to be redirected to. Needs
+    #     * redirect_url: the URL the user is about to be redirected to.
-    #                     manual escaping (see
-    #                     https://jinja.palletsprojects.com/en/2.11.x/templates/#html-escaping).
     #
     #     * display_url: the same as `redirect_url`, but with the query
     #                    parameters stripped. The intention is to have a
     #                    human-readable URL to show to users, not to use it as
-    #                    the final address to redirect to. Needs manual escaping
+    #                    the final address to redirect to.
-    #                    (see https://jinja.palletsprojects.com/en/2.11.x/templates/#html-escaping).
     #
     #     * server_name: the homeserver's name.
     #
@@ -2068,9 +2064,7 @@ sso:
     #   process: 'sso_auth_confirm.html'.
     #
     #   When rendering, this template is given the following variables:
-    #     * redirect_url: the URL the user is about to be redirected to. Needs
+    #     * redirect_url: the URL the user is about to be redirected to.
-    #                     manual escaping (see
-    #                     https://jinja.palletsprojects.com/en/2.11.x/templates/#html-escaping).
     #
     #     * description: the operation which the user is being asked to confirm
     #

synapse/config/sso.py

@@ -106,8 +106,7 @@ class SSOConfig(Config):
             #
             #   When rendering, this template is given the following variables:
             #     * redirect_url: the URL that the user will be redirected to after
-            #       login. Needs manual escaping (see
+            #       login.
-            #       https://jinja.palletsprojects.com/en/2.11.x/templates/#html-escaping).
             #
             #     * server_name: the homeserver's name.
             #
@@ -185,15 +184,12 @@ class SSOConfig(Config):
             #
             #   When rendering, this template is given the following variables:
             #
-            #     * redirect_url: the URL the user is about to be redirected to. Needs
+            #     * redirect_url: the URL the user is about to be redirected to.
-            #                     manual escaping (see
-            #                     https://jinja.palletsprojects.com/en/2.11.x/templates/#html-escaping).
             #
             #     * display_url: the same as `redirect_url`, but with the query
             #                    parameters stripped. The intention is to have a
             #                    human-readable URL to show to users, not to use it as
-            #                    the final address to redirect to. Needs manual escaping
+            #                    the final address to redirect to.
-            #                    (see https://jinja.palletsprojects.com/en/2.11.x/templates/#html-escaping).
             #
             #     * server_name: the homeserver's name.
             #
@@ -213,9 +209,7 @@ class SSOConfig(Config):
             #   process: 'sso_auth_confirm.html'.
             #
             #   When rendering, this template is given the following variables:
-            #     * redirect_url: the URL the user is about to be redirected to. Needs
+            #     * redirect_url: the URL the user is about to be redirected to.
-            #                     manual escaping (see
-            #                     https://jinja.palletsprojects.com/en/2.11.x/templates/#html-escaping).
             #
             #     * description: the operation which the user is being asked to confirm
             #

synapse/handlers/federation.py

@@ -1619,7 +1619,9 @@ class FederationHandler(BaseHandler):
 
         # We retrieve the room member handler here as to not cause a cyclic dependency
         member_handler = self.hs.get_room_member_handler()
-        member_handler.ratelimit_invite(event.room_id, event.state_key)
+        # We don't rate limit based on room ID, as that should be done by
+        # sending server.
+        member_handler.ratelimit_invite(None, event.state_key)
 
         # keep a record of the room version, if we don't yet know it.
         # (this may get overwritten if we later get a different room version in a

synapse/handlers/room_member.py

@@ -155,10 +155,14 @@ class RoomMemberHandler(metaclass=abc.ABCMeta):
         """
         raise NotImplementedError()
 
-    def ratelimit_invite(self, room_id: str, invitee_user_id: str):
+    def ratelimit_invite(self, room_id: Optional[str], invitee_user_id: str):
         """Ratelimit invites by room and by target user.
+
+        If room ID is missing then we just rate limit by target user.
         """
-        self._invites_per_room_limiter.ratelimit(room_id)
+        if room_id:
+            self._invites_per_room_limiter.ratelimit(room_id)
+
         self._invites_per_user_limiter.ratelimit(invitee_user_id)
 
     async def _local_membership_update(
@@ -406,7 +410,9 @@ class RoomMemberHandler(metaclass=abc.ABCMeta):
         if effective_membership_state == Membership.INVITE:
             target_id = target.to_string()
             if ratelimit:
-                self.ratelimit_invite(room_id, target_id)
+                # Don't ratelimit application services.
+                if not requester.app_service or requester.app_service.is_rate_limited():
+                    self.ratelimit_invite(room_id, target_id)
 
             # block any attempts to invite the server notices mxid
             if target_id == self._server_notices_mxid:

synapse/res/templates/sso_auth_account_details.html

@@ -35,6 +35,19 @@
         font-size: 12px;
       }
 
+      .username_input.invalid {
+        border-color: #FE2928;
+      }
+
+      .username_input.invalid input, .username_input.invalid label {
+        color: #FE2928;
+      }
+
+      .username_input div, .username_input input {
+        line-height: 18px;
+        font-size: 14px;
+      }
+
       .username_input label {
         position: absolute;
         top: -5px;
@@ -104,6 +117,15 @@
         display: block;
         margin-top: 8px;
       }
+
+      output {
+        padding: 0 14px;
+        display: block;
+      }
+
+      output.error {
+        color: #FE2928;
+      }
     </style>
   </head>
   <body>
@@ -113,12 +135,13 @@
     </header>
     <main>
       <form method="post" class="form__input" id="form">
-        <div class="username_input">
+        <div class="username_input" id="username_input">
           <label for="field-username">Username</label>
           <div class="prefix">@</div>
-          <input type="text" name="username" id="field-username" autofocus required pattern="[a-z0-9\-=_\/\.]+">
+          <input type="text" name="username" id="field-username" autofocus>
           <div class="postfix">:{{ server_name }}</div>
         </div>
+        <output for="username_input" id="field-username-output"></output>
         <input type="submit" value="Continue" class="primary-button">
         {% if user_attributes.avatar_url or user_attributes.display_name or user_attributes.emails %}
         <section class="idp-pick-details">

synapse/res/templates/sso_auth_account_details.js

@@ -1,14 +1,24 @@
 const usernameField = document.getElementById("field-username");
+const usernameOutput = document.getElementById("field-username-output");
+const form = document.getElementById("form");
+
+// needed to validate on change event when no input was changed
+let needsValidation = true;
+let isValid = false;
 
 function throttle(fn, wait) {
     let timeout;
-    return function() {
+    const throttleFn = function() {
         const args = Array.from(arguments);
         if (timeout) {
             clearTimeout(timeout);
         }
         timeout = setTimeout(fn.bind.apply(fn, [null].concat(args)), wait);
-    }
+    };
+    throttleFn.cancelQueued = function() {
+        clearTimeout(timeout);
+    };
+    return throttleFn;
 }
 
 function checkUsernameAvailable(username) {
@@ -16,14 +26,14 @@ function checkUsernameAvailable(username) {
     return fetch(check_uri, {
         // include the cookie
         "credentials": "same-origin",
-    }).then((response) => {
+    }).then(function(response) {
         if(!response.ok) {
             // for non-200 responses, raise the body of the response as an exception
             return response.text().then((text) => { throw new Error(text); });
         } else {
             return response.json();
         }
-    }).then((json) => {
+    }).then(function(json) {
         if(json.error) {
             return {message: json.error};
         } else if(json.available) {
@@ -34,33 +44,49 @@ function checkUsernameAvailable(username) {
     });
 }
 
+const allowedUsernameCharacters = new RegExp("^[a-z0-9\\.\\_\\-\\/\\=]+$");
+const allowedCharactersString = "lowercase letters, digits, ., _, -, /, =";
+
+function reportError(error) {
+    throttledCheckUsernameAvailable.cancelQueued();
+    usernameOutput.innerText = error;
+    usernameOutput.classList.add("error");
+    usernameField.parentElement.classList.add("invalid");
+    usernameField.focus();
+}
+
 function validateUsername(username) {
-    usernameField.setCustomValidity("");
+    isValid = false;
-    if (usernameField.validity.valueMissing) {
+    needsValidation = false;
-        usernameField.setCustomValidity("Please provide a username");
+    usernameOutput.innerText = "";
-        return;
+    usernameField.parentElement.classList.remove("invalid");
+    usernameOutput.classList.remove("error");
+    if (!username) {
+        return reportError("Please provide a username");
     }
-    if (usernameField.validity.patternMismatch) {
+    if (username.length > 255) {
-        usernameField.setCustomValidity("Invalid username, please only use " + allowedCharactersString);
+        return reportError("Too long, please choose something shorter");
-        return;
     }
-    usernameField.setCustomValidity("Checking if username is available …");
+    if (!allowedUsernameCharacters.test(username)) {
+        return reportError("Invalid username, please only use " + allowedCharactersString);
+    }
+    usernameOutput.innerText = "Checking if username is available …";
     throttledCheckUsernameAvailable(username);
 }
 
 const throttledCheckUsernameAvailable = throttle(function(username) {
-    const handleError =  function(err) {
+    const handleError = function(err) {
         // don't prevent form submission on error
-        usernameField.setCustomValidity("");
+        usernameOutput.innerText = "";
-        console.log(err.message);
+        isValid = true;
     };
     try {
         checkUsernameAvailable(username).then(function(result) {
             if (!result.available) {
-                usernameField.setCustomValidity(result.message);
+                reportError(result.message);
-                usernameField.reportValidity();
             } else {
-                usernameField.setCustomValidity("");
+                isValid = true;
+                usernameOutput.innerText = "";
             }
         }, handleError);
     } catch (err) {
@@ -68,9 +94,23 @@ const throttledCheckUsernameAvailable = throttle(function(username) {
     }
 }, 500);
 
+form.addEventListener("submit", function(evt) {
+    if (needsValidation) {
+        validateUsername(usernameField.value);
+        evt.preventDefault();
+        return;
+    }
+    if (!isValid) {
+        evt.preventDefault();
+        usernameField.focus();
+        return;
+    }
+});
 usernameField.addEventListener("input", function(evt) {
     validateUsername(usernameField.value);
 });
 usernameField.addEventListener("change", function(evt) {
-    validateUsername(usernameField.value);
+    if (needsValidation) {
+        validateUsername(usernameField.value);
+    }
 });

tests/handlers/test_federation.py

@@ -191,53 +191,6 @@ class FederationTestCase(unittest.HomeserverTestCase):
 
         self.assertEqual(sg, sg2)
 
-    @unittest.override_config(
-        {"rc_invites": {"per_room": {"per_second": 0.5, "burst_count": 3}}}
-    )
-    def test_invite_by_room_ratelimit(self):
-        """Tests that invites from federation in a room are actually rate-limited.
-        """
-        other_server = "otherserver"
-        other_user = "@otheruser:" + other_server
-
-        # create the room
-        user_id = self.register_user("kermit", "test")
-        tok = self.login("kermit", "test")
-        room_id = self.helper.create_room_as(room_creator=user_id, tok=tok)
-        room_version = self.get_success(self.store.get_room_version(room_id))
-
-        def create_invite_for(local_user):
-            return event_from_pdu_json(
-                {
-                    "type": EventTypes.Member,
-                    "content": {"membership": "invite"},
-                    "room_id": room_id,
-                    "sender": other_user,
-                    "state_key": local_user,
-                    "depth": 32,
-                    "prev_events": [],
-                    "auth_events": [],
-                    "origin_server_ts": self.clock.time_msec(),
-                },
-                room_version,
-            )
-
-        for i in range(3):
-            self.get_success(
-                self.handler.on_invite_request(
-                    other_server,
-                    create_invite_for("@user-%d:test" % (i,)),
-                    room_version,
-                )
-            )
-
-        self.get_failure(
-            self.handler.on_invite_request(
-                other_server, create_invite_for("@user-4:test"), room_version,
-            ),
-            exc=LimitExceededError,
-        )
-
     @unittest.override_config(
         {"rc_invites": {"per_user": {"per_second": 0.5, "burst_count": 3}}}
     )