Apply user email & picture during OIDC registration if present & selected

changelog.d/17120.bugfix

@@ -0,0 +1 @@
+Apply user email & picture during OIDC registration if present & selected.

docs/sso_mapping_providers.md

@@ -98,6 +98,7 @@ A custom mapping provider must specify the following methods:
  either accept this localpart or pick their own username. Otherwise this
  option has no effect. If omitted, defaults to `False`.
  - `display_name`: An optional string, the display name for the user.
+ - `picture`: An optional string, the avatar url for the user.
  - `emails`: A list of strings, the email address(es) to associate with
  this user. If omitted, defaults to an empty list.
 * `async def get_extra_attributes(self, userinfo, token)`

synapse/handlers/sso.py

@@ -169,6 +169,7 @@ class UsernameMappingSession:
  # attributes returned by the ID mapper
  display_name: Optional[str]
  emails: StrCollection
+ avatar_url: Optional[str]
 
  # An optional dictionary of extra attributes to be provided to the client in the
  # login response.
@@ -183,6 +184,7 @@ class UsernameMappingSession:
  # choices made by the user
  chosen_localpart: Optional[str] = None
  use_display_name: bool = True
+ use_avatar: bool = True
  emails_to_use: StrCollection = ()
  terms_accepted_version: Optional[str] = None
 
