diff options
| -rw-r--r-- | changelog.d/10733.bugfix | 1 | ||||
| -rw-r--r-- | synapse/handlers/sso.py | 81 | ||||
| -rw-r--r-- | synapse/rest/synapse/client/new_user_consent.py | 4 |
3 files changed, 63 insertions, 23 deletions
diff --git a/changelog.d/10733.bugfix b/changelog.d/10733.bugfix new file mode 100644 index 0000000000..7053a00ca4 --- /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. Contributed by @AndrewFerr. diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 0fdc6dd9e7..05aa76d6a6 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -444,14 +444,16 @@ class SsoHandler: 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( + 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, ) @@ -530,18 +532,53 @@ class SsoHandler: ) return attributes - async def _redirect_to_username_picker( + def _get_url_for_next_new_user_step( + self, + attributes: Optional[UserAttributes] = None, + session: Optional[UsernameMappingSession] = None, + ) -> 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, + returns the URL to redirect to for the next step of the registration flow. + + Args: + attributes: the user attributes returned by the user mapping provider, + from before a UsernameMappingSession has begun. + + session: an active UsernameMappingSession, possibly with some of its + attributes chosen by the user. + + Returns: + 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) + + 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 ( + session and session.terms_accepted_version + ): + return b"/_synapse/client/new_user_consent" + else: + return b"/_synapse/client/sso_register" if session else b"" + + 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: bytes, extra_login_attributes: Optional[JsonDict], ) -> NoReturn: """Creates a UsernameMappingSession and redirects the browser - 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. + 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. @@ -554,12 +591,15 @@ class SsoHandler: 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() session = UsernameMappingSession( @@ -570,13 +610,18 @@ class SsoHandler: client_redirect_url=client_redirect_url, expiry_time_ms=now + self._MAPPING_SESSION_VALIDITY_PERIOD_MS, extra_login_attributes=extra_login_attributes, + # 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, + # 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/pick_username/account_details") + # 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")) @@ -805,16 +850,9 @@ class SsoHandler: ) 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 @@ -842,8 +880,9 @@ class SsoHandler: 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. diff --git a/synapse/rest/synapse/client/new_user_consent.py b/synapse/rest/synapse/client/new_user_consent.py index fc62a09b7f..3869d18003 100644 --- a/synapse/rest/synapse/client/new_user_consent.py +++ b/synapse/rest/synapse/client/new_user_consent.py @@ -63,8 +63,8 @@ class NewUserConsentResource(DirectServeHtmlResource): 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( |