diff options
| -rw-r--r-- | changelog.d/9297.feature | 1 | ||||
| -rw-r--r-- | changelog.d/9302.bugfix | 1 | ||||
| -rw-r--r-- | changelog.d/9310.doc | 1 | ||||
| -rw-r--r-- | docs/sample_config.yaml | 14 | ||||
| -rw-r--r-- | synapse/config/sso.py | 14 | ||||
| -rw-r--r-- | synapse/handlers/federation.py | 4 | ||||
| -rw-r--r-- | synapse/handlers/room_member.py | 12 | ||||
| -rw-r--r-- | synapse/res/templates/sso_auth_account_details.html | 27 | ||||
| -rw-r--r-- | synapse/res/templates/sso_auth_account_details.js | 78 | ||||
| -rw-r--r-- | tests/handlers/test_federation.py | 47 |
10 files changed, 107 insertions, 92 deletions
diff --git a/changelog.d/9297.feature b/changelog.d/9297.feature new file mode 100644 index 0000000000..a2d0b27da4 --- /dev/null +++ b/changelog.d/9297.feature @@ -0,0 +1 @@ +Further improvements to the user experience of registration via single sign-on. diff --git a/changelog.d/9302.bugfix b/changelog.d/9302.bugfix new file mode 100644 index 0000000000..c1cdea52a3 --- /dev/null +++ b/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. diff --git a/changelog.d/9310.doc b/changelog.d/9310.doc new file mode 100644 index 0000000000..f61705b73a --- /dev/null +++ b/changelog.d/9310.doc @@ -0,0 +1 @@ +Clarify the sample configuration for changes made to the template loading code. diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 6d265d2972..236abd9a3f 100644 --- a/docs/sample_config.yaml +++ b/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 - # https://jinja.palletsprojects.com/en/2.11.x/templates/#html-escaping). + # login. # # * 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 - # manual escaping (see - # https://jinja.palletsprojects.com/en/2.11.x/templates/#html-escaping). + # * redirect_url: the URL the user is about to be redirected to. # # * 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 - # (see https://jinja.palletsprojects.com/en/2.11.x/templates/#html-escaping). + # the final address to redirect to. # # * 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 - # manual escaping (see - # https://jinja.palletsprojects.com/en/2.11.x/templates/#html-escaping). + # * redirect_url: the URL the user is about to be redirected to. # # * description: the operation which the user is being asked to confirm # diff --git a/synapse/config/sso.py b/synapse/config/sso.py index 939eeac6de..6c60c6fea4 100644 --- a/synapse/config/sso.py +++ b/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 - # https://jinja.palletsprojects.com/en/2.11.x/templates/#html-escaping). + # login. # # * 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 - # manual escaping (see - # https://jinja.palletsprojects.com/en/2.11.x/templates/#html-escaping). + # * redirect_url: the URL the user is about to be redirected to. # # * 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 - # (see https://jinja.palletsprojects.com/en/2.11.x/templates/#html-escaping). + # the final address to redirect to. # # * 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 - # manual escaping (see - # https://jinja.palletsprojects.com/en/2.11.x/templates/#html-escaping). + # * redirect_url: the URL the user is about to be redirected to. # # * description: the operation which the user is being asked to confirm # diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index dbdfd56ff5..eddc7582d0 100644 --- a/synapse/handlers/federation.py +++ b/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 diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index d335da6f19..a5da97cfe0 100644 --- a/synapse/handlers/room_member.py +++ b/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: diff --git a/synapse/res/templates/sso_auth_account_details.html b/synapse/res/templates/sso_auth_account_details.html index 7ad58ad214..f4fdc40b22 100644 --- a/synapse/res/templates/sso_auth_account_details.html +++ b/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"> diff --git a/synapse/res/templates/sso_auth_account_details.js b/synapse/res/templates/sso_auth_account_details.js index deef419bb6..3c45df9078 100644 --- a/synapse/res/templates/sso_auth_account_details.js +++ b/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(""); - if (usernameField.validity.valueMissing) { - usernameField.setCustomValidity("Please provide a username"); - return; + isValid = false; + needsValidation = false; + usernameOutput.innerText = ""; + usernameField.parentElement.classList.remove("invalid"); + usernameOutput.classList.remove("error"); + if (!username) { + return reportError("Please provide a username"); } - if (usernameField.validity.patternMismatch) { - usernameField.setCustomValidity("Invalid username, please only use " + allowedCharactersString); - return; + if (username.length > 255) { + return reportError("Too long, please choose something shorter"); } - 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(""); - console.log(err.message); + usernameOutput.innerText = ""; + isValid = true; }; try { checkUsernameAvailable(username).then(function(result) { if (!result.available) { - usernameField.setCustomValidity(result.message); - usernameField.reportValidity(); + reportError(result.message); } 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); + } }); diff --git a/tests/handlers/test_federation.py b/tests/handlers/test_federation.py index 74503112f5..983e368592 100644 --- a/tests/handlers/test_federation.py +++ b/tests/handlers/test_federation.py @@ -192,53 +192,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}}} ) def test_invite_by_user_ratelimit(self): |