From 8aed29dc615bee75019fc526a5c91cdc2638b665 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Mon, 1 Feb 2021 15:50:56 +0000 Subject: [PATCH] Improve styling and wording of SSO redirect confirm template (#9272) --- changelog.d/9272.feature | 1 + docs/sample_config.yaml | 14 +++- synapse/config/sso.py | 14 +++- synapse/handlers/auth.py | 24 +++++- synapse/handlers/sso.py | 10 ++- synapse/module_api/__init__.py | 10 ++- synapse/res/templates/sso.css | 83 +++++++++++++++++++ .../res/templates/sso_redirect_confirm.html | 34 ++++++-- tests/handlers/test_cas.py | 8 +- tests/handlers/test_oidc.py | 24 +++--- tests/handlers/test_saml.py | 8 +- 11 files changed, 200 insertions(+), 30 deletions(-) create mode 100644 changelog.d/9272.feature create mode 100644 synapse/res/templates/sso.css diff --git a/changelog.d/9272.feature b/changelog.d/9272.feature new file mode 100644 index 0000000000..c21b197ca1 --- /dev/null +++ b/changelog.d/9272.feature @@ -0,0 +1 @@ +Improve the user experience of setting up an account via single-sign on. diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 8777e3254d..05506a7787 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1971,7 +1971,8 @@ sso: # * HTML page for a confirmation step before redirecting back to the client # with the login token: 'sso_redirect_confirm.html'. # - # When rendering, this template is given three variables: + # 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). @@ -1984,6 +1985,17 @@ sso: # # * server_name: the homeserver's name. # + # * new_user: a boolean indicating whether this is the user's first time + # logging in. + # + # * user_id: the user's matrix ID. + # + # * user_profile.avatar_url: an MXC URI for the user's avatar, if any. + # None if the user has not set an avatar. + # + # * user_profile.display_name: the user's display name. None if the user + # has not set a display name. + # # * HTML page which notifies the user that they are authenticating to confirm # an operation on their account during the user interactive authentication # process: 'sso_auth_confirm.html'. diff --git a/synapse/config/sso.py b/synapse/config/sso.py index 59be825532..a470112ed4 100644 --- a/synapse/config/sso.py +++ b/synapse/config/sso.py @@ -127,7 +127,8 @@ class SSOConfig(Config): # * HTML page for a confirmation step before redirecting back to the client # with the login token: 'sso_redirect_confirm.html'. # - # When rendering, this template is given three variables: + # 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). @@ -140,6 +141,17 @@ class SSOConfig(Config): # # * server_name: the homeserver's name. # + # * new_user: a boolean indicating whether this is the user's first time + # logging in. + # + # * user_id: the user's matrix ID. + # + # * user_profile.avatar_url: an MXC URI for the user's avatar, if any. + # None if the user has not set an avatar. + # + # * user_profile.display_name: the user's display name. None if the user + # has not set a display name. + # # * HTML page which notifies the user that they are authenticating to confirm # an operation on their account during the user interactive authentication # process: 'sso_auth_confirm.html'. diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 0e98db22b3..c722a4afa8 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -61,6 +61,7 @@ from synapse.http.site import SynapseRequest from synapse.logging.context import defer_to_thread from synapse.metrics.background_process_metrics import run_as_background_process from synapse.module_api import ModuleApi +from synapse.storage.roommember import ProfileInfo from synapse.types import JsonDict, Requester, UserID from synapse.util import stringutils as stringutils from synapse.util.async_helpers import maybe_awaitable @@ -1396,6 +1397,7 @@ class AuthHandler(BaseHandler): request: Request, client_redirect_url: str, extra_attributes: Optional[JsonDict] = None, + new_user: bool = False, ): """Having figured out a mxid for this user, complete the HTTP request @@ -1406,6 +1408,8 @@ class AuthHandler(BaseHandler): process. extra_attributes: Extra attributes which will be passed to the client during successful login. Must be JSON serializable. + new_user: True if we should use wording appropriate to a user who has just + registered. """ # If the account has been deactivated, do not proceed with the login # flow. @@ -1414,8 +1418,17 @@ class AuthHandler(BaseHandler): respond_with_html(request, 403, self._sso_account_deactivated_template) return + profile = await self.store.get_profileinfo( + UserID.from_string(registered_user_id).localpart + ) + self._complete_sso_login( - registered_user_id, request, client_redirect_url, extra_attributes + registered_user_id, + request, + client_redirect_url, + extra_attributes, + new_user=new_user, + user_profile_data=profile, ) def _complete_sso_login( @@ -1424,12 +1437,18 @@ class AuthHandler(BaseHandler): request: Request, client_redirect_url: str, extra_attributes: Optional[JsonDict] = None, + new_user: bool = False, + user_profile_data: Optional[ProfileInfo] = None, ): """ The synchronous portion of complete_sso_login. This exists purely for backwards compatibility of synapse.module_api.ModuleApi. """ + + if user_profile_data is None: + user_profile_data = ProfileInfo(None, None) + # Store any extra attributes which will be passed in the login response. # Note that this is per-user so it may overwrite a previous value, this # is considered OK since the newest SSO attributes should be most valid. @@ -1467,6 +1486,9 @@ class AuthHandler(BaseHandler): display_url=redirect_url_no_params, redirect_url=redirect_url, server_name=self._server_name, + new_user=new_user, + user_id=registered_user_id, + user_profile=user_profile_data, ) respond_with_html(request, 200, html) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 50c5ae142a..ceaeb5a376 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -391,6 +391,8 @@ class SsoHandler: to an additional page. (e.g. to prompt for more information) """ + new_user = False + # grab a lock while we try to find a mapping for this user. This seems... # optimistic, especially for implementations that end up redirecting to # interstitial pages. @@ -431,9 +433,14 @@ class SsoHandler: get_request_user_agent(request), request.getClientIP(), ) + new_user = True await self._auth_handler.complete_sso_login( - user_id, request, client_redirect_url, extra_login_attributes + user_id, + request, + client_redirect_url, + extra_login_attributes, + new_user=new_user, ) async def _call_attribute_mapper( @@ -778,6 +785,7 @@ class SsoHandler: request, session.client_redirect_url, session.extra_login_attributes, + new_user=True, ) def _expire_old_sessions(self): diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 72ab5750cc..401d577293 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -279,7 +279,11 @@ class ModuleApi: ) async def complete_sso_login_async( - self, registered_user_id: str, request: SynapseRequest, client_redirect_url: str + self, + registered_user_id: str, + request: SynapseRequest, + client_redirect_url: str, + new_user: bool = False, ): """Complete a SSO login by redirecting the user to a page to confirm whether they want their access token sent to `client_redirect_url`, or redirect them to that @@ -291,9 +295,11 @@ class ModuleApi: request: The request to respond to. client_redirect_url: The URL to which to offer to redirect the user (or to redirect them directly if whitelisted). + new_user: set to true to use wording for the consent appropriate to a user + who has just registered. """ await self._auth_handler.complete_sso_login( - registered_user_id, request, client_redirect_url, + registered_user_id, request, client_redirect_url, new_user=new_user ) @defer.inlineCallbacks diff --git a/synapse/res/templates/sso.css b/synapse/res/templates/sso.css new file mode 100644 index 0000000000..ff9dc94032 --- /dev/null +++ b/synapse/res/templates/sso.css @@ -0,0 +1,83 @@ +body { + font-family: "Inter", "Helvetica", "Arial", sans-serif; + font-size: 14px; + color: #17191C; +} + +header { + max-width: 480px; + width: 100%; + margin: 24px auto; + text-align: center; +} + +header p { + color: #737D8C; + line-height: 24px; +} + +h1 { + font-size: 24px; +} + +h2 { + font-size: 14px; +} + +h2 img { + vertical-align: middle; + margin-right: 8px; + width: 24px; + height: 24px; +} + +label { + cursor: pointer; +} + +main { + max-width: 360px; + width: 100%; + margin: 24px auto; +} + +.primary-button { + border: none; + text-decoration: none; + padding: 12px; + color: white; + background-color: #418DED; + font-weight: bold; + display: block; + border-radius: 12px; + width: 100%; + margin: 16px 0; + cursor: pointer; + text-align: center; +} + +.profile { + display: flex; + justify-content: center; + margin: 24px 0; +} + +.profile .avatar { + width: 36px; + height: 36px; + border-radius: 100%; + display: block; + margin-right: 8px; +} + +.profile .display-name { + font-weight: bold; + margin-bottom: 4px; +} +.profile .user-id { + color: #737D8C; +} + +.profile .display-name, .profile .user-id { + line-height: 18px; +} \ No newline at end of file diff --git a/synapse/res/templates/sso_redirect_confirm.html b/synapse/res/templates/sso_redirect_confirm.html index 20a15e1e74..ce4f573848 100644 --- a/synapse/res/templates/sso_redirect_confirm.html +++ b/synapse/res/templates/sso_redirect_confirm.html @@ -3,12 +3,34 @@ SSO redirect confirmation + + -

The application at {{ display_url | e }} is requesting full access to your {{ server_name }} Matrix account.

-

If you don't recognise this address, you should ignore this and close this tab.

-

- I trust this address -

+
+ {% if new_user %} +

Your account is now ready

+

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

+ {% else %} +

Log in

+ {% endif %} +

Continue to confirm you trust {{ display_url | e }}.

+
+
+ {% if user_profile.avatar_url %} +
+ +
+ {% if user_profile.display_name %} +
{{ user_profile.display_name | e }}
+ {% endif %} +
{{ user_id | e }}
+
+
+ {% endif %} + Continue +
- \ No newline at end of file + diff --git a/tests/handlers/test_cas.py b/tests/handlers/test_cas.py index c37bb6440e..7baf224f7e 100644 --- a/tests/handlers/test_cas.py +++ b/tests/handlers/test_cas.py @@ -62,7 +62,7 @@ class CasHandlerTestCase(HomeserverTestCase): # check that the auth handler got called as expected auth_handler.complete_sso_login.assert_called_once_with( - "@test_user:test", request, "redirect_uri", None + "@test_user:test", request, "redirect_uri", None, new_user=True ) def test_map_cas_user_to_existing_user(self): @@ -85,7 +85,7 @@ class CasHandlerTestCase(HomeserverTestCase): # check that the auth handler got called as expected auth_handler.complete_sso_login.assert_called_once_with( - "@test_user:test", request, "redirect_uri", None + "@test_user:test", request, "redirect_uri", None, new_user=False ) # Subsequent calls should map to the same mxid. @@ -94,7 +94,7 @@ class CasHandlerTestCase(HomeserverTestCase): self.handler._handle_cas_response(request, cas_response, "redirect_uri", "") ) auth_handler.complete_sso_login.assert_called_once_with( - "@test_user:test", request, "redirect_uri", None + "@test_user:test", request, "redirect_uri", None, new_user=False ) def test_map_cas_user_to_invalid_localpart(self): @@ -112,7 +112,7 @@ class CasHandlerTestCase(HomeserverTestCase): # check that the auth handler got called as expected auth_handler.complete_sso_login.assert_called_once_with( - "@f=c3=b6=c3=b6:test", request, "redirect_uri", None + "@f=c3=b6=c3=b6:test", request, "redirect_uri", None, new_user=True ) diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index b3dfa40d25..d8f90b9a80 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -419,7 +419,7 @@ class OidcHandlerTestCase(HomeserverTestCase): self.get_success(self.handler.handle_oidc_callback(request)) auth_handler.complete_sso_login.assert_called_once_with( - expected_user_id, request, client_redirect_url, None, + expected_user_id, request, client_redirect_url, None, new_user=True ) self.provider._exchange_code.assert_called_once_with(code) self.provider._parse_id_token.assert_called_once_with(token, nonce=nonce) @@ -450,7 +450,7 @@ class OidcHandlerTestCase(HomeserverTestCase): self.get_success(self.handler.handle_oidc_callback(request)) auth_handler.complete_sso_login.assert_called_once_with( - expected_user_id, request, client_redirect_url, None, + expected_user_id, request, client_redirect_url, None, new_user=False ) self.provider._exchange_code.assert_called_once_with(code) self.provider._parse_id_token.assert_not_called() @@ -623,7 +623,11 @@ class OidcHandlerTestCase(HomeserverTestCase): self.get_success(self.handler.handle_oidc_callback(request)) auth_handler.complete_sso_login.assert_called_once_with( - "@foo:test", request, client_redirect_url, {"phone": "1234567"}, + "@foo:test", + request, + client_redirect_url, + {"phone": "1234567"}, + new_user=True, ) def test_map_userinfo_to_user(self): @@ -637,7 +641,7 @@ class OidcHandlerTestCase(HomeserverTestCase): } self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) auth_handler.complete_sso_login.assert_called_once_with( - "@test_user:test", ANY, ANY, None, + "@test_user:test", ANY, ANY, None, new_user=True ) auth_handler.complete_sso_login.reset_mock() @@ -648,7 +652,7 @@ class OidcHandlerTestCase(HomeserverTestCase): } self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) auth_handler.complete_sso_login.assert_called_once_with( - "@test_user_2:test", ANY, ANY, None, + "@test_user_2:test", ANY, ANY, None, new_user=True ) auth_handler.complete_sso_login.reset_mock() @@ -685,14 +689,14 @@ class OidcHandlerTestCase(HomeserverTestCase): } self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) auth_handler.complete_sso_login.assert_called_once_with( - user.to_string(), ANY, ANY, None, + user.to_string(), ANY, ANY, None, new_user=False ) auth_handler.complete_sso_login.reset_mock() # Subsequent calls should map to the same mxid. self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) auth_handler.complete_sso_login.assert_called_once_with( - user.to_string(), ANY, ANY, None, + user.to_string(), ANY, ANY, None, new_user=False ) auth_handler.complete_sso_login.reset_mock() @@ -707,7 +711,7 @@ class OidcHandlerTestCase(HomeserverTestCase): } self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) auth_handler.complete_sso_login.assert_called_once_with( - user.to_string(), ANY, ANY, None, + user.to_string(), ANY, ANY, None, new_user=False ) auth_handler.complete_sso_login.reset_mock() @@ -743,7 +747,7 @@ class OidcHandlerTestCase(HomeserverTestCase): self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) auth_handler.complete_sso_login.assert_called_once_with( - "@TEST_USER_2:test", ANY, ANY, None, + "@TEST_USER_2:test", ANY, ANY, None, new_user=False ) def test_map_userinfo_to_invalid_localpart(self): @@ -779,7 +783,7 @@ class OidcHandlerTestCase(HomeserverTestCase): # test_user is already taken, so test_user1 gets registered instead. auth_handler.complete_sso_login.assert_called_once_with( - "@test_user1:test", ANY, ANY, None, + "@test_user1:test", ANY, ANY, None, new_user=True ) auth_handler.complete_sso_login.reset_mock() diff --git a/tests/handlers/test_saml.py b/tests/handlers/test_saml.py index 261c7083d1..a8d6c0f617 100644 --- a/tests/handlers/test_saml.py +++ b/tests/handlers/test_saml.py @@ -131,7 +131,7 @@ class SamlHandlerTestCase(HomeserverTestCase): # check that the auth handler got called as expected auth_handler.complete_sso_login.assert_called_once_with( - "@test_user:test", request, "redirect_uri", None + "@test_user:test", request, "redirect_uri", None, new_user=True ) @override_config({"saml2_config": {"grandfathered_mxid_source_attribute": "mxid"}}) @@ -157,7 +157,7 @@ class SamlHandlerTestCase(HomeserverTestCase): # check that the auth handler got called as expected auth_handler.complete_sso_login.assert_called_once_with( - "@test_user:test", request, "", None + "@test_user:test", request, "", None, new_user=False ) # Subsequent calls should map to the same mxid. @@ -166,7 +166,7 @@ class SamlHandlerTestCase(HomeserverTestCase): self.handler._handle_authn_response(request, saml_response, "") ) auth_handler.complete_sso_login.assert_called_once_with( - "@test_user:test", request, "", None + "@test_user:test", request, "", None, new_user=False ) def test_map_saml_response_to_invalid_localpart(self): @@ -214,7 +214,7 @@ class SamlHandlerTestCase(HomeserverTestCase): # test_user is already taken, so test_user1 gets registered instead. auth_handler.complete_sso_login.assert_called_once_with( - "@test_user1:test", request, "", None + "@test_user1:test", request, "", None, new_user=True ) auth_handler.complete_sso_login.reset_mock()