From 62a187b5705c0e9acedb7332279bdadc2ab1cf1b Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Thu, 24 Sep 2020 12:40:50 -0400
Subject: [PATCH 01/14] Allow passing additional OIDC parameters to the client.

---
 docs/sample_config.yaml          |  8 ++++
 docs/sso_mapping_providers.md    | 12 +++++-
 synapse/config/oidc_config.py    |  8 ++++
 synapse/handlers/auth.py         | 30 ++++++++++++-
 synapse/handlers/oidc_handler.py | 47 ++++++++++++++++++++-
 synapse/rest/client/v1/login.py  | 18 +++++---
 tests/handlers/test_oidc.py      | 72 ++++++++++++++++++++++++++++++--
 7 files changed, 183 insertions(+), 12 deletions(-)

diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml
index 845f53779530..8201770f0ac1 100644
--- a/docs/sample_config.yaml
+++ b/docs/sample_config.yaml
@@ -1735,6 +1735,14 @@ oidc_config:
       #
       #display_name_template: "{{ user.given_name }} {{ user.last_name }}"
 
+      # Jinja2 templates for extra attributes to send back to the client during
+      # login.
+      #
+      # Note that these are non-standard and clients will ignore them without modifications.
+      #
+      #extra_attributes:
+        #birthdate: "{ user.birthdate }"
+
 
 
 # Enable CAS for registration and login.
diff --git a/docs/sso_mapping_providers.md b/docs/sso_mapping_providers.md
index abea432343f7..35961a83aec0 100644
--- a/docs/sso_mapping_providers.md
+++ b/docs/sso_mapping_providers.md
@@ -57,7 +57,7 @@ A custom mapping provider must specify the following methods:
     - This method must return a string, which is the unique identifier for the
       user. Commonly the ``sub`` claim of the response.
 * `map_user_attributes(self, userinfo, token)`
-    - This method should be async.
+    - This method must be async.
     - Arguments:
       - `userinfo` - A `authlib.oidc.core.claims.UserInfo` object to extract user
                      information from.
@@ -66,6 +66,16 @@ A custom mapping provider must specify the following methods:
     - Returns a dictionary with two keys:
       - localpart: A required string, used to generate the Matrix ID.
       - displayname: An optional string, the display name for the user.
+* `get_extra_attributes(self, userinfo, token)`
+    - This method must be async.
+    - Arguments:
+      - `userinfo` - A `authlib.oidc.core.claims.UserInfo` object to extract user
+                     information from.
+      - `token` - A dictionary which includes information necessary to make
+                  further requests to the OpenID provider.
+    - Returns a dictionary that is suitable to be serialized to JSON. This
+      dictionary will be returned under the `extra_attributes` key in the
+      response during a successful login.
 
 ### Default OpenID Mapping Provider
 
diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py
index 70fc8a2f6268..e29d6fdfb203 100644
--- a/synapse/config/oidc_config.py
+++ b/synapse/config/oidc_config.py
@@ -204,6 +204,14 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
               # If unset, no displayname will be set.
               #
               #display_name_template: "{{{{ user.given_name }}}} {{{{ user.last_name }}}}"
+
+              # Jinja2 templates for extra attributes to send back to the client during
+              # login.
+              #
+              # Note that these are non-standard and clients will ignore them without modifications.
+              #
+              #extra_attributes:
+                #birthdate: "{{ user.birthdate }}"
         """.format(
             mapping_provider=DEFAULT_USER_MAPPING_PROVIDER
         )
diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index 0322b60cfc63..77fe333716ba 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -239,6 +239,10 @@ def __init__(self, hs):
         # cast to tuple for use with str.startswith
         self._whitelisted_sso_clients = tuple(hs.config.sso_client_whitelist)
 
+        # A mapping of user ID to extra attributes to include in the login
+        # response.
+        self._extra_attributes = {}  # type: Dict[str, Dict[str, Any]]
+
     async def validate_user_via_ui_auth(
         self,
         requester: Requester,
@@ -1165,6 +1169,7 @@ async def complete_sso_login(
         registered_user_id: str,
         request: SynapseRequest,
         client_redirect_url: str,
+        extra_attributes: Optional[JsonDict] = None,
     ):
         """Having figured out a mxid for this user, complete the HTTP request
 