@@ -660,6 +662,9 @@ async def _redirect_to_next_new_user_step(
  remote_user_id=remote_user_id,
  display_name=attributes.display_name,
  emails=attributes.emails,
+ avatar_url=attributes.picture,
+ # Default to using all mapped emails. Will be overwritten in handle_submit_username_request.
+ emails_to_use=attributes.emails,
  client_redirect_url=client_redirect_url,
  expiry_time_ms=now + self._MAPPING_SESSION_VALIDITY_PERIOD_MS,
  extra_login_attributes=extra_login_attributes,
@@ -966,6 +971,7 @@ async def handle_submit_username_request(
  session_id: str,
  localpart: str,
  use_display_name: bool,
+ use_avatar: bool,
  emails_to_use: Iterable[str],
  ) -> None:
  """Handle a request to the username-picker 'submit' endpoint
@@ -988,6 +994,7 @@ async def handle_submit_username_request(
  # update the session with the user's choices
  session.chosen_localpart = localpart
  session.use_display_name = use_display_name
+ session.use_avatar = use_avatar
 
  emails_from_idp = set(session.emails)
  filtered_emails: Set[str] = set()
@@ -1068,6 +1075,9 @@ async def register_sso_user(self, request: Request, session_id: str) -> None:
  if session.use_display_name:
  attributes.display_name = session.display_name
 
+ if session.use_avatar:
+ attributes.picture = session.avatar_url
+
  # the following will raise a 400 error if the username has been taken in the
  # meantime.
  user_id = await self._register_mapped_user(

synapse/rest/synapse/client/pick_username.py

@@ -113,6 +113,7 @@ async def _async_render_GET(self, request: Request) -> None:
  "display_name": session.display_name,
  "emails": session.emails,
  "localpart": localpart,
+ "avatar_url": session.avatar_url,
  },
  }
 
@@ -134,6 +135,7 @@ async def _async_render_POST(self, request: SynapseRequest) -> None:
  try:
  localpart = parse_string(request, "username", required=True)
  use_display_name = parse_boolean(request, "use_display_name", default=False)
+ use_avatar = parse_boolean(request, "use_avatar", default=False)
 
  try:
  emails_to_use: List[str] = [
@@ -147,5 +149,5 @@ async def _async_render_POST(self, request: SynapseRequest) -> None:
  return
 
  await self._sso_handler.handle_submit_username_request(
- request, session_id, localpart, use_display_name, emails_to_use
+ request, session_id, localpart, use_display_name, use_avatar, emails_to_use
  )

tests/rest/client/test_login.py

@@ -20,7 +20,17 @@
 #
 import time
 import urllib.parse
-from typing import Any, Collection, Dict, List, Optional, Tuple, Union
+from typing import (
+ Any,
+ BinaryIO,
+ Callable,
+ Collection,
+ Dict,
+ List,
+ Optional,
+ Tuple,
+ Union,
+)
 from unittest.mock import Mock
 from urllib.parse import urlencode
 
@@ -34,8 +44,9 @@
 from synapse.api.constants import ApprovalNoticeMedium, LoginType
 from synapse.api.errors import Codes
 from synapse.appservice import ApplicationService
+from synapse.http.client import RawHeaders
 from synapse.module_api import ModuleApi
-from synapse.rest.client import devices, login, logout, register
+from synapse.rest.client import account, devices, login, logout, profile, register
 from synapse.rest.client.account import WhoamiRestServlet
 from synapse.rest.synapse.client import build_synapse_client_resource_tree
 from synapse.server import HomeServer
@@ -48,6 +59,7 @@
 from tests.rest.client.utils import TEST_OIDC_CONFIG
 from tests.server import FakeChannel
 from tests.test_utils.html_parsers import TestHtmlParser
+from tests.test_utils.oidc import FakeOidcServer
 from tests.unittest import HomeserverTestCase, override_config, skip_unless
 
 try:
@@ -1421,7 +1433,19 @@ def test_login_appservice_no_token(self) -> None:
 class UsernamePickerTestCase(HomeserverTestCase):
  """Tests for the username picker flow of SSO login"""
 
- servlets = [login.register_servlets]
+ servlets = [
+ login.register_servlets,
+ profile.register_servlets,
+ account.register_servlets,
+ ]
+
+ def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer:
+ self.http_client = Mock(spec=["get_file"])
+ self.http_client.get_file.side_effect = mock_get_file
+ hs = self.setup_test_homeserver(
+ proxied_blocklisted_http_client=self.http_client
+ )
+ return hs
 
  def default_config(self) -> Dict[str, Any]:
  config = super().default_config()
@@ -1430,7 +1454,11 @@ def default_config(self) -> Dict[str, Any]:
  config["oidc_config"] = {}
  config["oidc_config"].update(TEST_OIDC_CONFIG)
  config["oidc_config"]["user_mapping_provider"] = {
- "config": {"display_name_template": "{{ user.displayname }}"}
+ "config": {
+ "display_name_template": "{{ user.displayname }}",
+ "email_template": "{{ user.email }}",
+ "picture_template": "{{ user.picture }}",
+ }
  }
 
  # whitelist this client URI so we redirect straight to it rather than
@@ -1443,15 +1471,22 @@ def create_resource_dict(self) -> Dict[str, Resource]:
  d.update(build_synapse_client_resource_tree(self.hs))
  return d
 
- def test_username_picker(self) -> None:
- """Test the happy path of a username picker flow."""
-
- fake_oidc_server = self.helper.fake_oidc_server()
-
+ def proceed_to_username_picker_page(
+ self,
+ fake_oidc_server: FakeOidcServer,
+ displayname: str,
+ email: str,
+ picture: str,
+ ) -> Tuple[str, str]:
  # do the start of the login flow
  channel, _ = self.helper.auth_via_oidc(
  fake_oidc_server,
- {"sub": "tester", "displayname": "Jonny"},
+ {
+ "sub": "tester",
+ "displayname": displayname,
+ "picture": picture,
+ "email": email,
+ },
  TEST_CLIENT_REDIRECT_URL,
  )
 
@@ -1478,16 +1513,132 @@ def test_username_picker(self) -> None:
  )
  session = username_mapping_sessions[session_id]
  self.assertEqual(session.remote_user_id, "tester")
- self.assertEqual(session.display_name, "Jonny")
+ self.assertEqual(session.display_name, displayname)
+ self.assertEqual(session.emails, [email])
+ self.assertEqual(session.avatar_url, picture)
  self.assertEqual(session.client_redirect_url, TEST_CLIENT_REDIRECT_URL)
 
  # the expiry time should be about 15 minutes away
  expected_expiry = self.clock.time_msec() + (15 * 60 * 1000)
  self.assertApproximates(session.expiry_time_ms, expected_expiry, tolerance=1000)
 
+ return picker_url, session_id
+
+ def test_username_picker_use_displayname_avatar_and_email(self) -> None:
+ """Test the happy path of a username picker flow with using displayname, avatar and email."""
+
+ fake_oidc_server = self.helper.fake_oidc_server()
+
+ mxid = "@bobby:test"
+ displayname = "Jonny"
+ email = "bobby@test.com"
+ picture = "mxc://test/avatar_url"
+
+ picker_url, session_id = self.proceed_to_username_picker_page(
+ fake_oidc_server, displayname, email, picture
+ )
+
+ # Now, submit a username to the username picker, which should serve a redirect
+ # to the completion page.
+ # Also specify that we should use the provided displayname, avatar and email.
+ content = urlencode(
+ {
+ b"username": b"bobby",
+ b"use_display_name": b"true",
+ b"use_avatar": b"true",
+ b"use_email": email,
+ }
+ ).encode("utf8")
+ chan = self.make_request(
+ "POST",
+ path=picker_url,
+ content=content,
+ content_is_form=True,
+ custom_headers=[
+ ("Cookie", "username_mapping_session=" + session_id),
+ # old versions of twisted don't do form-parsing without a valid
+ # content-length header.
+ ("Content-Length", str(len(content))),
+ ],
+ )
+ self.assertEqual(chan.code, 302, chan.result)
+ location_headers = chan.headers.getRawHeaders("Location")
+ assert location_headers
+
+ # send a request to the completion page, which should 302 to the client redirectUrl
+ chan = self.make_request(
+ "GET",
+ path=location_headers[0],
+ custom_headers=[("Cookie", "username_mapping_session=" + session_id)],
+ )
+ self.assertEqual(chan.code, 302, chan.result)
+ location_headers = chan.headers.getRawHeaders("Location")
+ assert location_headers
+
+ # ensure that the returned location matches the requested redirect URL
+ path, query = location_headers[0].split("?", 1)
+ self.assertEqual(path, "https://x")
+
+ # it will have url-encoded the params properly, so we'll have to parse them
+ params = urllib.parse.parse_qsl(
+ query, keep_blank_values=True, strict_parsing=True, errors="strict"
+ )
+ self.assertEqual(params[0:2], EXPECTED_CLIENT_REDIRECT_URL_PARAMS)
+ self.assertEqual(params[2][0], "loginToken")
+
+ # fish the login token out of the returned redirect uri
+ login_token = params[2][1]
+
+ # finally, submit the matrix login token to the login API, which gives us our
+ # matrix access token, mxid, and device id.
+ chan = self.make_request(
+ "POST",
+ "/login",
+ content={"type": "m.login.token", "token": login_token},
+ )
+ self.assertEqual(chan.code, 200, chan.result)
+ self.assertEqual(chan.json_body["user_id"], mxid)
+
+ # ensure the displayname and avatar from the OIDC response have been configured for the user.
+ channel = self.make_request(
+ "GET", "/profile/" + mxid, access_token=chan.json_body["access_token"]
+ )
+ self.assertEqual(channel.code, 200, channel.result)
+ self.assertIn("mxc://test", channel.json_body["avatar_url"])
+ self.assertEqual(displayname, channel.json_body["displayname"])
+
+ # ensure the email from the OIDC response has been configured for the user.
+ channel = self.make_request(
+ "GET", "/account/3pid", access_token=chan.json_body["access_token"]
+ )
+ self.assertEqual(channel.code, 200, channel.result)
+ self.assertEqual(email, channel.json_body["threepids"][0]["address"])
+
+ def test_username_picker_dont_use_displayname_avatar_or_email(self) -> None:
+ """Test the happy path of a username picker flow without using displayname, avatar or email."""
+
+ fake_oidc_server = self.helper.fake_oidc_server()
+
+ mxid = "@bobby:test"
+ displayname = "Jonny"
+ email = "bobby@test.com"
+ picture = "mxc://test/avatar_url"
+ username = "bobby"
+
+ picker_url, session_id = self.proceed_to_username_picker_page(
+ fake_oidc_server, displayname, email, picture
+ )
+
  # Now, submit a username to the username picker, which should serve a redirect
- # to the completion page
- content = urlencode({b"username": b"bobby"}).encode("utf8")
+ # to the completion page.
+ # Also specify that we should not use the provided displayname, avatar or email.
+ content = urlencode(
+ {
+ b"username": username,
+ b"use_display_name": b"false",
+ b"use_avatar": b"false",
+ }
+ ).encode("utf8")
  chan = self.make_request(
  "POST",
  path=picker_url,
@@ -1536,4 +1687,29 @@ def test_username_picker(self) -> None:
  content={"type": "m.login.token", "token": login_token},
  )
  self.assertEqual(chan.code, 200, chan.result)
- self.assertEqual(chan.json_body["user_id"], "@bobby:test")
+ self.assertEqual(chan.json_body["user_id"], mxid)
+
+ # ensure the displayname and avatar from the OIDC response have not been configured for the user.
+ channel = self.make_request(
+ "GET", "/profile/" + mxid, access_token=chan.json_body["access_token"]
+ )
+ self.assertEqual(channel.code, 200, channel.result)
+ self.assertNotIn("avatar_url", channel.json_body)
+ self.assertEqual(username, channel.json_body["displayname"])
+
+ # ensure the email from the OIDC response has not been configured for the user.
+ channel = self.make_request(
+ "GET", "/account/3pid", access_token=chan.json_body["access_token"]
+ )
+ self.assertEqual(channel.code, 200, channel.result)
+ self.assertListEqual([], channel.json_body["threepids"])
+
+
+async def mock_get_file(
+ url: str,
+ output_stream: BinaryIO,
+ max_size: Optional[int] = None,
+ headers: Optional[RawHeaders] = None,
+ is_allowed_content_type: Optional[Callable[[str], bool]] = None,
+) -> Tuple[int, Dict[bytes, List[bytes]], str, int]:
+ return 0, {b"Content-Type": [b"image/png"]}, "", 200