diff --git a/changelog.d/9296.bugfix b/changelog.d/9296.bugfix new file mode 100644 index 000000000..d723f8c5b --- /dev/null +++ b/changelog.d/9296.bugfix @@ -0,0 +1 @@ +Fix bug in Synapse 1.27.0rc1 which meant the "session expired" error page during SSO registration was badly formatted. diff --git a/changelog.d/9297.feature b/changelog.d/9297.feature new file mode 100644 index 000000000..a2d0b27da --- /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/9300.feature b/changelog.d/9300.feature new file mode 100644 index 000000000..a2d0b27da --- /dev/null +++ b/changelog.d/9300.feature @@ -0,0 +1 @@ +Further improvements to the user experience of registration via single sign-on. diff --git a/changelog.d/9301.feature b/changelog.d/9301.feature new file mode 100644 index 000000000..a2d0b27da --- /dev/null +++ b/changelog.d/9301.feature @@ -0,0 +1 @@ +Further improvements to the user experience of registration via single sign-on. diff --git a/changelog.d/9307.misc b/changelog.d/9307.misc new file mode 100644 index 000000000..2f54d1ad0 --- /dev/null +++ b/changelog.d/9307.misc @@ -0,0 +1 @@ +Improve logging for OIDC login flow. diff --git a/changelog.d/9310.doc b/changelog.d/9310.doc new file mode 100644 index 000000000..f61705b73 --- /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 601a8338e..9dbba63ba 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1971,8 +1971,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. # @@ -2050,15 +2049,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. # @@ -2078,9 +2074,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 939eeac6d..6c60c6fea 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/auth.py b/synapse/handlers/auth.py index a19c55643..648fe91f5 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -1472,10 +1472,22 @@ class AuthHandler(BaseHandler): # Remove the query parameters from the redirect URL to get a shorter version of # it. This is only to display a human-readable URL in the template, but not the # URL we redirect users to. - redirect_url_no_params = client_redirect_url.split("?")[0] + url_parts = urllib.parse.urlsplit(client_redirect_url) + + if url_parts.scheme == "https": + # for an https uri, just show the netloc (ie, the hostname. Specifically, + # the bit between "//" and "/"; this includes any potential + # "username:password@" prefix.) + display_url = url_parts.netloc + else: + # for other uris, strip the query-params (including the login token) and + # fragment. + display_url = urllib.parse.urlunsplit( + (url_parts.scheme, url_parts.netloc, url_parts.path, "", "") + ) html = self._sso_redirect_confirm_template.render( - display_url=redirect_url_no_params, + display_url=display_url, redirect_url=redirect_url, server_name=self._server_name, new_user=new_user, diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index 71008ec50..3adc75fa4 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -123,7 +123,6 @@ class OidcHandler: Args: request: the incoming request from the browser. """ - # The provider might redirect with an error. # In that case, just display it as-is. if b"error" in request.args: @@ -137,8 +136,12 @@ class OidcHandler: # either the provider misbehaving or Synapse being misconfigured. # The only exception of that is "access_denied", where the user # probably cancelled the login flow. In other cases, log those errors. - if error != "access_denied": - logger.error("Error from the OIDC provider: %s %s", error, description) + logger.log( + logging.INFO if error == "access_denied" else logging.ERROR, + "Received OIDC callback with error: %s %s", + error, + description, + ) self._sso_handler.render_error(request, error, description) return @@ -149,7 +152,7 @@ class OidcHandler: # Fetch the session cookie session = request.getCookie(SESSION_COOKIE_NAME) # type: Optional[bytes] if session is None: - logger.info("No session cookie found") + logger.info("Received OIDC callback, with no session cookie") self._sso_handler.render_error( request, "missing_session", "No session cookie found" ) @@ -169,7 +172,7 @@ class OidcHandler: # Check for the state query parameter if b"state" not in request.args: - logger.info("State parameter is missing") + logger.info("Received OIDC callback, with no state parameter") self._sso_handler.render_error( request, "invalid_request", "State parameter is missing" ) @@ -183,14 +186,16 @@ class OidcHandler: session, state ) except (MacaroonDeserializationException, ValueError) as e: - logger.exception("Invalid session") + logger.exception("Invalid session for OIDC callback") self._sso_handler.render_error(request, "invalid_session", str(e)) return except MacaroonInvalidSignatureException as e: - logger.exception("Could not verify session") + logger.exception("Could not verify session for OIDC callback") self._sso_handler.render_error(request, "mismatching_session", str(e)) return + logger.info("Received OIDC callback for IdP %s", session_data.idp_id) + oidc_provider = self._providers.get(session_data.idp_id) if not oidc_provider: logger.error("OIDC session uses unknown IdP %r", oidc_provider) @@ -565,6 +570,7 @@ class OidcProvider: Returns: UserInfo: an object representing the user. """ + logger.debug("Using the OAuth2 access_token to request userinfo") metadata = await self.load_metadata() resp = await self._http_client.get_json( @@ -572,6 +578,8 @@ class OidcProvider: headers={"Authorization": ["Bearer {}".format(token["access_token"])]}, ) + logger.debug("Retrieved user info from userinfo endpoint: %r", resp) + return UserInfo(resp) async def _parse_id_token(self, token: Token, nonce: str) -> UserInfo: @@ -600,17 +608,19 @@ class OidcProvider: claims_cls = ImplicitIDToken alg_values = metadata.get("id_token_signing_alg_values_supported", ["RS256"]) - jwt = JsonWebToken(alg_values) claim_options = {"iss": {"values": [metadata["issuer"]]}} + id_token = token["id_token"] + logger.debug("Attempting to decode JWT id_token %r", id_token) + # Try to decode the keys in cache first, then retry by forcing the keys # to be reloaded jwk_set = await self.load_jwks() try: claims = jwt.decode( - token["id_token"], + id_token, key=jwk_set, claims_cls=claims_cls, claims_options=claim_options, @@ -620,13 +630,15 @@ class OidcProvider: logger.info("Reloading JWKS after decode error") jwk_set = await self.load_jwks(force=True) # try reloading the jwks claims = jwt.decode( - token["id_token"], + id_token, key=jwk_set, claims_cls=claims_cls, claims_options=claim_options, claims_params=claims_params, ) + logger.debug("Decoded id_token JWT %r; validating", claims) + claims.validate(leeway=120) # allows 2 min of clock skew return UserInfo(claims) @@ -726,19 +738,18 @@ class OidcProvider: """ # Exchange the code with the provider try: - logger.debug("Exchanging code") + logger.debug("Exchanging OAuth2 code for a token") token = await self._exchange_code(code) except OidcError as e: - logger.exception("Could not exchange code") + logger.exception("Could not exchange OAuth2 code") self._sso_handler.render_error(request, e.error, e.error_description) return - logger.debug("Successfully obtained OAuth2 access token") + logger.debug("Successfully obtained OAuth2 token data: %r", token) # Now that we have a token, get the userinfo, either by decoding the # `id_token` or by fetching the `userinfo_endpoint`. if self._uses_userinfo: - logger.debug("Fetching userinfo") try: userinfo = await self._fetch_userinfo(token) except Exception as e: @@ -746,7 +757,6 @@ class OidcProvider: self._sso_handler.render_error(request, "fetch_error", str(e)) return else: - logger.debug("Extracting userinfo from id_token") try: userinfo = await self._parse_id_token(token, nonce=session_data.nonce) except Exception as e: diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index b450668f1..96ccd991e 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -742,7 +742,11 @@ class SsoHandler: use_display_name: whether the user wants to use the suggested display name emails_to_use: emails that the user would like to use """ - session = self.get_mapping_session(session_id) + try: + session = self.get_mapping_session(session_id) + except SynapseError as e: + self.render_error(request, "bad_session", e.msg, code=e.code) + return # update the session with the user's choices session.chosen_localpart = localpart @@ -793,7 +797,12 @@ class SsoHandler: session_id, terms_version, ) - session = self.get_mapping_session(session_id) + try: + session = self.get_mapping_session(session_id) + except SynapseError as e: + self.render_error(request, "bad_session", e.msg, code=e.code) + return + session.terms_accepted_version = terms_version # we're done; now we can register the user @@ -808,7 +817,11 @@ class SsoHandler: request: HTTP request session_id: ID of the username mapping session, extracted from a cookie """ - session = self.get_mapping_session(session_id) + try: + session = self.get_mapping_session(session_id) + except SynapseError as e: + self.render_error(request, "bad_session", e.msg, code=e.code) + return logger.info( "[session %s] Registering localpart %s", diff --git a/synapse/res/templates/sso.css b/synapse/res/templates/sso.css index 46b309ea4..338214f5d 100644 --- a/synapse/res/templates/sso.css +++ b/synapse/res/templates/sso.css @@ -1,16 +1,26 @@ -body { +body, input, select, textarea { font-family: "Inter", "Helvetica", "Arial", sans-serif; font-size: 14px; color: #17191C; } -header { +header, footer { max-width: 480px; width: 100%; margin: 24px auto; text-align: center; } +@media screen and (min-width: 800px) { + header { + margin-top: 90px; + } +} + +header { + min-height: 60px; +} + header p { color: #737D8C; line-height: 24px; @@ -20,6 +30,10 @@ h1 { font-size: 24px; } +a { + color: #418DED; +} + .error_page h1 { color: #FE2928; } @@ -47,6 +61,9 @@ main { .primary-button { border: none; + -webkit-appearance: none; + -moz-appearance: none; + appearance: none; text-decoration: none; padding: 12px; color: white; @@ -63,8 +80,17 @@ main { .profile { display: flex; + flex-direction: column; + align-items: center; justify-content: center; - margin: 24px 0; + margin: 24px; + padding: 13px; + border: 1px solid #E9ECF1; + border-radius: 4px; +} + +.profile.with-avatar { + margin-top: 42px; /* (36px / 2) + 24px*/ } .profile .avatar { @@ -72,17 +98,32 @@ main { height: 36px; border-radius: 100%; display: block; - margin-right: 8px; + margin-top: -32px; + margin-bottom: 8px; } .profile .display-name { font-weight: bold; margin-bottom: 4px; + font-size: 15px; + line-height: 18px; } .profile .user-id { color: #737D8C; + font-size: 12px; + line-height: 12px; } -.profile .display-name, .profile .user-id { - line-height: 18px; +footer { + margin-top: 80px; } + +footer svg { + display: block; + width: 46px; + margin: 0px auto 12px auto; +} + +footer p { + color: #737D8C; +} \ No newline at end of file diff --git a/synapse/res/templates/sso_account_deactivated.html b/synapse/res/templates/sso_account_deactivated.html index 50a0979c2..c3e4deed9 100644 --- a/synapse/res/templates/sso_account_deactivated.html +++ b/synapse/res/templates/sso_account_deactivated.html @@ -20,5 +20,6 @@ administrator.