@@ -1173,6 +1178,8 @@ async def complete_sso_login(
             request: The request to complete.
             client_redirect_url: The URL to which to redirect the user at the end of the
                 process.
+            extra_attributes: Extra attributes which will be passed to the client
+                during successful login. Must be JSON serializable.
         """
         # If the account has been deactivated, do not proceed with the login
         # flow.
@@ -1181,19 +1188,26 @@ async def complete_sso_login(
             respond_with_html(request, 403, self._sso_account_deactivated_template)
             return
 
-        self._complete_sso_login(registered_user_id, request, client_redirect_url)
+        self._complete_sso_login(
+            registered_user_id, request, client_redirect_url, extra_attributes
+        )
 
     def _complete_sso_login(
         self,
         registered_user_id: str,
         request: SynapseRequest,
         client_redirect_url: str,
+        extra_attributes: Optional[JsonDict] = None,
     ):
         """
         The synchronous portion of complete_sso_login.
 
         This exists purely for backwards compatibility of synapse.module_api.ModuleApi.
         """
+        # Store any extra attributes which will be passed in the login response.
+        if extra_attributes:
+            self._extra_attributes[registered_user_id] = extra_attributes
+
         # Create a login token
         login_token = self.macaroon_gen.generate_short_term_login_token(
             registered_user_id
@@ -1226,6 +1240,20 @@ def _complete_sso_login(
         )
         respond_with_html(request, 200, html)
 
+    async def _sso_login_callback(self, login_result: JsonDict) -> None:
+        """
+        A login callback which might add additional attributes to the login response.
+
+        Args:
+            login_result: The data to be sent to the client. Includes the user
+                ID and access token.
+        """
+        extra_attributes = self._extra_attributes.get(login_result["user_id"])
+        if extra_attributes:
+            login_result.update(
+                (("extra_attributes", extra_attributes.extra_attributes),)
+            )
+
     @staticmethod
     def add_query_param_to_url(url: str, param_name: str, param: Any):
         url_parts = list(urllib.parse.urlparse(url))
diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index 0e06e4408d3b..e1e1cf8e7073 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -37,7 +37,7 @@
 from synapse.http.server import respond_with_html
 from synapse.http.site import SynapseRequest
 from synapse.logging.context import make_deferred_yieldable
-from synapse.types import UserID, map_username_to_mxid_localpart
+from synapse.types import JsonDict, UserID, map_username_to_mxid_localpart
 from synapse.util import json_decoder
 
 if TYPE_CHECKING:
@@ -707,6 +707,18 @@ async def handle_oidc_callback(self, request: SynapseRequest) -> None:
             self._render_error(request, "mapping_error", str(e))
             return
 
+        # Mapping providers might not have get_extra_attributes: only call this
+        # method if it exists. It might also raise NotImplementedError.
+        extra_attributes = None
+        get_extra_attributes = getattr(
+            self._user_mapping_provider, "get_extra_attributes"
+        )
+        if get_extra_attributes:
+            try:
+                extra_attributes = await get_extra_attributes(userinfo, token)
+            except NotImplementedError:
+                pass
+
         # and finally complete the login
         if ui_auth_session_id:
             await self._auth_handler.complete_sso_ui_auth(
@@ -714,7 +726,7 @@ async def handle_oidc_callback(self, request: SynapseRequest) -> None:
             )
         else:
             await self._auth_handler.complete_sso_login(
-                user_id, request, client_redirect_url
+                user_id, request, client_redirect_url, extra_attributes
             )
 
     def _generate_oidc_session_token(
@@ -995,6 +1007,18 @@ async def map_user_attributes(
         """
         raise NotImplementedError()
 
+    async def get_extra_attributes(self, userinfo: UserInfo, token: Token) -> JsonDict:
+        """Map a ``UserInfo`` objects into additional attributes passed to the client during login.
+
+        Args:
+            userinfo: An object representing the user given by the OIDC provider
+            token: A dict with the tokens returned by the provider
+
+        Returns:
+            A dict containing additional attributes. Must be JSON serializable.
+        """
+        raise NotImplementedError()
+
 
 # Used to clear out "None" values in templates
 def jinja_finalize(thing):
@@ -1009,6 +1033,7 @@ class JinjaOidcMappingConfig:
     subject_claim = attr.ib()  # type: str
     localpart_template = attr.ib()  # type: Template
     display_name_template = attr.ib()  # type: Optional[Template]
+    extra_attributes = attr.ib()  # type: Dict[str, Template]
 
 
 class JinjaOidcMappingProvider(OidcMappingProvider[JinjaOidcMappingConfig]):
@@ -1047,10 +1072,22 @@ def parse_config(config: dict) -> JinjaOidcMappingConfig:
                     % (e,)
                 )
 
+        extra_attributes = {}  # type Dict[str, Template]
+        if "extra_attributes" in config:
+            for key, value in config["extra_attributes"].items():
+                try:
+                    extra_attributes[key] = env.from_string(value)
+                except Exception as e:
+                    raise ConfigError(
+                        "invalid jinja template for oidc_config.user_mapping_provider.config.extra_attributes.%s: %r"
+                        % (key, e,)
+                    )
+
         return JinjaOidcMappingConfig(
             subject_claim=subject_claim,
             localpart_template=localpart_template,
             display_name_template=display_name_template,
+            extra_attributes=extra_attributes,
         )
 
     def get_remote_user_id(self, userinfo: UserInfo) -> str:
