Ask consent on SSO registration with default mxid (#10733)

Fixes #10732: consent flow skipped during SSO user registration if username is left at default

Signed-off-by: Andrew Ferrazzutti [email protected]

Author: AndrewFerr

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.

synapse/handlers/sso.py

@@ -444,14 +444,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(
+                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 @@ async def _call_attribute_mapper(
             )
         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 @@ async def _redirect_to_username_picker(
             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 @@ async def _redirect_to_username_picker(
             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 @@ 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
@@ -842,8 +880,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.

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(