+ {% include "sso_footer.html" without context %} diff --git a/synapse/res/templates/sso_auth_account_details.html b/synapse/res/templates/sso_auth_account_details.html index 105063825..f4fdc40b2 100644 --- a/synapse/res/templates/sso_auth_account_details.html +++ b/synapse/res/templates/sso_auth_account_details.html @@ -1,12 +1,29 @@ - Synapse Login + Create your account + @@ -87,50 +135,52 @@
-
+
@
- +
:{{ server_name }}
+ - {% if user_attributes %} + {% if user_attributes.avatar_url or user_attributes.display_name or user_attributes.emails %}

Information from {{ idp.idp_name }}

{% if user_attributes.avatar_url %} -
+
+ {% endif %} {% if user_attributes.display_name %} -
+
+ {% endif %} {% for email in user_attributes.emails %} -
+
+ {% endfor %}
{% endif %}
+ {% include "sso_footer.html" without context %} diff --git a/synapse/res/templates/sso_auth_account_details.js b/synapse/res/templates/sso_auth_account_details.js index deef419bb..3c45df907 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/synapse/res/templates/sso_auth_bad_user.html b/synapse/res/templates/sso_auth_bad_user.html index c9bd4bef2..da579ffe6 100644 --- a/synapse/res/templates/sso_auth_bad_user.html +++ b/synapse/res/templates/sso_auth_bad_user.html @@ -21,5 +21,6 @@ the Identity Provider as when you log into your account.

