From fe80ef5a81e3200d9e1009a04d7ad56f1a2c3f5f Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Sat, 13 Feb 2021 11:39:19 +0100 Subject: [PATCH 01/46] WIP: MSC2918 Signed-off-by: Quentin Gliech --- synapse/handlers/auth.py | 13 ++- synapse/handlers/register.py | 40 +++++++- synapse/module_api/__init__.py | 2 +- synapse/replication/http/login.py | 10 +- synapse/rest/client/v1/login.py | 119 ++++++++++++++++++----- synapse/rest/client/v2_alpha/register.py | 11 ++- 6 files changed, 160 insertions(+), 35 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 08e413bc98e0..255fda49a0a1 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -30,6 +30,7 @@ Optional, Tuple, Union, + cast, ) import attr @@ -71,6 +72,7 @@ if TYPE_CHECKING: from synapse.server import HomeServer + from synapse.rest.client.v1.login import LoginResponse logger = logging.getLogger(__name__) @@ -919,7 +921,7 @@ async def validate_login( self, login_submission: Dict[str, Any], ratelimit: bool = False, - ) -> Tuple[str, Optional[Callable[[Dict[str, str]], Awaitable[None]]]]: + ) -> Tuple[str, Optional[Callable[["LoginResponse"], Awaitable[None]]]]: """Authenticates the user for the /login API Also used by the user-interactive auth flow to validate auth types which don't @@ -1064,7 +1066,7 @@ async def _validate_userid_login( self, username: str, login_submission: Dict[str, Any], - ) -> Tuple[str, Optional[Callable[[Dict[str, str]], Awaitable[None]]]]: + ) -> Tuple[str, Optional[Callable[["LoginResponse"], Awaitable[None]]]]: """Helper for validate_login Handles login, once we've mapped 3pids onto userids @@ -1142,7 +1144,7 @@ async def _validate_userid_login( async def check_password_provider_3pid( self, medium: str, address: str, password: str - ) -> Tuple[Optional[str], Optional[Callable[[Dict[str, str]], Awaitable[None]]]]: + ) -> Tuple[Optional[str], Optional[Callable[["LoginResponse"], Awaitable[None]]]]: """Check if a password provider is able to validate a thirdparty login Args: @@ -1541,7 +1543,7 @@ def _complete_sso_login( ) respond_with_html(request, 200, html) - async def _sso_login_callback(self, login_result: JsonDict) -> None: + async def _sso_login_callback(self, login_result: "LoginResponse") -> None: """ A login callback which might add additional attributes to the login response. @@ -1555,7 +1557,8 @@ 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) + login_result_dict = cast(Dict[str, Any], login_result) + login_result_dict.update(extra_attributes.extra_attributes) def _expire_sso_extra_attributes(self) -> None: """ diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 3b6660c873f7..d7ccc357cc7b 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -19,6 +19,7 @@ from typing import TYPE_CHECKING, Dict, Iterable, List, Optional, Tuple from prometheus_client import Counter +from typing_extensions import TypedDict from synapse import types from synapse.api.constants import MAX_USERID_LENGTH, EventTypes, JoinRules, LoginType @@ -55,6 +56,16 @@ ["guest", "auth_provider"], ) +LoginDict = TypedDict( + "LoginDict", + { + "device_id": str, + "access_token": str, + "valid_until_ms": int, + "refresh_token": Optional[str], + }, +) + class RegistrationHandler(BaseHandler): def __init__(self, hs: "HomeServer"): @@ -666,7 +677,8 @@ async def register_device( is_guest: bool = False, is_appservice_ghost: bool = False, auth_provider_id: Optional[str] = None, - ) -> Tuple[str, str]: + should_issue_refresh_token: bool = False, + ) -> Tuple[str, str, Optional[int], Optional[str]]: """Register a device for a user and generate an access token. The access token will be limited by the homeserver's session_lifetime config. @@ -679,7 +691,7 @@ async def register_device( auth_provider_id: The SSO IdP the user used, if any (just used for the prometheus metrics). Returns: - Tuple of device ID and access token + Tuple of device ID, access token, access token expiration time and refresh token """ res = await self._register_device_client( user_id=user_id, @@ -687,6 +699,7 @@ async def register_device( initial_display_name=initial_display_name, is_guest=is_guest, is_appservice_ghost=is_appservice_ghost, + should_issue_refresh_token=should_issue_refresh_token, ) login_counter.labels( @@ -694,7 +707,12 @@ async def register_device( auth_provider=(auth_provider_id or ""), ).inc() - return res["device_id"], res["access_token"] + return ( + res["device_id"], + res["access_token"], + res["valid_until_ms"], + res["refresh_token"], + ) async def register_device_inner( self, @@ -703,7 +721,8 @@ async def register_device_inner( initial_display_name: Optional[str], is_guest: bool = False, is_appservice_ghost: bool = False, - ) -> Dict[str, str]: + should_issue_refresh_token: bool = False, + ) -> LoginDict: """Helper for register_device Does the bits that need doing on the main process. Not for use outside this @@ -718,6 +737,12 @@ class and RegisterDeviceReplicationServlet. ) valid_until_ms = self.clock.time_msec() + self.session_lifetime + refresh_token = None + + if should_issue_refresh_token: + refresh_token = "FAKEREFRESHTOKEN" + valid_until_ms = self.clock.time_msec() + 60 * 1000 + registered_device_id = await self.device_handler.check_device_registered( user_id, device_id, initial_display_name ) @@ -734,7 +759,12 @@ class and RegisterDeviceReplicationServlet. is_appservice_ghost=is_appservice_ghost, ) - return {"device_id": registered_device_id, "access_token": access_token} + return { + "device_id": registered_device_id, + "access_token": access_token, + "valid_until_ms": valid_until_ms, + "refresh_token": refresh_token, + } async def post_registration_actions( self, user_id: str, auth_result: dict, access_token: Optional[str] diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index ca1bd4cdc96a..52d6e75d8446 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -148,7 +148,7 @@ def register(self, localpart, displayname=None, emails: Optional[List[str]] = No "Using deprecated ModuleApi.register which creates a dummy user device." ) user_id = yield self.register_user(localpart, displayname, emails or []) - _, access_token = yield self.register_device(user_id) + _, access_token, _, _ = yield self.register_device(user_id) return user_id, access_token def register_user( diff --git a/synapse/replication/http/login.py b/synapse/replication/http/login.py index 4ec1bfa6eaf9..66a78f312558 100644 --- a/synapse/replication/http/login.py +++ b/synapse/replication/http/login.py @@ -37,7 +37,12 @@ def __init__(self, hs): @staticmethod async def _serialize_payload( - user_id, device_id, initial_display_name, is_guest, is_appservice_ghost + user_id, + device_id, + initial_display_name, + is_guest, + is_appservice_ghost, + should_issue_refresh_token, ): """ Args: @@ -51,6 +56,7 @@ async def _serialize_payload( "initial_display_name": initial_display_name, "is_guest": is_guest, "is_appservice_ghost": is_appservice_ghost, + "should_issue_refresh_token": should_issue_refresh_token, } async def _handle_request(self, request, user_id): @@ -60,6 +66,7 @@ async def _handle_request(self, request, user_id): initial_display_name = content["initial_display_name"] is_guest = content["is_guest"] is_appservice_ghost = content["is_appservice_ghost"] + should_issue_refresh_token = content["should_issue_refresh_token"] res = await self.registration_handler.register_device_inner( user_id, @@ -67,6 +74,7 @@ async def _handle_request(self, request, user_id): initial_display_name, is_guest, is_appservice_ghost=is_appservice_ghost, + should_issue_refresh_token=should_issue_refresh_token, ) return 200, res diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py index 3151e72d4f19..b1514929a50a 100644 --- a/synapse/rest/client/v1/login.py +++ b/synapse/rest/client/v1/login.py @@ -15,7 +15,9 @@ import logging import re -from typing import TYPE_CHECKING, Awaitable, Callable, Dict, Optional +from typing import TYPE_CHECKING, Any, Awaitable, Callable, Dict, List, Optional, cast + +from typing_extensions import TypedDict from synapse.api.errors import Codes, LoginError, SynapseError from synapse.api.ratelimiting import Ratelimiter @@ -40,6 +42,21 @@ logger = logging.getLogger(__name__) +LoginResponse = TypedDict( + "LoginResponse", + { + "user_id": str, + "access_token": str, + "home_server": str, + "expires_in": Optional[int], + "refresh_token": Optional[str], + "device_id": str, + "well_known": Optional[Dict[str, Any]], + }, + total=False, +) + + class LoginRestServlet(RestServlet): PATTERNS = client_patterns("/login$", v1=True) CAS_TYPE = "m.login.cas" @@ -48,6 +65,7 @@ class LoginRestServlet(RestServlet): JWT_TYPE = "org.matrix.login.jwt" JWT_TYPE_DEPRECATED = "m.login.jwt" APPSERVICE_TYPE = "uk.half-shot.msc2778.login.application_service" + REFRESH_TOKEN_PARAM = "org.matrix.msc2918.refresh_token" def __init__(self, hs: "HomeServer"): super().__init__() @@ -68,6 +86,8 @@ def __init__(self, hs: "HomeServer"): self.auth = hs.get_auth() + self.clock = hs.get_clock() + self.auth_handler = self.hs.get_auth_handler() self.registration_handler = hs.get_registration_handler() self._sso_handler = hs.get_sso_handler() @@ -138,6 +158,12 @@ def on_GET(self, request: SynapseRequest): async def on_POST(self, request: SynapseRequest): login_submission = parse_json_object_from_request(request) + refresh_token_param = cast( + List[bytes], + request.args.get(bytes(LoginRestServlet.REFRESH_TOKEN_PARAM, "utf-8"), []), + ) + should_issue_refresh_token = any((i == b"true" for i in refresh_token_param)) + try: if login_submission["type"] == LoginRestServlet.APPSERVICE_TYPE: appservice = self.auth.get_appservice_by_req(request) @@ -147,19 +173,32 @@ async def on_POST(self, request: SynapseRequest): None, request.getClientIP() ) - result = await self._do_appservice_login(login_submission, appservice) + result = await self._do_appservice_login( + login_submission, + appservice, + should_issue_refresh_token=should_issue_refresh_token, + ) elif self.jwt_enabled and ( login_submission["type"] == LoginRestServlet.JWT_TYPE or login_submission["type"] == LoginRestServlet.JWT_TYPE_DEPRECATED ): await self._address_ratelimiter.ratelimit(None, request.getClientIP()) - result = await self._do_jwt_login(login_submission) + result = await self._do_jwt_login( + login_submission, + should_issue_refresh_token=should_issue_refresh_token, + ) elif login_submission["type"] == LoginRestServlet.TOKEN_TYPE: await self._address_ratelimiter.ratelimit(None, request.getClientIP()) - result = await self._do_token_login(login_submission) + result = await self._do_token_login( + login_submission, + should_issue_refresh_token=should_issue_refresh_token, + ) else: await self._address_ratelimiter.ratelimit(None, request.getClientIP()) - result = await self._do_other_login(login_submission) + result = await self._do_other_login( + login_submission, + should_issue_refresh_token=should_issue_refresh_token, + ) except KeyError: raise SynapseError(400, "Missing JSON keys.") @@ -169,7 +208,10 @@ async def on_POST(self, request: SynapseRequest): return 200, result async def _do_appservice_login( - self, login_submission: JsonDict, appservice: ApplicationService + self, + login_submission: JsonDict, + appservice: ApplicationService, + should_issue_refresh_token: bool = False, ): identifier = login_submission.get("identifier") logger.info("Got appservice login request with identifier: %r", identifier) @@ -198,10 +240,15 @@ async def _do_appservice_login( raise LoginError(403, "Invalid access_token", errcode=Codes.FORBIDDEN) return await self._complete_login( - qualified_user_id, login_submission, ratelimit=appservice.is_rate_limited() + qualified_user_id, + login_submission, + ratelimit=appservice.is_rate_limited(), + should_issue_refresh_token=should_issue_refresh_token, ) - async def _do_other_login(self, login_submission: JsonDict) -> Dict[str, str]: + async def _do_other_login( + self, login_submission: JsonDict, should_issue_refresh_token: bool = False + ) -> LoginResponse: """Handle non-token/saml/jwt logins Args: @@ -224,7 +271,10 @@ async def _do_other_login(self, login_submission: JsonDict) -> Dict[str, str]: login_submission, ratelimit=True ) result = await self._complete_login( - canonical_user_id, login_submission, callback + canonical_user_id, + login_submission, + callback, + should_issue_refresh_token=should_issue_refresh_token, ) return result @@ -232,11 +282,12 @@ async def _complete_login( self, user_id: str, login_submission: JsonDict, - callback: Optional[Callable[[Dict[str, str]], Awaitable[None]]] = None, + callback: Optional[Callable[[LoginResponse], Awaitable[None]]] = None, create_non_existent_users: bool = False, ratelimit: bool = True, auth_provider_id: Optional[str] = None, - ) -> Dict[str, str]: + should_issue_refresh_token: bool = False, + ) -> LoginResponse: """Called when we've successfully authed the user and now need to actually login them in (e.g. create devices). This gets called on all successful logins. @@ -274,23 +325,41 @@ async def _complete_login( device_id = login_submission.get("device_id") initial_display_name = login_submission.get("initial_device_display_name") - device_id, access_token = await self.registration_handler.register_device( - user_id, device_id, initial_display_name, auth_provider_id=auth_provider_id + ( + device_id, + access_token, + valid_until_ms, + refresh_token, + ) = await self.registration_handler.register_device( + user_id, + device_id, + initial_display_name, + auth_provider_id=auth_provider_id, + should_issue_refresh_token=should_issue_refresh_token, ) - result = { - "user_id": user_id, - "access_token": access_token, - "home_server": self.hs.hostname, - "device_id": device_id, - } + result = LoginResponse( + user_id=user_id, + access_token=access_token, + home_server=self.hs.hostname, + device_id=device_id, + ) + + if valid_until_ms is not None: + expires_in_ms = valid_until_ms - self.clock.time_msec() + result["expires_in"] = int(expires_in_ms / 1000) + + if refresh_token is not None: + result["refresh_token"] = refresh_token if callback is not None: await callback(result) return result - async def _do_token_login(self, login_submission: JsonDict) -> Dict[str, str]: + async def _do_token_login( + self, login_submission: JsonDict, should_issue_refresh_token: bool = False + ) -> LoginResponse: """ Handle the final stage of SSO login. @@ -309,9 +378,12 @@ async def _do_token_login(self, login_submission: JsonDict) -> Dict[str, str]: login_submission, self.auth_handler._sso_login_callback, auth_provider_id=res.auth_provider_id, + should_issue_refresh_token=should_issue_refresh_token, ) - async def _do_jwt_login(self, login_submission: JsonDict) -> Dict[str, str]: + async def _do_jwt_login( + self, login_submission: JsonDict, should_issue_refresh_token: bool = False + ) -> LoginResponse: token = login_submission.get("token", None) if token is None: raise LoginError( @@ -342,7 +414,10 @@ async def _do_jwt_login(self, login_submission: JsonDict) -> Dict[str, str]: user_id = UserID(user, self.hs.hostname).to_string() result = await self._complete_login( - user_id, login_submission, create_non_existent_users=True + user_id, + login_submission, + create_non_existent_users=True, + should_issue_refresh_token=should_issue_refresh_token, ) return result diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index c212da0cb29c..08e6f1b516b1 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -693,7 +693,12 @@ async def _create_registration_details( if not params.get("inhibit_login", False): device_id = params.get("device_id") initial_display_name = params.get("initial_device_display_name") - device_id, access_token = await self.registration_handler.register_device( + ( + device_id, + access_token, + valid_until_ms, + refresh_token, + ) = await self.registration_handler.register_device( user_id, device_id, initial_display_name, @@ -702,6 +707,10 @@ async def _create_registration_details( ) result.update({"access_token": access_token, "device_id": device_id}) + + if refresh_token is not None: + result["refresh_token"] = refresh_token + return result async def _do_guest_registration(self, params, address=None): From 523d8cf490daadd5ed023d37c2f4c1d6e23ab3a0 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 19 Feb 2021 16:13:11 +0100 Subject: [PATCH 02/46] MSC2918: implement refresh tokens Signed-off-by: Quentin Gliech --- synapse/handlers/auth.py | 51 +++++++++++++++ synapse/handlers/register.py | 15 +++-- synapse/rest/client/v1/login.py | 35 ++++++++++ .../storage/databases/main/registration.py | 64 +++++++++++++++++++ .../delta/60/01refresh_tokens.sql.sqlite | 31 +++++++++ synapse/storage/prepare_database.py | 2 +- 6 files changed, 193 insertions(+), 5 deletions(-) create mode 100644 synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.sqlite diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 255fda49a0a1..ec70fe391276 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -772,6 +772,40 @@ def _auth_dict_for_flows( "params": params, } + async def refresh_token( + self, refresh_token: str, valid_until_ms: Optional[int], + ) -> Tuple[str, str]: + existing_token = await self.store.lookup_refresh_token(refresh_token) + if existing_token is None: + raise SynapseError(400, "refresh token does not exist") + + if not existing_token.valid: + raise SynapseError(400, "refresh token isn't valid anymore") + + ( + new_refresh_token, + new_refresh_token_id, + ) = await self.get_refresh_token_for_user_id( + user_id=existing_token.user_id, device_id=existing_token.device_id + ) + access_token = await self.get_access_token_for_user_id( + user_id=existing_token.user_id, + device_id=existing_token.device_id, + valid_until_ms=valid_until_ms, + refresh_token_id=new_refresh_token_id, + ) + await self.store.invalidate_refresh_token(existing_token.token_id) + return access_token, new_refresh_token + + async def get_refresh_token_for_user_id( + self, user_id: str, device_id: Optional[str], + ) -> Tuple[str, int]: + refresh_token = self.macaroon_gen.generate_refresh_token(user_id) + refresh_token_id = await self.store.add_refresh_token_to_user( + user_id=user_id, token=refresh_token, device_id=device_id, + ) + return refresh_token, refresh_token_id + async def get_access_token_for_user_id( self, user_id: str, @@ -779,6 +813,7 @@ async def get_access_token_for_user_id( valid_until_ms: Optional[int], puppets_user_id: Optional[str] = None, is_appservice_ghost: bool = False, + refresh_token_id: Optional[int] = None, ) -> str: """ Creates a new access token for the user with the given user ID. @@ -829,6 +864,7 @@ async def get_access_token_for_user_id( device_id=device_id, valid_until_ms=valid_until_ms, puppets_user_id=puppets_user_id, + refresh_token_id=refresh_token_id, ) # the device *should* have been registered before we got here; however, @@ -1589,6 +1625,21 @@ class MacaroonGenerator: hs = attr.ib() + def generate_refresh_token( + self, user_id: str, extra_caveats: Optional[List[str]] = None + ) -> str: + extra_caveats = extra_caveats or [] + macaroon = self._generate_base_macaroon(user_id) + macaroon.add_first_party_caveat("type = refresh") + # Include a nonce, to make sure that each login gets a different + # access token. + macaroon.add_first_party_caveat( + "nonce = %s" % (stringutils.random_string_with_symbols(16),) + ) + for caveat in extra_caveats: + macaroon.add_first_party_caveat(caveat) + return macaroon.serialize() + def generate_access_token( self, user_id: str, extra_caveats: Optional[List[str]] = None ) -> str: diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index d7ccc357cc7b..65209a8fc040 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -738,10 +738,7 @@ class and RegisterDeviceReplicationServlet. valid_until_ms = self.clock.time_msec() + self.session_lifetime refresh_token = None - - if should_issue_refresh_token: - refresh_token = "FAKEREFRESHTOKEN" - valid_until_ms = self.clock.time_msec() + 60 * 1000 + refresh_token_id = None registered_device_id = await self.device_handler.check_device_registered( user_id, device_id, initial_display_name @@ -752,11 +749,21 @@ class and RegisterDeviceReplicationServlet. user_id, ["guest = true"] ) else: + if should_issue_refresh_token: + ( + refresh_token, + refresh_token_id, + ) = await self._auth_handler.get_refresh_token_for_user_id( + user_id, device_id=registered_device_id, + ) + valid_until_ms = self.clock.time_msec() + 60 * 1000 + access_token = await self._auth_handler.get_access_token_for_user_id( user_id, device_id=registered_device_id, valid_until_ms=valid_until_ms, is_appservice_ghost=is_appservice_ghost, + refresh_token_id=refresh_token_id, ) return { diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py index b1514929a50a..dbe3aa3e51eb 100644 --- a/synapse/rest/client/v1/login.py +++ b/synapse/rest/client/v1/login.py @@ -446,6 +446,40 @@ def _get_auth_flow_dict_for_idp( return e +class RefreshTokenServlet(RestServlet): + PATTERNS = client_patterns( + "/org.matrix.msc2918.refresh_token/refresh$", releases=(), unstable=True + ) + + def __init__(self, hs: "HomeServer"): + self._auth_handler = hs.get_auth_handler() + self._clock = hs.get_clock() + + async def on_POST( + self, request: SynapseRequest, + ): + refresh_submission = parse_json_object_from_request(request) + + try: + token = refresh_submission["refresh_token"] + valid_until_ms = self._clock.time_msec() + 60 * 1000 + access_token, refresh_token = await self._auth_handler.refresh_token( + token, valid_until_ms + ) + expires_in_ms = valid_until_ms - self._clock.time_msec() + return ( + 200, + { + "access_token": access_token, + "refresh_token": refresh_token, + "expires_in": int(expires_in_ms / 1000), + }, + ) + + except KeyError: + raise SynapseError(400, "Missing JSON keys.") + + class SsoRedirectServlet(RestServlet): PATTERNS = list(client_patterns("/login/(cas|sso)/redirect$", v1=True)) + [ re.compile( @@ -553,6 +587,7 @@ async def on_GET(self, request: SynapseRequest) -> None: def register_servlets(hs, http_server): LoginRestServlet(hs).register(http_server) + RefreshTokenServlet(hs).register(http_server) SsoRedirectServlet(hs).register(http_server) if hs.config.cas_enabled: CasTicketServlet(hs).register(http_server) diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index 90a8f664ef95..ede29cd5bedc 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -69,6 +69,14 @@ def _default_token_owner(self): return self.user_id +@attr.s(frozen=True, slots=True) +class RefreshTokenLookupResult: + user_id = attr.ib(type=str) + device_id = attr.ib(type=str) + token_id = attr.ib(type=int) + valid = attr.ib(type=bool) + + class RegistrationWorkerStore(CacheInvalidationWorkerStore): def __init__( self, @@ -1042,6 +1050,32 @@ async def update_access_token_last_validated(self, token_id: int) -> None: desc="update_access_token_last_validated", ) + async def lookup_refresh_token( + self, token: str + ) -> Optional[RefreshTokenLookupResult]: + d = await self.db_pool.simple_select_one( + "refresh_tokens", + {"token": token}, + ["id", "user_id", "device_id", "valid"], + allow_none=True, + desc="lookup_refresh_token", + ) + + if d is not None: + d["token_id"] = d["id"] + del d["id"] + return RefreshTokenLookupResult(**d) + + return None + + async def invalidate_refresh_token(self, token_id: int) -> None: + await self.db_pool.simple_update_one( + "refresh_tokens", + {"id": token_id}, + {"valid": False}, + desc="invalidate_refresh_token", + ) + class RegistrationBackgroundUpdateStore(RegistrationWorkerStore): def __init__( @@ -1233,6 +1267,7 @@ def __init__(self, database: DatabasePool, db_conn: Connection, hs: "HomeServer" self._ignore_unknown_session_error = hs.config.request_token_inhibit_3pid_errors self._access_tokens_id_gen = IdGenerator(db_conn, "access_tokens", "id") + self._refresh_tokens_id_gen = IdGenerator(db_conn, "refresh_tokens", "id") async def add_access_token_to_user( self, @@ -1241,6 +1276,7 @@ async def add_access_token_to_user( device_id: Optional[str], valid_until_ms: Optional[int], puppets_user_id: Optional[str] = None, + refresh_token_id: Optional[int] = None, ) -> int: """Adds an access token for the given user. @@ -1267,6 +1303,26 @@ async def add_access_token_to_user( "valid_until_ms": valid_until_ms, "puppets_user_id": puppets_user_id, "last_validated": now, + "refresh_token_id": refresh_token_id, + }, + desc="add_access_token_to_user", + ) + + return next_id + + async def add_refresh_token_to_user( + self, user_id: str, token: str, device_id: Optional[str], + ) -> int: + next_id = self._refresh_tokens_id_gen.get_next() + + await self.db_pool.simple_insert( + "refresh_tokens", + { + "id": next_id, + "user_id": user_id, + "device_id": device_id, + "token": token, + "valid": True, }, desc="add_access_token_to_user", ) @@ -1569,6 +1625,14 @@ def f(txn): await self.db_pool.runInteraction("delete_access_token", f) + async def delete_refresh_token(self, refresh_token: str) -> None: + def f(txn): + self.db_pool.simple_delete_one_txn( + txn, table="refresh_tokens", keyvalues={"token": refresh_token} + ) + + await self.db_pool.runInteraction("delete_refresh_token", f) + async def add_user_pending_deactivation(self, user_id: str) -> None: """ Adds a user to the table of users who need to be parted from all the rooms they're diff --git a/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.sqlite b/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.sqlite new file mode 100644 index 000000000000..10007ec16d33 --- /dev/null +++ b/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.sqlite @@ -0,0 +1,31 @@ +CREATE TABLE refresh_tokens ( + id BIGINT PRIMARY KEY, + user_id TEXT NOT NULL, + device_id TEXT, + token TEXT NOT NULL, + valid BOOLEAN NOT NULL, + UNIQUE(token) +); + +-- Adding the refresh_token column to access tokens +CREATE TABLE "access_tokens2" ( + id BIGINT PRIMARY KEY, + user_id TEXT NOT NULL, + device_id TEXT, + token TEXT NOT NULL, + valid_until_ms BIGINT, + puppets_user_id TEXT, + last_validated BIGINT, + refresh_token_id BIGINT, + UNIQUE(token), + FOREIGN KEY (refresh_token_id) + REFERENCES refresh_tokens (id) ON DELETE CASCADE +); + +INSERT INTO access_tokens2(id, user_id, device_id, token, valid_until_ms, puppets_user_id, last_validated) + SELECT id, user_id, device_id, token, valid_until_ms, puppets_user_id, last_validated FROM access_tokens; + +DROP TABLE access_tokens; +ALTER TABLE access_tokens2 RENAME TO access_tokens; + +CREATE INDEX access_tokens_device_id ON access_tokens (user_id, device_id); diff --git a/synapse/storage/prepare_database.py b/synapse/storage/prepare_database.py index c7f0b8ccb530..2dfe12d2c9ec 100644 --- a/synapse/storage/prepare_database.py +++ b/synapse/storage/prepare_database.py @@ -35,7 +35,7 @@ # Remember to update this number every time a change is made to database # schema files, so the users will be informed on server restarts. -SCHEMA_VERSION = 59 +SCHEMA_VERSION = 60 dir_path = os.path.abspath(os.path.dirname(__file__)) From 358da2265fef3fa59e971d8f79520d7fa0593d0d Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 26 Mar 2021 14:18:56 +0100 Subject: [PATCH 03/46] MSC2918: Changelog --- changelog.d/9450.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/9450.feature diff --git a/changelog.d/9450.feature b/changelog.d/9450.feature new file mode 100644 index 000000000000..455936a41d64 --- /dev/null +++ b/changelog.d/9450.feature @@ -0,0 +1 @@ +Implement refresh tokens as specified by [MSC2918](https://github.com/matrix-org/matrix-doc/pull/2918). From f53466e6db8034c6a8da8fc0ae3d23bc26c12700 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 26 Mar 2021 14:31:28 +0100 Subject: [PATCH 04/46] MSC2918: fix mypy and lint errors --- synapse/handlers/auth.py | 14 ++++++++++---- synapse/handlers/register.py | 7 ++++--- synapse/rest/client/v1/login.py | 3 ++- synapse/storage/databases/main/registration.py | 5 ++++- 4 files changed, 20 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index ec70fe391276..5a56de5deeb0 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -71,8 +71,8 @@ from synapse.util.threepids import canonicalise_email if TYPE_CHECKING: - from synapse.server import HomeServer from synapse.rest.client.v1.login import LoginResponse + from synapse.server import HomeServer logger = logging.getLogger(__name__) @@ -773,7 +773,9 @@ def _auth_dict_for_flows( } async def refresh_token( - self, refresh_token: str, valid_until_ms: Optional[int], + self, + refresh_token: str, + valid_until_ms: Optional[int], ) -> Tuple[str, str]: existing_token = await self.store.lookup_refresh_token(refresh_token) if existing_token is None: @@ -798,11 +800,15 @@ async def refresh_token( return access_token, new_refresh_token async def get_refresh_token_for_user_id( - self, user_id: str, device_id: Optional[str], + self, + user_id: str, + device_id: Optional[str], ) -> Tuple[str, int]: refresh_token = self.macaroon_gen.generate_refresh_token(user_id) refresh_token_id = await self.store.add_refresh_token_to_user( - user_id=user_id, token=refresh_token, device_id=device_id, + user_id=user_id, + token=refresh_token, + device_id=device_id, ) return refresh_token, refresh_token_id diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 65209a8fc040..9cab679d0e65 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -16,7 +16,7 @@ """Contains functions for registering clients.""" import logging -from typing import TYPE_CHECKING, Dict, Iterable, List, Optional, Tuple +from typing import TYPE_CHECKING, Iterable, List, Optional, Tuple from prometheus_client import Counter from typing_extensions import TypedDict @@ -61,7 +61,7 @@ { "device_id": str, "access_token": str, - "valid_until_ms": int, + "valid_until_ms": Optional[int], "refresh_token": Optional[str], }, ) @@ -754,7 +754,8 @@ class and RegisterDeviceReplicationServlet. refresh_token, refresh_token_id, ) = await self._auth_handler.get_refresh_token_for_user_id( - user_id, device_id=registered_device_id, + user_id, + device_id=registered_device_id, ) valid_until_ms = self.clock.time_msec() + 60 * 1000 diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py index dbe3aa3e51eb..ab8a91db6514 100644 --- a/synapse/rest/client/v1/login.py +++ b/synapse/rest/client/v1/login.py @@ -456,7 +456,8 @@ def __init__(self, hs: "HomeServer"): self._clock = hs.get_clock() async def on_POST( - self, request: SynapseRequest, + self, + request: SynapseRequest, ): refresh_submission = parse_json_object_from_request(request) diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index ede29cd5bedc..ef52299c6ce3 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -1311,7 +1311,10 @@ async def add_access_token_to_user( return next_id async def add_refresh_token_to_user( - self, user_id: str, token: str, device_id: Optional[str], + self, + user_id: str, + token: str, + device_id: Optional[str], ) -> int: next_id = self._refresh_tokens_id_gen.get_next() From 324d7bfa5f1c8d175e28cabf8e73054ccd2cddd5 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 26 Mar 2021 15:04:51 +0100 Subject: [PATCH 05/46] MSC2918: add PostgreSQL schema --- .../schema/delta/60/01refresh_tokens.sql.postgres | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.postgres diff --git a/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.postgres b/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.postgres new file mode 100644 index 000000000000..655370e9ef07 --- /dev/null +++ b/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.postgres @@ -0,0 +1,13 @@ +CREATE TABLE refresh_tokens ( + id BIGINT PRIMARY KEY, + user_id TEXT NOT NULL, + device_id TEXT, + token TEXT NOT NULL, + valid BOOLEAN NOT NULL, + UNIQUE(token) +); + +CREATE SEQUENCE IF NOT EXISTS refresh_token_id; + +ALTER TABLE "access_tokens" + ADD COLUMN refresh_token_id BIGINT REFERENCES refresh_tokens (id) ON DELETE CASCADE; From 450a962e0a43d7f9671b0517d6bccd4f76b4ccfa Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 9 Apr 2021 17:58:26 +0200 Subject: [PATCH 06/46] MSC2918: do not invalidate refresh token immediately & fix tests This checks for child token usage to validate the refresh token validity. This means that a token can be refreshed multiple times until one of the child tokens gets used. Signed-off-by: Quentin Gliech --- docs/sample_config.yaml | 4 ++ synapse/config/registration.py | 8 +++ synapse/handlers/auth.py | 4 +- synapse/handlers/register.py | 3 +- synapse/rest/client/v2_alpha/register.py | 26 +++++---- .../storage/databases/main/registration.py | 58 ++++++++++++------- .../delta/60/01refresh_tokens.sql.postgres | 2 +- .../delta/60/01refresh_tokens.sql.sqlite | 2 +- tests/api/test_auth.py | 1 + tests/handlers/test_device.py | 2 +- 10 files changed, 74 insertions(+), 36 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 9182dcd98716..9bb87d763e07 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1249,6 +1249,10 @@ account_validity: # #session_lifetime: 24h +# MSC2918 +# TODO: docs +access_token_lifetime: 5m + # The user must provide all of the below types of 3PID when registering. # #registrations_require_3pid: diff --git a/synapse/config/registration.py b/synapse/config/registration.py index f27d1e14acba..7eb07e9a5523 100644 --- a/synapse/config/registration.py +++ b/synapse/config/registration.py @@ -186,6 +186,10 @@ def read_config(self, config, **kwargs): session_lifetime = self.parse_duration(session_lifetime) self.session_lifetime = session_lifetime + access_token_lifetime = config.get("access_token_lifetime", "5m") + access_token_lifetime = self.parse_duration(access_token_lifetime) + self.access_token_lifetime = access_token_lifetime # type: int + # The success template used during fallback auth. self.fallback_success_template = self.read_template("auth_success.html") @@ -281,6 +285,10 @@ def generate_config_section(self, generate_secrets=False, **kwargs): # By default, this is infinite. # #session_lifetime: 24h + + # MSC2918 + # TODO: docs + access_token_lifetime: 5m # The user must provide all of the below types of 3PID when registering. # diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 5a56de5deeb0..0575fbc779a0 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -781,7 +781,7 @@ async def refresh_token( if existing_token is None: raise SynapseError(400, "refresh token does not exist") - if not existing_token.valid: + if existing_token.has_next_access_token_been_used or existing_token.has_next_refresh_token_been_refreshed: raise SynapseError(400, "refresh token isn't valid anymore") ( @@ -796,7 +796,7 @@ async def refresh_token( valid_until_ms=valid_until_ms, refresh_token_id=new_refresh_token_id, ) - await self.store.invalidate_refresh_token(existing_token.token_id) + await self.store.replace_refresh_token(existing_token.token_id, new_refresh_token_id) return access_token, new_refresh_token async def get_refresh_token_for_user_id( diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 9cab679d0e65..94a035b5d736 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -97,6 +97,7 @@ def __init__(self, hs: "HomeServer"): self.pusher_pool = hs.get_pusherpool() self.session_lifetime = hs.config.session_lifetime + self.access_token_lifetime = hs.config.access_token_lifetime async def check_username( self, @@ -757,7 +758,7 @@ class and RegisterDeviceReplicationServlet. user_id, device_id=registered_device_id, ) - valid_until_ms = self.clock.time_msec() + 60 * 1000 + valid_until_ms = self.clock.time_msec() + self.access_token_lifetime access_token = await self._auth_handler.get_access_token_for_user_id( user_id, diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index 08e6f1b516b1..96830162c4ed 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -724,19 +724,25 @@ async def _do_guest_registration(self, params, address=None): # we have nowhere to store it. device_id = synapse.api.auth.GUEST_DEVICE_ID initial_display_name = params.get("initial_device_display_name") - device_id, access_token = await self.registration_handler.register_device( + device_id, access_token, valid_until_ms, refresh_token = await self.registration_handler.register_device( user_id, device_id, initial_display_name, is_guest=True ) - return ( - 200, - { - "user_id": user_id, - "device_id": device_id, - "access_token": access_token, - "home_server": self.hs.hostname, - }, - ) + result = { + "user_id": user_id, + "device_id": device_id, + "access_token": access_token, + "home_server": self.hs.hostname, + } + + if valid_until_ms is not None: + expires_in_ms = valid_until_ms - self.clock.time_msec() + result["expires_in"] = int(expires_in_ms / 1000) + + if refresh_token is not None: + result["refresh_token"] = refresh_token + + return 200, result def _calculate_registration_flows( diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index ef52299c6ce3..ec9a1d84ab5d 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -74,7 +74,9 @@ class RefreshTokenLookupResult: user_id = attr.ib(type=str) device_id = attr.ib(type=str) token_id = attr.ib(type=int) - valid = attr.ib(type=bool) + next_token_id = attr.ib(type=int) + has_next_refresh_token_been_refreshed = attr.ib(type=bool) + has_next_access_token_been_used = attr.ib(type=bool) class RegistrationWorkerStore(CacheInvalidationWorkerStore): @@ -1050,30 +1052,46 @@ async def update_access_token_last_validated(self, token_id: int) -> None: desc="update_access_token_last_validated", ) - async def lookup_refresh_token( - self, token: str - ) -> Optional[RefreshTokenLookupResult]: - d = await self.db_pool.simple_select_one( - "refresh_tokens", - {"token": token}, - ["id", "user_id", "device_id", "valid"], - allow_none=True, - desc="lookup_refresh_token", - ) + async def lookup_refresh_token(self, token: str) -> Optional[RefreshTokenLookupResult]: + """Lookup a refresh token with hints about its validity. + """ - if d is not None: - d["token_id"] = d["id"] - del d["id"] - return RefreshTokenLookupResult(**d) + def _lookup_refresh_token_txn(txn) -> Optional[RefreshTokenLookupResult]: + txn.execute(""" + SELECT + rt.id token_id, + rt.user_id, + rt.device_id, + rt.next_token_id, + (nrt.next_token_id IS NOT NULL) has_next_refresh_token_been_refreshed, + (at.last_validated IS NOT NULL) has_next_access_token_been_used + FROM refresh_tokens rt + LEFT JOIN refresh_tokens nrt ON rt.next_token_id = nrt.id + LEFT JOIN access_tokens at ON at.refresh_token_id = nrt.id + WHERE rt.token = ? + """, (token,)) + row = txn.fetchone() + + if row is None: + return None - return None + return RefreshTokenLookupResult( + token_id=row[0], + user_id=row[1], + device_id=row[2], + next_token_id=row[3], + has_next_refresh_token_been_refreshed=row[4], + has_next_access_token_been_used=row[5], + ) + + return await self.db_pool.runInteraction("lookup_refresh_token", _lookup_refresh_token_txn) - async def invalidate_refresh_token(self, token_id: int) -> None: + async def replace_refresh_token(self, token_id: int, next_token_id: int) -> None: await self.db_pool.simple_update_one( "refresh_tokens", {"id": token_id}, - {"valid": False}, - desc="invalidate_refresh_token", + {"next_token_id": next_token_id}, + desc="replace_refresh_token", ) @@ -1325,7 +1343,7 @@ async def add_refresh_token_to_user( "user_id": user_id, "device_id": device_id, "token": token, - "valid": True, + "next_token_id": None, }, desc="add_access_token_to_user", ) diff --git a/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.postgres b/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.postgres index 655370e9ef07..7dd5758b3bdc 100644 --- a/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.postgres +++ b/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.postgres @@ -3,7 +3,7 @@ CREATE TABLE refresh_tokens ( user_id TEXT NOT NULL, device_id TEXT, token TEXT NOT NULL, - valid BOOLEAN NOT NULL, + replaced_by BIGINT REFERENCES refresh_tokens (id) ON DELETE CASCADE, UNIQUE(token) ); diff --git a/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.sqlite b/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.sqlite index 10007ec16d33..bdef51c97317 100644 --- a/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.sqlite +++ b/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.sqlite @@ -3,7 +3,7 @@ CREATE TABLE refresh_tokens ( user_id TEXT NOT NULL, device_id TEXT, token TEXT NOT NULL, - valid BOOLEAN NOT NULL, + replaced_by BIGINT REFERENCES refresh_tokens (id) ON DELETE CASCADE, UNIQUE(token) ); diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py index 28d77f0ca238..5d6a39300635 100644 --- a/tests/api/test_auth.py +++ b/tests/api/test_auth.py @@ -270,6 +270,7 @@ def test_cannot_use_regular_token_as_guest(self): device_id="DEVICE", valid_until_ms=None, puppets_user_id=None, + refresh_token_id=None, ) async def get_user(tok): diff --git a/tests/handlers/test_device.py b/tests/handlers/test_device.py index 821629bc38a3..764a56f4746f 100644 --- a/tests/handlers/test_device.py +++ b/tests/handlers/test_device.py @@ -258,7 +258,7 @@ def test_dehydrate_and_rehydrate_device(self): self.assertEqual(device_data, {"device_data": {"foo": "bar"}}) # Create a new login for the user and dehydrated the device - device_id, access_token = self.get_success( + device_id, access_token, _expiration_time, _refresh_token = self.get_success( self.registration.register_device( user_id=user_id, device_id=None, From 022485e84161b43ae0a540499ffb206d51c4a69d Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 9 Apr 2021 20:30:32 +0200 Subject: [PATCH 07/46] MSC2918: lint fixes Signed-off-by: Quentin Gliech --- synapse/config/registration.py | 2 +- synapse/handlers/auth.py | 9 +++++++-- synapse/handlers/register.py | 1 + synapse/rest/client/v1/login.py | 11 ++++++----- synapse/rest/client/v2_alpha/register.py | 7 ++++++- synapse/storage/databases/main/registration.py | 18 ++++++++++++------ 6 files changed, 33 insertions(+), 15 deletions(-) diff --git a/synapse/config/registration.py b/synapse/config/registration.py index 7eb07e9a5523..55aa73944b24 100644 --- a/synapse/config/registration.py +++ b/synapse/config/registration.py @@ -285,7 +285,7 @@ def generate_config_section(self, generate_secrets=False, **kwargs): # By default, this is infinite. # #session_lifetime: 24h - + # MSC2918 # TODO: docs access_token_lifetime: 5m diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 0575fbc779a0..3a6251d0543e 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -781,7 +781,10 @@ async def refresh_token( if existing_token is None: raise SynapseError(400, "refresh token does not exist") - if existing_token.has_next_access_token_been_used or existing_token.has_next_refresh_token_been_refreshed: + if ( + existing_token.has_next_access_token_been_used + or existing_token.has_next_refresh_token_been_refreshed + ): raise SynapseError(400, "refresh token isn't valid anymore") ( @@ -796,7 +799,9 @@ async def refresh_token( valid_until_ms=valid_until_ms, refresh_token_id=new_refresh_token_id, ) - await self.store.replace_refresh_token(existing_token.token_id, new_refresh_token_id) + await self.store.replace_refresh_token( + existing_token.token_id, new_refresh_token_id + ) return access_token, new_refresh_token async def get_refresh_token_for_user_id( diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 94a035b5d736..a2d7612b29cf 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -691,6 +691,7 @@ async def register_device( is_guest: Whether this is a guest account auth_provider_id: The SSO IdP the user used, if any (just used for the prometheus metrics). + should_issue_refresh_token: Whether it should also issue a refresh token Returns: Tuple of device ID, access token, access token expiration time and refresh token """ diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py index ab8a91db6514..e76010ce27a5 100644 --- a/synapse/rest/client/v1/login.py +++ b/synapse/rest/client/v1/login.py @@ -15,7 +15,7 @@ import logging import re -from typing import TYPE_CHECKING, Any, Awaitable, Callable, Dict, List, Optional, cast +from typing import TYPE_CHECKING, Any, Awaitable, Callable, Dict, Optional from typing_extensions import TypedDict @@ -158,11 +158,12 @@ def on_GET(self, request: SynapseRequest): async def on_POST(self, request: SynapseRequest): login_submission = parse_json_object_from_request(request) - refresh_token_param = cast( - List[bytes], - request.args.get(bytes(LoginRestServlet.REFRESH_TOKEN_PARAM, "utf-8"), []), + param = bytes(LoginRestServlet.REFRESH_TOKEN_PARAM, "utf-8") + should_issue_refresh_token = ( + request.args is not None + and param in request.args + and request.args[param][0] == b"true" ) - should_issue_refresh_token = any((i == b"true" for i in refresh_token_param)) try: if login_submission["type"] == LoginRestServlet.APPSERVICE_TYPE: diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index 96830162c4ed..00a4e85dfadd 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -724,7 +724,12 @@ async def _do_guest_registration(self, params, address=None): # we have nowhere to store it. device_id = synapse.api.auth.GUEST_DEVICE_ID initial_display_name = params.get("initial_device_display_name") - device_id, access_token, valid_until_ms, refresh_token = await self.registration_handler.register_device( + ( + device_id, + access_token, + valid_until_ms, + refresh_token, + ) = await self.registration_handler.register_device( user_id, device_id, initial_display_name, is_guest=True ) diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index ec9a1d84ab5d..1f53fb940fb2 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -1052,12 +1052,14 @@ async def update_access_token_last_validated(self, token_id: int) -> None: desc="update_access_token_last_validated", ) - async def lookup_refresh_token(self, token: str) -> Optional[RefreshTokenLookupResult]: - """Lookup a refresh token with hints about its validity. - """ + async def lookup_refresh_token( + self, token: str + ) -> Optional[RefreshTokenLookupResult]: + """Lookup a refresh token with hints about its validity.""" def _lookup_refresh_token_txn(txn) -> Optional[RefreshTokenLookupResult]: - txn.execute(""" + txn.execute( + """ SELECT rt.id token_id, rt.user_id, @@ -1069,7 +1071,9 @@ def _lookup_refresh_token_txn(txn) -> Optional[RefreshTokenLookupResult]: LEFT JOIN refresh_tokens nrt ON rt.next_token_id = nrt.id LEFT JOIN access_tokens at ON at.refresh_token_id = nrt.id WHERE rt.token = ? - """, (token,)) + """, + (token,), + ) row = txn.fetchone() if row is None: @@ -1084,7 +1088,9 @@ def _lookup_refresh_token_txn(txn) -> Optional[RefreshTokenLookupResult]: has_next_access_token_been_used=row[5], ) - return await self.db_pool.runInteraction("lookup_refresh_token", _lookup_refresh_token_txn) + return await self.db_pool.runInteraction( + "lookup_refresh_token", _lookup_refresh_token_txn + ) async def replace_refresh_token(self, token_id: int, next_token_id: int) -> None: await self.db_pool.simple_update_one( From 51ba1c3129763dbeefdfaaedac2ffd727f648b7a Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 22 Apr 2021 15:00:56 +0200 Subject: [PATCH 08/46] MSC2918: also delete refresh tokens when logging out --- synapse/storage/databases/main/registration.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index 1f53fb940fb2..8418560c4f0f 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -1598,7 +1598,7 @@ async def user_delete_access_tokens( device_id: Optional[str] = None, ) -> List[Tuple[str, int, Optional[str]]]: """ - Invalidate access tokens belonging to a user + Invalidate access and refresh tokens belonging to a user Args: user_id: ID of user the tokens belong to @@ -1618,7 +1618,13 @@ def f(txn): items = keyvalues.items() where_clause = " AND ".join(k + " = ?" for k, _ in items) values = [v for _, v in items] # type: List[Union[str, int]] + # Conveniently, refresh_tokens and access_tokens both use the user_id and device_id fields. Only caveat + # is the `except_token_id` param that is tricky to get right, so for now we're just using the same where + # clause and values before we handle that. This seems to be only used in the "set password" handler. + refresh_where_clause = where_clause + refresh_values = values.copy() if except_token_id: + # TODO: support that for refresh tokens where_clause += " AND id != ?" values.append(except_token_id) @@ -1636,6 +1642,11 @@ def f(txn): txn.execute("DELETE FROM access_tokens WHERE %s" % where_clause, values) + txn.execute( + "DELETE FROM refresh_tokens WHERE %s" % refresh_where_clause, + refresh_values, + ) + return tokens_and_devices return await self.db_pool.runInteraction("user_delete_access_tokens", f) From d281f7e7fde59e3b3522488c47c184f54735da59 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 22 Apr 2021 17:02:07 +0200 Subject: [PATCH 09/46] MSC2918: fix field name in migrations --- .../main/schema/delta/60/01refresh_tokens.sql.postgres | 2 +- .../databases/main/schema/delta/60/01refresh_tokens.sql.sqlite | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.postgres b/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.postgres index 7dd5758b3bdc..40bbdb82a4c4 100644 --- a/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.postgres +++ b/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.postgres @@ -3,7 +3,7 @@ CREATE TABLE refresh_tokens ( user_id TEXT NOT NULL, device_id TEXT, token TEXT NOT NULL, - replaced_by BIGINT REFERENCES refresh_tokens (id) ON DELETE CASCADE, + next_token_id BIGINT REFERENCES refresh_tokens (id) ON DELETE CASCADE, UNIQUE(token) ); diff --git a/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.sqlite b/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.sqlite index bdef51c97317..a8835a35c4a5 100644 --- a/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.sqlite +++ b/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.sqlite @@ -3,7 +3,7 @@ CREATE TABLE refresh_tokens ( user_id TEXT NOT NULL, device_id TEXT, token TEXT NOT NULL, - replaced_by BIGINT REFERENCES refresh_tokens (id) ON DELETE CASCADE, + next_token_id BIGINT REFERENCES refresh_tokens (id) ON DELETE CASCADE, UNIQUE(token) ); From f499d6349821e4c367a79e37263a38ea2879bd4b Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Wed, 5 May 2021 10:49:00 +0200 Subject: [PATCH 10/46] MSC2918: merge SQLite and PostgreSQL schema deltas This also rolls back the SCHEMA_VERSION to 59 since this does not introduce any breaking database change. Signed-off-by: Quentin Gliech --- .../main/schema/delta/59/12refresh_tokens.sql | 11 +++++++ .../delta/59/12refresh_tokens.sql.postgres | 1 + .../delta/60/01refresh_tokens.sql.postgres | 13 -------- .../delta/60/01refresh_tokens.sql.sqlite | 31 ------------------- synapse/storage/prepare_database.py | 2 +- 5 files changed, 13 insertions(+), 45 deletions(-) create mode 100644 synapse/storage/databases/main/schema/delta/59/12refresh_tokens.sql create mode 100644 synapse/storage/databases/main/schema/delta/59/12refresh_tokens.sql.postgres delete mode 100644 synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.postgres delete mode 100644 synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.sqlite diff --git a/synapse/storage/databases/main/schema/delta/59/12refresh_tokens.sql b/synapse/storage/databases/main/schema/delta/59/12refresh_tokens.sql new file mode 100644 index 000000000000..95ff9cfc3575 --- /dev/null +++ b/synapse/storage/databases/main/schema/delta/59/12refresh_tokens.sql @@ -0,0 +1,11 @@ +CREATE TABLE refresh_tokens ( + id BIGINT PRIMARY KEY, + user_id TEXT NOT NULL, + device_id TEXT, + token TEXT NOT NULL, + next_token_id BIGINT REFERENCES refresh_tokens (id) ON DELETE CASCADE, + UNIQUE(token) +); + +ALTER TABLE "access_tokens" + ADD COLUMN refresh_token_id BIGINT REFERENCES refresh_tokens (id) ON DELETE CASCADE; diff --git a/synapse/storage/databases/main/schema/delta/59/12refresh_tokens.sql.postgres b/synapse/storage/databases/main/schema/delta/59/12refresh_tokens.sql.postgres new file mode 100644 index 000000000000..9a566a59699a --- /dev/null +++ b/synapse/storage/databases/main/schema/delta/59/12refresh_tokens.sql.postgres @@ -0,0 +1 @@ +CREATE SEQUENCE IF NOT EXISTS refresh_token_id; \ No newline at end of file diff --git a/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.postgres b/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.postgres deleted file mode 100644 index 40bbdb82a4c4..000000000000 --- a/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.postgres +++ /dev/null @@ -1,13 +0,0 @@ -CREATE TABLE refresh_tokens ( - id BIGINT PRIMARY KEY, - user_id TEXT NOT NULL, - device_id TEXT, - token TEXT NOT NULL, - next_token_id BIGINT REFERENCES refresh_tokens (id) ON DELETE CASCADE, - UNIQUE(token) -); - -CREATE SEQUENCE IF NOT EXISTS refresh_token_id; - -ALTER TABLE "access_tokens" - ADD COLUMN refresh_token_id BIGINT REFERENCES refresh_tokens (id) ON DELETE CASCADE; diff --git a/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.sqlite b/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.sqlite deleted file mode 100644 index a8835a35c4a5..000000000000 --- a/synapse/storage/databases/main/schema/delta/60/01refresh_tokens.sql.sqlite +++ /dev/null @@ -1,31 +0,0 @@ -CREATE TABLE refresh_tokens ( - id BIGINT PRIMARY KEY, - user_id TEXT NOT NULL, - device_id TEXT, - token TEXT NOT NULL, - next_token_id BIGINT REFERENCES refresh_tokens (id) ON DELETE CASCADE, - UNIQUE(token) -); - --- Adding the refresh_token column to access tokens -CREATE TABLE "access_tokens2" ( - id BIGINT PRIMARY KEY, - user_id TEXT NOT NULL, - device_id TEXT, - token TEXT NOT NULL, - valid_until_ms BIGINT, - puppets_user_id TEXT, - last_validated BIGINT, - refresh_token_id BIGINT, - UNIQUE(token), - FOREIGN KEY (refresh_token_id) - REFERENCES refresh_tokens (id) ON DELETE CASCADE -); - -INSERT INTO access_tokens2(id, user_id, device_id, token, valid_until_ms, puppets_user_id, last_validated) - SELECT id, user_id, device_id, token, valid_until_ms, puppets_user_id, last_validated FROM access_tokens; - -DROP TABLE access_tokens; -ALTER TABLE access_tokens2 RENAME TO access_tokens; - -CREATE INDEX access_tokens_device_id ON access_tokens (user_id, device_id); diff --git a/synapse/storage/prepare_database.py b/synapse/storage/prepare_database.py index 2dfe12d2c9ec..c7f0b8ccb530 100644 --- a/synapse/storage/prepare_database.py +++ b/synapse/storage/prepare_database.py @@ -35,7 +35,7 @@ # Remember to update this number every time a change is made to database # schema files, so the users will be informed on server restarts. -SCHEMA_VERSION = 60 +SCHEMA_VERSION = 59 dir_path = os.path.abspath(os.path.dirname(__file__)) From e402a077ede7d0144e462fc6006a5c1476023ae4 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Wed, 5 May 2021 10:53:48 +0200 Subject: [PATCH 11/46] MSC2918: fix sample config Signed-off-by: Quentin Gliech --- docs/sample_config.yaml | 2 +- synapse/config/registration.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 9bb87d763e07..8708cb86fdc9 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1251,7 +1251,7 @@ account_validity: # MSC2918 # TODO: docs -access_token_lifetime: 5m +#access_token_lifetime: 5m # The user must provide all of the below types of 3PID when registering. # diff --git a/synapse/config/registration.py b/synapse/config/registration.py index 55aa73944b24..46345c184967 100644 --- a/synapse/config/registration.py +++ b/synapse/config/registration.py @@ -188,7 +188,7 @@ def read_config(self, config, **kwargs): access_token_lifetime = config.get("access_token_lifetime", "5m") access_token_lifetime = self.parse_duration(access_token_lifetime) - self.access_token_lifetime = access_token_lifetime # type: int + self.access_token_lifetime = access_token_lifetime # The success template used during fallback auth. self.fallback_success_template = self.read_template("auth_success.html") @@ -288,7 +288,7 @@ def generate_config_section(self, generate_secrets=False, **kwargs): # MSC2918 # TODO: docs - access_token_lifetime: 5m + #access_token_lifetime: 5m # The user must provide all of the below types of 3PID when registering. # From adc6eabfd88f3260a694b4038e8d2255cb151a02 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Wed, 5 May 2021 12:20:02 +0200 Subject: [PATCH 12/46] MSC2918: use parse_boolean to get query parameter value Signed-off-by: Quentin Gliech --- synapse/rest/client/v1/login.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py index e76010ce27a5..6ca47965a8f4 100644 --- a/synapse/rest/client/v1/login.py +++ b/synapse/rest/client/v1/login.py @@ -19,6 +19,7 @@ from typing_extensions import TypedDict +import synapse.http.servlet from synapse.api.errors import Codes, LoginError, SynapseError from synapse.api.ratelimiting import Ratelimiter from synapse.api.urls import CLIENT_API_PREFIX @@ -30,6 +31,7 @@ RestServlet, parse_json_object_from_request, parse_string, + parse_boolean, ) from synapse.http.site import SynapseRequest from synapse.rest.client.v2_alpha._base import client_patterns @@ -158,12 +160,7 @@ def on_GET(self, request: SynapseRequest): async def on_POST(self, request: SynapseRequest): login_submission = parse_json_object_from_request(request) - param = bytes(LoginRestServlet.REFRESH_TOKEN_PARAM, "utf-8") - should_issue_refresh_token = ( - request.args is not None - and param in request.args - and request.args[param][0] == b"true" - ) + should_issue_refresh_token = parse_boolean(request, name=LoginRestServlet.REFRESH_TOKEN_PARAM, default=False) try: if login_submission["type"] == LoginRestServlet.APPSERVICE_TYPE: From 6963fe0161978c8260eb41371cb118d67370c4a2 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Wed, 5 May 2021 12:20:24 +0200 Subject: [PATCH 13/46] MSC2918: use attr.s instead of TypedDict Signed-off-by: Quentin Gliech --- synapse/handlers/register.py | 40 +++++++++++++++++------------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index a2d7612b29cf..5698787d528d 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -19,7 +19,7 @@ from typing import TYPE_CHECKING, Iterable, List, Optional, Tuple from prometheus_client import Counter -from typing_extensions import TypedDict +import attr from synapse import types from synapse.api.constants import MAX_USERID_LENGTH, EventTypes, JoinRules, LoginType @@ -56,15 +56,13 @@ ["guest", "auth_provider"], ) -LoginDict = TypedDict( - "LoginDict", - { - "device_id": str, - "access_token": str, - "valid_until_ms": Optional[int], - "refresh_token": Optional[str], - }, -) + +@attr.s(frozen=True, slots=True) +class DeviceRegistrationResult: + device_id = attr.ib(type=str) + access_token = attr.ib(type=str) + valid_until_ms = attr.ib(type=Optional[int]) + refresh_token = attr.ib(type=Optional[str]) class RegistrationHandler(BaseHandler): @@ -710,10 +708,10 @@ async def register_device( ).inc() return ( - res["device_id"], - res["access_token"], - res["valid_until_ms"], - res["refresh_token"], + res.device_id, + res.access_token, + res.valid_until_ms, + res.refresh_token, ) async def register_device_inner( @@ -724,7 +722,7 @@ async def register_device_inner( is_guest: bool = False, is_appservice_ghost: bool = False, should_issue_refresh_token: bool = False, - ) -> LoginDict: + ) -> DeviceRegistrationResult: """Helper for register_device Does the bits that need doing on the main process. Not for use outside this @@ -769,12 +767,12 @@ class and RegisterDeviceReplicationServlet. refresh_token_id=refresh_token_id, ) - return { - "device_id": registered_device_id, - "access_token": access_token, - "valid_until_ms": valid_until_ms, - "refresh_token": refresh_token, - } + return DeviceRegistrationResult( + device_id=registered_device_id, + access_token=access_token, + valid_until_ms=valid_until_ms, + refresh_token=refresh_token, + ) async def post_registration_actions( self, user_id: str, auth_result: dict, access_token: Optional[str] From 318b74c0610a673c68a5220417396814bd3914f0 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Wed, 5 May 2021 12:20:42 +0200 Subject: [PATCH 14/46] MSC2918: remove unused sequence in refresh_tokens Signed-off-by: Quentin Gliech --- .../databases/main/schema/delta/59/12refresh_tokens.sql.postgres | 1 - 1 file changed, 1 deletion(-) delete mode 100644 synapse/storage/databases/main/schema/delta/59/12refresh_tokens.sql.postgres diff --git a/synapse/storage/databases/main/schema/delta/59/12refresh_tokens.sql.postgres b/synapse/storage/databases/main/schema/delta/59/12refresh_tokens.sql.postgres deleted file mode 100644 index 9a566a59699a..000000000000 --- a/synapse/storage/databases/main/schema/delta/59/12refresh_tokens.sql.postgres +++ /dev/null @@ -1 +0,0 @@ -CREATE SEQUENCE IF NOT EXISTS refresh_token_id; \ No newline at end of file From 29806b4f1a18ccb56848dd66183ca825a7f25964 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Wed, 5 May 2021 12:21:28 +0200 Subject: [PATCH 15/46] MSC2918: try fixing port_db script when a table references itself Signed-off-by: Quentin Gliech --- scripts/synapse_port_db | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/synapse_port_db b/scripts/synapse_port_db index 58edf6af6c8c..3aa00ad38bcb 100755 --- a/scripts/synapse_port_db +++ b/scripts/synapse_port_db @@ -309,7 +309,8 @@ class Porter(object): information_schema.table_constraints AS tc INNER JOIN information_schema.constraint_column_usage AS ccu USING (table_schema, constraint_name) - WHERE tc.constraint_type = 'FOREIGN KEY'; + WHERE tc.constraint_type = 'FOREIGN KEY' + AND tc.table_name != ccu.table_name; """ txn.execute(sql) From 72e5c250690215f825f16ce5070b907da8183987 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Wed, 5 May 2021 15:42:09 +0200 Subject: [PATCH 16/46] MSC2918: lint --- synapse/handlers/register.py | 2 +- synapse/rest/client/v1/login.py | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 5698787d528d..f4559e2ba1f8 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -18,8 +18,8 @@ import logging from typing import TYPE_CHECKING, Iterable, List, Optional, Tuple -from prometheus_client import Counter import attr +from prometheus_client import Counter from synapse import types from synapse.api.constants import MAX_USERID_LENGTH, EventTypes, JoinRules, LoginType diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py index 6ca47965a8f4..4fc563dfd41a 100644 --- a/synapse/rest/client/v1/login.py +++ b/synapse/rest/client/v1/login.py @@ -19,7 +19,6 @@ from typing_extensions import TypedDict -import synapse.http.servlet from synapse.api.errors import Codes, LoginError, SynapseError from synapse.api.ratelimiting import Ratelimiter from synapse.api.urls import CLIENT_API_PREFIX @@ -29,9 +28,9 @@ from synapse.http.server import HttpServer, finish_request from synapse.http.servlet import ( RestServlet, + parse_boolean, parse_json_object_from_request, parse_string, - parse_boolean, ) from synapse.http.site import SynapseRequest from synapse.rest.client.v2_alpha._base import client_patterns @@ -160,7 +159,9 @@ def on_GET(self, request: SynapseRequest): async def on_POST(self, request: SynapseRequest): login_submission = parse_json_object_from_request(request) - should_issue_refresh_token = parse_boolean(request, name=LoginRestServlet.REFRESH_TOKEN_PARAM, default=False) + should_issue_refresh_token = parse_boolean( + request, name=LoginRestServlet.REFRESH_TOKEN_PARAM, default=False + ) try: if login_submission["type"] == LoginRestServlet.APPSERVICE_TYPE: From eb9f6805cbe89e710356f63377bb9eb8548afbb0 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Wed, 5 May 2021 23:54:38 +0200 Subject: [PATCH 17/46] Revert "MSC2918: use attr.s instead of TypedDict" This reverts commit 6963fe0161978c8260eb41371cb118d67370c4a2. --- synapse/handlers/register.py | 40 +++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index f4559e2ba1f8..a2d7612b29cf 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -18,8 +18,8 @@ import logging from typing import TYPE_CHECKING, Iterable, List, Optional, Tuple -import attr from prometheus_client import Counter +from typing_extensions import TypedDict from synapse import types from synapse.api.constants import MAX_USERID_LENGTH, EventTypes, JoinRules, LoginType @@ -56,13 +56,15 @@ ["guest", "auth_provider"], ) - -@attr.s(frozen=True, slots=True) -class DeviceRegistrationResult: - device_id = attr.ib(type=str) - access_token = attr.ib(type=str) - valid_until_ms = attr.ib(type=Optional[int]) - refresh_token = attr.ib(type=Optional[str]) +LoginDict = TypedDict( + "LoginDict", + { + "device_id": str, + "access_token": str, + "valid_until_ms": Optional[int], + "refresh_token": Optional[str], + }, +) class RegistrationHandler(BaseHandler): @@ -708,10 +710,10 @@ async def register_device( ).inc() return ( - res.device_id, - res.access_token, - res.valid_until_ms, - res.refresh_token, + res["device_id"], + res["access_token"], + res["valid_until_ms"], + res["refresh_token"], ) async def register_device_inner( @@ -722,7 +724,7 @@ async def register_device_inner( is_guest: bool = False, is_appservice_ghost: bool = False, should_issue_refresh_token: bool = False, - ) -> DeviceRegistrationResult: + ) -> LoginDict: """Helper for register_device Does the bits that need doing on the main process. Not for use outside this @@ -767,12 +769,12 @@ class and RegisterDeviceReplicationServlet. refresh_token_id=refresh_token_id, ) - return DeviceRegistrationResult( - device_id=registered_device_id, - access_token=access_token, - valid_until_ms=valid_until_ms, - refresh_token=refresh_token, - ) + return { + "device_id": registered_device_id, + "access_token": access_token, + "valid_until_ms": valid_until_ms, + "refresh_token": refresh_token, + } async def post_registration_actions( self, user_id: str, auth_result: dict, access_token: Optional[str] From 417a34ad36d95fb3c59bd738c6915e90a449076b Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 20 May 2021 12:18:46 +0200 Subject: [PATCH 18/46] MSC2918: random signed token instead of macaroons for refresh tokens --- synapse/handlers/auth.py | 81 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 79 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 3a6251d0543e..0f5adc156cb3 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -14,7 +14,10 @@ # 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 base64 +import binascii import logging +import random import time import unicodedata import urllib.parse @@ -35,6 +38,7 @@ import attr import bcrypt +import nacl.signing import pymacaroons from twisted.web.server import Request @@ -197,6 +201,8 @@ def __init__(self, hs: "HomeServer"): self.bcrypt_rounds = hs.config.bcrypt_rounds + self.signing_keys = hs.config.key.signing_key + # we can't use hs.get_module_api() here, because to do so will create an # import loop. # @@ -777,6 +783,23 @@ async def refresh_token( refresh_token: str, valid_until_ms: Optional[int], ) -> Tuple[str, str]: + """ + Consumes a refresh token and generate both a new access token and a new refresh token from it. + + The consumed refresh token is considered invalid after the first use of the new access token or the new refresh token. + + Args: + refresh_token: The token to consume. + valid_until_ms: The expiration timestamp of the new access token. + + Returns: + A tuple containing the new access token and refresh token + """ + + # Verify the token signature first before looking up the token + if not self.verify_refresh_token(refresh_token): + raise SynapseError(400, "invalid refresh token") + existing_token = await self.store.lookup_refresh_token(refresh_token) if existing_token is None: raise SynapseError(400, "refresh token does not exist") @@ -804,12 +827,64 @@ async def refresh_token( ) return access_token, new_refresh_token + def _generate_refresh_token(self) -> str: + """ + Generates a random and signed refresh token. + + The token is signed with the first signing key in config + and can verified with verify_refresh_token. + """ + rand = random.randbytes(128) + signed = self.signing_keys[0].sign(rand) + return "%s.%s" % ( + base64.urlsafe_b64encode(signed.signature).decode("ascii"), + base64.urlsafe_b64encode(signed.message).decode("ascii"), + ) + + def verify_refresh_token(self, token: str) -> bool: + """ + Verifies a refresh token signature. + + Args: + token: The refresh token to verify + + Returns: + Whether the token is valid or not + """ + parts = token.split(".", maxsplit=2) + if len(parts) != 2: + return False + try: + signature = base64.urlsafe_b64decode(parts[0]) + message = base64.urlsafe_b64decode(parts[1]) + except binascii.Error: + return False + + for key in self.signing_keys: + try: + key.verify_key.verify(message, signature) + return True + except nacl.signing.BadSignatureError: + pass + + return False + async def get_refresh_token_for_user_id( self, user_id: str, - device_id: Optional[str], + device_id: str, ) -> Tuple[str, int]: - refresh_token = self.macaroon_gen.generate_refresh_token(user_id) + """ + Creates a new refresh token for the user with the given user ID. + + Args: + user_id: canonical user ID + device_id: the device ID to associate with the token. + + Returns: + The newly created refresh token and its ID in the database + """ + refresh_token = self._generate_refresh_token() refresh_token_id = await self.store.add_refresh_token_to_user( user_id=user_id, token=refresh_token, @@ -842,6 +917,8 @@ async def get_access_token_for_user_id( valid_until_ms: when the token is valid until. None for no expiry. is_appservice_ghost: Whether the user is an application ghost user + refresh_token_id: the refresh token ID that will be associated with + this access token. Returns: The access token for the user's session. Raises: From 45177a67932bcef09e53e0af25df9a5837e5996f Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 20 May 2021 12:22:41 +0200 Subject: [PATCH 19/46] MSC2918: some docstrings and minor changes --- synapse/replication/http/login.py | 3 ++ synapse/rest/client/v1/login.py | 9 ++++- .../storage/databases/main/registration.py | 2 +- .../main/schema/delta/59/12refresh_tokens.sql | 33 +++++++++++++++---- 4 files changed, 38 insertions(+), 9 deletions(-) diff --git a/synapse/replication/http/login.py b/synapse/replication/http/login.py index 66a78f312558..b18dba533b3a 100644 --- a/synapse/replication/http/login.py +++ b/synapse/replication/http/login.py @@ -46,10 +46,13 @@ async def _serialize_payload( ): """ Args: + user_id (int) device_id (str|None): Device ID to use, if None a new one is generated. initial_display_name (str|None) is_guest (bool) + is_appservice_ghost (bool) + should_issue_refresh_token (bool) """ return { "device_id": device_id, diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py index 4fc563dfd41a..929f79044e50 100644 --- a/synapse/rest/client/v1/login.py +++ b/synapse/rest/client/v1/login.py @@ -159,6 +159,7 @@ def on_GET(self, request: SynapseRequest): async def on_POST(self, request: SynapseRequest): login_submission = parse_json_object_from_request(request) + # Check if this login should also issue a refresh token, as per MSC2918 should_issue_refresh_token = parse_boolean( request, name=LoginRestServlet.REFRESH_TOKEN_PARAM, default=False ) @@ -252,6 +253,8 @@ async def _do_other_login( Args: login_submission: + should_issue_refresh_token: Wheter this login should issue a + refresh token alongside the access token. Returns: HTTP response @@ -303,6 +306,8 @@ async def _complete_login( ratelimit: Whether to ratelimit the login request. auth_provider_id: The SSO IdP the user used, if any (just used for the prometheus metrics). + should_issue_refresh_token: Wheter this login should issue a + refresh token alongside the access token. Returns: result: Dictionary of account information after successful login. @@ -363,7 +368,9 @@ async def _do_token_login( Handle the final stage of SSO login. Args: - login_submission: The JSON request body. + login_submission: The JSON request body. + should_issue_refresh_token: Wheter this login should issue a + refresh token alongside the access token. Returns: The body of the JSON response. diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index 8418560c4f0f..be19001f09b6 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -74,7 +74,7 @@ class RefreshTokenLookupResult: user_id = attr.ib(type=str) device_id = attr.ib(type=str) token_id = attr.ib(type=int) - next_token_id = attr.ib(type=int) + next_token_id = attr.ib(type=Optional[int]) has_next_refresh_token_been_refreshed = attr.ib(type=bool) has_next_access_token_been_used = attr.ib(type=bool) diff --git a/synapse/storage/databases/main/schema/delta/59/12refresh_tokens.sql b/synapse/storage/databases/main/schema/delta/59/12refresh_tokens.sql index 95ff9cfc3575..a17116dd929e 100644 --- a/synapse/storage/databases/main/schema/delta/59/12refresh_tokens.sql +++ b/synapse/storage/databases/main/schema/delta/59/12refresh_tokens.sql @@ -1,11 +1,30 @@ +/* Copyright 2021 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. + */ + +-- Holds MSC2918 refresh tokens CREATE TABLE refresh_tokens ( - id BIGINT PRIMARY KEY, - user_id TEXT NOT NULL, - device_id TEXT, - token TEXT NOT NULL, - next_token_id BIGINT REFERENCES refresh_tokens (id) ON DELETE CASCADE, - UNIQUE(token) + id BIGINT PRIMARY KEY, + user_id TEXT NOT NULL, + device_id TEXT NOT NULL, + token TEXT NOT NULL, + -- When consumed, a new refresh token is generated, which is tracked by + -- this foreign key + next_token_id BIGINT REFERENCES refresh_tokens (id) ON DELETE CASCADE, + UNIQUE(token) ); +-- Add a reference to the refresh token generated alongside each access token ALTER TABLE "access_tokens" - ADD COLUMN refresh_token_id BIGINT REFERENCES refresh_tokens (id) ON DELETE CASCADE; + ADD COLUMN refresh_token_id BIGINT REFERENCES refresh_tokens (id) ON DELETE CASCADE; From e37f53a1e324d4b727ef09d4cb41a13e417fe012 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 27 May 2021 11:25:21 +0200 Subject: [PATCH 20/46] MSC2918: expires_in -> expires_in_ms --- synapse/rest/client/v1/login.py | 9 +++++---- synapse/rest/client/v2_alpha/register.py | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py index 929f79044e50..8f80a44e7c9a 100644 --- a/synapse/rest/client/v1/login.py +++ b/synapse/rest/client/v1/login.py @@ -49,7 +49,7 @@ "user_id": str, "access_token": str, "home_server": str, - "expires_in": Optional[int], + "expires_in_ms": Optional[int], "refresh_token": Optional[str], "device_id": str, "well_known": Optional[Dict[str, Any]], @@ -351,7 +351,7 @@ async def _complete_login( if valid_until_ms is not None: expires_in_ms = valid_until_ms - self.clock.time_msec() - result["expires_in"] = int(expires_in_ms / 1000) + result["expires_in_ms"] = expires_in_ms if refresh_token is not None: result["refresh_token"] = refresh_token @@ -460,6 +460,7 @@ class RefreshTokenServlet(RestServlet): def __init__(self, hs: "HomeServer"): self._auth_handler = hs.get_auth_handler() self._clock = hs.get_clock() + self.access_token_lifetime = hs.config.access_token_lifetime async def on_POST( self, @@ -469,7 +470,7 @@ async def on_POST( try: token = refresh_submission["refresh_token"] - valid_until_ms = self._clock.time_msec() + 60 * 1000 + valid_until_ms = self._clock.time_msec() + self.access_token_lifetime access_token, refresh_token = await self._auth_handler.refresh_token( token, valid_until_ms ) @@ -479,7 +480,7 @@ async def on_POST( { "access_token": access_token, "refresh_token": refresh_token, - "expires_in": int(expires_in_ms / 1000), + "expires_in_ms": expires_in_ms, }, ) diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index 00a4e85dfadd..f00efe5d76ec 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -742,7 +742,7 @@ async def _do_guest_registration(self, params, address=None): if valid_until_ms is not None: expires_in_ms = valid_until_ms - self.clock.time_msec() - result["expires_in"] = int(expires_in_ms / 1000) + result["expires_in_ms"] = expires_in_ms if refresh_token is not None: result["refresh_token"] = refresh_token From 262d1ab17405eecba65e045ae75b3f958bb4b599 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 27 May 2021 15:48:15 +0200 Subject: [PATCH 21/46] MSC2918: properly figure out whether an access token was already used or not --- synapse/api/auth.py | 5 +++++ .../storage/databases/main/registration.py | 21 +++++++++++++++++-- .../main/schema/delta/59/12refresh_tokens.sql | 4 ++++ 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/synapse/api/auth.py b/synapse/api/auth.py index 7d9930ae7b7c..a7ad21e5ad07 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -249,6 +249,11 @@ async def get_user_by_req( errcode=Codes.GUEST_ACCESS_FORBIDDEN, ) + # Mark the token as used. This is used to invalidate old refresh + # tokens after some time. + if not user_info.token_used: + await self.store.mark_access_token_as_used(user_info.token_id) + requester = synapse.types.create_requester( user_info.user_id, token_id, diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index be19001f09b6..1c151944874f 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -62,6 +62,7 @@ class TokenLookupResult: device_id = attr.ib(type=Optional[str], default=None) valid_until_ms = attr.ib(type=Optional[int], default=None) token_owner = attr.ib(type=str) + token_used = attr.ib(type=bool, default=False) # Make the token owner default to the user ID, which is the common case. @token_owner.default @@ -421,7 +422,8 @@ def _query_for_auth(self, txn, token: str) -> Optional[TokenLookupResult]: access_tokens.id as token_id, access_tokens.device_id, access_tokens.valid_until_ms, - access_tokens.user_id as token_owner + access_tokens.user_id as token_owner, + access_tokens.used as token_used FROM users INNER JOIN access_tokens on users.name = COALESCE(puppets_user_id, access_tokens.user_id) WHERE token = ? @@ -1052,6 +1054,21 @@ async def update_access_token_last_validated(self, token_id: int) -> None: desc="update_access_token_last_validated", ) + async def mark_access_token_as_used(self, token_id: int) -> None: + """Mark the access token as used, which invalidates the old refresh token. + + Args: + token_id: The ID of the access token to update. + Raises: + StoreError if there was a problem updating this. + """ + await self.db_pool.simple_update_one( + "access_tokens", + {"id": token_id}, + {"used": True}, + desc="mark_access_token_as_used", + ) + async def lookup_refresh_token( self, token: str ) -> Optional[RefreshTokenLookupResult]: @@ -1066,7 +1083,7 @@ def _lookup_refresh_token_txn(txn) -> Optional[RefreshTokenLookupResult]: rt.device_id, rt.next_token_id, (nrt.next_token_id IS NOT NULL) has_next_refresh_token_been_refreshed, - (at.last_validated IS NOT NULL) has_next_access_token_been_used + at.used has_next_access_token_been_used FROM refresh_tokens rt LEFT JOIN refresh_tokens nrt ON rt.next_token_id = nrt.id LEFT JOIN access_tokens at ON at.refresh_token_id = nrt.id diff --git a/synapse/storage/databases/main/schema/delta/59/12refresh_tokens.sql b/synapse/storage/databases/main/schema/delta/59/12refresh_tokens.sql index a17116dd929e..898f18f70363 100644 --- a/synapse/storage/databases/main/schema/delta/59/12refresh_tokens.sql +++ b/synapse/storage/databases/main/schema/delta/59/12refresh_tokens.sql @@ -28,3 +28,7 @@ CREATE TABLE refresh_tokens ( -- Add a reference to the refresh token generated alongside each access token ALTER TABLE "access_tokens" ADD COLUMN refresh_token_id BIGINT REFERENCES refresh_tokens (id) ON DELETE CASCADE; + +-- Add a flag whether the token was already used or not +ALTER TABLE "access_tokens" + ADD COLUMN used BOOLEAN NOT NULL DEFAULT FALSE; From 75ce9e5add771d2d5e73117dcd97f13b09112606 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 27 May 2021 15:49:12 +0200 Subject: [PATCH 22/46] MSC2918: implement for registration endpoint --- synapse/rest/client/v2_alpha/register.py | 32 +++++++++++++++++++++--- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index f00efe5d76ec..bc6a66089d37 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -43,6 +43,7 @@ from synapse.http.servlet import ( RestServlet, assert_params_in_dict, + parse_boolean, parse_json_object_from_request, parse_string, ) @@ -422,6 +423,12 @@ async def on_POST(self, request): "Do not understand membership kind: %s" % (kind.decode("utf8"),) ) + # Check if this registration should also issue a refresh token, as per + # MSC2918 + should_issue_refresh_token = parse_boolean( + request, name="org.matrix.msc2918.refresh_token", default=False + ) + # Pull out the provided username and do basic sanity checks early since # the auth layer will store these in sessions. desired_username = None @@ -455,7 +462,10 @@ async def on_POST(self, request): raise SynapseError(400, "Desired Username is missing or not a string") result = await self._do_appservice_registration( - desired_username, access_token, body + desired_username, + access_token, + body, + should_issue_refresh_token=should_issue_refresh_token, ) return 200, result @@ -653,7 +663,9 @@ async def on_POST(self, request): registered = True return_dict = await self._create_registration_details( - registered_user_id, params + registered_user_id, + params, + should_issue_refresh_token=should_issue_refresh_token, ) if registered: @@ -665,7 +677,9 @@ async def on_POST(self, request): return 200, return_dict - async def _do_appservice_registration(self, username, as_token, body): + async def _do_appservice_registration( + self, username, as_token, body, should_issue_refresh_token: bool = False + ): user_id = await self.registration_handler.appservice_register( username, as_token ) @@ -673,10 +687,15 @@ async def _do_appservice_registration(self, username, as_token, body): user_id, body, is_appservice_ghost=True, + should_issue_refresh_token=should_issue_refresh_token, ) async def _create_registration_details( - self, user_id, params, is_appservice_ghost=False + self, + user_id, + params, + is_appservice_ghost=False, + should_issue_refresh_token: bool = False, ): """Complete registration of newly-registered user @@ -704,10 +723,15 @@ async def _create_registration_details( initial_display_name, is_guest=False, is_appservice_ghost=is_appservice_ghost, + should_issue_refresh_token=should_issue_refresh_token, ) result.update({"access_token": access_token, "device_id": device_id}) + if valid_until_ms is not None: + expires_in_ms = valid_until_ms - self.clock.time_msec() + result["expires_in_ms"] = expires_in_ms + if refresh_token is not None: result["refresh_token"] = refresh_token From 6f2cc6187b371ef63dd724b83780fd1835603640 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 27 May 2021 15:49:38 +0200 Subject: [PATCH 23/46] MSC2918: properly replace old-next refresh token --- .../storage/databases/main/registration.py | 33 ++++++++++++++++--- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index 1c151944874f..63979f7b4ebf 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -1110,11 +1110,34 @@ def _lookup_refresh_token_txn(txn) -> Optional[RefreshTokenLookupResult]: ) async def replace_refresh_token(self, token_id: int, next_token_id: int) -> None: - await self.db_pool.simple_update_one( - "refresh_tokens", - {"id": token_id}, - {"next_token_id": next_token_id}, - desc="replace_refresh_token", + def _replace_refresh_token_txn(txn) -> None: + # First check if there was an existing refresh token + old_next_token_id = self.db_pool.simple_select_one_onecol_txn( + txn, + "refresh_tokens", + {"id": token_id}, + "next_token_id", + allow_none=True, + ) + + self.db_pool.simple_update_one_txn( + txn, + "refresh_tokens", + {"id": token_id}, + {"next_token_id": next_token_id}, + ) + + # Delete the old "next" token if it exists. This should cascade and + # delete the associated access_token + if old_next_token_id is not None: + self.db_pool.simple_delete_one_txn( + txn, + "refresh_tokens", + {"id": old_next_token_id}, + ) + + await self.db_pool.runInteraction( + "replace_refresh_token", _replace_refresh_token_txn ) From b7b17edd34e8abf09adeb8e29b22482701ed7a52 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 27 May 2021 15:50:07 +0200 Subject: [PATCH 24/46] MSC2918: add tests --- tests/rest/client/v2_alpha/test_auth.py | 220 +++++++++++++++++++++++- 1 file changed, 219 insertions(+), 1 deletion(-) diff --git a/tests/rest/client/v2_alpha/test_auth.py b/tests/rest/client/v2_alpha/test_auth.py index ed433d93334c..a7a8ba93ab43 100644 --- a/tests/rest/client/v2_alpha/test_auth.py +++ b/tests/rest/client/v2_alpha/test_auth.py @@ -21,7 +21,7 @@ from synapse.api.constants import LoginType from synapse.handlers.ui_auth.checkers import UserInteractiveAuthChecker from synapse.rest.client.v1 import login -from synapse.rest.client.v2_alpha import auth, devices, register +from synapse.rest.client.v2_alpha import account, auth, devices, register from synapse.rest.synapse.client import build_synapse_client_resource_tree from synapse.types import JsonDict, UserID @@ -499,3 +499,221 @@ def test_ui_auth_fails_for_incorrect_sso_user(self): self.delete_device( self.user_tok, self.device_id, 403, body={"auth": {"session": session_id}} ) + + +class RefreshAuthTests(unittest.HomeserverTestCase): + servlets = [ + auth.register_servlets, + account.register_servlets, + login.register_servlets, + synapse.rest.admin.register_servlets_for_client_rest_resource, + register.register_servlets, + ] + hijack_auth = False + + def prepare(self, reactor, clock, hs): + self.user_pass = "pass" + self.user = self.register_user("test", self.user_pass) + + def test_login_issue_refresh_token(self): + """ + A login response should include a refresh_token only if asked. + """ + # Test login + body = {"type": "m.login.password", "user": "test", "password": self.user_pass} + + login_without_refresh = self.make_request( + "POST", "/_matrix/client/r0/login", body + ) + self.assertEqual(login_without_refresh.code, 200, login_without_refresh.result) + self.assertNotIn("refresh_token", login_without_refresh.json_body) + + login_with_refresh = self.make_request( + "POST", + "/_matrix/client/r0/login?org.matrix.msc2918.refresh_token=true", + body, + ) + self.assertEqual(login_with_refresh.code, 200, login_with_refresh.result) + self.assertIn("refresh_token", login_with_refresh.json_body) + self.assertIn("expires_in_ms", login_with_refresh.json_body) + + def test_register_issue_refresh_token(self): + """ + A register response should include a refresh_token only if asked. + """ + register_without_refresh = self.make_request( + "POST", + "/_matrix/client/r0/register", + { + "username": "test2", + "password": self.user_pass, + "auth": {"type": LoginType.DUMMY}, + }, + ) + self.assertEqual( + register_without_refresh.code, 200, register_without_refresh.result + ) + self.assertNotIn("refresh_token", register_without_refresh.json_body) + + register_with_refresh = self.make_request( + "POST", + "/_matrix/client/r0/register?org.matrix.msc2918.refresh_token=true", + { + "username": "test3", + "password": self.user_pass, + "auth": {"type": LoginType.DUMMY}, + }, + ) + self.assertEqual(register_with_refresh.code, 200, register_with_refresh.result) + self.assertIn("refresh_token", register_with_refresh.json_body) + self.assertIn("expires_in_ms", register_with_refresh.json_body) + + def test_token_refresh(self): + """ + A refresh token can be used to issue a new access token. + """ + body = {"type": "m.login.password", "user": "test", "password": self.user_pass} + login_response = self.make_request( + "POST", + "/_matrix/client/r0/login?org.matrix.msc2918.refresh_token=true", + body, + ) + self.assertEqual(login_response.code, 200, login_response.result) + + refresh_response = self.make_request( + "POST", + "/_matrix/client/unstable/org.matrix.msc2918.refresh_token/refresh", + {"refresh_token": login_response.json_body["refresh_token"]}, + ) + self.assertEqual(refresh_response.code, 200, refresh_response.result) + self.assertIn("access_token", refresh_response.json_body) + self.assertIn("refresh_token", refresh_response.json_body) + self.assertIn("expires_in_ms", refresh_response.json_body) + + # The access and refresh tokens should be different from the original ones after refresh + self.assertNotEqual( + login_response.json_body["access_token"], + refresh_response.json_body["access_token"], + ) + self.assertNotEqual( + login_response.json_body["refresh_token"], + refresh_response.json_body["refresh_token"], + ) + + @override_config({"access_token_lifetime": "1m"}) + def test_refresh_token_expiration(self): + """ + The access token should have some time as specified in the config. + """ + body = {"type": "m.login.password", "user": "test", "password": self.user_pass} + login_response = self.make_request( + "POST", + "/_matrix/client/r0/login?org.matrix.msc2918.refresh_token=true", + body, + ) + self.assertEqual(login_response.code, 200, login_response.result) + self.assertApproximates( + login_response.json_body["expires_in_ms"], 60 * 1000, 100 + ) + + refresh_response = self.make_request( + "POST", + "/_matrix/client/unstable/org.matrix.msc2918.refresh_token/refresh", + {"refresh_token": login_response.json_body["refresh_token"]}, + ) + self.assertEqual(refresh_response.code, 200, refresh_response.result) + self.assertApproximates( + refresh_response.json_body["expires_in_ms"], 60 * 1000, 100 + ) + + def test_refresh_token_invalidation(self): + """Refresh tokens are invalidated after first use of the next token. + + A refresh token is considered invalid if: + - it was already used at least once + - and either + - the next access token was used + - the next refresh token was used + + The chain of tokens goes like this: + + login -|-> first_refresh -> third_refresh (fails) + |-> second_refresh -> fifth_refresh + |-> fourth_refresh (fails) + """ + + body = {"type": "m.login.password", "user": "test", "password": self.user_pass} + login_response = self.make_request( + "POST", + "/_matrix/client/r0/login?org.matrix.msc2918.refresh_token=true", + body, + ) + self.assertEqual(login_response.code, 200, login_response.result) + + # This first refresh should work properly + first_refresh_response = self.make_request( + "POST", + "/_matrix/client/unstable/org.matrix.msc2918.refresh_token/refresh", + {"refresh_token": login_response.json_body["refresh_token"]}, + ) + self.assertEqual( + first_refresh_response.code, 200, first_refresh_response.result + ) + + # This one as well, since the token in the first one was never used + second_refresh_response = self.make_request( + "POST", + "/_matrix/client/unstable/org.matrix.msc2918.refresh_token/refresh", + {"refresh_token": login_response.json_body["refresh_token"]}, + ) + self.assertEqual( + second_refresh_response.code, 200, second_refresh_response.result + ) + + # This one should not, since the token from the first refresh is not valid anymore + third_refresh_response = self.make_request( + "POST", + "/_matrix/client/unstable/org.matrix.msc2918.refresh_token/refresh", + {"refresh_token": first_refresh_response.json_body["refresh_token"]}, + ) + self.assertEqual( + third_refresh_response.code, 400, third_refresh_response.result + ) + + # The associated access token should also be invalid + whoami_response = self.make_request( + "GET", + "/_matrix/client/r0/account/whoami", + access_token=first_refresh_response.json_body["access_token"], + ) + self.assertEqual(whoami_response.code, 401, whoami_response.result) + + # But all other tokens should work (they will expire after some time) + for access_token in [ + second_refresh_response.json_body["access_token"], + login_response.json_body["access_token"], + ]: + whoami_response = self.make_request( + "GET", "/_matrix/client/r0/account/whoami", access_token=access_token + ) + self.assertEqual(whoami_response.code, 200, whoami_response.result) + + # Now that the access token from the last valid refresh was used once, refreshing with the N-1 token should fail + fourth_refresh_response = self.make_request( + "POST", + "/_matrix/client/unstable/org.matrix.msc2918.refresh_token/refresh", + {"refresh_token": login_response.json_body["refresh_token"]}, + ) + self.assertEqual( + fourth_refresh_response.code, 400, fourth_refresh_response.result + ) + + # But refreshing from the last valid refresh token still works + fifth_refresh_response = self.make_request( + "POST", + "/_matrix/client/unstable/org.matrix.msc2918.refresh_token/refresh", + {"refresh_token": second_refresh_response.json_body["refresh_token"]}, + ) + self.assertEqual( + fifth_refresh_response.code, 200, fifth_refresh_response.result + ) From c7eab51ab8a4d532112ec17cad3d0e07e84183b0 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 27 May 2021 16:30:35 +0200 Subject: [PATCH 25/46] MSC2918: use secrets.token_bytes instead of random.randbytes random.randbytes isn't available in python < 3.9 while secrets.token_bytes is available starting python 3.6 Also remove some leftover in the macaroon generator --- synapse/handlers/auth.py | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 0f5adc156cb3..d949036f2398 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -17,7 +17,7 @@ import base64 import binascii import logging -import random +import secrets import time import unicodedata import urllib.parse @@ -834,7 +834,7 @@ def _generate_refresh_token(self) -> str: The token is signed with the first signing key in config and can verified with verify_refresh_token. """ - rand = random.randbytes(128) + rand = secrets.token_bytes(128) signed = self.signing_keys[0].sign(rand) return "%s.%s" % ( base64.urlsafe_b64encode(signed.signature).decode("ascii"), @@ -1713,21 +1713,6 @@ class MacaroonGenerator: hs = attr.ib() - def generate_refresh_token( - self, user_id: str, extra_caveats: Optional[List[str]] = None - ) -> str: - extra_caveats = extra_caveats or [] - macaroon = self._generate_base_macaroon(user_id) - macaroon.add_first_party_caveat("type = refresh") - # Include a nonce, to make sure that each login gets a different - # access token. - macaroon.add_first_party_caveat( - "nonce = %s" % (stringutils.random_string_with_symbols(16),) - ) - for caveat in extra_caveats: - macaroon.add_first_party_caveat(caveat) - return macaroon.serialize() - def generate_access_token( self, user_id: str, extra_caveats: Optional[List[str]] = None ) -> str: From 088e0233359b4dd87244b6db81aa04a92e004056 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 27 May 2021 18:40:55 +0200 Subject: [PATCH 26/46] MSC2918: mark new column as boolean in port_db --- scripts/synapse_port_db | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/synapse_port_db b/scripts/synapse_port_db index 3aa00ad38bcb..310382054d9b 100755 --- a/scripts/synapse_port_db +++ b/scripts/synapse_port_db @@ -94,6 +94,7 @@ BOOLEAN_COLUMNS = { "local_media_repository": ["safe_from_quarantine"], "users": ["shadow_banned"], "e2e_fallback_keys_json": ["used"], + "access_tokens": ["used"], } From 624722869a22e7fdc9fef09d5062c0eabbdef20f Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 27 May 2021 18:41:26 +0200 Subject: [PATCH 27/46] MSC2918: fix existing auth test --- tests/api/test_auth.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py index 5d6a39300635..5ea3d5416c8e 100644 --- a/tests/api/test_auth.py +++ b/tests/api/test_auth.py @@ -61,6 +61,7 @@ def test_get_user_by_req_user_valid_token(self): user_id=self.test_user, token_id=5, device_id="device" ) self.store.get_user_by_access_token = simple_async_mock(user_info) + self.store.mark_access_token_as_used = simple_async_mock(None) request = Mock(args={}) request.args[b"access_token"] = [self.test_token] @@ -285,6 +286,7 @@ async def get_user(tok): self.store.get_user_by_access_token = get_user self.store.get_user_by_id = simple_async_mock({"is_guest": False}) + self.store.mark_access_token_as_used = simple_async_mock(None) # check the token works request = Mock(args={}) From 2ec853c64ea0a9320b739bb161b1d406e2215592 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 28 May 2021 11:07:20 +0200 Subject: [PATCH 28/46] MSC2918: use the same pattern as access tokens for refresh tokens --- synapse/handlers/auth.py | 46 +++++++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 17 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index f089e6ecd078..9c61f167421b 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -38,7 +38,6 @@ import attr import bcrypt -import nacl.signing import pymacaroons import unpaddedbase64 @@ -845,31 +844,31 @@ def _generate_refresh_token(self) -> str: def verify_refresh_token(self, token: str) -> bool: """ - Verifies a refresh token signature. + Verifies the shape of a refresh token. Args: token: The refresh token to verify Returns: - Whether the token is valid or not + Whether the token has the right shape """ - parts = token.split(".", maxsplit=2) - if len(parts) != 2: + parts = token.split("_", maxsplit=4) + if len(parts) != 4: return False - try: - signature = base64.urlsafe_b64decode(parts[0]) - message = base64.urlsafe_b64decode(parts[1]) - except binascii.Error: + + type, localpart, rand, crc = parts + + # Refresh tokens are prefixed by "syr_", let's check that + if type != "syr": return False - for key in self.signing_keys: - try: - key.verify_key.verify(message, signature) - return True - except nacl.signing.BadSignatureError: - pass + # Check the CRC + base = f"{type}_{localpart}_{rand}" + expected_crc = base62_encode(crc32(base.encode("ascii")), minwidth=6) + if crc != expected_crc: + return False - return False + return True async def get_refresh_token_for_user_id( self, @@ -886,7 +885,7 @@ async def get_refresh_token_for_user_id( Returns: The newly created refresh token and its ID in the database """ - refresh_token = self._generate_refresh_token() + refresh_token = self.generate_refresh_token(UserID.from_string(user_id)) refresh_token_id = await self.store.add_refresh_token_to_user( user_id=user_id, token=refresh_token, @@ -1336,6 +1335,19 @@ def generate_access_token(self, for_user: UserID) -> str: crc = base62_encode(crc32(base.encode("ascii")), minwidth=6) return f"{base}_{crc}" + def generate_refresh_token(self, for_user: UserID) -> str: + """Generates an opaque string, for use as a refresh token""" + + # we use the following format for access tokens: + # syr___ + + b64local = unpaddedbase64.encode_base64(for_user.localpart.encode("utf-8")) + random_string = stringutils.random_string(20) + base = f"syr_{b64local}_{random_string}" + + crc = base62_encode(crc32(base.encode("ascii")), minwidth=6) + return f"{base}_{crc}" + async def validate_short_term_login_token( self, login_token: str ) -> LoginTokenAttributes: From 9e7ce1fe4f8fcccbe8894ff104691a89986ae204 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 28 May 2021 11:10:59 +0200 Subject: [PATCH 29/46] MSC2918: lint: remove unused import --- synapse/handlers/auth.py | 1 - 1 file changed, 1 deletion(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 9c61f167421b..d3769d348cfa 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -14,7 +14,6 @@ # See the License for the specific language governing permissions and # limitations under the License. import base64 -import binascii import logging import secrets import time From 45e2eaf229321b194fa512cf1042bb3ea8dced2e Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 28 May 2021 11:14:31 +0200 Subject: [PATCH 30/46] MSC2918: fix typing issue --- synapse/api/auth.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/api/auth.py b/synapse/api/auth.py index 0e39b673a210..dc338d553c97 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -250,8 +250,8 @@ async def get_user_by_req( # Mark the token as used. This is used to invalidate old refresh # tokens after some time. - if not user_info.token_used: - await self.store.mark_access_token_as_used(user_info.token_id) + if not user_info.token_used and token_id is not None: + await self.store.mark_access_token_as_used(token_id) requester = create_requester( user_info.user_id, From c20f94aaa95b94d6eaae197eabbf4469c97fbfb5 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 28 May 2021 11:49:45 +0200 Subject: [PATCH 31/46] MSC2918: properly check refresh_token parameter --- synapse/rest/client/v1/login.py | 35 +++++++++++++++++---------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py index 2166a3f18efe..0d73264ea38f 100644 --- a/synapse/rest/client/v1/login.py +++ b/synapse/rest/client/v1/login.py @@ -27,6 +27,7 @@ from synapse.http.server import HttpServer, finish_request from synapse.http.servlet import ( RestServlet, + assert_params_in_dict, parse_boolean, parse_json_object_from_request, parse_string, @@ -467,24 +468,24 @@ async def on_POST( ): refresh_submission = parse_json_object_from_request(request) - try: - token = refresh_submission["refresh_token"] - valid_until_ms = self._clock.time_msec() + self.access_token_lifetime - access_token, refresh_token = await self._auth_handler.refresh_token( - token, valid_until_ms - ) - expires_in_ms = valid_until_ms - self._clock.time_msec() - return ( - 200, - { - "access_token": access_token, - "refresh_token": refresh_token, - "expires_in_ms": expires_in_ms, - }, - ) + assert_params_in_dict(refresh_submission, ["refresh_token"]) + token = refresh_submission["refresh_token"] + if not isinstance(token, str): + raise SynapseError(400, "Invalid param: refresh_token", Codes.INVALID_PARAM) - except KeyError: - raise SynapseError(400, "Missing JSON keys.") + valid_until_ms = self._clock.time_msec() + self.access_token_lifetime + access_token, refresh_token = await self._auth_handler.refresh_token( + token, valid_until_ms + ) + expires_in_ms = valid_until_ms - self._clock.time_msec() + return ( + 200, + { + "access_token": access_token, + "refresh_token": refresh_token, + "expires_in_ms": expires_in_ms, + }, + ) class SsoRedirectServlet(RestServlet): From 790baacc4a36651eff308d29578f2f0b0a254d91 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 28 May 2021 17:03:18 +0200 Subject: [PATCH 32/46] MSC2918: cleanup old refresh token generation code --- synapse/handlers/auth.py | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index d3769d348cfa..91375ba8282e 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -13,9 +13,7 @@ # 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 base64 import logging -import secrets import time import unicodedata import urllib.parse @@ -201,8 +199,6 @@ def __init__(self, hs: "HomeServer"): self.bcrypt_rounds = hs.config.bcrypt_rounds - self.signing_keys = hs.config.key.signing_key - # we can't use hs.get_module_api() here, because to do so will create an # import loop. # @@ -827,20 +823,6 @@ async def refresh_token( ) return access_token, new_refresh_token - def _generate_refresh_token(self) -> str: - """ - Generates a random and signed refresh token. - - The token is signed with the first signing key in config - and can verified with verify_refresh_token. - """ - rand = secrets.token_bytes(128) - signed = self.signing_keys[0].sign(rand) - return "%s.%s" % ( - base64.urlsafe_b64encode(signed.signature).decode("ascii"), - base64.urlsafe_b64encode(signed.message).decode("ascii"), - ) - def verify_refresh_token(self, token: str) -> bool: """ Verifies the shape of a refresh token. From 01b074004aac89e4c6d1f9fbe8a1e8756b2de03b Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 3 Jun 2021 15:58:14 +0200 Subject: [PATCH 33/46] MSC2918: add more docstrings Also applies some of richvdh's suggestions --- synapse/handlers/auth.py | 4 +- synapse/rest/client/v1/login.py | 12 ++--- .../storage/databases/main/registration.py | 45 +++++++++++++++++-- 3 files changed, 50 insertions(+), 11 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 91375ba8282e..8ef46c35c920 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -793,7 +793,7 @@ async def refresh_token( """ # Verify the token signature first before looking up the token - if not self.verify_refresh_token(refresh_token): + if not self._verify_refresh_token(refresh_token): raise SynapseError(400, "invalid refresh token") existing_token = await self.store.lookup_refresh_token(refresh_token) @@ -823,7 +823,7 @@ async def refresh_token( ) return access_token, new_refresh_token - def verify_refresh_token(self, token: str) -> bool: + def _verify_refresh_token(self, token: str) -> bool: """ Verifies the shape of a refresh token. diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py index 0d73264ea38f..76483d22f3bd 100644 --- a/synapse/rest/client/v1/login.py +++ b/synapse/rest/client/v1/login.py @@ -253,8 +253,8 @@ async def _do_other_login( Args: login_submission: - should_issue_refresh_token: Wheter this login should issue a - refresh token alongside the access token. + should_issue_refresh_token: True if this login should issue + a refresh token alongside the access token. Returns: HTTP response @@ -306,8 +306,8 @@ async def _complete_login( ratelimit: Whether to ratelimit the login request. auth_provider_id: The SSO IdP the user used, if any (just used for the prometheus metrics). - should_issue_refresh_token: Wheter this login should issue a - refresh token alongside the access token. + should_issue_refresh_token: True if this login should issue + a refresh token alongside the access token. Returns: result: Dictionary of account information after successful login. @@ -369,8 +369,8 @@ async def _do_token_login( Args: login_submission: The JSON request body. - should_issue_refresh_token: Wheter this login should issue a - refresh token alongside the access token. + should_issue_refresh_token: True if this login should issue + a refresh token alongside the access token. Returns: The body of the JSON response. diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index b22339cc7d4d..9e1d68423ee7 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -53,6 +53,7 @@ class TokenLookupResult: valid_until_ms: The timestamp the token expires, if any. token_owner: The "owner" of the token. This is either the same as the user, or a server admin who is logged in as the user. + token_used: True if this token was used at least once in a request. """ user_id = attr.ib(type=str) @@ -72,12 +73,25 @@ def _default_token_owner(self): @attr.s(frozen=True, slots=True) class RefreshTokenLookupResult: + """Result of looking up a refresh token.""" + user_id = attr.ib(type=str) + """The user this token belongs to.""" + device_id = attr.ib(type=str) + """The device associated with this refresh token.""" + token_id = attr.ib(type=int) + """The ID of this refresh token.""" + next_token_id = attr.ib(type=Optional[int]) + """The ID of the refresh token which replaced this one.""" + has_next_refresh_token_been_refreshed = attr.ib(type=bool) + """True if the next refresh token was used for another refresh.""" + has_next_access_token_been_used = attr.ib(type=bool) + """True if the next access token was already used at least once.""" class RegistrationWorkerStore(CacheInvalidationWorkerStore): @@ -1085,7 +1099,9 @@ async def update_access_token_last_validated(self, token_id: int) -> None: ) async def mark_access_token_as_used(self, token_id: int) -> None: - """Mark the access token as used, which invalidates the old refresh token. + """ + Mark the access token as used, which invalidates the refresh token used + to obtain it. Args: token_id: The ID of the access token to update. @@ -1140,6 +1156,15 @@ def _lookup_refresh_token_txn(txn) -> Optional[RefreshTokenLookupResult]: ) async def replace_refresh_token(self, token_id: int, next_token_id: int) -> None: + """ + Set the successor of a refresh token, removing the existing successor + if any. + + Args: + token_id: ID of the refresh token to update. + next_token_id: ID of its successor. + """ + def _replace_refresh_token_txn(txn) -> None: # First check if there was an existing refresh token old_next_token_id = self.db_pool.simple_select_one_onecol_txn( @@ -1377,8 +1402,11 @@ async def add_access_token_to_user( Args: user_id: The user ID. token: The new access token to add. - device_id: ID of the device to associate with the access token + device_id: ID of the device to associate with the access token. valid_until_ms: when the token is valid until. None for no expiry. + puppets_user_id + refresh_token_id: ID of the refresh token generated alongside this + access token. Raises: StoreError if there was a problem adding this. Returns: @@ -1410,6 +1438,17 @@ async def add_refresh_token_to_user( token: str, device_id: Optional[str], ) -> int: + """Adds a refresh token for the given user. + + Args: + user_id: The user ID. + token: The new access token to add. + device_id: ID of the device to associate with the refresh token. + Raises: + StoreError if there was a problem adding this. + Returns: + The token ID + """ next_id = self._refresh_tokens_id_gen.get_next() await self.db_pool.simple_insert( @@ -1421,7 +1460,7 @@ async def add_refresh_token_to_user( "token": token, "next_token_id": None, }, - desc="add_access_token_to_user", + desc="add_refresh_token_to_user", ) return next_id From 797e0d398c7cf5f32d1a9569d98ead00a1b52e53 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 3 Jun 2021 17:32:53 +0200 Subject: [PATCH 34/46] MSC2918: change refresh token API error codes Aligns with https://github.com/matrix-org/matrix-doc/pull/2918/commits/d433e3b7f19c771411a2a2098fab8cc765d85bf6 --- synapse/handlers/auth.py | 8 +++++--- tests/rest/client/v2_alpha/test_auth.py | 4 ++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 8ef46c35c920..0bcee522ff24 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -794,17 +794,19 @@ async def refresh_token( # Verify the token signature first before looking up the token if not self._verify_refresh_token(refresh_token): - raise SynapseError(400, "invalid refresh token") + raise SynapseError(401, "invalid refresh token", Codes.UNKNOWN_TOKEN) existing_token = await self.store.lookup_refresh_token(refresh_token) if existing_token is None: - raise SynapseError(400, "refresh token does not exist") + raise SynapseError(401, "refresh token does not exist", Codes.UNKNOWN_TOKEN) if ( existing_token.has_next_access_token_been_used or existing_token.has_next_refresh_token_been_refreshed ): - raise SynapseError(400, "refresh token isn't valid anymore") + raise SynapseError( + 401, "refresh token isn't valid anymore", Codes.UNKNOWN_TOKEN + ) ( new_refresh_token, diff --git a/tests/rest/client/v2_alpha/test_auth.py b/tests/rest/client/v2_alpha/test_auth.py index 2ab4de313666..e6d2a70a0080 100644 --- a/tests/rest/client/v2_alpha/test_auth.py +++ b/tests/rest/client/v2_alpha/test_auth.py @@ -676,7 +676,7 @@ def test_refresh_token_invalidation(self): {"refresh_token": first_refresh_response.json_body["refresh_token"]}, ) self.assertEqual( - third_refresh_response.code, 400, third_refresh_response.result + third_refresh_response.code, 401, third_refresh_response.result ) # The associated access token should also be invalid @@ -704,7 +704,7 @@ def test_refresh_token_invalidation(self): {"refresh_token": login_response.json_body["refresh_token"]}, ) self.assertEqual( - fourth_refresh_response.code, 400, fourth_refresh_response.result + fourth_refresh_response.code, 401, fourth_refresh_response.result ) # But refreshing from the last valid refresh token still works From 8f8f3690e091378061345d11e7d814fa78580845 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 3 Jun 2021 18:51:02 +0200 Subject: [PATCH 35/46] MSC2918: disable refresh tokens when session_lifetime is set --- docs/sample_config.yaml | 4 ---- synapse/config/registration.py | 19 +++++++++++++------ synapse/rest/client/v1/login.py | 16 +++++++++++----- synapse/rest/client/v2_alpha/register.py | 14 +++++++++----- 4 files changed, 33 insertions(+), 20 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 6a09f71f8d80..7b97f73a296b 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1176,10 +1176,6 @@ url_preview_accept_language: # #session_lifetime: 24h -# MSC2918 -# TODO: docs -#access_token_lifetime: 5m - # The user must provide all of the below types of 3PID when registering. # #registrations_require_3pid: diff --git a/synapse/config/registration.py b/synapse/config/registration.py index f50bb214a0f2..dbfc81647b54 100644 --- a/synapse/config/registration.py +++ b/synapse/config/registration.py @@ -119,10 +119,21 @@ def read_config(self, config, **kwargs): session_lifetime = self.parse_duration(session_lifetime) self.session_lifetime = session_lifetime - access_token_lifetime = config.get("access_token_lifetime", "5m") - access_token_lifetime = self.parse_duration(access_token_lifetime) + access_token_lifetime = config.get( + "access_token_lifetime", "5m" if session_lifetime is None else None + ) + if access_token_lifetime is not None: + access_token_lifetime = self.parse_duration(access_token_lifetime) self.access_token_lifetime = access_token_lifetime + if session_lifetime is not None and access_token_lifetime is not None: + raise ConfigError( + "The refresh token mechanism is incompatible with the " + "`session_lifetime` option. Consider disabling the " + "`session_lifetime` option or disabling the refresh token " + "mechanism by removing the `access_token_lifetime` option." + ) + # The success template used during fallback auth. self.fallback_success_template = self.read_template("auth_success.html") @@ -156,10 +167,6 @@ def generate_config_section(self, generate_secrets=False, **kwargs): # #session_lifetime: 24h - # MSC2918 - # TODO: docs - #access_token_lifetime: 5m - # The user must provide all of the below types of 3PID when registering. # #registrations_require_3pid: diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py index 76483d22f3bd..8688610468f8 100644 --- a/synapse/rest/client/v1/login.py +++ b/synapse/rest/client/v1/login.py @@ -84,6 +84,7 @@ def __init__(self, hs: "HomeServer"): self.cas_enabled = hs.config.cas_enabled self.oidc_enabled = hs.config.oidc_enabled self._msc2858_enabled = hs.config.experimental.msc2858_enabled + self._msc2918_enabled = hs.config.access_token_lifetime is not None self.auth = hs.get_auth() @@ -159,10 +160,14 @@ def on_GET(self, request: SynapseRequest): async def on_POST(self, request: SynapseRequest): login_submission = parse_json_object_from_request(request) - # Check if this login should also issue a refresh token, as per MSC2918 - should_issue_refresh_token = parse_boolean( - request, name=LoginRestServlet.REFRESH_TOKEN_PARAM, default=False - ) + if self._msc2918_enabled: + # Check if this login should also issue a refresh token, as per + # MSC2918 + should_issue_refresh_token = parse_boolean( + request, name=LoginRestServlet.REFRESH_TOKEN_PARAM, default=False + ) + else: + should_issue_refresh_token = False try: if login_submission["type"] == LoginRestServlet.APPSERVICE_TYPE: @@ -595,7 +600,8 @@ async def on_GET(self, request: SynapseRequest) -> None: def register_servlets(hs, http_server): LoginRestServlet(hs).register(http_server) - RefreshTokenServlet(hs).register(http_server) + if hs.config.access_token_lifetime is not None: + RefreshTokenServlet(hs).register(http_server) SsoRedirectServlet(hs).register(http_server) if hs.config.cas_enabled: CasTicketServlet(hs).register(http_server) diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index 1225d93a0c9f..dc3e9f590c72 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -400,6 +400,7 @@ def __init__(self, hs): self.password_policy_handler = hs.get_password_policy_handler() self.clock = hs.get_clock() self._registration_enabled = self.hs.config.enable_registration + self._msc2918_enabled = hs.config.access_token_lifetime is not None self._registration_flows = _calculate_registration_flows( hs.config, self.auth_handler @@ -425,11 +426,14 @@ async def on_POST(self, request): "Do not understand membership kind: %s" % (kind.decode("utf8"),) ) - # Check if this registration should also issue a refresh token, as per - # MSC2918 - should_issue_refresh_token = parse_boolean( - request, name="org.matrix.msc2918.refresh_token", default=False - ) + if self._msc2918_enabled: + # Check if this registration should also issue a refresh token, as + # per MSC2918 + should_issue_refresh_token = parse_boolean( + request, name="org.matrix.msc2918.refresh_token", default=False + ) + else: + should_issue_refresh_token = False # Pull out the provided username and do basic sanity checks early since # the auth layer will store these in sessions. From 6024ed89983c0cf756a354279cf9eb464249acc1 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 3 Jun 2021 19:06:36 +0200 Subject: [PATCH 36/46] MSC2918: add missing docstring --- synapse/rest/client/v2_alpha/register.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index dc3e9f590c72..4d31584acd20 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -47,6 +47,7 @@ ) from synapse.metrics import threepid_send_requests from synapse.push.mailer import Mailer +from synapse.types import JsonDict from synapse.util.msisdn import phone_number_to_msisdn from synapse.util.ratelimitutils import FederationRateLimiter from synapse.util.stringutils import assert_valid_client_secret, random_string @@ -708,9 +709,9 @@ async def _do_appservice_registration( async def _create_registration_details( self, - user_id, - params, - is_appservice_ghost=False, + user_id: str, + params: JsonDict, + is_appservice_ghost: bool = False, should_issue_refresh_token: bool = False, ): """Complete registration of newly-registered user @@ -718,9 +719,12 @@ async def _create_registration_details( Allocates device_id if one was not given; also creates access_token. Args: - (str) user_id: full canonical @user:id - (object) params: registration parameters, from which we pull - device_id, initial_device_name and inhibit_login + user_id: full canonical @user:id + params: registration parameters, from which we pull device_id, + initial_device_name and inhibit_login + is_appservice_ghost + should_issue_refresh_token: True if this registration should issue + a refresh token alongside the access token. Returns: dictionary for response from /register """ From 908c27929cdf5f8d0f16f16ee97c503cffeff007 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 3 Jun 2021 22:08:56 +0200 Subject: [PATCH 37/46] MSC2918: temp: mark the access token as used only once This is a temporary fix to work around the cache. It's only there to confirm it broke tests, and check if CI passes with this fix. --- synapse/api/auth.py | 6 ++++++ synapse/storage/databases/main/registration.py | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/synapse/api/auth.py b/synapse/api/auth.py index dc338d553c97..713cea82ff9b 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -253,6 +253,12 @@ async def get_user_by_req( if not user_info.token_used and token_id is not None: await self.store.mark_access_token_as_used(token_id) + # TODO: this is a temporary fix because if we don't do this, it + # will do the update every time because of the cache. This + # required unfreezing the TokenLookupResult class, which is not + # ideal. + user_info.token_used = True + requester = create_requester( user_info.user_id, token_id, diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index 9e1d68423ee7..aead55d3e303 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -40,7 +40,7 @@ logger = logging.getLogger(__name__) -@attr.s(frozen=True, slots=True) +@attr.s(slots=True) class TokenLookupResult: """Result of looking up an access token. From cdfd871bc3136661fe37102e9403544f5f04b773 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 4 Jun 2021 11:50:57 +0200 Subject: [PATCH 38/46] MSC2918: explicit cast on access_tokens.used Boolean handling seems to be broken with older versions of sqlite making the py3-old environment fail in CI. This explicitely casts the `access_tokens.used` column to INTEGER then compares it with "1" when querying it. --- synapse/storage/databases/main/registration.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index aead55d3e303..f2a802c9c0a7 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -467,7 +467,7 @@ def _query_for_auth(self, txn, token: str) -> Optional[TokenLookupResult]: access_tokens.device_id, access_tokens.valid_until_ms, access_tokens.user_id as token_owner, - access_tokens.used as token_used + (CAST(access_tokens.used AS INTEGER) = 1) as token_used FROM users INNER JOIN access_tokens on users.name = COALESCE(puppets_user_id, access_tokens.user_id) WHERE token = ? @@ -1129,7 +1129,7 @@ def _lookup_refresh_token_txn(txn) -> Optional[RefreshTokenLookupResult]: rt.device_id, rt.next_token_id, (nrt.next_token_id IS NOT NULL) has_next_refresh_token_been_refreshed, - at.used has_next_access_token_been_used + (CAST(at.used AS INTEGER) = 1) has_next_access_token_been_used FROM refresh_tokens rt LEFT JOIN refresh_tokens nrt ON rt.next_token_id = nrt.id LEFT JOIN access_tokens at ON at.refresh_token_id = nrt.id @@ -1147,8 +1147,8 @@ def _lookup_refresh_token_txn(txn) -> Optional[RefreshTokenLookupResult]: user_id=row[1], device_id=row[2], next_token_id=row[3], - has_next_refresh_token_been_refreshed=row[4], - has_next_access_token_been_used=row[5], + has_next_refresh_token_been_refreshed=bool(row[4]), + has_next_access_token_been_used=bool(row[5]), ) return await self.db_pool.runInteraction( From b169a62157d55c1c5b3de02794eeaf433f5ae555 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 4 Jun 2021 12:22:12 +0200 Subject: [PATCH 39/46] Revert "MSC2918: explicit cast on access_tokens.used" This reverts commit cdfd871bc3136661fe37102e9403544f5f04b773. --- synapse/storage/databases/main/registration.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index f2a802c9c0a7..aead55d3e303 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -467,7 +467,7 @@ def _query_for_auth(self, txn, token: str) -> Optional[TokenLookupResult]: access_tokens.device_id, access_tokens.valid_until_ms, access_tokens.user_id as token_owner, - (CAST(access_tokens.used AS INTEGER) = 1) as token_used + access_tokens.used as token_used FROM users INNER JOIN access_tokens on users.name = COALESCE(puppets_user_id, access_tokens.user_id) WHERE token = ? @@ -1129,7 +1129,7 @@ def _lookup_refresh_token_txn(txn) -> Optional[RefreshTokenLookupResult]: rt.device_id, rt.next_token_id, (nrt.next_token_id IS NOT NULL) has_next_refresh_token_been_refreshed, - (CAST(at.used AS INTEGER) = 1) has_next_access_token_been_used + at.used has_next_access_token_been_used FROM refresh_tokens rt LEFT JOIN refresh_tokens nrt ON rt.next_token_id = nrt.id LEFT JOIN access_tokens at ON at.refresh_token_id = nrt.id @@ -1147,8 +1147,8 @@ def _lookup_refresh_token_txn(txn) -> Optional[RefreshTokenLookupResult]: user_id=row[1], device_id=row[2], next_token_id=row[3], - has_next_refresh_token_been_refreshed=bool(row[4]), - has_next_access_token_been_used=bool(row[5]), + has_next_refresh_token_been_refreshed=row[4], + has_next_access_token_been_used=row[5], ) return await self.db_pool.runInteraction( From e07ef9b8d97e4d7500d6045519af4dd83a7f5440 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 4 Jun 2021 12:38:39 +0200 Subject: [PATCH 40/46] MSC2918: properly fix access_tokens.used column on old SQLite --- .../schema/main/delta/59/14refresh_tokens.sql | 4 ---- .../delta/59/14refresh_tokens.sql.postgres | 18 ++++++++++++++++++ .../main/delta/59/14refresh_tokens.sql.sqlite | 18 ++++++++++++++++++ 3 files changed, 36 insertions(+), 4 deletions(-) create mode 100644 synapse/storage/schema/main/delta/59/14refresh_tokens.sql.postgres create mode 100644 synapse/storage/schema/main/delta/59/14refresh_tokens.sql.sqlite diff --git a/synapse/storage/schema/main/delta/59/14refresh_tokens.sql b/synapse/storage/schema/main/delta/59/14refresh_tokens.sql index 898f18f70363..a17116dd929e 100644 --- a/synapse/storage/schema/main/delta/59/14refresh_tokens.sql +++ b/synapse/storage/schema/main/delta/59/14refresh_tokens.sql @@ -28,7 +28,3 @@ CREATE TABLE refresh_tokens ( -- Add a reference to the refresh token generated alongside each access token ALTER TABLE "access_tokens" ADD COLUMN refresh_token_id BIGINT REFERENCES refresh_tokens (id) ON DELETE CASCADE; - --- Add a flag whether the token was already used or not -ALTER TABLE "access_tokens" - ADD COLUMN used BOOLEAN NOT NULL DEFAULT FALSE; diff --git a/synapse/storage/schema/main/delta/59/14refresh_tokens.sql.postgres b/synapse/storage/schema/main/delta/59/14refresh_tokens.sql.postgres new file mode 100644 index 000000000000..23916d61e35b --- /dev/null +++ b/synapse/storage/schema/main/delta/59/14refresh_tokens.sql.postgres @@ -0,0 +1,18 @@ +/* Copyright 2021 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. + */ + +-- Add a flag whether the token was already used or not +ALTER TABLE "access_tokens" + ADD COLUMN used BOOLEAN NOT NULL DEFAULT FALSE; diff --git a/synapse/storage/schema/main/delta/59/14refresh_tokens.sql.sqlite b/synapse/storage/schema/main/delta/59/14refresh_tokens.sql.sqlite new file mode 100644 index 000000000000..6c828abcabe5 --- /dev/null +++ b/synapse/storage/schema/main/delta/59/14refresh_tokens.sql.sqlite @@ -0,0 +1,18 @@ +/* Copyright 2021 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. + */ + +-- Add a flag whether the token was already used or not +ALTER TABLE "access_tokens" + ADD COLUMN used BOOLEAN NOT NULL DEFAULT 0; From ef0e051d6efeeb10bf60afdabbe41846e269cf6d Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 4 Jun 2021 14:42:35 +0200 Subject: [PATCH 41/46] MSC2918: properly fix "mark_access_token_as_used" by caching it --- synapse/api/auth.py | 6 ------ synapse/storage/databases/main/registration.py | 9 ++++++++- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/synapse/api/auth.py b/synapse/api/auth.py index 803495ea1572..324de029fee1 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -253,12 +253,6 @@ async def get_user_by_req( if not user_info.token_used and token_id is not None: await self.store.mark_access_token_as_used(token_id) - # TODO: this is a temporary fix because if we don't do this, it - # will do the update every time because of the cache. This - # required unfreezing the TokenLookupResult class, which is not - # ideal. - user_info.token_used = True - requester = create_requester( user_info.user_id, token_id, diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index aead55d3e303..d4cca261ccf7 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -40,7 +40,7 @@ logger = logging.getLogger(__name__) -@attr.s(slots=True) +@attr.s(frozen=True, slots=True) class TokenLookupResult: """Result of looking up an access token. @@ -1098,11 +1098,18 @@ async def update_access_token_last_validated(self, token_id: int) -> None: desc="update_access_token_last_validated", ) + @cached() async def mark_access_token_as_used(self, token_id: int) -> None: """ Mark the access token as used, which invalidates the refresh token used to obtain it. + Because get_user_by_access_token is cached, this function might be + called multiple times for the same token, effectively doing unnecessary + SQL updates. Because updating the `used` field only goes one way (from + False to True) it is safe to cache this function as well to avoid this + issue. + Args: token_id: The ID of the access token to update. Raises: From ab443a34ede872e0d40d4ee8433c8551ff85431a Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 17 Jun 2021 12:13:45 +0200 Subject: [PATCH 42/46] MSC2918: add comments as suggested by richvdh Signed-off-by: Quentin Gliech --- synapse/config/registration.py | 5 +++++ synapse/handlers/auth.py | 2 +- synapse/storage/databases/main/registration.py | 2 ++ 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/synapse/config/registration.py b/synapse/config/registration.py index dbfc81647b54..53a179b4793a 100644 --- a/synapse/config/registration.py +++ b/synapse/config/registration.py @@ -119,6 +119,11 @@ def read_config(self, config, **kwargs): session_lifetime = self.parse_duration(session_lifetime) self.session_lifetime = session_lifetime + # The `access_token_lifetime` applies for tokens that can be renewed + # using a refresh token, as per MSC2918. This behaviour can be disabled + # by setting it to `None` (`null` in the YAML config). Since it is + # incompatible with the `session_lifetime` mechanism, it is set to + # `None` by default if a `session_lifetime` is set. access_token_lifetime = config.get( "access_token_lifetime", "5m" if session_lifetime is None else None ) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 0bcee522ff24..7b6aeb483142 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -1321,7 +1321,7 @@ def generate_access_token(self, for_user: UserID) -> str: def generate_refresh_token(self, for_user: UserID) -> str: """Generates an opaque string, for use as a refresh token""" - # we use the following format for access tokens: + # we use the following format for refresh tokens: # syr___ b64local = unpaddedbase64.encode_base64(for_user.localpart.encode("utf-8")) diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index d4cca261ccf7..a706a448f7b4 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -54,6 +54,8 @@ class TokenLookupResult: token_owner: The "owner" of the token. This is either the same as the user, or a server admin who is logged in as the user. token_used: True if this token was used at least once in a request. + This field can be out of date since `get_user_by_access_token` is + cached. """ user_id = attr.ib(type=str) From 18628fc3adeb28fb820c4e5e1da3b8bece4ce40f Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 18 Jun 2021 10:05:24 +0200 Subject: [PATCH 43/46] MSC2918: make access_tokens.used nullable This avoids rewriting the whole table on disk on Postgres < 11 Signed-off-by: Quentin Gliech --- synapse/storage/databases/main/registration.py | 13 +++++++++++-- .../schema/main/delta/59/14refresh_tokens.sql | 4 ++++ .../delta/59/14refresh_tokens.sql.postgres | 18 ------------------ .../main/delta/59/14refresh_tokens.sql.sqlite | 18 ------------------ 4 files changed, 15 insertions(+), 38 deletions(-) delete mode 100644 synapse/storage/schema/main/delta/59/14refresh_tokens.sql.postgres delete mode 100644 synapse/storage/schema/main/delta/59/14refresh_tokens.sql.sqlite diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index a706a448f7b4..e31c5864acef 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -477,8 +477,15 @@ def _query_for_auth(self, txn, token: str) -> Optional[TokenLookupResult]: txn.execute(sql, (token,)) rows = self.db_pool.cursor_to_dict(txn) + if rows: - return TokenLookupResult(**rows[0]) + row = rows[0] + + # This field is nullable, ensure it comes out as a boolean + if row["token_used"] is None: + row["token_used"] = False + + return TokenLookupResult(**row) return None @@ -1157,7 +1164,8 @@ def _lookup_refresh_token_txn(txn) -> Optional[RefreshTokenLookupResult]: device_id=row[2], next_token_id=row[3], has_next_refresh_token_been_refreshed=row[4], - has_next_access_token_been_used=row[5], + # This column is nullable, ensure it's a boolean + has_next_access_token_been_used=(row[5] or False), ) return await self.db_pool.runInteraction( @@ -1435,6 +1443,7 @@ async def add_access_token_to_user( "puppets_user_id": puppets_user_id, "last_validated": now, "refresh_token_id": refresh_token_id, + "used": False, }, desc="add_access_token_to_user", ) diff --git a/synapse/storage/schema/main/delta/59/14refresh_tokens.sql b/synapse/storage/schema/main/delta/59/14refresh_tokens.sql index a17116dd929e..9a6bce1e3e9f 100644 --- a/synapse/storage/schema/main/delta/59/14refresh_tokens.sql +++ b/synapse/storage/schema/main/delta/59/14refresh_tokens.sql @@ -28,3 +28,7 @@ CREATE TABLE refresh_tokens ( -- Add a reference to the refresh token generated alongside each access token ALTER TABLE "access_tokens" ADD COLUMN refresh_token_id BIGINT REFERENCES refresh_tokens (id) ON DELETE CASCADE; + +-- Add a flag whether the token was already used or not +ALTER TABLE "access_tokens" + ADD COLUMN used BOOLEAN; diff --git a/synapse/storage/schema/main/delta/59/14refresh_tokens.sql.postgres b/synapse/storage/schema/main/delta/59/14refresh_tokens.sql.postgres deleted file mode 100644 index 23916d61e35b..000000000000 --- a/synapse/storage/schema/main/delta/59/14refresh_tokens.sql.postgres +++ /dev/null @@ -1,18 +0,0 @@ -/* Copyright 2021 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. - */ - --- Add a flag whether the token was already used or not -ALTER TABLE "access_tokens" - ADD COLUMN used BOOLEAN NOT NULL DEFAULT FALSE; diff --git a/synapse/storage/schema/main/delta/59/14refresh_tokens.sql.sqlite b/synapse/storage/schema/main/delta/59/14refresh_tokens.sql.sqlite deleted file mode 100644 index 6c828abcabe5..000000000000 --- a/synapse/storage/schema/main/delta/59/14refresh_tokens.sql.sqlite +++ /dev/null @@ -1,18 +0,0 @@ -/* Copyright 2021 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. - */ - --- Add a flag whether the token was already used or not -ALTER TABLE "access_tokens" - ADD COLUMN used BOOLEAN NOT NULL DEFAULT 0; From bcc33e25fe7bb8a724b9495ce479a1d24baa8570 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 18 Jun 2021 10:07:21 +0200 Subject: [PATCH 44/46] MSC2918: 403 when using a refresh token twice This could help differenciate errors where the refresh token was never valid from errors where it is not valid anymore Signed-off-by: Quentin Gliech --- synapse/handlers/auth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 44d881388cfd..e2ac595a6214 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -810,7 +810,7 @@ async def refresh_token( or existing_token.has_next_refresh_token_been_refreshed ): raise SynapseError( - 401, "refresh token isn't valid anymore", Codes.UNKNOWN_TOKEN + 403, "refresh token isn't valid anymore", Codes.FORBIDDEN ) ( From ddfc2a4bd7228ea934e7fb580bd703c6ac010e7d Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 18 Jun 2021 10:10:21 +0200 Subject: [PATCH 45/46] MSC2918: clarify comment about access_token_lifetime and session_lifetime interaction in config Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- synapse/config/registration.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/synapse/config/registration.py b/synapse/config/registration.py index 53a179b4793a..0ad919b1394b 100644 --- a/synapse/config/registration.py +++ b/synapse/config/registration.py @@ -120,9 +120,10 @@ def read_config(self, config, **kwargs): self.session_lifetime = session_lifetime # The `access_token_lifetime` applies for tokens that can be renewed - # using a refresh token, as per MSC2918. This behaviour can be disabled - # by setting it to `None` (`null` in the YAML config). Since it is - # incompatible with the `session_lifetime` mechanism, it is set to + # using a refresh token, as per MSC2918. If it is `None`, the refresh + # token mechanism is disabled. + # + # Since it is incompatible with the `session_lifetime` mechanism, it is set to # `None` by default if a `session_lifetime` is set. access_token_lifetime = config.get( "access_token_lifetime", "5m" if session_lifetime is None else None From 9fe55563efbb1f4caed08bdd5dbb34aab4ce782c Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 18 Jun 2021 10:57:39 +0200 Subject: [PATCH 46/46] MSC2918: fix refresh token invalidation test Signed-off-by: Quentin Gliech --- tests/rest/client/v2_alpha/test_auth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/rest/client/v2_alpha/test_auth.py b/tests/rest/client/v2_alpha/test_auth.py index e6d2a70a0080..6b90f838b6da 100644 --- a/tests/rest/client/v2_alpha/test_auth.py +++ b/tests/rest/client/v2_alpha/test_auth.py @@ -704,7 +704,7 @@ def test_refresh_token_invalidation(self): {"refresh_token": login_response.json_body["refresh_token"]}, ) self.assertEqual( - fourth_refresh_response.code, 401, fourth_refresh_response.result + fourth_refresh_response.code, 403, fourth_refresh_response.result ) # But refreshing from the last valid refresh token still works