@@ -1071,3 +1108,9 @@ async def map_user_attributes(
                 display_name = None
 
         return UserAttribute(localpart=localpart, display_name=display_name)
+
+    async def get_extra_attributes(self, userinfo: UserInfo, token: Token) -> JsonDict:
+        extras = {}  # type: Dict[str, str]
+        for key, template in self._config.extra_attributes.items():
+            extras[key] = template.render(user=userinfo).strip()
+        return extras
diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py
index 250b03a02536..6fa429675686 100644
--- a/synapse/rest/client/v1/login.py
+++ b/synapse/rest/client/v1/login.py
@@ -284,9 +284,7 @@ async def _complete_login(
         self,
         user_id: str,
         login_submission: JsonDict,
-        callback: Optional[
-            Callable[[Dict[str, str]], Awaitable[Dict[str, str]]]
-        ] = None,
+        callback: Optional[Callable[[Dict[str, str]], Awaitable[None]]] = None,
         create_non_existent_users: bool = False,
     ) -> Dict[str, str]:
         """Called when we've successfully authed the user and now need to
@@ -339,14 +337,24 @@ async def _complete_login(
         return result
 
     async def _do_token_login(self, login_submission: JsonDict) -> Dict[str, str]:
+        """
+        Handle the final stage of SSO login.
+
+        Args:
+             login_submission: The JSON request body.
+
+        Returns:
+            The body of the JSON response.
+        """
         token = login_submission["token"]
         auth_handler = self.auth_handler
         user_id = await auth_handler.validate_short_term_login_token_and_get_user_id(
             token
         )
 
-        result = await self._complete_login(user_id, login_submission)
-        return result
+        return await self._complete_login(
+            user_id, login_submission, self.auth_handler._sso_login_callback
+        )
 
     async def _do_jwt_login(self, login_submission: JsonDict) -> Dict[str, str]:
         token = login_submission.get("token", None)
diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index 5910772aa8d5..319eb4a46e7a 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -87,6 +87,13 @@ def get_remote_user_id(self, userinfo):
     async def map_user_attributes(self, userinfo, token):
         return {"localpart": userinfo["username"], "display_name": None}
 
+    # Do not include get_extra_attributes to test backwards compatibility paths.
+
+
+class TestMappingProviderExtra(TestMappingProvider):
+    async def get_extra_attributes(self, userinfo, token):
+        return {"phone": userinfo["phone"]}
+
 
 def simple_async_mock(return_value=None, raises=None):
     # AsyncMock is not available in python3.5, this mimics part of its behaviour
@@ -377,7 +384,7 @@ def test_callback(self):
             "sub": "foo",
             "preferred_username": "bar",
         }
-        user_id = UserID("foo", "domain.org")
+        user_id = "@foo:domain.org"
         self.handler._render_error = Mock(return_value=None)
         self.handler._exchange_code = simple_async_mock(return_value=token)
         self.handler._parse_id_token = simple_async_mock(return_value=userinfo)
@@ -413,7 +420,7 @@ def test_callback(self):
         yield defer.ensureDeferred(self.handler.handle_oidc_callback(request))
 
         self.handler._auth_handler.complete_sso_login.assert_called_once_with(
-            user_id, request, client_redirect_url,
+            user_id, request, client_redirect_url, None,
         )
         self.handler._exchange_code.assert_called_once_with(code)
         self.handler._parse_id_token.assert_called_once_with(token, nonce=nonce)
@@ -447,7 +454,7 @@ def test_callback(self):
         yield defer.ensureDeferred(self.handler.handle_oidc_callback(request))
 
         self.handler._auth_handler.complete_sso_login.assert_called_once_with(
-            user_id, request, client_redirect_url,
+            user_id, request, client_redirect_url, None,
         )
         self.handler._exchange_code.assert_called_once_with(code)
         self.handler._parse_id_token.assert_not_called()
@@ -591,6 +598,65 @@ def test_exchange_code(self):
             yield defer.ensureDeferred(self.handler._exchange_code(code))
         self.assertEqual(exc.exception.error, "some_error")
 
+    @defer.inlineCallbacks
+    def test_extra_attributes(self):
+        """
+        Login while using a mapping provider that implements get_extra_attributes.
+        """
+        # We cannot just override the configuration since the mapping provider
+        # gets stored on the handler.
+        self.handler._user_mapping_provider = TestMappingProviderExtra(
+            self.hs.config.oidc_user_mapping_provider_config
+        )
+
+        token = {
+            "type": "bearer",
+            "id_token": "id_token",
+            "access_token": "access_token",
+        }
+        userinfo = {
+            "sub": "foo",
+            "phone": "1234567",
+        }
+        user_id = "@foo:domain.org"
+        self.handler._render_error = Mock(return_value=None)
+        self.handler._exchange_code = simple_async_mock(return_value=token)
+        self.handler._parse_id_token = simple_async_mock(return_value=userinfo)
+        self.handler._fetch_userinfo = simple_async_mock(return_value=userinfo)
+        self.handler._map_userinfo_to_user = simple_async_mock(return_value=user_id)
+        self.handler._auth_handler.complete_sso_login = simple_async_mock()
+        request = Mock(
+            spec=["args", "getCookie", "addCookie", "requestHeaders", "getClientIP"]
+        )
+
+        code = "code"
+        state = "state"
+        nonce = "nonce"
+        client_redirect_url = "http://client/redirect"
+        user_agent = "Browser"
+        ip_address = "10.0.0.1"
+        session = self.handler._generate_oidc_session_token(
+            state=state,
+            nonce=nonce,
+            client_redirect_url=client_redirect_url,
+            ui_auth_session_id=None,
+        )
+        request.getCookie.return_value = session
+
+        request.args = {}
+        request.args[b"code"] = [code.encode("utf-8")]
+        request.args[b"state"] = [state.encode("utf-8")]
+
+        request.requestHeaders = Mock(spec=["getRawHeaders"])
+        request.requestHeaders.getRawHeaders.return_value = [user_agent.encode("ascii")]
+        request.getClientIP.return_value = ip_address
+
+        yield defer.ensureDeferred(self.handler.handle_oidc_callback(request))
+
+        self.handler._auth_handler.complete_sso_login.assert_called_once_with(
+            user_id, request, client_redirect_url, {"phone": "1234567"},
+        )
+
     def test_map_userinfo_to_user(self):
         """Ensure that mapping the userinfo returned from a provider to an MXID works properly."""
         userinfo = {

From f6b3a0bd97bbf4fe85111343ef4d7a5c9354b6de Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Fri, 25 Sep 2020 13:44:20 -0400
Subject: [PATCH 02/14] Expire the extra attributes when the login token  has
 expired.

---
 synapse/handlers/auth.py | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index 77fe333716ba..490b93c4156c 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -137,6 +137,15 @@ def login_id_phone_to_thirdparty(identifier: JsonDict) -> Dict[str, str]:
     }
 
 
+@attr.s(slots=True)
+class SsoLoginExtraAttributes:
+    """Data we track about SAML2 sessions"""
+
+    # time the session was created, in milliseconds
+    creation_time = attr.ib(type=int)
+    extra_attributes = attr.ib(type=JsonDict)
+
+
 class AuthHandler(BaseHandler):
     SESSION_EXPIRE_MS = 48 * 60 * 60 * 1000
 
@@ -241,7 +250,7 @@ def __init__(self, hs):
 
         # A mapping of user ID to extra attributes to include in the login
         # response.
-        self._extra_attributes = {}  # type: Dict[str, Dict[str, Any]]
+        self._extra_attributes = {}  # type: Dict[str, SsoLoginExtraAttributes]
 
     async def validate_user_via_ui_auth(
         self,
@@ -1205,8 +1214,12 @@ def _complete_sso_login(
         This exists purely for backwards compatibility of synapse.module_api.ModuleApi.
         """
         # Store any extra attributes which will be passed in the login response.
+        # Note that this is per-user so it may overwrite a previous value, this
+        # is considered OK since the newest SSO attributes should be most valid.
         if extra_attributes:
-            self._extra_attributes[registered_user_id] = extra_attributes
+            self._extra_attributes[registered_user_id] = SsoLoginExtraAttributes(
+                self._clock.time_msec(), extra_attributes,
+            )
 
         # Create a login token
         login_token = self.macaroon_gen.generate_short_term_login_token(
@@ -1248,12 +1261,31 @@ async def _sso_login_callback(self, login_result: JsonDict) -> None:
             login_result: The data to be sent to the client. Includes the user
                 ID and access token.
         """
+        # Expire attributes before processing. Note that there shouldn't be any
+        # valid logins that still have extra attributes.
+        self._expire_sso_extra_attributes()
+
         extra_attributes = self._extra_attributes.get(login_result["user_id"])
         if extra_attributes:
             login_result.update(
                 (("extra_attributes", extra_attributes.extra_attributes),)
             )
 
+    def _expire_sso_extra_attributes(self) -> None:
+        """
+        Iterate through the mapping of user IDs to extra attributes and remove any that are no longer valid.
+        """
+        # TODO This should match the amount of time the macaroon is valid for.
+        LOGIN_TOKEN_EXPIRATION_TIME = 2 * 60 * 1000
+        expire_before = self._clock.time_msec() - LOGIN_TOKEN_EXPIRATION_TIME
+        to_expire = set()
+        for user_id, data in self._extra_attributes.items():
+            if data.creation_time < expire_before:
+                to_expire.add(user_id)
+        for user_id in to_expire:
+            logger.debug("Expiring extra attributes for user %s", user_id)
+            del self._extra_attributes[user_id]
+
     @staticmethod
     def add_query_param_to_url(url: str, param_name: str, param: Any):
         url_parts = list(urllib.parse.urlparse(url))

From 0065b5969cefb55bab8e6b9aa5b5e7e9b83b9edf Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Mon, 28 Sep 2020 09:46:54 -0400
Subject: [PATCH 03/14] Add a changelog.

---
 changelog.d/8413.feature | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 changelog.d/8413.feature

diff --git a/changelog.d/8413.feature b/changelog.d/8413.feature
new file mode 100644
index 000000000000..abe40a901cf6
--- /dev/null
+++ b/changelog.d/8413.feature
@@ -0,0 +1 @@
+Support passing additional single sign-on parameters to the client.

From 3d5733b4f1120f95e6685d4802ca9f2b5ac7cf49 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Wed, 30 Sep 2020 07:11:08 -0400
Subject: [PATCH 04/14] Remove some unnecessary mocks.

---
 tests/handlers/test_oidc.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index 319eb4a46e7a..534773150f21 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -619,10 +619,8 @@ def test_extra_attributes(self):
             "phone": "1234567",
         }
         user_id = "@foo:domain.org"
-        self.handler._render_error = Mock(return_value=None)
         self.handler._exchange_code = simple_async_mock(return_value=token)
         self.handler._parse_id_token = simple_async_mock(return_value=userinfo)
-        self.handler._fetch_userinfo = simple_async_mock(return_value=userinfo)
         self.handler._map_userinfo_to_user = simple_async_mock(return_value=user_id)
         self.handler._auth_handler.complete_sso_login = simple_async_mock()
         request = Mock(

From a91fdae606fe522201e57388b12a7384da2b48c0 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Wed, 30 Sep 2020 07:31:14 -0400
Subject: [PATCH 05/14] Add notes about worker config.

---
 docs/workers.md | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/docs/workers.md b/docs/workers.md
index df0ac84d9466..ad4d8ca9f25a 100644
--- a/docs/workers.md
+++ b/docs/workers.md
@@ -243,6 +243,22 @@ for the room are in flight:
 
     ^/_matrix/client/(api/v1|r0|unstable)/rooms/.*/messages$
 
+Additionally, the following endpoints should be included if Synapse is configured
+to use SSO (you only need to include the ones for whichever SSO provider you're
+using):
+
+    # OpenID Connect requests.
+    ^/_matrix/client/(api/v1|r0|unstable)/login/sso/redirect$
+    ^/_synapse/oidc/callback$
+
+    # SAML requests.
+    ^/_matrix/client/(api/v1|r0|unstable)/login/sso/redirect$
+    ^/_matrix/saml2/authn_response$
+
+    # CAS requests.
+    ^/_matrix/client/(api/v1|r0|unstable)/login/(cas|sso)/redirect$
+    ^/_matrix/client/(api/v1|r0|unstable)/login/cas/ticket$
+
 Note that a HTTP listener with `client` and `federation` resources must be
 configured in the `worker_listeners` option in the worker config.
 

From 28acff73da2188ad1b6613d9df72d6f78b3742d5 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Wed, 30 Sep 2020 11:21:23 -0400
Subject: [PATCH 06/14] Do not namespace the returned values.

---
 docs/sso_mapping_providers.md | 6 ++++--
 synapse/handlers/auth.py      | 4 +---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/docs/sso_mapping_providers.md b/docs/sso_mapping_providers.md
index 35961a83aec0..c3858d7420df 100644
--- a/docs/sso_mapping_providers.md
+++ b/docs/sso_mapping_providers.md
@@ -74,8 +74,10 @@ A custom mapping provider must specify the following methods:
       - `token` - A dictionary which includes information necessary to make
                   further requests to the OpenID provider.
     - Returns a dictionary that is suitable to be serialized to JSON. This
-      dictionary will be returned under the `extra_attributes` key in the
-      response during a successful login.
+      will be returned as part of the response during a successful login.
+      
+      Note that care should be taken to not overwrite any of the parameters
+      usually returned as part of the [login response](https://matrix.org/docs/spec/client_server/latest#post-matrix-client-r0-login).
 
 ### Default OpenID Mapping Provider
 
diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index 490b93c4156c..00eae9205267 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -1267,9 +1267,7 @@ async def _sso_login_callback(self, login_result: JsonDict) -> None:
 
         extra_attributes = self._extra_attributes.get(login_result["user_id"])
         if extra_attributes:
-            login_result.update(
-                (("extra_attributes", extra_attributes.extra_attributes),)
-            )
+            login_result.update(extra_attributes.extra_attributes)
 
     def _expire_sso_extra_attributes(self) -> None:
         """

From 02392c65549f518dcdd89fad3243f727a5e57a6f Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Wed, 30 Sep 2020 11:21:44 -0400
Subject: [PATCH 07/14] Fix jinja formatting, comments, getattr call.

---
 docs/sso_mapping_providers.md    | 2 +-
 synapse/config/oidc_config.py    | 2 +-
 synapse/handlers/oidc_handler.py | 8 ++++----
 synapse/rest/client/v1/login.py  | 4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/docs/sso_mapping_providers.md b/docs/sso_mapping_providers.md
index c3858d7420df..32b06aa2c570 100644
--- a/docs/sso_mapping_providers.md
+++ b/docs/sso_mapping_providers.md
@@ -75,7 +75,7 @@ A custom mapping provider must specify the following methods:
                   further requests to the OpenID provider.
     - Returns a dictionary that is suitable to be serialized to JSON. This
       will be returned as part of the response during a successful login.
-      
+
       Note that care should be taken to not overwrite any of the parameters
       usually returned as part of the [login response](https://matrix.org/docs/spec/client_server/latest#post-matrix-client-r0-login).
 
diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py
index e29d6fdfb203..f92411681999 100644
--- a/synapse/config/oidc_config.py
+++ b/synapse/config/oidc_config.py
@@ -211,7 +211,7 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
               # Note that these are non-standard and clients will ignore them without modifications.
               #
               #extra_attributes:
-                #birthdate: "{{ user.birthdate }}"
+                #birthdate: "{{{{ user.birthdate }}}}"
         """.format(
             mapping_provider=DEFAULT_USER_MAPPING_PROVIDER
         )
diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index e1e1cf8e7073..b68d499c94bd 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -711,7 +711,7 @@ async def handle_oidc_callback(self, request: SynapseRequest) -> None:
         # method if it exists. It might also raise NotImplementedError.
         extra_attributes = None
         get_extra_attributes = getattr(
-            self._user_mapping_provider, "get_extra_attributes"
+            self._user_mapping_provider, "get_extra_attributes", None
         )
         if get_extra_attributes:
             try:
@@ -996,7 +996,7 @@ def get_remote_user_id(self, userinfo: UserInfo) -> str:
     async def map_user_attributes(
         self, userinfo: UserInfo, token: Token
     ) -> UserAttribute:
-        """Map a ``UserInfo`` objects into user attributes.
+        """Map a `UserInfo` object into user attributes.
 
         Args:
             userinfo: An object representing the user given by the OIDC provider
@@ -1008,7 +1008,7 @@ async def map_user_attributes(
         raise NotImplementedError()
 
     async def get_extra_attributes(self, userinfo: UserInfo, token: Token) -> JsonDict:
-        """Map a ``UserInfo`` objects into additional attributes passed to the client during login.
+        """Map a `UserInfo` object into additional attributes passed to the client during login.
 
         Args:
             userinfo: An object representing the user given by the OIDC provider
@@ -1080,7 +1080,7 @@ def parse_config(config: dict) -> JinjaOidcMappingConfig:
                 except Exception as e:
                     raise ConfigError(
                         "invalid jinja template for oidc_config.user_mapping_provider.config.extra_attributes.%s: %r"
-                        % (key, e,)
+                        % (key, e)
                     )
 
         return JinjaOidcMappingConfig(
diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py
index 6fa429675686..b9347b87c7c6 100644
--- a/synapse/rest/client/v1/login.py
+++ b/synapse/rest/client/v1/login.py
@@ -297,12 +297,12 @@ async def _complete_login(
         Args:
             user_id: ID of the user to register.
             login_submission: Dictionary of login information.
-            callback: Callback function to run after registration.
+            callback: Callback function to run after login.
             create_non_existent_users: Whether to create the user if they don't
                 exist. Defaults to False.
 
         Returns:
-            result: Dictionary of account information after successful registration.
+            result: Dictionary of account information after successful login.
         """
 
         # Before we actually log them in we check if they've already logged in

From f5e268b9abb4c42a40298bbe700a465a33c6290b Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Wed, 30 Sep 2020 11:37:46 -0400
Subject: [PATCH 08/14] Add better error handling of config values.

---
 synapse/handlers/oidc_handler.py | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index b68d499c94bd..e1eb724e9d56 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -1074,7 +1074,13 @@ def parse_config(config: dict) -> JinjaOidcMappingConfig:
 
         extra_attributes = {}  # type Dict[str, Template]
         if "extra_attributes" in config:
-            for key, value in config["extra_attributes"].items():
+            extra_attributes_config = config.get("extra_attributes") or {}
+            if not isinstance(extra_attributes_config, dict):
+                raise ConfigError(
+                    "oidc_config.user_mapping_provider.config.extra_attributes must be a dict"
+                )
+
+            for key, value in extra_attributes_config.items():
                 try:
                     extra_attributes[key] = env.from_string(value)
                 except Exception as e:

From 8f7236e348f33d3e524accf42cfd34cbdfa03f0d Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Wed, 30 Sep 2020 11:46:19 -0400
Subject: [PATCH 09/14] Stop raising NotImplementedError.

---
 synapse/handlers/oidc_handler.py | 9 +++------
 tests/handlers/test_oidc.py      | 4 ++--
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index e1eb724e9d56..8b6c57b6ea67 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -708,16 +708,13 @@ async def handle_oidc_callback(self, request: SynapseRequest) -> None:
             return
 
         # Mapping providers might not have get_extra_attributes: only call this
-        # method if it exists. It might also raise NotImplementedError.
+        # method if it exists.
         extra_attributes = None
         get_extra_attributes = getattr(
             self._user_mapping_provider, "get_extra_attributes", None
         )
         if get_extra_attributes:
-            try:
-                extra_attributes = await get_extra_attributes(userinfo, token)
-            except NotImplementedError:
-                pass
+            extra_attributes = await get_extra_attributes(userinfo, token)
 
         # and finally complete the login
         if ui_auth_session_id:
@@ -1017,7 +1014,7 @@ async def get_extra_attributes(self, userinfo: UserInfo, token: Token) -> JsonDi
         Returns:
             A dict containing additional attributes. Must be JSON serializable.
         """
-        raise NotImplementedError()
+        return {}
 
 
 # Used to clear out "None" values in templates
diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index 534773150f21..61d1afeb6f7f 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -420,7 +420,7 @@ def test_callback(self):
         yield defer.ensureDeferred(self.handler.handle_oidc_callback(request))
 
         self.handler._auth_handler.complete_sso_login.assert_called_once_with(
-            user_id, request, client_redirect_url, None,
+            user_id, request, client_redirect_url, {},
         )
         self.handler._exchange_code.assert_called_once_with(code)
         self.handler._parse_id_token.assert_called_once_with(token, nonce=nonce)
@@ -454,7 +454,7 @@ def test_callback(self):
         yield defer.ensureDeferred(self.handler.handle_oidc_callback(request))
 
         self.handler._auth_handler.complete_sso_login.assert_called_once_with(
-            user_id, request, client_redirect_url, None,
+            user_id, request, client_redirect_url, {},
         )
         self.handler._exchange_code.assert_called_once_with(code)
         self.handler._parse_id_token.assert_not_called()

From e0c126f99888865c33d8f5322160524f43403f95 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Wed, 30 Sep 2020 11:46:31 -0400
Subject: [PATCH 10/14] Inline values which are used only once in tests.

---
 tests/handlers/test_oidc.py | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index 61d1afeb6f7f..bf9a9f5a163d 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -401,13 +401,12 @@ def test_callback(self):
         client_redirect_url = "http://client/redirect"
         user_agent = "Browser"
         ip_address = "10.0.0.1"
-        session = self.handler._generate_oidc_session_token(
+        request.getCookie.return_value = self.handler._generate_oidc_session_token(
             state=state,
             nonce=nonce,
             client_redirect_url=client_redirect_url,
             ui_auth_session_id=None,
         )
-        request.getCookie.return_value = session
 
         request.args = {}
         request.args[b"code"] = [code.encode("utf-8")]
@@ -627,27 +626,22 @@ def test_extra_attributes(self):
             spec=["args", "getCookie", "addCookie", "requestHeaders", "getClientIP"]
         )
 
-        code = "code"
         state = "state"
-        nonce = "nonce"
         client_redirect_url = "http://client/redirect"
-        user_agent = "Browser"
-        ip_address = "10.0.0.1"
-        session = self.handler._generate_oidc_session_token(
+        request.getCookie.return_value = self.handler._generate_oidc_session_token(
             state=state,
-            nonce=nonce,
+            nonce="nonce",
             client_redirect_url=client_redirect_url,
             ui_auth_session_id=None,
         )
-        request.getCookie.return_value = session
 
         request.args = {}
-        request.args[b"code"] = [code.encode("utf-8")]
+        request.args[b"code"] = [b"code"]
         request.args[b"state"] = [state.encode("utf-8")]
 
         request.requestHeaders = Mock(spec=["getRawHeaders"])
-        request.requestHeaders.getRawHeaders.return_value = [user_agent.encode("ascii")]
-        request.getClientIP.return_value = ip_address
+        request.requestHeaders.getRawHeaders.return_value = [b"Browser"]
+        request.getClientIP.return_value = "10.0.0.1"
 
         yield defer.ensureDeferred(self.handler.handle_oidc_callback(request))
 

From 3c3e84e964fe3cf016134f9a4b68f7ba45b81d0e Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Wed, 30 Sep 2020 12:06:20 -0400
Subject: [PATCH 11/14] Convert tests away from inlineCallbacks.

---
 tests/handlers/test_oidc.py | 87 +++++++++++++++----------------------
 1 file changed, 36 insertions(+), 51 deletions(-)

diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index bf9a9f5a163d..3e2e2c5c55dc 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -21,7 +21,6 @@
 import attr
 import pymacaroons
 
-from twisted.internet import defer
 from twisted.python.failure import Failure
 from twisted.web._newclient import ResponseDone
 
@@ -172,11 +171,10 @@ def test_config(self):
         self.assertEqual(self.handler._client_auth.client_secret, CLIENT_SECRET)
 
     @override_config({"oidc_config": {"discover": True}})
-    @defer.inlineCallbacks
     def test_discovery(self):
         """The handler should discover the endpoints from OIDC discovery document."""
         # This would throw if some metadata were invalid
-        metadata = yield defer.ensureDeferred(self.handler.load_metadata())
+        metadata = self.get_success(self.handler.load_metadata())
         self.http_client.get_json.assert_called_once_with(WELL_KNOWN)
 
         self.assertEqual(metadata.issuer, ISSUER)
@@ -188,43 +186,40 @@ def test_discovery(self):
 
         # subsequent calls should be cached
         self.http_client.reset_mock()
-        yield defer.ensureDeferred(self.handler.load_metadata())
+        self.get_success(self.handler.load_metadata())
         self.http_client.get_json.assert_not_called()
 
     @override_config({"oidc_config": COMMON_CONFIG})
-    @defer.inlineCallbacks
     def test_no_discovery(self):
         """When discovery is disabled, it should not try to load from discovery document."""
-        yield defer.ensureDeferred(self.handler.load_metadata())
+        self.get_success(self.handler.load_metadata())
         self.http_client.get_json.assert_not_called()
 
     @override_config({"oidc_config": COMMON_CONFIG})
-    @defer.inlineCallbacks
     def test_load_jwks(self):
         """JWKS loading is done once (then cached) if used."""
-        jwks = yield defer.ensureDeferred(self.handler.load_jwks())
+        jwks = self.get_success(self.handler.load_jwks())
         self.http_client.get_json.assert_called_once_with(JWKS_URI)
         self.assertEqual(jwks, {"keys": []})
 
         # subsequent calls should be cached…
         self.http_client.reset_mock()
-        yield defer.ensureDeferred(self.handler.load_jwks())
+        self.get_success(self.handler.load_jwks())
         self.http_client.get_json.assert_not_called()
 
         # …unless forced
         self.http_client.reset_mock()
-        yield defer.ensureDeferred(self.handler.load_jwks(force=True))
+        self.get_success(self.handler.load_jwks(force=True))
         self.http_client.get_json.assert_called_once_with(JWKS_URI)
 
         # Throw if the JWKS uri is missing
         with self.metadata_edit({"jwks_uri": None}):
-            with self.assertRaises(RuntimeError):
-                yield defer.ensureDeferred(self.handler.load_jwks(force=True))
+            self.get_failure(self.handler.load_jwks(force=True), RuntimeError)
 
         # Return empty key set if JWKS are not used
         self.handler._scopes = []  # not asking the openid scope
         self.http_client.get_json.reset_mock()
-        jwks = yield defer.ensureDeferred(self.handler.load_jwks(force=True))
+        jwks = self.get_success(self.handler.load_jwks(force=True))
         self.http_client.get_json.assert_not_called()
         self.assertEqual(jwks, {"keys": []})
 
@@ -306,11 +301,10 @@ def test_skip_verification(self):
             # This should not throw
             self.handler._validate_metadata()
 
-    @defer.inlineCallbacks
     def test_redirect_request(self):
         """The redirect request has the right arguments & generates a valid session cookie."""
         req = Mock(spec=["addCookie"])
-        url = yield defer.ensureDeferred(
+        url = self.get_success(
             self.handler.handle_redirect_request(req, b"http://client/redirect")
         )
         url = urlparse(url)
@@ -350,20 +344,18 @@ def test_redirect_request(self):
         self.assertEqual(params["nonce"], [nonce])
         self.assertEqual(redirect, "http://client/redirect")
 
-    @defer.inlineCallbacks
     def test_callback_error(self):
         """Errors from the provider returned in the callback are displayed."""
         self.handler._render_error = Mock()
         request = Mock(args={})
         request.args[b"error"] = [b"invalid_client"]
-        yield defer.ensureDeferred(self.handler.handle_oidc_callback(request))
+        self.get_success(self.handler.handle_oidc_callback(request))
         self.assertRenderedError("invalid_client", "")
 
         request.args[b"error_description"] = [b"some description"]
-        yield defer.ensureDeferred(self.handler.handle_oidc_callback(request))
+        self.get_success(self.handler.handle_oidc_callback(request))
         self.assertRenderedError("invalid_client", "some description")
 
-    @defer.inlineCallbacks
     def test_callback(self):
         """Code callback works and display errors if something went wrong.
 
@@ -416,7 +408,7 @@ def test_callback(self):
         request.requestHeaders.getRawHeaders.return_value = [user_agent.encode("ascii")]
         request.getClientIP.return_value = ip_address
 
-        yield defer.ensureDeferred(self.handler.handle_oidc_callback(request))
+        self.get_success(self.handler.handle_oidc_callback(request))
 
         self.handler._auth_handler.complete_sso_login.assert_called_once_with(
             user_id, request, client_redirect_url, {},
@@ -433,13 +425,13 @@ def test_callback(self):
         self.handler._map_userinfo_to_user = simple_async_mock(
             raises=MappingException()
         )
-        yield defer.ensureDeferred(self.handler.handle_oidc_callback(request))
+        self.get_success(self.handler.handle_oidc_callback(request))
         self.assertRenderedError("mapping_error")
         self.handler._map_userinfo_to_user = simple_async_mock(return_value=user_id)
 
         # Handle ID token errors
         self.handler._parse_id_token = simple_async_mock(raises=Exception())
-        yield defer.ensureDeferred(self.handler.handle_oidc_callback(request))
+        self.get_success(self.handler.handle_oidc_callback(request))
         self.assertRenderedError("invalid_token")
 
         self.handler._auth_handler.complete_sso_login.reset_mock()
@@ -450,7 +442,7 @@ def test_callback(self):
 
         # With userinfo fetching
         self.handler._scopes = []  # do not ask the "openid" scope
-        yield defer.ensureDeferred(self.handler.handle_oidc_callback(request))
+        self.get_success(self.handler.handle_oidc_callback(request))
 
         self.handler._auth_handler.complete_sso_login.assert_called_once_with(
             user_id, request, client_redirect_url, {},
@@ -465,17 +457,16 @@ def test_callback(self):
 
         # Handle userinfo fetching error
         self.handler._fetch_userinfo = simple_async_mock(raises=Exception())
-        yield defer.ensureDeferred(self.handler.handle_oidc_callback(request))
+        self.get_success(self.handler.handle_oidc_callback(request))
         self.assertRenderedError("fetch_error")
 
         # Handle code exchange failure
         self.handler._exchange_code = simple_async_mock(
             raises=OidcError("invalid_request")
         )
-        yield defer.ensureDeferred(self.handler.handle_oidc_callback(request))
+        self.get_success(self.handler.handle_oidc_callback(request))
         self.assertRenderedError("invalid_request")
 
-    @defer.inlineCallbacks
     def test_callback_session(self):
         """The callback verifies the session presence and validity"""
         self.handler._render_error = Mock(return_value=None)
@@ -484,20 +475,20 @@ def test_callback_session(self):
         # Missing cookie
         request.args = {}
         request.getCookie.return_value = None
-        yield defer.ensureDeferred(self.handler.handle_oidc_callback(request))
+        self.get_success(self.handler.handle_oidc_callback(request))
         self.assertRenderedError("missing_session", "No session cookie found")
 
         # Missing session parameter
         request.args = {}
         request.getCookie.return_value = "session"
-        yield defer.ensureDeferred(self.handler.handle_oidc_callback(request))
+        self.get_success(self.handler.handle_oidc_callback(request))
         self.assertRenderedError("invalid_request", "State parameter is missing")
 
         # Invalid cookie
         request.args = {}
         request.args[b"state"] = [b"state"]
         request.getCookie.return_value = "session"
-        yield defer.ensureDeferred(self.handler.handle_oidc_callback(request))
+        self.get_success(self.handler.handle_oidc_callback(request))
         self.assertRenderedError("invalid_session")
 
         # Mismatching session
@@ -510,18 +501,17 @@ def test_callback_session(self):
         request.args = {}
         request.args[b"state"] = [b"mismatching state"]
         request.getCookie.return_value = session
-        yield defer.ensureDeferred(self.handler.handle_oidc_callback(request))
+        self.get_success(self.handler.handle_oidc_callback(request))
         self.assertRenderedError("mismatching_session")
 
         # Valid session
         request.args = {}
         request.args[b"state"] = [b"state"]
         request.getCookie.return_value = session
-        yield defer.ensureDeferred(self.handler.handle_oidc_callback(request))
+        self.get_success(self.handler.handle_oidc_callback(request))
         self.assertRenderedError("invalid_request")
 
     @override_config({"oidc_config": {"client_auth_method": "client_secret_post"}})
-    @defer.inlineCallbacks
     def test_exchange_code(self):
         """Code exchange behaves correctly and handles various error scenarios."""
         token = {"type": "bearer"}
@@ -530,7 +520,7 @@ def test_exchange_code(self):
             return_value=FakeResponse(code=200, phrase=b"OK", body=token_json)
         )
         code = "code"
-        ret = yield defer.ensureDeferred(self.handler._exchange_code(code))
+        ret = self.get_success(self.handler._exchange_code(code))
         kwargs = self.http_client.request.call_args[1]
 
         self.assertEqual(ret, token)
@@ -552,10 +542,9 @@ def test_exchange_code(self):
                 body=b'{"error": "foo", "error_description": "bar"}',
             )
         )
-        with self.assertRaises(OidcError) as exc:
-            yield defer.ensureDeferred(self.handler._exchange_code(code))
-        self.assertEqual(exc.exception.error, "foo")
-        self.assertEqual(exc.exception.error_description, "bar")
+        exc = self.get_failure(self.handler._exchange_code(code), OidcError)
+        self.assertEqual(exc.value.error, "foo")
+        self.assertEqual(exc.value.error_description, "bar")
 
         # Internal server error with no JSON body
         self.http_client.request = simple_async_mock(
@@ -563,9 +552,8 @@ def test_exchange_code(self):
                 code=500, phrase=b"Internal Server Error", body=b"Not JSON",
             )
         )
-        with self.assertRaises(OidcError) as exc:
-            yield defer.ensureDeferred(self.handler._exchange_code(code))
-        self.assertEqual(exc.exception.error, "server_error")
+        exc = self.get_failure(self.handler._exchange_code(code), OidcError)
+        self.assertEqual(exc.value.error, "server_error")
 
         # Internal server error with JSON body
         self.http_client.request = simple_async_mock(
@@ -575,17 +563,16 @@ def test_exchange_code(self):
                 body=b'{"error": "internal_server_error"}',
             )
         )