+ {% include "sso_footer.html" without context %} diff --git a/synapse/res/templates/sso_auth_confirm.html b/synapse/res/templates/sso_auth_confirm.html index 2099c2f1f..f9d0456f0 100644 --- a/synapse/res/templates/sso_auth_confirm.html +++ b/synapse/res/templates/sso_auth_confirm.html @@ -2,7 +2,7 @@ - Authentication + Confirm it's you -
-

{{ server_name }} Login

-
-

Choose one of the following identity providers:

-
- -
-
+ {% endif %} + {{ p.idp_name }} + + + {% endfor %} + + + {% include "sso_footer.html" without context %} diff --git a/synapse/res/templates/sso_new_user_consent.html b/synapse/res/templates/sso_new_user_consent.html index 8c33787c5..68c8b9f33 100644 --- a/synapse/res/templates/sso_new_user_consent.html +++ b/synapse/res/templates/sso_new_user_consent.html @@ -2,7 +2,7 @@ - SSO redirect confirmation + Agree to terms and conditions
- {% if new_user %} -

Your account is now ready

-

You've made your account on {{ server_name }}.

- {% else %} -

Log in

- {% endif %} -

Continue to confirm you trust {{ display_url }}.

+

Continue to your account

- {% if user_profile.avatar_url %} -
- -
- {% if user_profile.display_name %} -
{{ user_profile.display_name }}
- {% endif %} -
{{ user_id }}
-
-
- {% endif %} + {% include "sso_partial_profile.html" %} +

