diff --git a/changelog.d/18549.feature b/changelog.d/18549.feature new file mode 100644 index 00000000000..4d78ae57ab4 --- /dev/null +++ b/changelog.d/18549.feature @@ -0,0 +1 @@ +Support for the stable endpoint and scopes of [MSC3861](https://github.com/matrix-org/matrix-spec-proposals/pull/3861) & co. diff --git a/synapse/api/auth/mas.py b/synapse/api/auth/mas.py index 00bad768564..40b4a5bd34b 100644 --- a/synapse/api/auth/mas.py +++ b/synapse/api/auth/mas.py @@ -13,7 +13,7 @@ # # import logging -from typing import TYPE_CHECKING, Optional +from typing import TYPE_CHECKING, Optional, Set from urllib.parse import urlencode from synapse._pydantic_compat import ( @@ -57,8 +57,10 @@ # Scope as defined by MSC2967 # https://github.com/matrix-org/matrix-spec-proposals/pull/2967 -SCOPE_MATRIX_API = "urn:matrix:org.matrix.msc2967.client:api:*" -SCOPE_MATRIX_DEVICE_PREFIX = "urn:matrix:org.matrix.msc2967.client:device:" +UNSTABLE_SCOPE_MATRIX_API = "urn:matrix:org.matrix.msc2967.client:api:*" +UNSTABLE_SCOPE_MATRIX_DEVICE_PREFIX = "urn:matrix:org.matrix.msc2967.client:device:" +STABLE_SCOPE_MATRIX_API = "urn:matrix:client:api:*" +STABLE_SCOPE_MATRIX_DEVICE_PREFIX = "urn:matrix:client:device:" class ServerMetadata(BaseModel): @@ -334,7 +336,10 @@ async def get_user_by_access_token( scope = introspection_result.get_scope_set() # Determine type of user based on presence of particular scopes - if SCOPE_MATRIX_API not in scope: + if ( + UNSTABLE_SCOPE_MATRIX_API not in scope + and STABLE_SCOPE_MATRIX_API not in scope + ): raise InvalidClientTokenError( "Token doesn't grant access to the Matrix C-S API" ) @@ -366,11 +371,12 @@ async def get_user_by_access_token( # We only allow a single device_id in the scope, so we find them all in the # scope list, and raise if there are more than one. The OIDC server should be # the one enforcing valid scopes, so we raise a 500 if we find an invalid scope. - device_ids = [ - tok[len(SCOPE_MATRIX_DEVICE_PREFIX) :] - for tok in scope - if tok.startswith(SCOPE_MATRIX_DEVICE_PREFIX) - ] + device_ids: Set[str] = set() + for tok in scope: + if tok.startswith(UNSTABLE_SCOPE_MATRIX_DEVICE_PREFIX): + device_ids.add(tok[len(UNSTABLE_SCOPE_MATRIX_DEVICE_PREFIX) :]) + elif tok.startswith(STABLE_SCOPE_MATRIX_DEVICE_PREFIX): + device_ids.add(tok[len(STABLE_SCOPE_MATRIX_DEVICE_PREFIX) :]) if len(device_ids) > 1: raise AuthError( @@ -378,7 +384,7 @@ async def get_user_by_access_token( "Multiple device IDs in scope", ) - device_id = device_ids[0] if device_ids else None + device_id = next(iter(device_ids), None) if device_id is not None: # Sanity check the device_id diff --git a/synapse/api/auth/msc3861_delegated.py b/synapse/api/auth/msc3861_delegated.py index 928b2c8f8b7..c406c683e71 100644 --- a/synapse/api/auth/msc3861_delegated.py +++ b/synapse/api/auth/msc3861_delegated.py @@ -20,7 +20,7 @@ # import logging from dataclasses import dataclass -from typing import TYPE_CHECKING, Any, Callable, Dict, List, Optional +from typing import TYPE_CHECKING, Any, Callable, Dict, List, Optional, Set from urllib.parse import urlencode from authlib.oauth2 import ClientAuth @@ -34,7 +34,6 @@ AuthError, HttpResponseException, InvalidClientTokenError, - OAuthInsufficientScopeError, SynapseError, UnrecognizedRequestError, ) @@ -63,9 +62,10 @@ # Scope as defined by MSC2967 # https://github.com/matrix-org/matrix-spec-proposals/pull/2967 -SCOPE_MATRIX_API = "urn:matrix:org.matrix.msc2967.client:api:*" -SCOPE_MATRIX_GUEST = "urn:matrix:org.matrix.msc2967.client:api:guest" -SCOPE_MATRIX_DEVICE_PREFIX = "urn:matrix:org.matrix.msc2967.client:device:" +UNSTABLE_SCOPE_MATRIX_API = "urn:matrix:org.matrix.msc2967.client:api:*" +UNSTABLE_SCOPE_MATRIX_DEVICE_PREFIX = "urn:matrix:org.matrix.msc2967.client:device:" +STABLE_SCOPE_MATRIX_API = "urn:matrix:client:api:*" +STABLE_SCOPE_MATRIX_DEVICE_PREFIX = "urn:matrix:client:device:" # Scope which allows access to the Synapse admin API SCOPE_SYNAPSE_ADMIN = "urn:synapse:admin:*" @@ -444,9 +444,6 @@ async def _wrapped_get_user_by_req( if not self._is_access_token_the_admin_token(access_token): await self._record_request(request, requester) - if not allow_guest and requester.is_guest: - raise OAuthInsufficientScopeError([SCOPE_MATRIX_API]) - request.requester = requester return requester @@ -528,10 +525,11 @@ async def get_user_by_access_token( scope: List[str] = introspection_result.get_scope_list() # Determine type of user based on presence of particular scopes - has_user_scope = SCOPE_MATRIX_API in scope - has_guest_scope = SCOPE_MATRIX_GUEST in scope + has_user_scope = ( + UNSTABLE_SCOPE_MATRIX_API in scope or STABLE_SCOPE_MATRIX_API in scope + ) - if not has_user_scope and not has_guest_scope: + if not has_user_scope: raise InvalidClientTokenError("No scope in token granting user rights") # Match via the sub claim @@ -579,11 +577,12 @@ async def get_user_by_access_token( # We only allow a single device_id in the scope, so we find them all in the # scope list, and raise if there are more than one. The OIDC server should be # the one enforcing valid scopes, so we raise a 500 if we find an invalid scope. - device_ids = [ - tok[len(SCOPE_MATRIX_DEVICE_PREFIX) :] - for tok in scope - if tok.startswith(SCOPE_MATRIX_DEVICE_PREFIX) - ] + device_ids: Set[str] = set() + for tok in scope: + if tok.startswith(UNSTABLE_SCOPE_MATRIX_DEVICE_PREFIX): + device_ids.add(tok[len(UNSTABLE_SCOPE_MATRIX_DEVICE_PREFIX) :]) + elif tok.startswith(STABLE_SCOPE_MATRIX_DEVICE_PREFIX): + device_ids.add(tok[len(STABLE_SCOPE_MATRIX_DEVICE_PREFIX) :]) if len(device_ids) > 1: raise AuthError( @@ -591,7 +590,7 @@ async def get_user_by_access_token( "Multiple device IDs in scope", ) - device_id = device_ids[0] if device_ids else None + device_id = next(iter(device_ids), None) if device_id is not None: # Sanity check the device_id @@ -617,5 +616,4 @@ async def get_user_by_access_token( user_id=user_id, device_id=device_id, scope=scope, - is_guest=(has_guest_scope and not has_user_scope), ) diff --git a/synapse/rest/client/auth_metadata.py b/synapse/rest/client/auth_metadata.py index 25e01a65747..4b5d9974780 100644 --- a/synapse/rest/client/auth_metadata.py +++ b/synapse/rest/client/auth_metadata.py @@ -76,11 +76,17 @@ class AuthMetadataServlet(RestServlet): Advertises the OAuth 2.0 server metadata for the homeserver. """ - PATTERNS = client_patterns( - "/org.matrix.msc2965/auth_metadata$", - unstable=True, - releases=(), - ) + PATTERNS = [ + *client_patterns( + "/auth_metadata$", + releases=("v1",), + ), + *client_patterns( + "/org.matrix.msc2965/auth_metadata$", + unstable=True, + releases=(), + ), + ] def __init__(self, hs: "HomeServer"): super().__init__() diff --git a/tests/handlers/test_oauth_delegation.py b/tests/handlers/test_oauth_delegation.py index 2b0638bc125..d24614f6a35 100644 --- a/tests/handlers/test_oauth_delegation.py +++ b/tests/handlers/test_oauth_delegation.py @@ -25,11 +25,11 @@ from http import HTTPStatus from http.server import BaseHTTPRequestHandler, HTTPServer from io import BytesIO -from typing import Any, Coroutine, Dict, Generator, Optional, TypeVar, Union +from typing import Any, ClassVar, Coroutine, Dict, Generator, Optional, TypeVar, Union from unittest.mock import ANY, AsyncMock, Mock from urllib.parse import parse_qs -from parameterized import parameterized_class +from parameterized.parameterized import parameterized_class from signedjson.key import ( encode_verify_key_base64, generate_signing_key, @@ -46,7 +46,6 @@ Codes, HttpResponseException, InvalidClientTokenError, - OAuthInsufficientScopeError, SynapseError, ) from synapse.appservice import ApplicationService @@ -78,11 +77,7 @@ INTROSPECTION_ENDPOINT = ISSUER + "introspect" SYNAPSE_ADMIN_SCOPE = "urn:synapse:admin:*" -MATRIX_USER_SCOPE = "urn:matrix:org.matrix.msc2967.client:api:*" -MATRIX_GUEST_SCOPE = "urn:matrix:org.matrix.msc2967.client:api:guest" -MATRIX_DEVICE_SCOPE_PREFIX = "urn:matrix:org.matrix.msc2967.client:device:" DEVICE = "AABBCCDD" -MATRIX_DEVICE_SCOPE = MATRIX_DEVICE_SCOPE_PREFIX + DEVICE SUBJECT = "abc-def-ghi" USERNAME = "test-user" USER_ID = "@" + USERNAME + ":" + SERVER_NAME @@ -112,7 +107,24 @@ async def get_json(url: str) -> JsonDict: @skip_unless(HAS_AUTHLIB, "requires authlib") +@parameterized_class( + ("device_scope_prefix", "api_scope"), + [ + ("urn:matrix:client:device:", "urn:matrix:client:api:*"), + ( + "urn:matrix:org.matrix.msc2967.client:device:", + "urn:matrix:org.matrix.msc2967.client:api:*", + ), + ], +) class MSC3861OAuthDelegation(HomeserverTestCase): + device_scope_prefix: ClassVar[str] + api_scope: ClassVar[str] + + @property + def device_scope(self) -> str: + return self.device_scope_prefix + DEVICE + servlets = [ account.register_servlets, keys.register_servlets, @@ -212,7 +224,7 @@ def test_active_user_no_subject(self) -> None: """The handler should return a 500 when no subject is present.""" self._set_introspection_returnvalue( - {"active": True, "scope": " ".join([MATRIX_USER_SCOPE])} + {"active": True, "scope": " ".join([self.api_scope])}, ) request = Mock(args={}) @@ -235,7 +247,7 @@ def test_active_no_user_scope(self) -> None: { "active": True, "sub": SUBJECT, - "scope": " ".join([MATRIX_DEVICE_SCOPE]), + "scope": " ".join([self.device_scope]), } ) request = Mock(args={}) @@ -282,7 +294,7 @@ def test_active_admin(self) -> None: { "active": True, "sub": SUBJECT, - "scope": " ".join([SYNAPSE_ADMIN_SCOPE, MATRIX_USER_SCOPE]), + "scope": " ".join([SYNAPSE_ADMIN_SCOPE, self.api_scope]), "username": USERNAME, } ) @@ -312,9 +324,7 @@ def test_active_admin_highest_privilege(self) -> None: { "active": True, "sub": SUBJECT, - "scope": " ".join( - [SYNAPSE_ADMIN_SCOPE, MATRIX_USER_SCOPE, MATRIX_GUEST_SCOPE] - ), + "scope": " ".join([SYNAPSE_ADMIN_SCOPE, self.api_scope]), "username": USERNAME, } ) @@ -344,7 +354,7 @@ def test_active_user(self) -> None: { "active": True, "sub": SUBJECT, - "scope": " ".join([MATRIX_USER_SCOPE]), + "scope": " ".join([self.api_scope]), "username": USERNAME, } ) @@ -374,7 +384,7 @@ def test_active_user_with_device(self) -> None: { "active": True, "sub": SUBJECT, - "scope": " ".join([MATRIX_USER_SCOPE, MATRIX_DEVICE_SCOPE]), + "scope": " ".join([self.api_scope, self.device_scope]), "username": USERNAME, } ) @@ -404,7 +414,7 @@ def test_active_user_with_device_explicit_device_id(self) -> None: { "active": True, "sub": SUBJECT, - "scope": " ".join([MATRIX_USER_SCOPE]), + "scope": " ".join([self.api_scope]), "device_id": DEVICE, "username": USERNAME, } @@ -444,9 +454,9 @@ def test_multiple_devices(self) -> None: "sub": SUBJECT, "scope": " ".join( [ - MATRIX_USER_SCOPE, - f"{MATRIX_DEVICE_SCOPE_PREFIX}AABBCC", - f"{MATRIX_DEVICE_SCOPE_PREFIX}DDEEFF", + self.api_scope, + f"{self.device_scope_prefix}AABBCC", + f"{self.device_scope_prefix}DDEEFF", ] ), "username": USERNAME, @@ -457,68 +467,6 @@ def test_multiple_devices(self) -> None: request.requestHeaders.getRawHeaders = mock_getRawHeaders() self.get_failure(self.auth.get_user_by_req(request), AuthError) - def test_active_guest_not_allowed(self) -> None: - """The handler should return an insufficient scope error.""" - - self._set_introspection_returnvalue( - { - "active": True, - "sub": SUBJECT, - "scope": " ".join([MATRIX_GUEST_SCOPE, MATRIX_DEVICE_SCOPE]), - "username": USERNAME, - } - ) - request = Mock(args={}) - request.args[b"access_token"] = [b"mockAccessToken"] - request.requestHeaders.getRawHeaders = mock_getRawHeaders() - error = self.get_failure( - self.auth.get_user_by_req(request), OAuthInsufficientScopeError - ) - self.http_client.get_json.assert_called_once_with(WELL_KNOWN) - self._rust_client.post.assert_called_once_with( - url=INTROSPECTION_ENDPOINT, - response_limit=ANY, - request_body=ANY, - headers=ANY, - ) - self._assertParams() - self.assertEqual( - getattr(error.value, "headers", {})["WWW-Authenticate"], - 'Bearer error="insufficient_scope", scope="urn:matrix:org.matrix.msc2967.client:api:*"', - ) - - def test_active_guest_allowed(self) -> None: - """The handler should return a requester with guest user rights and a device ID.""" - - self._set_introspection_returnvalue( - { - "active": True, - "sub": SUBJECT, - "scope": " ".join([MATRIX_GUEST_SCOPE, MATRIX_DEVICE_SCOPE]), - "username": USERNAME, - } - ) - request = Mock(args={}) - request.args[b"access_token"] = [b"mockAccessToken"] - request.requestHeaders.getRawHeaders = mock_getRawHeaders() - requester = self.get_success( - self.auth.get_user_by_req(request, allow_guest=True) - ) - self.http_client.get_json.assert_called_once_with(WELL_KNOWN) - self._rust_client.post.assert_called_once_with( - url=INTROSPECTION_ENDPOINT, - response_limit=ANY, - request_body=ANY, - headers=ANY, - ) - self._assertParams() - self.assertEqual(requester.user.to_string(), "@%s:%s" % (USERNAME, SERVER_NAME)) - self.assertEqual(requester.is_guest, True) - self.assertEqual( - get_awaitable_result(self.auth.is_server_admin(requester)), False - ) - self.assertEqual(requester.device_id, DEVICE) - def test_unavailable_introspection_endpoint(self) -> None: """The handler should return an internal server error.""" request = Mock(args={}) @@ -562,8 +510,8 @@ def test_cached_expired_introspection(self) -> None: "sub": SUBJECT, "scope": " ".join( [ - MATRIX_USER_SCOPE, - f"{MATRIX_DEVICE_SCOPE_PREFIX}AABBCC", + self.api_scope, + f"{self.device_scope_prefix}AABBCC", ] ), "username": USERNAME, @@ -611,7 +559,7 @@ def test_cross_signing(self) -> None: { "active": True, "sub": SUBJECT, - "scope": " ".join([MATRIX_USER_SCOPE, MATRIX_DEVICE_SCOPE]), + "scope": " ".join([self.api_scope, self.device_scope]), "username": USERNAME, } ) @@ -676,7 +624,7 @@ async def mock_http_client_request( return json.dumps( { "active": True, - "scope": MATRIX_USER_SCOPE, + "scope": self.api_scope, "sub": SUBJECT, "username": USERNAME, }, @@ -842,8 +790,24 @@ def endpoint(self) -> str: T = TypeVar("T") +@parameterized_class( + ("device_scope_prefix", "api_scope"), + [ + ("urn:matrix:client:device:", "urn:matrix:client:api:*"), + ( + "urn:matrix:org.matrix.msc2967.client:device:", + "urn:matrix:org.matrix.msc2967.client:api:*", + ), + ], +) class MasAuthDelegation(HomeserverTestCase): server: FakeMasServer + device_scope_prefix: ClassVar[str] + api_scope: ClassVar[str] + + @property + def device_scope(self) -> str: + return self.device_scope_prefix + DEVICE def till_deferred_has_result( self, @@ -914,12 +878,7 @@ def test_simple_introspection(self) -> None: self.server.introspection_response = { "active": True, "sub": SUBJECT, - "scope": " ".join( - [ - MATRIX_USER_SCOPE, - f"{MATRIX_DEVICE_SCOPE_PREFIX}{DEVICE}", - ] - ), + "scope": " ".join([self.api_scope, self.device_scope]), "username": USERNAME, "expires_in": 60, } @@ -943,12 +902,7 @@ def test_unexpiring_token(self) -> None: self.server.introspection_response = { "active": True, "sub": SUBJECT, - "scope": " ".join( - [ - MATRIX_USER_SCOPE, - f"{MATRIX_DEVICE_SCOPE_PREFIX}{DEVICE}", - ] - ), + "scope": " ".join([self.api_scope, self.device_scope]), "username": USERNAME, } @@ -971,12 +925,7 @@ def test_inexistent_device(self) -> None: self.server.introspection_response = { "active": True, "sub": SUBJECT, - "scope": " ".join( - [ - MATRIX_USER_SCOPE, - f"{MATRIX_DEVICE_SCOPE_PREFIX}ABCDEF", - ] - ), + "scope": " ".join([self.api_scope, f"{self.device_scope_prefix}ABCDEF"]), "username": USERNAME, "expires_in": 60, } @@ -993,7 +942,7 @@ def test_inexistent_user(self) -> None: self.server.introspection_response = { "active": True, "sub": SUBJECT, - "scope": " ".join([MATRIX_USER_SCOPE]), + "scope": " ".join([self.api_scope]), "username": "inexistent_user", "expires_in": 60, } @@ -1039,7 +988,7 @@ def test_device_id_in_body(self) -> None: self.server.introspection_response = { "active": True, "sub": SUBJECT, - "scope": MATRIX_USER_SCOPE, + "scope": self.api_scope, "username": USERNAME, "expires_in": 60, "device_id": DEVICE, @@ -1057,7 +1006,7 @@ def test_admin_scope(self) -> None: self.server.introspection_response = { "active": True, "sub": SUBJECT, - "scope": " ".join([SYNAPSE_ADMIN_SCOPE, MATRIX_USER_SCOPE]), + "scope": " ".join([SYNAPSE_ADMIN_SCOPE, self.api_scope]), "username": USERNAME, "expires_in": 60, } @@ -1079,12 +1028,7 @@ def test_cached_expired_introspection(self) -> None: self.server.introspection_response = { "active": True, "sub": SUBJECT, - "scope": " ".join( - [ - MATRIX_USER_SCOPE, - f"{MATRIX_DEVICE_SCOPE_PREFIX}{DEVICE}", - ] - ), + "scope": " ".join([self.api_scope, self.device_scope]), "username": USERNAME, "expires_in": 60, } diff --git a/tests/rest/client/test_auth_metadata.py b/tests/rest/client/test_auth_metadata.py index a935533b099..c13d4106361 100644 --- a/tests/rest/client/test_auth_metadata.py +++ b/tests/rest/client/test_auth_metadata.py @@ -18,8 +18,11 @@ # [This file includes modifications made by New Vector Limited] # from http import HTTPStatus +from typing import ClassVar from unittest.mock import AsyncMock +from parameterized import parameterized_class + from synapse.rest.client import auth_metadata from tests.unittest import HomeserverTestCase, override_config, skip_unless @@ -85,17 +88,22 @@ def test_returns_issuer_when_oidc_enabled(self) -> None: req_mock.assert_not_called() +@parameterized_class( + ("endpoint",), + [ + ("/_matrix/client/unstable/org.matrix.msc2965/auth_metadata",), + ("/_matrix/client/v1/auth_metadata",), + ], +) class AuthMetadataTestCase(HomeserverTestCase): + endpoint: ClassVar[str] servlets = [ auth_metadata.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_metadata", - ) + channel = self.make_request("GET", self.endpoint) self.assertEqual(channel.code, HTTPStatus.NOT_FOUND) @skip_unless(HAS_AUTHLIB, "requires authlib") @@ -124,10 +132,7 @@ def test_returns_issuer_when_oidc_enabled(self) -> None: ) 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_metadata", - ) + channel = self.make_request("GET", self.endpoint) self.assertEqual(channel.code, HTTPStatus.OK) self.assertEqual(