-        with self.assertRaises(OidcError) as exc:
-            yield defer.ensureDeferred(self.handler._exchange_code(code))
-        self.assertEqual(exc.exception.error, "internal_server_error")
+
+        exc = self.get_failure(self.handler._exchange_code(code), OidcError)
+        self.assertEqual(exc.value.error, "internal_server_error")
 
         # 4xx error without "error" field
         self.http_client.request = simple_async_mock(
             return_value=FakeResponse(code=400, phrase=b"Bad request", body=b"{}",)
         )
-        with self.assertRaises(OidcError) as exc:
-            yield defer.ensureDeferred(self.handler._exchange_code(code))
-        self.assertEqual(exc.exception.error, "server_error")
+        exc = self.get_failure(self.handler._exchange_code(code), OidcError)
+        self.assertEqual(exc.value.error, "server_error")
 
         # 2xx error with "error" field
         self.http_client.request = simple_async_mock(
@@ -593,11 +580,9 @@ def test_exchange_code(self):
                 code=200, phrase=b"OK", body=b'{"error": "some_error"}',
             )
         )
-        with self.assertRaises(OidcError) as exc:
-            yield defer.ensureDeferred(self.handler._exchange_code(code))
-        self.assertEqual(exc.exception.error, "some_error")
+        exc = self.get_failure(self.handler._exchange_code(code), OidcError)
+        self.assertEqual(exc.value.error, "some_error")
 