Continuing will grant {{ display_url }} access to your account.

Continue
+ {% include "sso_footer.html" without context %} diff --git a/tests/rest/client/v1/test_login.py b/tests/rest/client/v1/test_login.py index 66dfdaffb..ceb4ad236 100644 --- a/tests/rest/client/v1/test_login.py +++ b/tests/rest/client/v1/test_login.py @@ -15,7 +15,7 @@ import time import urllib.parse -from typing import Any, Dict, Union +from typing import Any, Dict, List, Union from urllib.parse import urlencode from mock import Mock @@ -493,13 +493,21 @@ class MultiSSOTestCase(unittest.HomeserverTestCase): self.assertEqual(channel.code, 200, channel.result) # parse the form to check it has fields assumed elsewhere in this class + html = channel.result["body"].decode("utf-8") p = TestHtmlParser() - p.feed(channel.result["body"].decode("utf-8")) + p.feed(html) p.close() - self.assertCountEqual(p.radios["idp"], ["cas", "oidc", "oidc-idp1", "saml"]) + # there should be a link for each href + returned_idps = [] # type: List[str] + for link in p.links: + path, query = link.split("?", 1) + self.assertEqual(path, "pick_idp") + params = urllib.parse.parse_qs(query) + self.assertEqual(params["redirectUrl"], [TEST_CLIENT_REDIRECT_URL]) + returned_idps.append(params["idp"][0]) - self.assertEqual(p.hiddens["redirectUrl"], TEST_CLIENT_REDIRECT_URL) + self.assertCountEqual(returned_idps, ["cas", "oidc", "oidc-idp1", "saml"]) def test_multi_sso_redirect_to_cas(self): """If CAS is chosen, should redirect to the CAS server"""