matrix-org · anoadragon453 · Dec 30, 2020 · Dec 16, 2020 · Dec 16, 2020 · Dec 17, 2020
diff --git a/synapse/config/registration.py b/synapse/config/registration.py
@@ -13,11 +13,8 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import os
 from distutils.util import strtobool
 
-import pkg_resources
-
 from synapse.api.constants import RoomCreationPreset
 from synapse.config._base import Config, ConfigError
 from synapse.types import RoomAlias, UserID
@@ -34,6 +31,11 @@ def __init__(self, config, synapse_config):
         self.enabled = config.get("enabled", False)
         self.renew_by_email_enabled = "renew_at" in config
 
+        # These are set in RegistrationConfig.read_config
+        self.account_renewed_template = None
+        self.account_previously_renewed_template = None
+        self.invalid_token_template = None
+
         if self.enabled:
             if "period" in config:
                 self.period = self.parse_duration(config["period"])
@@ -54,33 +56,6 @@ def __init__(self, config, synapse_config):
             if "public_baseurl" not in synapse_config:
                 raise ConfigError("Can't send renewal emails without 'public_baseurl'")
 
-        template_dir = config.get("template_dir")
-
-        if not template_dir:
-            template_dir = pkg_resources.resource_filename("synapse", "res/templates")
-
-        if "account_renewed_html_path" in config:
-            file_path = os.path.join(template_dir, config["account_renewed_html_path"])
-
-            self.account_renewed_html_content = self.read_file(
-                file_path, "account_validity.account_renewed_html_path"
-            )
-        else:
-            self.account_renewed_html_content = (
-                "<html><body>Your account has been successfully renewed.</body><html>"
-            )
-
-        if "invalid_token_html_path" in config:
-            file_path = os.path.join(template_dir, config["invalid_token_html_path"])
-
-            self.invalid_token_html_content = self.read_file(
-                file_path, "account_validity.invalid_token_html_path"
-            )
-        else:
-            self.invalid_token_html_content = (
-                "<html><body>Invalid renewal token.</body><html>"
-            )
-
 
 class RegistrationConfig(Config):
     section = "registration"
@@ -98,6 +73,30 @@ def read_config(self, config, **kwargs):
             config.get("account_validity") or {}, config
         )
 