-    @defer.inlineCallbacks
     def test_extra_attributes(self):
         """
         Login while using a mapping provider that implements get_extra_attributes.
@@ -643,7 +628,7 @@ def test_extra_attributes(self):
         request.requestHeaders.getRawHeaders.return_value = [b"Browser"]
         request.getClientIP.return_value = "10.0.0.1"
 
-        yield defer.ensureDeferred(self.handler.handle_oidc_callback(request))
+        self.get_success(self.handler.handle_oidc_callback(request))
 
         self.handler._auth_handler.complete_sso_login.assert_called_once_with(
             user_id, request, client_redirect_url, {"phone": "1234567"},

From f2b60846a85c877bf120549ad872bd0f84658122 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Wed, 30 Sep 2020 12:11:35 -0400
Subject: [PATCH 12/14] Properly override the mapping provider config in tests.

---
 tests/handlers/test_oidc.py | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index 3e2e2c5c55dc..d5087e58be9a 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -132,7 +132,7 @@ def make_homeserver(self, reactor, clock):
 
         config = self.default_config()
         config["public_baseurl"] = BASE_URL
-        oidc_config = config.get("oidc_config", {})
+        oidc_config = {}
         oidc_config["enabled"] = True
         oidc_config["client_id"] = CLIENT_ID
         oidc_config["client_secret"] = CLIENT_SECRET
@@ -141,6 +141,10 @@ def make_homeserver(self, reactor, clock):
         oidc_config["user_mapping_provider"] = {
             "module": __name__ + ".TestMappingProvider",
         }
+
+        # Update this config with what's in the default config so that
+        # override_config works as expected.
+        oidc_config.update(config.get("oidc_config", {}))
         config["oidc_config"] = oidc_config
 
         hs = self.setup_test_homeserver(
@@ -583,16 +587,19 @@ def test_exchange_code(self):
         exc = self.get_failure(self.handler._exchange_code(code), OidcError)
         self.assertEqual(exc.value.error, "some_error")
 
+    @override_config(
+        {
+            "oidc_config": {
+                "user_mapping_provider": {
+                    "module": __name__ + ".TestMappingProviderExtra"
+                }
+            }
+        }
+    )
     def test_extra_attributes(self):
         """
         Login while using a mapping provider that implements get_extra_attributes.
         """
-        # We cannot just override the configuration since the mapping provider
-        # gets stored on the handler.
-        self.handler._user_mapping_provider = TestMappingProviderExtra(
-            self.hs.config.oidc_user_mapping_provider_config
-        )
-
         token = {
             "type": "bearer",
             "id_token": "id_token",

From 9b4d9648b10f1c1a6a19220a4c70dc3fadf5f35c Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Wed, 30 Sep 2020 12:30:13 -0400
Subject: [PATCH 13/14] Add error handling for rendering Jinja.

---
 synapse/handlers/oidc_handler.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index 8b6c57b6ea67..19cd65267535 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -1115,5 +1115,9 @@ async def map_user_attributes(
     async def get_extra_attributes(self, userinfo: UserInfo, token: Token) -> JsonDict:
         extras = {}  # type: Dict[str, str]
         for key, template in self._config.extra_attributes.items():
-            extras[key] = template.render(user=userinfo).strip()
+            try:
+                extras[key] = template.render(user=userinfo).strip()
+            except Exception as e:
+                # Log an error and skip this value (don't break login for this).
+                logger.error("Failed to render OIDC extra attribute %s: %s" % (key, e))
         return extras

From b0b23a5aad24648c7d5b299cca26b77d09da4ef0 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Wed, 30 Sep 2020 12:33:05 -0400
Subject: [PATCH 14/14] Regenerate the sample config.

---
 docs/sample_config.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml
index 8201770f0ac1..baaf35a4e218 100644
--- a/docs/sample_config.yaml
+++ b/docs/sample_config.yaml
@@ -1741,7 +1741,7 @@ oidc_config:
       # Note that these are non-standard and clients will ignore them without modifications.
       #
       #extra_attributes:
-        #birthdate: "{ user.birthdate }"
+        #birthdate: "{{ user.birthdate }}"