From c0af9a281b234c22dc46974b758d67858b7ed394 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 11 Mar 2022 10:13:54 +0000 Subject: [PATCH 1/7] Update the SSO username picker template to comply with SIWA guidelines Fixes https://github.com/matrix-org/synapse/issues/12205 --- synapse/res/templates/sso_auth_account_details.html | 6 +++--- synapse/rest/synapse/client/pick_username.py | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/synapse/res/templates/sso_auth_account_details.html b/synapse/res/templates/sso_auth_account_details.html index 00e1dcdbb866..7e5c42b9a989 100644 --- a/synapse/res/templates/sso_auth_account_details.html +++ b/synapse/res/templates/sso_auth_account_details.html @@ -130,15 +130,15 @@
-

Your account is nearly ready

-

Check your details before creating an account on {{ server_name }}

+

Choose your user name

+

This is required to create your account on {{ server_name }}, and you can't change this later.

@
- +
:{{ server_name }}
diff --git a/synapse/rest/synapse/client/pick_username.py b/synapse/rest/synapse/client/pick_username.py index 28ae08349705..09c783d824ea 100644 --- a/synapse/rest/synapse/client/pick_username.py +++ b/synapse/rest/synapse/client/pick_username.py @@ -27,7 +27,7 @@ ) from synapse.http.servlet import parse_boolean, parse_string from synapse.http.site import SynapseRequest -from synapse.types import JsonDict +from synapse.types import JsonDict, map_username_to_mxid_localpart from synapse.util.templates import build_jinja_env if TYPE_CHECKING: @@ -98,6 +98,7 @@ async def _async_render_GET(self, request: Request) -> None: "user_attributes": { "display_name": session.display_name, "emails": session.emails, + "remote_user_id": map_username_to_mxid_localpart(session.remote_user_id) }, } From 4c39961a7524154ceee0a90bd3306ecad5b59a32 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 11 Mar 2022 10:16:00 +0000 Subject: [PATCH 2/7] Changelog --- changelog.d/12210.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/12210.misc diff --git a/changelog.d/12210.misc b/changelog.d/12210.misc new file mode 100644 index 000000000000..3f6a8747c256 --- /dev/null +++ b/changelog.d/12210.misc @@ -0,0 +1 @@ +Update the SSO username picker template to comply with SIWA guidelines. From 5646b289fcbebe804235611d671fc6baccf29fc3 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 11 Mar 2022 10:20:05 +0000 Subject: [PATCH 3/7] Lint --- synapse/rest/synapse/client/pick_username.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/synapse/rest/synapse/client/pick_username.py b/synapse/rest/synapse/client/pick_username.py index 09c783d824ea..9118e65efb09 100644 --- a/synapse/rest/synapse/client/pick_username.py +++ b/synapse/rest/synapse/client/pick_username.py @@ -98,7 +98,9 @@ async def _async_render_GET(self, request: Request) -> None: "user_attributes": { "display_name": session.display_name, "emails": session.emails, - "remote_user_id": map_username_to_mxid_localpart(session.remote_user_id) + "remote_user_id": map_username_to_mxid_localpart( + session.remote_user_id + ), }, } From be023cad8c82149a60d30c09c9897392876f036d Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 11 Mar 2022 10:39:08 +0000 Subject: [PATCH 4/7] Document new variable --- docs/templates.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/docs/templates.md b/docs/templates.md index 2b66e9d86294..274936ce9612 100644 --- a/docs/templates.md +++ b/docs/templates.md @@ -176,8 +176,10 @@ Below are the templates Synapse will look for when generating pages related to S for the brand of the IdP * `user_attributes`: an object containing details about the user that we received from the IdP. May have the following attributes: - * display_name: the user's display_name - * emails: a list of email addresses + * `display_name`: the user's display name + * `emails`: a list of email addresses + * `remote_user_id`: the user's ID on the IdP, [converted](https://spec.matrix.org/v1.2/appendices/#mapping-from-other-character-sets) + to the valid syntax for a Matrix username The template should render a form which submits the following fields: * `username`: the localpart of the user's chosen user id * `sso_new_user_consent.html`: HTML page allowing the user to consent to the From 7a2b74bb1692e95cdfabf9ea66dee6d1f39d1554 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 11 Mar 2022 12:22:16 +0000 Subject: [PATCH 5/7] Ask the config whether to confirm a generated local part --- docs/sample_config.yaml | 9 +++++++-- docs/templates.md | 5 +++-- synapse/config/oidc.py | 9 +++++++-- synapse/handlers/oidc.py | 7 ++++++- synapse/handlers/sso.py | 8 +++++--- synapse/res/templates/sso_auth_account_details.html | 2 +- synapse/rest/synapse/client/pick_username.py | 13 +++++++++---- 7 files changed, 38 insertions(+), 15 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 6f3623c88ab9..ef25a3175f98 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1947,8 +1947,13 @@ saml2_config: # # localpart_template: Jinja2 template for the localpart of the MXID. # If this is not set, the user will be prompted to choose their -# own username (see 'sso_auth_account_details.html' in the 'sso' -# section of this file). +# own username (see the documentation for the +# 'sso_auth_account_details.html' template). +# +# confirm_localpart: Whether to prompt the user to validate (or +# change) the generated localpart (see the documentation for the +# 'sso_auth_account_details.html' template), instead of +# registering the account right away. # # display_name_template: Jinja2 template for the display name to set # on first login. If unset, no displayname will be set. diff --git a/docs/templates.md b/docs/templates.md index 274936ce9612..b251d05cb9eb 100644 --- a/docs/templates.md +++ b/docs/templates.md @@ -178,8 +178,9 @@ Below are the templates Synapse will look for when generating pages related to S we received from the IdP. May have the following attributes: * `display_name`: the user's display name * `emails`: a list of email addresses - * `remote_user_id`: the user's ID on the IdP, [converted](https://spec.matrix.org/v1.2/appendices/#mapping-from-other-character-sets) - to the valid syntax for a Matrix username + * `localpart`: the local part of the Matrix user ID to register, + if `localpart_template` is set in the mapping provider configuration (empty + string if not) The template should render a form which submits the following fields: * `username`: the localpart of the user's chosen user id * `sso_new_user_consent.html`: HTML page allowing the user to consent to the diff --git a/synapse/config/oidc.py b/synapse/config/oidc.py index f7e4f9ef22b1..fc95912d9b3f 100644 --- a/synapse/config/oidc.py +++ b/synapse/config/oidc.py @@ -182,8 +182,13 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs) -> str # # localpart_template: Jinja2 template for the localpart of the MXID. # If this is not set, the user will be prompted to choose their - # own username (see 'sso_auth_account_details.html' in the 'sso' - # section of this file). + # own username (see the documentation for the + # 'sso_auth_account_details.html' template). + # + # confirm_localpart: Whether to prompt the user to validate (or + # change) the generated localpart (see the documentation for the + # 'sso_auth_account_details.html' template), instead of + # registering the account right away. # # display_name_template: Jinja2 template for the display name to set # on first login. If unset, no displayname will be set. diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index 593a2aac6691..c45939b1e193 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -1228,6 +1228,7 @@ class OidcSessionData: class UserAttributeDict(TypedDict): localpart: Optional[str] + confirm_localpart: bool display_name: Optional[str] emails: List[str] @@ -1316,6 +1317,7 @@ class JinjaOidcMappingConfig: display_name_template: Optional[Template] email_template: Optional[Template] extra_attributes: Dict[str, Template] + confirm_localpart: bool = False class JinjaOidcMappingProvider(OidcMappingProvider[JinjaOidcMappingConfig]): @@ -1398,7 +1400,10 @@ def render_template_field(template: Optional[Template]) -> Optional[str]: emails.append(email) return UserAttributeDict( - localpart=localpart, display_name=display_name, emails=emails + localpart=localpart, + display_name=display_name, + emails=emails, + confirm_localpart=self._config.confirm_localpart, ) async def get_extra_attributes(self, userinfo: UserInfo, token: Token) -> JsonDict: diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index ff5b5169cac1..4f02a060d953 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -132,6 +132,7 @@ class UserAttributes: # if `None`, the mapper has not picked a userid, and the user should be prompted to # enter one. localpart: Optional[str] + confirm_localpart: bool = False display_name: Optional[str] = None emails: Collection[str] = attr.Factory(list) @@ -561,9 +562,10 @@ def _get_url_for_next_new_user_step( # Must provide either attributes or session, not both assert (attributes is not None) != (session is not None) - if (attributes and attributes.localpart is None) or ( - session and session.chosen_localpart is None - ): + if ( + attributes + and (attributes.localpart is None or attributes.confirm_localpart is True) + ) or (session and session.chosen_localpart is None): return b"/_synapse/client/pick_username/account_details" elif self._consent_at_registration and not ( session and session.terms_accepted_version diff --git a/synapse/res/templates/sso_auth_account_details.html b/synapse/res/templates/sso_auth_account_details.html index 7e5c42b9a989..41315e4fd4da 100644 --- a/synapse/res/templates/sso_auth_account_details.html +++ b/synapse/res/templates/sso_auth_account_details.html @@ -138,7 +138,7 @@