+        # Load account validity templates.
+        # We do this here instead of in AccountValidityConfig as read_templates
+        # relies on state that hasn't been initialised in AccountValidityConfig
+        account_renewed_template_filename = config.get(
+            "account_renewed_html_path", "account_renewed.html"
+        )
+        account_previously_renewed_template_filename = config.get(
+            "account_previously_renewed_html_path", "account_previously_renewed.html"
+        )
+        invalid_token_template_filename = config.get(
+            "invalid_token_html_path", "invalid_token.html"
+        )
+        (
+            self.account_validity.account_renewed_template,
+            self.account_validity.account_previously_renewed_template,
+            self.account_validity.invalid_token_template,
+        ) = self.read_templates(
+            [
+                account_renewed_template_filename,
+                account_previously_renewed_template_filename,
+                invalid_token_template_filename,
+            ]
+        )
+
         self.registrations_require_3pid = config.get("registrations_require_3pid", [])
         self.allowed_local_3pids = config.get("allowed_local_3pids", [])
         self.check_is_for_allowed_local_3pids = config.get(

diff --git a/synapse/handlers/account_validity.py b/synapse/handlers/account_validity.py
@@ -18,7 +18,7 @@
 import logging
 from email.mime.multipart import MIMEMultipart
 from email.mime.text import MIMEText
-from typing import List
+from typing import List, Optional, Tuple
 
 from synapse.api.errors import StoreError
 from synapse.logging.context import make_deferred_yieldable
@@ -223,47 +223,87 @@ async def _get_renewal_token(self, user_id: str) -> str:
                 attempts += 1
         raise StoreError(500, "Couldn't generate a unique string as refresh string.")
 
-    async def renew_account(self, renewal_token: str) -> bool:
+    async def renew_account(self, renewal_token: str) -> Tuple[bool, bool, int]:
         """Renews the account attached to a given renewal token by pushing back the
         expiration date by the current validity period in the server's configuration.
 
+        If it turns out that the token is valid but has already been used, then the
+        token is considered stale. A token is stale if the 'token_used_ts_ms' db column
+        is non-null.
+
         Args:
             renewal_token: Token sent with the renewal request.
         Returns:
-            Whether the provided token is valid.
+            A tuple containing:
+              * A bool representing whether the token is valid.
+              * A bool representing whether the token is stale.
+              * An int representing the user's expiry timestamp as milliseconds since the
+                epoch, or 0 if the token was invalid.
         """
         try:
-            user_id = await self.store.get_user_from_renewal_token(renewal_token)
+            (
+                user_id,
+                current_expiration_ts,
+                token_used_ts,
+            ) = await self.store.get_user_from_renewal_token(renewal_token)
         except StoreError:
-            return False
+            return False, False, 0
+
+        # Check whether this token has already been used.
+        if token_used_ts:
+            logger.info(
+                "User '%s' attempted to use previously used token '%s' to renew account",
+                user_id,
+                renewal_token,
+            )
+            return False, True, current_expiration_ts
 
         logger.debug("Renewing an account for user %s", user_id)
-        await self.renew_account_for_user(user_id)
 
-        return True
+        # Renew the account. Pass the renewal_token here so that it is not cleared.
+        # We want to keep the token around in case the user attempts to renew their
+        # account with the same token twice (clicking the email link twice).
+        #
+        # In that case, the token will be accepted, but the account's expiration ts
+        # will remain unchanged.
+        new_expiration_ts = await self.renew_account_for_user(
+            user_id, renewal_token=renewal_token
+        )
+
+        return True, False, new_expiration_ts
 
     async def renew_account_for_user(
-        self, user_id: str, expiration_ts: int = None, email_sent: bool = False
+        self,
+        user_id: str,
+        expiration_ts: Optional[int] = None,
+        email_sent: Optional[bool] = False,
+        renewal_token: Optional[str] = None,
     ) -> int:
         """Renews the account attached to a given user by pushing back the
         expiration date by the current validity period in the server's
         configuration.
 
         Args:
-            renewal_token: Token sent with the renewal request.
+            user_id: The ID of the user to renew.
             expiration_ts: New expiration date. Defaults to now + validity period.
-            email_sen: Whether an email has been sent for this validity period.
-                Defaults to False.
+            email_sent: Whether an email has been sent for this validity period.
+            renewal_token: Token sent with the renewal request. The user's token
+                will be cleared if this is None.
 
         Returns:
             New expiration date for this account, as a timestamp in
             milliseconds since epoch.
         """
+        now = self.clock.time_msec()
         if expiration_ts is None:
-            expiration_ts = self.clock.time_msec() + self._account_validity.period
+            expiration_ts = now + self._account_validity.period
 
         await self.store.set_account_validity_for_user(
-            user_id=user_id, expiration_ts=expiration_ts, email_sent=email_sent
+            user_id=user_id,
+            expiration_ts=expiration_ts,
+            email_sent=email_sent,
+            renewal_token=renewal_token,
+            token_used_ts=now,
         )
 
         # Check if renewed users should be reintroduced to the user directory

diff --git a/synapse/res/templates/account_previously_renewed.html b/synapse/res/templates/account_previously_renewed.html
@@ -0,0 +1 @@
+<html><body>Your account is valid until {{ expiration_ts|format_ts("%d-%m-%Y") }}.</body><html>
diff --git a/synapse/res/templates/account_renewed.html b/synapse/res/templates/account_renewed.html
@@ -1 +1 @@
-<html><body>Your account has been successfully renewed.</body><html>
+<html><body>Your account has been successfully renewed and is valid until {{ expiration_ts|format_ts("%d-%m-%Y") }}.</body><html>
diff --git a/synapse/rest/client/v2_alpha/account_validity.py b/synapse/rest/client/v2_alpha/account_validity.py
@@ -37,24 +37,38 @@ def __init__(self, hs):
         self.hs = hs
         self.account_activity_handler = hs.get_account_validity_handler()
         self.auth = hs.get_auth()
-        self.success_html = hs.config.account_validity.account_renewed_html_content
-        self.failure_html = hs.config.account_validity.invalid_token_html_content
+        self.account_renewed_template = (
+            hs.config.account_validity.account_renewed_template
+        )
+        self.account_previously_renewed_template = (
+            hs.config.account_validity.account_previously_renewed_template
+        )
+        self.invalid_token_template = hs.config.account_validity.invalid_token_template
 
     async def on_GET(self, request):
         if b"token" not in request.args:
             raise SynapseError(400, "Missing renewal token")
         renewal_token = request.args[b"token"][0]
 
-        token_valid = await self.account_activity_handler.renew_account(
+        (
+            token_valid,
+            token_stale,
+            expiration_ts,
+        ) = await self.account_activity_handler.renew_account(
             renewal_token.decode("utf8")
         )
 
         if token_valid:
             status_code = 200
-            response = self.success_html
+            response = self.account_renewed_template.render(expiration_ts=expiration_ts)
+        elif token_stale:
+            status_code = 200
+            response = self.account_previously_renewed_template.render(
+                expiration_ts=expiration_ts
+            )
         else:
             status_code = 404
-            response = self.failure_html
+            response = self.invalid_token_template.render(expiration_ts=expiration_ts)
 
         respond_with_html(request, status_code, response)
 

diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py
@@ -139,6 +139,7 @@ async def set_account_validity_for_user(
         expiration_ts: int,
         email_sent: bool,
         renewal_token: Optional[str] = None,
+        token_used_ts: Optional[int] = None,
     ) -> None:
         """Updates the account validity properties of the given account, with the
         given values.
@@ -152,6 +153,8 @@ async def set_account_validity_for_user(
                 period.
             renewal_token: Renewal token the user can use to extend the validity
                 of their account. Defaults to no token.
+            token_used_ts: A timestamp of when the current token was used to renew
+                the account.
         """
 
         def set_account_validity_for_user_txn(txn):
@@ -163,6 +166,7 @@ def set_account_validity_for_user_txn(txn):
                     "expiration_ts_ms": expiration_ts,
                     "email_sent": email_sent,
                     "renewal_token": renewal_token,
+                    "token_used_ts_ms": token_used_ts,
                 },
             )
             self._invalidate_cache_and_stream(
@@ -207,7 +211,7 @@ def get_expired_users_txn(txn, now_ms):
     async def set_renewal_token_for_user(
         self, user_id: str, renewal_token: str
     ) -> None:
-        """Defines a renewal token for a given user.
+        """Defines a renewal token for a given user, and clears the token_used timestamp.
 
         Args:
             user_id: ID of the user to set the renewal token for.
@@ -220,26 +224,40 @@ async def set_renewal_token_for_user(
         await self.db_pool.simple_update_one(
             table="account_validity",
             keyvalues={"user_id": user_id},
-            updatevalues={"renewal_token": renewal_token},
+            updatevalues={"renewal_token": renewal_token, "token_used_ts_ms": None},
             desc="set_renewal_token_for_user",
         )
 
-    async def get_user_from_renewal_token(self, renewal_token: str) -> str:
-        """Get a user ID from a renewal token.
+    async def get_user_from_renewal_token(
+        self, renewal_token: str
+    ) -> Tuple[str, int, Optional[int]]:
+        """Get a user ID and renewal status from a renewal token.
 
         Args:
             renewal_token: The renewal token to perform the lookup with.
 
         Returns:
-            The ID of the user to which the token belongs.
+            A tuple of containing the following values:
+                * The ID of a user to which the token belongs.
+                * An int representing the user's expiry timestamp as milliseconds since the
+                    epoch, or 0 if the token was invalid.
+                * An optional int representing the timestamp of when the user renewed their
+                    account timestamp as milliseconds since the epoch. None if the account
+                    has not been renewed using the current token yet.
         """
-        return await self.db_pool.simple_select_one_onecol(
+        ret_dict = await self.db_pool.simple_select_one(
             table="account_validity",
             keyvalues={"renewal_token": renewal_token},
-            retcol="user_id",
+            retcols=["user_id", "email_sent", "expiration_ts_ms", "token_used_ts_ms"],
             desc="get_user_from_renewal_token",
         )
 
+        return (
+            ret_dict["user_id"],
+            ret_dict["expiration_ts_ms"],
+            ret_dict["token_used_ts_ms"],
+        )
+
     async def get_renewal_token_for_user(self, user_id: str) -> str:
         """Get the renewal token associated with a given user ID.
 

diff --git a/synapse/storage/databases/main/schema/delta/58/19account_validity_token_used_ts_ms.sql b/synapse/storage/databases/main/schema/delta/58/19account_validity_token_used_ts_ms.sql
@@ -0,0 +1,18 @@
+/* Copyright 2020 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.
+ */
+
+-- Track when users renew their account using the value of the 'renewal_token' column.
+-- This field should be set to NULL after a fresh token is generated.
+ALTER TABLE account_validity ADD token_used_ts_ms BIGINT;
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		<html><body>Your account is valid until {{ expiration_ts\|format_ts("%d-%m-%Y") }}.</body><html>
Original file line number	Diff line number	Diff line change
		@@ -1 +1 @@
		<html><body>Your account has been successfully renewed.</body><html>
		<html><body>Your account has been successfully renewed and is valid until {{ expiration_ts\|format_ts("%d-%m-%Y") }}.</body><html>