MSC3861: load the issuer and account management URLs from OIDC discovery (#17407)

sandhose · erikjohnston · commit 48303fcbccd5 · 2024-08-30T15:31:51.000+01:00
This will help mitigating any discrepancies between the issuer configured and the one returned by the OIDC provider. This also removes the need for configuring the `account_management_url` explicitely, as it will now be loaded from the OIDC discovery, as per MSC2965. Because we may now fetch stuff for the .well-known/matrix/client endpoint, this also transforms the client well-known resource to be asynchronous.
diff --git a/changelog.d/17407.misc b/changelog.d/17407.misc
@@ -0,0 +1 @@
+MSC3861: load the issuer and account management URLs from OIDC discovery.
diff --git a/synapse/api/auth/msc3861_delegated.py b/synapse/api/auth/msc3861_delegated.py
@@ -121,7 +121,9 @@ def __init__(self, hs: "HomeServer"):
         self._hostname = hs.hostname
         self._admin_token = self._config.admin_token
 
-        self._issuer_metadata = RetryOnExceptionCachedCall(self._load_metadata)
+        self._issuer_metadata = RetryOnExceptionCachedCall[OpenIDProviderMetadata](
+            self._load_metadata
+        )
 
         if isinstance(auth_method, PrivateKeyJWTWithKid):
             # Use the JWK as the client secret when using the private_key_jwt method
@@ -145,6 +147,33 @@ async def _load_metadata(self) -> OpenIDProviderMetadata:
         # metadata.validate_introspection_endpoint()
         return metadata
 
+    async def issuer(self) -> str:
+        """
+        Get the configured issuer
+
+        This will use the issuer value set in the metadata,
+        falling back to the one set in the config if not set in the metadata
+        """
+        metadata = await self._issuer_metadata.get()
+        return metadata.issuer or self._config.issuer
+
+    async def account_management_url(self) -> Optional[str]:
+        """
+        Get the configured account management URL
+
+        This will discover the account management URL from the issuer if it's not set in the config
+        """
+        if self._config.account_management_url is not None:
+            return self._config.account_management_url
+
+        try:
+            metadata = await self._issuer_metadata.get()
+            return metadata.get("account_management_uri", None)
+        # We don't want to raise here if we can't load the metadata
+        except Exception:
+            logger.warning("Failed to load metadata:", exc_info=True)
+            return None
+
     async def _introspection_endpoint(self) -> str:
         """
         Returns the introspection endpoint of the issuer
@@ -154,7 +183,7 @@ async def _introspection_endpoint(self) -> str:
         if self._config.introspection_endpoint is not None:
             return self._config.introspection_endpoint
 
-        metadata = await self._load_metadata()
+        metadata = await self._issuer_metadata.get()
         return metadata.get("introspection_endpoint")
 
     async def _introspect_token(self, token: str) -> IntrospectionToken:
diff --git a/synapse/rest/client/auth.py b/synapse/rest/client/auth.py
@@ -20,7 +20,7 @@
 #
 
 import logging
-from typing import TYPE_CHECKING
+from typing import TYPE_CHECKING, cast
 
 from twisted.web.server import Request
 
@@ -70,11 +70,17 @@ async def on_GET(self, request: SynapseRequest, stagetype: str) -> None:
             self.hs.config.experimental.msc3861.enabled
             and stagetype == "org.matrix.cross_signing_reset"
         ):
-            config = self.hs.config.experimental.msc3861
-            if config.account_management_url is not None:
-                url = f"{config.account_management_url}?action=org.matrix.cross_signing_reset"
+            # If MSC3861 is enabled, we can assume self._auth is an instance of MSC3861DelegatedAuth
+            # We import lazily here because of the authlib requirement
+            from synapse.api.auth.msc3861_delegated import MSC3861DelegatedAuth
+
+            auth = cast(MSC3861DelegatedAuth, self.auth)
+
+            url = await auth.account_management_url()
+            if url is not None:
+                url = f"{url}?action=org.matrix.cross_signing_reset"
             else:
-                url = config.issuer
+                url = await auth.issuer()
             respond_with_redirect(request, str.encode(url))
 
         if stagetype == LoginType.RECAPTCHA:
diff --git a/synapse/rest/client/auth_issuer.py b/synapse/rest/client/auth_issuer.py
@@ -13,7 +13,7 @@
 # limitations under the License.
 import logging
 import typing
-from typing import Tuple
+from typing import Tuple, cast
 
 from synapse.api.errors import Codes, SynapseError
 from synapse.http.server import HttpServer
@@ -43,10 +43,16 @@ class AuthIssuerServlet(RestServlet):
     def __init__(self, hs: "HomeServer"):
         super().__init__()
         self._config = hs.config
+        self._auth = hs.get_auth()
 
     async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
         if self._config.experimental.msc3861.enabled:
-            return 200, {"issuer": self._config.experimental.msc3861.issuer}
+            # If MSC3861 is enabled, we can assume self._auth is an instance of MSC3861DelegatedAuth
+            # We import lazily here because of the authlib requirement
+            from synapse.api.auth.msc3861_delegated import MSC3861DelegatedAuth
+
+            auth = cast(MSC3861DelegatedAuth, self._auth)
+            return 200, {"issuer": await auth.issuer()}
         else:
             # Wouldn't expect this to be reached: the servelet shouldn't have been
             # registered. Still, fail gracefully if we are registered for some reason.
diff --git a/synapse/rest/client/keys.py b/synapse/rest/client/keys.py
@@ -23,7 +23,7 @@
 import logging
 import re
 from collections import Counter
-from typing import TYPE_CHECKING, Any, Dict, Optional, Tuple
+from typing import TYPE_CHECKING, Any, Dict, Optional, Tuple, cast
 
 from synapse.api.errors import (
     InteractiveAuthIncompleteError,
@@ -406,11 +406,17 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
             # explicitly mark the master key as replaceable.
             if self.hs.config.experimental.msc3861.enabled:
                 if not master_key_updatable_without_uia:
-                    config = self.hs.config.experimental.msc3861
-                    if config.account_management_url is not None:
-                        url = f"{config.account_management_url}?action=org.matrix.cross_signing_reset"
+                    # If MSC3861 is enabled, we can assume self.auth is an instance of MSC3861DelegatedAuth
+                    # We import lazily here because of the authlib requirement
+                    from synapse.api.auth.msc3861_delegated import MSC3861DelegatedAuth
+
+                    auth = cast(MSC3861DelegatedAuth, self.auth)
+
+                    uri = await auth.account_management_url()
+                    if uri is not None:
+                        url = f"{uri}?action=org.matrix.cross_signing_reset"
                     else:
-                        url = config.issuer
+                        url = await auth.issuer()
 
                     # We use a dummy session ID as this isn't really a UIA flow, but we
                     # reuse the same API shape for better client compatibility.
diff --git a/synapse/rest/client/login.py b/synapse/rest/client/login.py
@@ -268,7 +268,7 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, LoginResponse]:
                     approval_notice_medium=ApprovalNoticeMedium.NONE,
                 )
 
-        well_known_data = self._well_known_builder.get_well_known()
+        well_known_data = await self._well_known_builder.get_well_known()
         if well_known_data:
             result["well_known"] = well_known_data
         return 200, result
diff --git a/synapse/rest/well_known.py b/synapse/rest/well_known.py
@@ -18,12 +18,13 @@
 #
 #
 import logging
-from typing import TYPE_CHECKING, Optional
+from typing import TYPE_CHECKING, Optional, Tuple, cast
 
 from twisted.web.resource import Resource
 from twisted.web.server import Request
 
-from synapse.http.server import set_cors_headers
+from synapse.api.errors import NotFoundError
+from synapse.http.server import DirectServeJsonResource
 from synapse.http.site import SynapseRequest
 from synapse.types import JsonDict
 from synapse.util import json_encoder
@@ -38,8 +39,9 @@
 class WellKnownBuilder:
     def __init__(self, hs: "HomeServer"):
         self._config = hs.config
+        self._auth = hs.get_auth()
 
-    def get_well_known(self) -> Optional[JsonDict]:
+    async def get_well_known(self) -> Optional[JsonDict]:
         if not self._config.server.serve_client_wellknown:
             return None
 
@@ -52,13 +54,20 @@ def get_well_known(self) -> Optional[JsonDict]:
 
         # We use the MSC3861 values as they are used by multiple MSCs
         if self._config.experimental.msc3861.enabled:
+            # If MSC3861 is enabled, we can assume self._auth is an instance of MSC3861DelegatedAuth
+            # We import lazily here because of the authlib requirement
+            from synapse.api.auth.msc3861_delegated import MSC3861DelegatedAuth
+
+            auth = cast(MSC3861DelegatedAuth, self._auth)
+
             result["org.matrix.msc2965.authentication"] = {
-                "issuer": self._config.experimental.msc3861.issuer
+                "issuer": await auth.issuer(),
             }
-            if self._config.experimental.msc3861.account_management_url is not None:
+            account_management_url = await auth.account_management_url()
+            if account_management_url is not None:
                 result["org.matrix.msc2965.authentication"][
                     "account"
-                ] = self._config.experimental.msc3861.account_management_url
+                ] = account_management_url
 
         if self._config.server.extra_well_known_client_content:
             for (
@@ -71,26 +80,22 @@ def get_well_known(self) -> Optional[JsonDict]:
         return result
 
 
-class ClientWellKnownResource(Resource):
+class ClientWellKnownResource(DirectServeJsonResource):
     """A Twisted web resource which renders the .well-known/matrix/client file"""
 
     isLeaf = 1
 
     def __init__(self, hs: "HomeServer"):
-        Resource.__init__(self)
+        super().__init__()
         self._well_known_builder = WellKnownBuilder(hs)
 
-    def render_GET(self, request: SynapseRequest) -> bytes:
-        set_cors_headers(request)
-        r = self._well_known_builder.get_well_known()
+    async def _async_render_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
+        r = await self._well_known_builder.get_well_known()
         if not r:
-            request.setResponseCode(404)
-            request.setHeader(b"Content-Type", b"text/plain")
-            return b".well-known not available"
+            raise NotFoundError(".well-known not available")
 
         logger.debug("returning: %s", r)
-        request.setHeader(b"Content-Type", b"application/json")
-        return json_encoder.encode(r).encode("utf-8")
+        return 200, r
 
 
 class ServerWellKnownResource(Resource):
diff --git a/tests/rest/client/test_auth_issuer.py b/tests/rest/client/test_auth_issuer.py
@@ -12,6 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 from http import HTTPStatus
+from unittest.mock import AsyncMock
 
 from synapse.rest.client import auth_issuer
 
@@ -50,10 +51,27 @@ def test_returns_404_when_msc3861_disabled(self) -> None:
         }
     )
     def test_returns_issuer_when_oidc_enabled(self) -> None:
-        # Make an unauthenticated request for the discovery info.
+        # Patch the HTTP client to return the issuer metadata
+        req_mock = AsyncMock(return_value={"issuer": ISSUER})
+        self.hs.get_proxied_http_client().get_json = req_mock  # type: ignore[method-assign]
+
+        channel = self.make_request(
+            "GET",
+            "/_matrix/client/unstable/org.matrix.msc2965/auth_issuer",
+        )
+
+        self.assertEqual(channel.code, HTTPStatus.OK)
+        self.assertEqual(channel.json_body, {"issuer": ISSUER})
+
+        req_mock.assert_called_with("https://account.example.com/.well-known/openid-configuration")
+        req_mock.reset_mock()
+
+        # Second call it should use the cached value
         channel = self.make_request(
             "GET",
             "/_matrix/client/unstable/org.matrix.msc2965/auth_issuer",
         )
+
         self.assertEqual(channel.code, HTTPStatus.OK)
         self.assertEqual(channel.json_body, {"issuer": ISSUER})
+        req_mock.assert_not_called()
diff --git a/tests/rest/test_well_known.py b/tests/rest/test_well_known.py
@@ -17,6 +17,8 @@
 # [This file includes modifications made by New Vector Limited]
 #
 #
+from unittest.mock import AsyncMock
+
 from twisted.web.resource import Resource
 
 from synapse.rest.well_known import well_known_resource
@@ -112,7 +114,6 @@ def test_server_well_known_disabled(self) -> None:
                 "msc3861": {
                     "enabled": True,
                     "issuer": "https://issuer",
-                    "account_management_url": "https://my-account.issuer",
                     "client_id": "id",
                     "client_auth_method": "client_secret_post",
                     "client_secret": "secret",
@@ -122,18 +123,26 @@ def test_server_well_known_disabled(self) -> None:
         }
     )
     def test_client_well_known_msc3861_oauth_delegation(self) -> None:
-        channel = self.make_request(
-            "GET", "/.well-known/matrix/client", shorthand=False
-        )
+        # Patch the HTTP client to return the issuer metadata
+        req_mock = AsyncMock(return_value={"issuer": "https://issuer", "account_management_uri": "https://my-account.issuer"})
+        self.hs.get_proxied_http_client().get_json = req_mock  # type: ignore[method-assign]
 
-        self.assertEqual(channel.code, 200)
-        self.assertEqual(
-            channel.json_body,
-            {
-                "m.homeserver": {"base_url": "https://homeserver/"},
-                "org.matrix.msc2965.authentication": {
-                    "issuer": "https://issuer",
-                    "account": "https://my-account.issuer",
+        for _ in range(2):
+            channel = self.make_request(
+                "GET", "/.well-known/matrix/client", shorthand=False
+            )
+
+            self.assertEqual(channel.code, 200)
+            self.assertEqual(
+                channel.json_body,
+                {
+                    "m.homeserver": {"base_url": "https://homeserver/"},
+                    "org.matrix.msc2965.authentication": {
+                        "issuer": "https://issuer",
+                        "account": "https://my-account.issuer",
+                    },
                 },
-            },
-        )
+            )
+
+        # It should have been called exactly once, because it gets cached
+        req_mock.assert_called_once_with("https://issuer/.well-known/openid-configuration")

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+MSC3861: load the issuer and account management URLs from OIDC discovery.`
Original file line number	Diff line number	Diff line change
`@@ -268,7 +268,7 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, LoginResponse]:`
`268`	`268`	`approval_notice_medium=ApprovalNoticeMedium.NONE,`
`269`	`269`	`)`
`270`	`270`
`271`		`- well_known_data = self._well_known_builder.get_well_known()`
	`271`	`+ well_known_data = await self._well_known_builder.get_well_known()`
`272`	`272`	`if well_known_data:`
`273`	`273`	`result["well_known"] = well_known_data`
`274`	`274`	`return 200, result`