Choose your user name

@
- +
:{{ server_name }}
diff --git a/synapse/rest/synapse/client/pick_username.py b/synapse/rest/synapse/client/pick_username.py index 9118e65efb09..6338fbaaa96d 100644 --- a/synapse/rest/synapse/client/pick_username.py +++ b/synapse/rest/synapse/client/pick_username.py @@ -27,7 +27,7 @@ ) from synapse.http.servlet import parse_boolean, parse_string from synapse.http.site import SynapseRequest -from synapse.types import JsonDict, map_username_to_mxid_localpart +from synapse.types import JsonDict from synapse.util.templates import build_jinja_env if TYPE_CHECKING: @@ -92,15 +92,20 @@ async def _async_render_GET(self, request: Request) -> None: self._sso_handler.render_error(request, "bad_session", e.msg, code=e.code) return + # The configuration might mandate going through this step to validate an + # automatically generated localpart, so session.chosen_localpart might already + # be set. + localpart = "" + if session.chosen_localpart is not None: + localpart = session.chosen_localpart + idp_id = session.auth_provider_id template_params = { "idp": self._sso_handler.get_identity_providers()[idp_id], "user_attributes": { "display_name": session.display_name, "emails": session.emails, - "remote_user_id": map_username_to_mxid_localpart( - session.remote_user_id - ), + "localpart": localpart, }, } From fe8cf27b66430bf953f36a8de480a76318695569 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 11 Mar 2022 12:24:38 +0000 Subject: [PATCH 6/7] Correctly pass through config flag --- synapse/handlers/oidc.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index c45939b1e193..9f2e4abe677e 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -1359,12 +1359,15 @@ def parse_template_config(option_name: str) -> Optional[Template]: "invalid jinja template", path=["extra_attributes", key] ) from e + confirm_localpart = config.get("confirm_localpart") or False + return JinjaOidcMappingConfig( subject_claim=subject_claim, localpart_template=localpart_template, display_name_template=display_name_template, email_template=email_template, extra_attributes=extra_attributes, + confirm_localpart=confirm_localpart, ) def get_remote_user_id(self, userinfo: UserInfo) -> str: From 51523bd50cdf806e8c7aefdfd443f67df9c613f6 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 11 Mar 2022 12:53:02 +0000 Subject: [PATCH 7/7] Raise a ConfigError if confirm_localpart isn't a bool --- synapse/handlers/oidc.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index 9f2e4abe677e..d98659edc761 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -1360,6 +1360,8 @@ def parse_template_config(option_name: str) -> Optional[Template]: ) from e confirm_localpart = config.get("confirm_localpart") or False + if not isinstance(confirm_localpart, bool): + raise ConfigError("must be a bool", path=["confirm_localpart"]) return JinjaOidcMappingConfig( subject_claim=subject_claim,