From 311ccf5eb57b1d095b9dfcd1c86b25b600e220cc Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Wed, 1 Sep 2021 02:24:09 -0400 Subject: [PATCH 1/6] Ask consent on SSO registration with default mxid Fixes #10732 Signed-off-by: Andrew Ferrazzutti --- synapse/handlers/sso.py | 67 +++++++++++++++++++ .../rest/synapse/client/new_user_consent.py | 4 +- 2 files changed, 69 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 0e6ebb574ecf..3faec611d2ed 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -459,6 +459,14 @@ async def complete_sso_login_request( client_redirect_url, extra_login_attributes, ) + elif self._consent_at_registration: + await self._redirect_to_new_user_consent( + auth_provider_id, + remote_user_id, + attributes, + client_redirect_url, + extra_login_attributes, + ) user_id = await self._register_mapped_user( attributes, @@ -588,6 +596,65 @@ async def _redirect_to_username_picker( ) raise e + async def _redirect_to_new_user_consent( + self, + auth_provider_id: str, + remote_user_id: str, + attributes: UserAttributes, + client_redirect_url: str, + extra_login_attributes: Optional[JsonDict], + ) -> NoReturn: + """Creates a ConsentMappingSession and redirects the browser + + Called if an otherwise successful new user registration requires consent. + Raises a RedirectException which redirects the browser to the consent form. + + Args: + auth_provider_id: A unique identifier for this SSO provider, e.g. + "oidc" or "saml". + + remote_user_id: The unique identifier from the SSO provider. + + attributes: the user attributes returned by the user mapping provider. + + client_redirect_url: The redirect URL passed in by the client, which we + will eventually redirect back to. + + extra_login_attributes: An optional dictionary of extra + attributes to be provided to the client in the login response. + + Raises: + RedirectException + """ + session_id = random_string(16) + now = self._clock.time_msec() + # Shortcut: pretend that the localpart set by the mapping provider was chosen by the user. + # TODO Maybe worth renaming UsernameMappingSession since it also handles the consent flow. + session = UsernameMappingSession( + auth_provider_id=auth_provider_id, + remote_user_id=remote_user_id, + display_name=attributes.display_name, + emails=attributes.emails, + client_redirect_url=client_redirect_url, + expiry_time_ms=now + self._MAPPING_SESSION_VALIDITY_PERIOD_MS, + extra_login_attributes=extra_login_attributes, + # Overridden settings + chosen_localpart=attributes.localpart, + use_display_name=True, + emails_to_use=[], + ) + + self._username_mapping_sessions[session_id] = session + logger.info("Recorded registration session id %s", session_id) + + # Set the cookie and redirect to the username picker + e = RedirectException(b"/_synapse/client/new_user_consent") + e.cookies.append( + b"%s=%s; path=/" + % (USERNAME_MAPPING_SESSION_COOKIE_NAME, session_id.encode("ascii")) + ) + raise e + async def _register_mapped_user( self, attributes: UserAttributes, diff --git a/synapse/rest/synapse/client/new_user_consent.py b/synapse/rest/synapse/client/new_user_consent.py index fc62a09b7f07..3869d18003ca 100644 --- a/synapse/rest/synapse/client/new_user_consent.py +++ b/synapse/rest/synapse/client/new_user_consent.py @@ -63,8 +63,8 @@ async def _async_render_GET(self, request: Request) -> None: self._sso_handler.render_error(request, "bad_session", e.msg, code=e.code) return - # It should be impossible to get here without having first been through - # the pick-a-username step, which ensures chosen_localpart gets set. + # It should be impossible to get here without either the user or the mapping provider + # having chosen a username, which ensures chosen_localpart gets set. if not session.chosen_localpart: logger.warning("Session has no user name selected") self._sso_handler.render_error( From bd3a84e2da02ca35294f49803c3eb1be8dfd8f45 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Wed, 1 Sep 2021 03:33:48 -0400 Subject: [PATCH 2/6] Add changelog for #10733 Signed-off-by: Andrew Ferrazzutti --- changelog.d/10733.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10733.bugfix diff --git a/changelog.d/10733.bugfix b/changelog.d/10733.bugfix new file mode 100644 index 000000000000..71ffe40a27f7 --- /dev/null +++ b/changelog.d/10733.bugfix @@ -0,0 +1 @@ +Allow user registration via SSO to require consent tracking for SSO mapping providers that don't prompt for Matrix ID selection. From a84749488e5f19175ad840b789a4d6a0e7c413e8 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Wed, 8 Sep 2021 03:16:54 -0400 Subject: [PATCH 3/6] Commonize checks for missing new user attributes Signed-off-by: Andrew Ferrazzutti --- synapse/handlers/sso.py | 130 ++++++++++++++++------------------------ 1 file changed, 51 insertions(+), 79 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 3faec611d2ed..769479c97088 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -449,22 +449,16 @@ async def complete_sso_login_request( if not user_id: attributes = await self._call_attribute_mapper(sso_to_matrix_id_mapper) - if attributes.localpart is None: - # the mapper doesn't return a username. bail out with a redirect to - # the username picker. - await self._redirect_to_username_picker( - auth_provider_id, - remote_user_id, - attributes, - client_redirect_url, - extra_login_attributes, - ) - elif self._consent_at_registration: - await self._redirect_to_new_user_consent( + next_step_url = self._get_url_for_next_new_user_step( + attributes=attributes + ) + if next_step_url: + await self._redirect_to_next_new_user_step( auth_provider_id, remote_user_id, attributes, client_redirect_url, + next_step_url, extra_login_attributes, ) @@ -543,71 +537,53 @@ async def _call_attribute_mapper( ) return attributes - async def _redirect_to_username_picker( + def _get_url_for_next_new_user_step( self, - auth_provider_id: str, - remote_user_id: str, - attributes: UserAttributes, - client_redirect_url: str, - extra_login_attributes: Optional[JsonDict], - ) -> NoReturn: - """Creates a UsernameMappingSession and redirects the browser + attributes: Optional[UserAttributes] = None, + session: Optional[UsernameMappingSession] = None, + ) -> Optional[bytes]: + """Returns the URL to redirect to for the next step of new user registration - Called if the user mapping provider doesn't return a localpart for a new user. - Raises a RedirectException which redirects the browser to the username picker. + Given attributes from the user mapping provider or a UsernameMappingSession, + returns the URL to redirect to for the next step of the registration flow. Args: - auth_provider_id: A unique identifier for this SSO provider, e.g. - "oidc" or "saml". - - remote_user_id: The unique identifier from the SSO provider. - - attributes: the user attributes returned by the user mapping provider. - - client_redirect_url: The redirect URL passed in by the client, which we - will eventually redirect back to. + attributes: the user attributes returned by the user mapping provider, + from before a UsernameMappingSession has begun. - extra_login_attributes: An optional dictionary of extra - attributes to be provided to the client in the login response. + session: an active UsernameMappingSession, possibly with some of its + attributes chosen by the user. - Raises: - RedirectException + Returns: + The URL to redirect to, or None if no redirect is necessary """ - session_id = random_string(16) - now = self._clock.time_msec() - session = UsernameMappingSession( - auth_provider_id=auth_provider_id, - remote_user_id=remote_user_id, - display_name=attributes.display_name, - emails=attributes.emails, - client_redirect_url=client_redirect_url, - expiry_time_ms=now + self._MAPPING_SESSION_VALIDITY_PERIOD_MS, - extra_login_attributes=extra_login_attributes, - ) + # Must provide either attributes or session, not both + assert (attributes is not None) != (session is not None) - self._username_mapping_sessions[session_id] = session - logger.info("Recorded registration session id %s", session_id) - - # Set the cookie and redirect to the username picker - e = RedirectException(b"/_synapse/client/pick_username/account_details") - e.cookies.append( - b"%s=%s; path=/" - % (USERNAME_MAPPING_SESSION_COOKIE_NAME, session_id.encode("ascii")) - ) - raise e + if (attributes and not attributes.localpart) or ( + session and not session.chosen_localpart + ): + return b"/_synapse/client/pick_username/account_details" + elif self._consent_at_registration and not ( + session and session.terms_accepted_version + ): + return b"/_synapse/client/new_user_consent" + else: + return b"/_synapse/client/sso_register" if session else None - async def _redirect_to_new_user_consent( + async def _redirect_to_next_new_user_step( self, auth_provider_id: str, remote_user_id: str, attributes: UserAttributes, client_redirect_url: str, + next_step_url: str, extra_login_attributes: Optional[JsonDict], ) -> NoReturn: - """Creates a ConsentMappingSession and redirects the browser + """Creates a UsernameMappingSession and redirects the browser - Called if an otherwise successful new user registration requires consent. - Raises a RedirectException which redirects the browser to the consent form. + Called if the user mapping provider doesn't return complete information for a new user. + Raises a RedirectException which redirects the browser to a specified URL. Args: auth_provider_id: A unique identifier for this SSO provider, e.g. @@ -620,16 +596,17 @@ async def _redirect_to_new_user_consent( client_redirect_url: The redirect URL passed in by the client, which we will eventually redirect back to. + next_step_url: The URL to redirect to for the next step of the new user flow. + extra_login_attributes: An optional dictionary of extra attributes to be provided to the client in the login response. Raises: RedirectException """ + # TODO: If needed, allow using/looking up an existing session here. session_id = random_string(16) now = self._clock.time_msec() - # Shortcut: pretend that the localpart set by the mapping provider was chosen by the user. - # TODO Maybe worth renaming UsernameMappingSession since it also handles the consent flow. session = UsernameMappingSession( auth_provider_id=auth_provider_id, remote_user_id=remote_user_id, @@ -638,17 +615,18 @@ async def _redirect_to_new_user_consent( client_redirect_url=client_redirect_url, expiry_time_ms=now + self._MAPPING_SESSION_VALIDITY_PERIOD_MS, extra_login_attributes=extra_login_attributes, - # Overridden settings + # Treat the localpart returned by the user mapping provider as though + # it was chosen by the user. If it's None, it must be chosen eventually. chosen_localpart=attributes.localpart, - use_display_name=True, - emails_to_use=[], + # TODO: Consider letting the user mapping provider specify defaults for + # other user-chosen attributes. ) self._username_mapping_sessions[session_id] = session logger.info("Recorded registration session id %s", session_id) - # Set the cookie and redirect to the username picker - e = RedirectException(b"/_synapse/client/new_user_consent") + # Set the cookie and redirect to the next step + e = RedirectException(next_step_url) e.cookies.append( b"%s=%s; path=/" % (USERNAME_MAPPING_SESSION_COOKIE_NAME, session_id.encode("ascii")) @@ -877,16 +855,9 @@ async def handle_submit_username_request( ) session.emails_to_use = filtered_emails - # we may now need to collect consent from the user, in which case, redirect - # to the consent-extraction-unit - if self._consent_at_registration: - redirect_url = b"/_synapse/client/new_user_consent" - - # otherwise, redirect to the completion page - else: - redirect_url = b"/_synapse/client/sso_register" - - respond_with_redirect(request, redirect_url) + respond_with_redirect( + request, self._get_url_for_next_new_user_step(session=session) + ) async def handle_terms_accepted( self, request: Request, session_id: str, terms_version: str @@ -914,8 +885,9 @@ async def handle_terms_accepted( session.terms_accepted_version = terms_version - # we're done; now we can register the user - respond_with_redirect(request, b"/_synapse/client/sso_register") + respond_with_redirect( + request, self._get_url_for_next_new_user_step(session=session) + ) async def register_sso_user(self, request: Request, session_id: str) -> None: """Called once we have all the info we need to register a new user. From 0083018ffab87b83b1136d495906a5b2e28703f3 Mon Sep 17 00:00:00 2001 From: AndrewFerr Date: Wed, 8 Sep 2021 03:19:03 -0400 Subject: [PATCH 4/6] Update changelog.d/10733.bugfix Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Signed-off-by: Andrew Ferrazzutti --- changelog.d/10733.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/10733.bugfix b/changelog.d/10733.bugfix index 71ffe40a27f7..7053a00ca4a7 100644 --- a/changelog.d/10733.bugfix +++ b/changelog.d/10733.bugfix @@ -1 +1 @@ -Allow user registration via SSO to require consent tracking for SSO mapping providers that don't prompt for Matrix ID selection. +Allow user registration via SSO to require consent tracking for SSO mapping providers that don't prompt for Matrix ID selection. Contributed by @AndrewFerr. From 7d0d77ce50fa65e084c315f96b3bc3ba1286d4d9 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Wed, 8 Sep 2021 18:36:38 -0400 Subject: [PATCH 5/6] Fix return types Signed-off-by: Andrew Ferrazzutti --- synapse/handlers/sso.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 769479c97088..59b23242401e 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -541,7 +541,7 @@ def _get_url_for_next_new_user_step( self, attributes: Optional[UserAttributes] = None, session: Optional[UsernameMappingSession] = None, - ) -> Optional[bytes]: + ) -> bytes: """Returns the URL to redirect to for the next step of new user registration Given attributes from the user mapping provider or a UsernameMappingSession, @@ -555,7 +555,7 @@ def _get_url_for_next_new_user_step( attributes chosen by the user. Returns: - The URL to redirect to, or None if no redirect is necessary + The URL to redirect to, or an empty value if no redirect is necessary """ # Must provide either attributes or session, not both assert (attributes is not None) != (session is not None) @@ -569,7 +569,7 @@ def _get_url_for_next_new_user_step( ): return b"/_synapse/client/new_user_consent" else: - return b"/_synapse/client/sso_register" if session else None + return b"/_synapse/client/sso_register" if session else b"" async def _redirect_to_next_new_user_step( self, @@ -577,7 +577,7 @@ async def _redirect_to_next_new_user_step( remote_user_id: str, attributes: UserAttributes, client_redirect_url: str, - next_step_url: str, + next_step_url: bytes, extra_login_attributes: Optional[JsonDict], ) -> NoReturn: """Creates a UsernameMappingSession and redirects the browser From 7a52696d37ea7f72a10ed3deb5bba17e2a71d394 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Thu, 9 Sep 2021 03:18:13 -0400 Subject: [PATCH 6/6] Explicitly check for null localparts Signed-off-by: Andrew Ferrazzutti --- synapse/handlers/sso.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 59b23242401e..f7bb58e65166 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -560,8 +560,8 @@ 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 not attributes.localpart) or ( - session and not session.chosen_localpart + if (attributes and attributes.localpart is None) or ( + session and session.chosen_localpart is None ): return b"/_synapse/client/pick_username/account_details" elif self._consent_at_registration and not (