MSC2918 Refresh tokens implementation

docs/sample_config.yaml

@@ -1180,10 +1180,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:

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:

synapse/handlers/auth.py

@@ -793,18 +793,20 @@ 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")
+        if not self._verify_refresh_token(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,
@@ -823,7 +825,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.
 

synapse/rest/client/v1/login.py

@@ -14,7 +14,7 @@
 
 import logging
 import re
-from typing import TYPE_CHECKING, Any, Awaitable, Callable, Dict, Optional
+from typing import TYPE_CHECKING, Any, Awaitable, Callable, Dict, List, Optional
 
 from typing_extensions import TypedDict
 
@@ -29,6 +29,7 @@
     RestServlet,
     assert_params_in_dict,
     parse_boolean,
+    parse_bytes_from_args,
     parse_json_object_from_request,
     parse_string,
 )
@@ -84,6 +85,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 +161,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:
@@ -253,8 +259,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 +312,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 +375,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.
@@ -594,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)

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
@@ -400,6 +401,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 +427,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.
@@ -704,19 +709,22 @@ 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
 
         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
         """

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):
@@ -1084,8 +1098,17 @@ 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 old refresh token.
+        """
+        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.
@@ -1140,6 +1163,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 +1409,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 +1445,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 +1467,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

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;

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;

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;