From 866f84caf4265547980080fab14039fe3621b25d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 4 Dec 2023 19:14:01 +0000 Subject: [PATCH 1/6] Expose OIDC discovery information under the CSAPI After this change, we continue to expose discovery information in the client well-known response for backwards compatability. The MSC [1] includes a new `actions_supported` which I don't think we currently have. I haven't done anything to add it in. LMK if you'd like me to do so. [1]: https://github.com/matrix-org/matrix-spec-proposals/pull/2965 --- synapse/config/experimental.py | 11 +++++ synapse/rest/__init__.py | 2 + synapse/rest/client/auth_issuer.py | 65 +++++++++++++++++++++++++++ synapse/rest/well_known.py | 12 ++--- tests/rest/client/test_auth_issuer.py | 61 +++++++++++++++++++++++++ 5 files changed, 142 insertions(+), 9 deletions(-) create mode 100644 synapse/rest/client/auth_issuer.py create mode 100644 tests/rest/client/test_auth_issuer.py diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index 6b9febe5a737..da8c7956d53a 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -423,3 +423,14 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: self.msc4069_profile_inhibit_propagation = experimental.get( "msc4069_profile_inhibit_propagation", False ) + + def get_msc2965_discovery_data(self) -> Optional[JsonDict]: + # We use the MSC3861 values as they are used by multiple MSCs + if not self.msc3861.enabled: + return None + + result = {"issuer": self.msc3861.issuer} + if self.msc3861.account_management_url is not None: + result["account"] = self.msc3861.account_management_url + + return result diff --git a/synapse/rest/__init__.py b/synapse/rest/__init__.py index 1be9c47c6121..53b8c319a67e 100644 --- a/synapse/rest/__init__.py +++ b/synapse/rest/__init__.py @@ -22,6 +22,7 @@ account_validity, appservice_ping, auth, + auth_issuer, capabilities, devices, directory, @@ -148,3 +149,4 @@ def register_servlets(client_resource: HttpServer, hs: "HomeServer") -> None: mutual_rooms.register_servlets(hs, client_resource) login_token_request.register_servlets(hs, client_resource) rendezvous.register_servlets(hs, client_resource) + auth_issuer.register_servlets(hs, client_resource) diff --git a/synapse/rest/client/auth_issuer.py b/synapse/rest/client/auth_issuer.py new file mode 100644 index 000000000000..96b49b912e0b --- /dev/null +++ b/synapse/rest/client/auth_issuer.py @@ -0,0 +1,65 @@ +# Copyright 2023 The Matrix.org Foundation C.I.C. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +import logging +import typing +from typing import Tuple + +from synapse.api.errors import Codes, SynapseError +from synapse.http.server import HttpServer +from synapse.http.servlet import RestServlet +from synapse.http.site import SynapseRequest +from synapse.rest.client._base import client_patterns +from synapse.types import JsonDict + +if typing.TYPE_CHECKING: + from synapse.server import HomeServer + + +logger = logging.getLogger(__name__) + + +class AuthIssuerServlet(RestServlet): + """ + Handles Client / Server API authentication in any situations where it + cannot be handled in the normal flow (with requests to the same endpoint). + Current use is for web fallback auth. + """ + + PATTERNS = client_patterns( + "/org.matrix.msc2965/auth_issuer$", + unstable=True, + releases=(), + ) + + def __init__(self, hs: "HomeServer"): + super().__init__() + self._config = hs.config + + async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: + discovery_data = self._config.experimental.get_msc2965_discovery_data() + if discovery_data is None: + # Wouldn't expect this to be reached: the servelet shouldn't have been + # registered. Still, fail gracefully if we are registered for some reason. + raise SynapseError( + 404, + "OIDC discovery has not been configured on this homeserver", + Codes.NOT_FOUND, + ) + return 200, discovery_data + + +def register_servlets(hs: "HomeServer", http_server: HttpServer) -> None: + # We use the MSC3861 values as they are used by multiple MSCs + if hs.config.experimental.msc3861.enabled: + AuthIssuerServlet(hs).register(http_server) diff --git a/synapse/rest/well_known.py b/synapse/rest/well_known.py index b8b4b5379b82..79bdb30fa12e 100644 --- a/synapse/rest/well_known.py +++ b/synapse/rest/well_known.py @@ -44,15 +44,9 @@ def get_well_known(self) -> Optional[JsonDict]: "base_url": self._config.registration.default_identity_server } - # We use the MSC3861 values as they are used by multiple MSCs - if self._config.experimental.msc3861.enabled: - result["org.matrix.msc2965.authentication"] = { - "issuer": self._config.experimental.msc3861.issuer - } - if self._config.experimental.msc3861.account_management_url is not None: - result["org.matrix.msc2965.authentication"][ - "account" - ] = self._config.experimental.msc3861.account_management_url + discovery_data = self._config.experimental.get_msc2965_discovery_data() + if discovery_data is not None: + result["org.matrix.msc2965.authentication"] = discovery_data if self._config.server.extra_well_known_client_content: for ( diff --git a/tests/rest/client/test_auth_issuer.py b/tests/rest/client/test_auth_issuer.py new file mode 100644 index 000000000000..e71afd5edda7 --- /dev/null +++ b/tests/rest/client/test_auth_issuer.py @@ -0,0 +1,61 @@ +# Copyright 2023 The Matrix.org Foundation C.I.C. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +from http import HTTPStatus + +from synapse.rest.client import auth_issuer + +from tests.unittest import HomeserverTestCase, override_config + +ISSUER = "https://account.example.com/" +ACCOUNT_MANAGEMENT_URL = "https://account.example.com/myaccount/" + + +class AccountDataTestCase(HomeserverTestCase): + servlets = [ + auth_issuer.register_servlets, + ] + + def test_returns_404_when_msc3861_disabled(self) -> None: + # Make an unauthenticated request for the discovery info. + channel = self.make_request( + "GET", + "/_matrix/client/unstable/org.matrix.msc2965/auth_issuer", + ) + self.assertEqual(channel.code, HTTPStatus.NOT_FOUND) + + @override_config( + { + "disable_registration": True, + "experimental_features": { + "msc3861": { + "enabled": True, + "issuer": ISSUER, + "account_management_url": ACCOUNT_MANAGEMENT_URL, + "client_id": "David Lister", + "client_auth_method": "client_secret_post", + "client_secret": "Who shot Mister Burns?", + } + }, + } + ) + def test_returns_discovery_data_when_oidc_enabled(self) -> None: + # Make an unauthenticated request for the discovery info. + 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, "account": ACCOUNT_MANAGEMENT_URL} + ) From a9843d1a8e39a2a718632bdb4dbf5533b17613c1 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 4 Dec 2023 19:17:56 +0000 Subject: [PATCH 2/6] Changelog --- changelog.d/16726.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/16726.misc diff --git a/changelog.d/16726.misc b/changelog.d/16726.misc new file mode 100644 index 000000000000..bac312465ca2 --- /dev/null +++ b/changelog.d/16726.misc @@ -0,0 +1 @@ +Update the implementation of [MSC2965](https://github.com/matrix-org/matrix-spec-proposals/pull/2965) (OIDC Provider discovery). From ad6e55b0ddf90484d2b06ce89821bba1bac488d1 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 5 Dec 2023 10:13:06 +0000 Subject: [PATCH 3/6] Correct testcase name Co-authored-by: Quentin Gliech --- tests/rest/client/test_auth_issuer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/rest/client/test_auth_issuer.py b/tests/rest/client/test_auth_issuer.py index e71afd5edda7..6d0b659df902 100644 --- a/tests/rest/client/test_auth_issuer.py +++ b/tests/rest/client/test_auth_issuer.py @@ -21,7 +21,7 @@ ACCOUNT_MANAGEMENT_URL = "https://account.example.com/myaccount/" -class AccountDataTestCase(HomeserverTestCase): +class AuthIssuerTestCase(HomeserverTestCase): servlets = [ auth_issuer.register_servlets, ] From 2970bbf682b6243c31904638b16d11a7ee190b0e Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 5 Dec 2023 10:13:28 +0000 Subject: [PATCH 4/6] Correct docstring Co-authored-by: Quentin Gliech --- synapse/rest/client/auth_issuer.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/synapse/rest/client/auth_issuer.py b/synapse/rest/client/auth_issuer.py index 96b49b912e0b..a95a753f333f 100644 --- a/synapse/rest/client/auth_issuer.py +++ b/synapse/rest/client/auth_issuer.py @@ -31,9 +31,7 @@ class AuthIssuerServlet(RestServlet): """ - Handles Client / Server API authentication in any situations where it - cannot be handled in the normal flow (with requests to the same endpoint). - Current use is for web fallback auth. + Advertises what OpenID Connect issuer clients should use to authorise users. """ PATTERNS = client_patterns( From 307424920b21448051ddf82e690ad3aafdd3f446 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 5 Dec 2023 12:33:03 +0000 Subject: [PATCH 5/6] Ditch the account management URL in new endpoint --- synapse/config/experimental.py | 11 ----------- synapse/rest/client/auth_issuer.py | 6 +++--- synapse/rest/well_known.py | 12 +++++++++--- tests/rest/client/test_auth_issuer.py | 8 ++------ 4 files changed, 14 insertions(+), 23 deletions(-) diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index da8c7956d53a..6b9febe5a737 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -423,14 +423,3 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: self.msc4069_profile_inhibit_propagation = experimental.get( "msc4069_profile_inhibit_propagation", False ) - - def get_msc2965_discovery_data(self) -> Optional[JsonDict]: - # We use the MSC3861 values as they are used by multiple MSCs - if not self.msc3861.enabled: - return None - - result = {"issuer": self.msc3861.issuer} - if self.msc3861.account_management_url is not None: - result["account"] = self.msc3861.account_management_url - - return result diff --git a/synapse/rest/client/auth_issuer.py b/synapse/rest/client/auth_issuer.py index a95a753f333f..77b972095692 100644 --- a/synapse/rest/client/auth_issuer.py +++ b/synapse/rest/client/auth_issuer.py @@ -45,8 +45,9 @@ def __init__(self, hs: "HomeServer"): self._config = hs.config async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: - discovery_data = self._config.experimental.get_msc2965_discovery_data() - if discovery_data is None: + if self._config.experimental.msc3861.enabled: + return 200, {"issuer": self._config.experimental.msc3861.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. raise SynapseError( @@ -54,7 +55,6 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: "OIDC discovery has not been configured on this homeserver", Codes.NOT_FOUND, ) - return 200, discovery_data def register_servlets(hs: "HomeServer", http_server: HttpServer) -> None: diff --git a/synapse/rest/well_known.py b/synapse/rest/well_known.py index 79bdb30fa12e..b8b4b5379b82 100644 --- a/synapse/rest/well_known.py +++ b/synapse/rest/well_known.py @@ -44,9 +44,15 @@ def get_well_known(self) -> Optional[JsonDict]: "base_url": self._config.registration.default_identity_server } - discovery_data = self._config.experimental.get_msc2965_discovery_data() - if discovery_data is not None: - result["org.matrix.msc2965.authentication"] = discovery_data + # We use the MSC3861 values as they are used by multiple MSCs + if self._config.experimental.msc3861.enabled: + result["org.matrix.msc2965.authentication"] = { + "issuer": self._config.experimental.msc3861.issuer + } + if self._config.experimental.msc3861.account_management_url is not None: + result["org.matrix.msc2965.authentication"][ + "account" + ] = self._config.experimental.msc3861.account_management_url if self._config.server.extra_well_known_client_content: for ( diff --git a/tests/rest/client/test_auth_issuer.py b/tests/rest/client/test_auth_issuer.py index 6d0b659df902..27a1dd945f31 100644 --- a/tests/rest/client/test_auth_issuer.py +++ b/tests/rest/client/test_auth_issuer.py @@ -18,7 +18,6 @@ from tests.unittest import HomeserverTestCase, override_config ISSUER = "https://account.example.com/" -ACCOUNT_MANAGEMENT_URL = "https://account.example.com/myaccount/" class AuthIssuerTestCase(HomeserverTestCase): @@ -41,7 +40,6 @@ def test_returns_404_when_msc3861_disabled(self) -> None: "msc3861": { "enabled": True, "issuer": ISSUER, - "account_management_url": ACCOUNT_MANAGEMENT_URL, "client_id": "David Lister", "client_auth_method": "client_secret_post", "client_secret": "Who shot Mister Burns?", @@ -49,13 +47,11 @@ def test_returns_404_when_msc3861_disabled(self) -> None: }, } ) - def test_returns_discovery_data_when_oidc_enabled(self) -> None: + def test_returns_issuer_when_oidc_enabled(self) -> None: # Make an unauthenticated request for the discovery info. 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, "account": ACCOUNT_MANAGEMENT_URL} - ) + self.assertEqual(channel.json_body, {"issuer": ISSUER}) From ab93316839ec9c6a2c31b60a77cc64636b3a6922 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 5 Dec 2023 12:44:57 +0000 Subject: [PATCH 6/6] skip unless HAS_AUTHLIB --- tests/rest/client/test_auth_issuer.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/rest/client/test_auth_issuer.py b/tests/rest/client/test_auth_issuer.py index 27a1dd945f31..964baeec324a 100644 --- a/tests/rest/client/test_auth_issuer.py +++ b/tests/rest/client/test_auth_issuer.py @@ -15,7 +15,8 @@ from synapse.rest.client import auth_issuer -from tests.unittest import HomeserverTestCase, override_config +from tests.unittest import HomeserverTestCase, override_config, skip_unless +from tests.utils import HAS_AUTHLIB ISSUER = "https://account.example.com/" @@ -33,6 +34,7 @@ def test_returns_404_when_msc3861_disabled(self) -> None: ) self.assertEqual(channel.code, HTTPStatus.NOT_FOUND) + @skip_unless(HAS_AUTHLIB, "requires authlib") @override_config( { "disable_registration": True,