From 0d75d6a2a386c59b27a68babfb735c2ebb524c09 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 4 May 2021 17:43:31 +0100 Subject: [PATCH 01/26] Support renewing an authenticated token --- email_account_validity/_base.py | 12 ++++++++++-- email_account_validity/_constants.py | 20 ++++++++++++++++++++ email_account_validity/_store.py | 10 ++++++++-- email_account_validity/servlets.py | 16 +++++++++++++++- 4 files changed, 53 insertions(+), 5 deletions(-) create mode 100644 email_account_validity/_constants.py diff --git a/email_account_validity/_base.py b/email_account_validity/_base.py index 423eb1f..cd63579 100644 --- a/email_account_validity/_base.py +++ b/email_account_validity/_base.py @@ -154,7 +154,11 @@ async def generate_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) -> Tuple[bool, bool, int]: + async def renew_account( + self, + renewal_token: str, + user_id: Optional[str] = None, + ) -> 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. @@ -164,6 +168,9 @@ async def renew_account(self, renewal_token: str) -> Tuple[bool, bool, int]: Args: renewal_token: Token sent with the renewal request. + user_id: The Matrix ID of the user to renew, if the renewal request was + authenticated. + Returns: A tuple containing: * A bool representing whether the token is valid and unused. @@ -176,12 +183,13 @@ async def renew_account(self, renewal_token: str) -> Tuple[bool, bool, int]: # was called from a place it shouldn't have been, e.g. the /send_mail servlet. raise SynapseError(500, "Tried to renew account in unexpected place") + # Verify if the token, or the (token, user_id) tuple, exists. try: ( user_id, current_expiration_ts, token_used_ts, - ) = await self._store.get_user_from_renewal_token(renewal_token) + ) = await self._store.get_user_from_renewal_token(renewal_token, user_id) except StoreError: return False, False, 0 diff --git a/email_account_validity/_constants.py b/email_account_validity/_constants.py new file mode 100644 index 0000000..e0a9799 --- /dev/null +++ b/email_account_validity/_constants.py @@ -0,0 +1,20 @@ +# -*- coding: utf-8 -*- +# 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. + +import re + + +UNAUTHENTICATED_TOKEN_REGEX = re.compile('[a-zA-Z]{32}') + diff --git a/email_account_validity/_store.py b/email_account_validity/_store.py index 4eda21a..051c60d 100644 --- a/email_account_validity/_store.py +++ b/email_account_validity/_store.py @@ -261,12 +261,14 @@ def set_renewal_token_for_user_txn(txn: LoggingTransaction): ) async def get_user_from_renewal_token( - self, renewal_token: str + self, renewal_token: str, user_id: Optional[str] = None, ) -> 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. + user_id: The Matrix ID of the user to renew, if the renewal request was + authenticated. Returns: A tuple of containing the following values: @@ -279,10 +281,14 @@ async def get_user_from_renewal_token( """ def get_user_from_renewal_token_txn(txn: LoggingTransaction): + keyvalues = {"renewal_token": renewal_token} + if user_id is not None: + keyvalues["user_id"] = user_id + return DatabasePool.simple_select_one_txn( txn=txn, table="email_account_validity", - keyvalues={"renewal_token": renewal_token}, + keyvalues=keyvalues, retcols=["user_id", "expiration_ts_ms", "token_used_ts_ms"], ) diff --git a/email_account_validity/servlets.py b/email_account_validity/servlets.py index 56e2452..2171219 100644 --- a/email_account_validity/servlets.py +++ b/email_account_validity/servlets.py @@ -15,6 +15,7 @@ from twisted.web.resource import Resource +from synapse.api.errors import InvalidClientCredentialsError from synapse.config._base import Config, ConfigError from synapse.http.server import ( DirectServeHtmlResource, @@ -26,6 +27,7 @@ from email_account_validity._base import EmailAccountValidityBase from email_account_validity._store import EmailAccountValidityStore +from email_account_validity._constants import UNAUTHENTICATED_TOKEN_REGEX class EmailAccountValidityServlet(Resource): @@ -77,11 +79,23 @@ async def _async_render_GET(self, request): renewal_token = request.args[b"token"][0].decode("utf-8") + user_id = None + if not UNAUTHENTICATED_TOKEN_REGEX.match(renewal_token): + # If the token doesn't look like one we might send as a clickable link via + # email, try to authenticate the request. + try: + requester = await self._api.get_user_by_req(request, allow_expired=True) + except InvalidClientCredentialsError: + respond_with_html(request, 404, self._invalid_token_template.render()) + return + + user_id = requester.user.to_string() + ( token_valid, token_stale, expiration_ts, - ) = await self.renew_account(renewal_token) + ) = await self.renew_account(renewal_token, user_id) if token_valid: status_code = 200 From 4b04f89cc3479e5cc9f7fd92591129ae212f9994 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 5 May 2021 16:00:57 +0100 Subject: [PATCH 02/26] Generate and send shorter authenticated tokens if configured to do so --- email_account_validity/_base.py | 32 +++++++++++++++---- .../{_constants.py => _utils.py} | 8 +++++ email_account_validity/servlets.py | 2 +- 3 files changed, 35 insertions(+), 7 deletions(-) rename email_account_validity/{_constants.py => _utils.py} (74%) diff --git a/email_account_validity/_base.py b/email_account_validity/_base.py index cd63579..b8f73a1 100644 --- a/email_account_validity/_base.py +++ b/email_account_validity/_base.py @@ -25,6 +25,7 @@ from synapse.util import stringutils from email_account_validity._store import EmailAccountValidityStore +from email_account_validity._utils import random_digit_string logger = logging.getLogger(__name__) @@ -35,6 +36,7 @@ def __init__(self, config: Any, api: ModuleApi, store: EmailAccountValidityStore self._store = store self._period = config.get("period") + self._send_links = config.get("send_links", True) (self._template_html, self._template_text,) = api.read_templates( ["notice_expiry.html", "notice_expiry.txt"], @@ -107,15 +109,22 @@ async def send_renewal_email(self, user_id: str, expiration_ts: int): renewal_token = await self.generate_renewal_token(user_id) - url = "%s_synapse/client/email_account_validity/renew?token=%s" % ( - self._api.public_baseurl, - renewal_token, - ) + if self._send_links: + url = "%s_synapse/client/email_account_validity/renew?token=%s" % ( + self._api.public_baseurl, + renewal_token, + ) + else: + # If we're not supposed to send a URL, fallback to the URL being just the + # token. Templates should be using the `renewal_token` variable, but we do + # this so old templates don't break. + url = renewal_token template_vars = { "display_name": display_name, "expiration_ts": expiration_ts, "url": url, + "renewal_token": renewal_token, } html_text = self._template_html.render(**template_vars) @@ -132,8 +141,8 @@ async def send_renewal_email(self, user_id: str, expiration_ts: int): await self._store.set_renewal_mail_status(user_id=user_id, email_sent=True) async def generate_renewal_token(self, user_id: str) -> str: - """Generates a 32-byte long random string that will be inserted into the - user's renewal email's unique link, then saves it into the database. + """Generates a random string that will be inserted into the user's renewal email, + then saves it into the database. Args: user_id: ID of the user to generate a string for. @@ -144,6 +153,17 @@ async def generate_renewal_token(self, user_id: str) -> str: Raises: StoreError(500): Couldn't generate a unique string after 5 attempts. """ + if not self._send_links: + # If the user isn't expected to click on a link, they're expected to enter a + # token manually into their client, which in turn sends the renewal request + # to the server, authenticated with an access token. This means that in this + # case we need the token to be shorter and less complex (hence the 8 digits + # string), but also that we don't need to make the token unique across the + # whole database. + renewal_token = random_digit_string(8) + await self._store.set_renewal_token_for_user(user_id, renewal_token) + return renewal_token + attempts = 0 while attempts < 5: try: diff --git a/email_account_validity/_constants.py b/email_account_validity/_utils.py similarity index 74% rename from email_account_validity/_constants.py rename to email_account_validity/_utils.py index e0a9799..8a08590 100644 --- a/email_account_validity/_constants.py +++ b/email_account_validity/_utils.py @@ -13,8 +13,16 @@ # See the License for the specific language governing permissions and # limitations under the License. +import random import re +import string UNAUTHENTICATED_TOKEN_REGEX = re.compile('[a-zA-Z]{32}') +# We use SystemRandom to make sure we get cryptographically-secure randoms. +rand = random.SystemRandom() + + +def random_digit_string(length): + return "".join(rand.choice(string.digits) for _ in range(length)) \ No newline at end of file diff --git a/email_account_validity/servlets.py b/email_account_validity/servlets.py index 2171219..9a0e472 100644 --- a/email_account_validity/servlets.py +++ b/email_account_validity/servlets.py @@ -27,7 +27,7 @@ from email_account_validity._base import EmailAccountValidityBase from email_account_validity._store import EmailAccountValidityStore -from email_account_validity._constants import UNAUTHENTICATED_TOKEN_REGEX +from email_account_validity._utils import UNAUTHENTICATED_TOKEN_REGEX class EmailAccountValidityServlet(Resource): From 28a8d4a4a769e8d6c7c355968fd745e589e7d96f Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 5 May 2021 16:01:20 +0100 Subject: [PATCH 03/26] Document send_link and templates in the README.md --- README.md | 46 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 1c01bbe..45b1bf4 100644 --- a/README.md +++ b/README.md @@ -26,6 +26,12 @@ Add the following in your Synapse config under `account_validity`: period: 6w # How long before an account expires should Synapse send it a renewal email. renew_at: 1w + # Whether to include a link to click in the emails sent to users. If false, only a + # renewal token is sent, in which case it is generated so it's simpler, and the + # user will need to copy it into a compatible client that will send an + # authenticated request to the server. + # Defaults to true. + send_link: true ``` Also under the HTTP client `listener`, configure an `additional_resource` as per below: @@ -38,12 +44,50 @@ Also under the HTTP client `listener`, configure an `additional_resource` as per # The maximum amount of time an account can stay valid for without being # renewed. period: 6w + # Whether to include a link to click in the emails sent to users. If false, + # only a renewal token is sent, in which case it is generated so it's simpler, + # and the user will need to copy it into a compatible client that will send an + # authenticated request to the server. + # Defaults to true. + send_link: true ``` The syntax for durations is the same as in the rest of Synapse's configuration file. +Configuration parameters with matching names that appear both in `account_validity` and +`listeners` __must__ have the same value in both places, otherwise the module will not +behave correctly. + +## Templates + If they are not already there, copy the [templates](/email_account_validity/templates) -into Synapse's templates directory. +into Synapse's templates directory (or replace them with your own). The templates the +module will use are: + +* `notice_expiry.(html|txt)`: The content of the renewal email. It gets passed the + following variables: + * `display_name`: The display name of the user needing renewal. + * `expiration_ts`: A timestamp in milliseconds representing when the account will + expire. Templates can use the `format_ts` (with a date format as the function's + parameter) to format this timestamp into a human-readable date. + * `url`: The URL the user is supposed to click on to renew their account. If + `send_links` is set to `false` in the module's configuration, the value of this + variable will be the token the user must copy into their client. + * `renewal_token`: The token to use in order to renew the user's account. If + `send_links` is set to `false`, templates should prefer this variable to `url`. +* `account_renewed.html`: The HTML to display to a user when they successfully renew + their account. It gets passed the following vaiables: + * `expiration_ts`: A timestamp in milliseconds representing when the account will + expire. Templates can use the `format_ts` (with a date format as the function's + parameter) to format this timestamp into a human-readable date. +* `account_previously_renewed.html`: The HTML to display to a user when they try to renew + their account with a token that's valid but previously used. It gets passed the same + variables as `account_renewed.html`. +* `invalid_token.html`: The HTML to display to a user when they try to renew their account + with the wrong token. It doesn't get passed any variable. + +Note that the templates directory contains two files that aren't templates (`mail.css` +and `mail-expiry.css`), but are used by email templates to apply visual adjustments. ## Routes From c1c0b48e04231d74e4701ba1e59f6330f09d49da Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 5 May 2021 17:39:50 +0100 Subject: [PATCH 04/26] Add test for authenticated tokens --- README.md | 4 ++-- email_account_validity/_base.py | 8 +++---- email_account_validity/_utils.py | 2 +- tests/__init__.py | 9 ++++--- tests/test_account_validity.py | 41 ++++++++++++++++++++++++++++++-- 5 files changed, 50 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 45b1bf4..b31d194 100644 --- a/README.md +++ b/README.md @@ -31,7 +31,7 @@ Add the following in your Synapse config under `account_validity`: # user will need to copy it into a compatible client that will send an # authenticated request to the server. # Defaults to true. - send_link: true + send_links: true ``` Also under the HTTP client `listener`, configure an `additional_resource` as per below: @@ -49,7 +49,7 @@ Also under the HTTP client `listener`, configure an `additional_resource` as per # and the user will need to copy it into a compatible client that will send an # authenticated request to the server. # Defaults to true. - send_link: true + send_links: true ``` The syntax for durations is the same as in the rest of Synapse's configuration file. diff --git a/email_account_validity/_base.py b/email_account_validity/_base.py index b8f73a1..062242d 100644 --- a/email_account_validity/_base.py +++ b/email_account_validity/_base.py @@ -132,10 +132,10 @@ async def send_renewal_email(self, user_id: str, expiration_ts: int): for address in addresses: await self._api.send_mail( - address, - self._renew_email_subject, - html_text, - plain_text, + recipient=address, + subject=self._renew_email_subject, + html=html_text, + text=plain_text, ) await self._store.set_renewal_mail_status(user_id=user_id, email_sent=True) diff --git a/email_account_validity/_utils.py b/email_account_validity/_utils.py index 8a08590..1e3d122 100644 --- a/email_account_validity/_utils.py +++ b/email_account_validity/_utils.py @@ -18,7 +18,7 @@ import string -UNAUTHENTICATED_TOKEN_REGEX = re.compile('[a-zA-Z]{32}') +UNAUTHENTICATED_TOKEN_REGEX = re.compile('^[a-zA-Z]{32}$') # We use SystemRandom to make sure we get cryptographically-secure randoms. rand = random.SystemRandom() diff --git a/tests/__init__.py b/tests/__init__.py index cc71f36..bb054f2 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -108,14 +108,13 @@ async def send_mail(recipient, subject, html, text): return None -async def create_account_validity_module() -> EmailAccountValidity: +async def create_account_validity_module(config_override={}) -> EmailAccountValidity: """Starts an EmailAccountValidity module with a basic config and a mock of the ModuleApi. """ - config = { - "period": 3628800000, # 6w - "renew_at": 604800000, # 1w - } + config = config_override + config.setdefault("period", 3628800000) # 6w + config.setdefault("renew_at", 604800000) # 1w store = SQLiteStore() diff --git a/tests/test_account_validity.py b/tests/test_account_validity.py index d38e523..368ed6d 100644 --- a/tests/test_account_validity.py +++ b/tests/test_account_validity.py @@ -14,6 +14,7 @@ # limitations under the License. import asyncio +import re import time # From Python 3.8 onwards, aiounittest.AsyncTestCase can be replaced by @@ -23,7 +24,7 @@ from synapse.module_api.errors import SynapseError -from email_account_validity import EmailAccountValidity +from email_account_validity._utils import UNAUTHENTICATED_TOKEN_REGEX from tests import create_account_validity_module @@ -122,6 +123,13 @@ async def get_threepids(user_id): await module.send_renewal_email_to_user(user_id) self.assertEqual(module._api.send_mail.call_count, 1) + # Test that the email content contains a link; we haven't set send_links in the + # module's config so its value should be the default (which is True). + kwargs = module._api.send_mail.call_args.kwargs + path = "_synapse/client/email_account_validity/renew" + self.assertNotEqual(kwargs["html"].find(path), -1) + self.assertNotEqual(kwargs["text"].find(path), -1) + # Test that trying to send an email to a known use that has no email address # attached to their account results in no email being sent. threepids = [] @@ -130,7 +138,7 @@ async def get_threepids(user_id): async def test_renewal_token(self): user_id = "@izzy:test" - module = await create_account_validity_module() # type: EmailAccountValidity + module = await create_account_validity_module() # Insert a row with an expiration timestamp and a renewal token for this user. await module._store.set_expiration_date_for_user(user_id) @@ -144,6 +152,7 @@ async def test_renewal_token(self): renewal_token = await module._store.get_renewal_token_for_user(user_id) self.assertTrue(isinstance(renewal_token, str)) self.assertGreater(len(renewal_token), 0) + self.assertTrue(UNAUTHENTICATED_TOKEN_REGEX.match(renewal_token)) # Sleep a bit so the new expiration timestamp isn't likely to be equal to the # previous one. @@ -184,3 +193,31 @@ async def test_renewal_token(self): self.assertFalse(token_stale) self.assertEqual(expiration_ts, 0) + async def test_send_link_false(self): + user_id = "@izzy:test" + # Create a module with a configuration forbidding it to send links via email. + module = await create_account_validity_module({"send_links": False}) + + async def get_threepids(user_id): + return [{ + "medium": "email", + "address": "izzy@test", + }] + module._api.get_threepids_for_user.side_effect = get_threepids + await module._store.set_expiration_date_for_user(user_id) + + # Test that, when an email is sent, it doesn't include a link. We do this by + # searching the email's content for the path for renewal requests. + await module.send_renewal_email_to_user(user_id) + self.assertEqual(module._api.send_mail.call_count, 1) + + kwargs = module._api.send_mail.call_args.kwargs + path = "_synapse/client/email_account_validity/renew" + self.assertEqual(kwargs["html"].find(path), -1) + self.assertEqual(kwargs["text"].find(path), -1) + + # Check that the renewal token is in the right format. It should be a 8 digit + # long string. + token = await module._store.get_renewal_token_for_user(user_id) + self.assertTrue(isinstance(token, str)) + self.assertTrue(re.compile("^[0-9]{8}$").match(token)) From 83852bffbc27d688fa9b83fc41ca8a730c29e228 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 5 May 2021 18:06:49 +0100 Subject: [PATCH 05/26] Actually error when trying to reuse a unique renewal token --- email_account_validity/_base.py | 16 +++++++++------- email_account_validity/_store.py | 31 ++++++++++++++++++++++++++++++- tests/test_account_validity.py | 31 ++++++++++++++++++++++++++++--- 3 files changed, 67 insertions(+), 11 deletions(-) diff --git a/email_account_validity/_base.py b/email_account_validity/_base.py index 423eb1f..c941129 100644 --- a/email_account_validity/_base.py +++ b/email_account_validity/_base.py @@ -18,9 +18,9 @@ from twisted.web.server import Request -from synapse.api.errors import StoreError, SynapseError from synapse.http.servlet import parse_json_object_from_request from synapse.module_api import ModuleApi +from synapse.module_api.errors import SynapseError from synapse.types import UserID from synapse.util import stringutils @@ -102,7 +102,7 @@ async def send_renewal_email(self, user_id: str, expiration_ts: int): display_name = profile.display_name if display_name is None: display_name = user_id - except StoreError: + except SynapseError: display_name = user_id renewal_token = await self.generate_renewal_token(user_id) @@ -142,17 +142,19 @@ async def generate_renewal_token(self, user_id: str) -> str: The generated string. Raises: - StoreError(500): Couldn't generate a unique string after 5 attempts. + SynapseError(500): Couldn't generate a unique string after 5 attempts. """ attempts = 0 while attempts < 5: try: renewal_token = stringutils.random_string(32) - await self._store.set_renewal_token_for_user(user_id, renewal_token) + await self._store.set_renewal_token_for_user( + user_id, renewal_token, unique=True, + ) return renewal_token - except StoreError: + except SynapseError: attempts += 1 - raise StoreError(500, "Couldn't generate a unique string as refresh string.") + raise SynapseError(500, "Couldn't generate a unique string as refresh string.") 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 @@ -182,7 +184,7 @@ async def renew_account(self, renewal_token: str) -> Tuple[bool, bool, int]: current_expiration_ts, token_used_ts, ) = await self._store.get_user_from_renewal_token(renewal_token) - except StoreError: + except SynapseError: return False, False, 0 # Check whether this token has already been used. diff --git a/email_account_validity/_store.py b/email_account_validity/_store.py index 4eda21a..813c412 100644 --- a/email_account_validity/_store.py +++ b/email_account_validity/_store.py @@ -18,6 +18,7 @@ from typing import Dict, List, Optional, Tuple, Union from synapse.module_api import ModuleApi +from synapse.module_api.errors import SynapseError from synapse.storage.database import DatabasePool, LoggingTransaction from synapse.util.caches.descriptors import cached @@ -246,8 +247,36 @@ def get_expiration_ts_for_user_txn(txn: LoggingTransaction): ) return res - async def set_renewal_token_for_user(self, user_id: str, renewal_token: str): + async def set_renewal_token_for_user( + self, + user_id: str, + renewal_token: str, + unique: bool, + ): + """Store the given renewal token for the given user. + + Args: + user_id: The user ID to store the renewal token for. + renewal_token: The renewal token to store for the user. + unique: Whether the token should be unique across the whole database, i.e. + whether it should be able to look the user up from the token. + + Raises: + SynapseError(409): unique is set to True and the token is already in use. + """ def set_renewal_token_for_user_txn(txn: LoggingTransaction): + if unique: + ret = DatabasePool.simple_select_one_onecol_txn( + txn=txn, + table="email_account_validity", + keyvalues={"renewal_token": renewal_token}, + retcol="user_id", + allow_none=True, + ) + + if ret is not None: + raise SynapseError(409, "Renewal token already in use") + DatabasePool.simple_update_one_txn( txn=txn, table="email_account_validity", diff --git a/tests/test_account_validity.py b/tests/test_account_validity.py index 6a690d6..617da4c 100644 --- a/tests/test_account_validity.py +++ b/tests/test_account_validity.py @@ -84,7 +84,7 @@ async def test_on_user_registration(self): expiration_ts = await module._store.get_expiration_ts_for_user(user_id) now_ms = int(time.time() * 1000) - self.assertTrue(isinstance(expiration_ts, int)) + self.assertIsInstance(expiration_ts, int) self.assertGreater(expiration_ts, now_ms) @@ -135,10 +135,10 @@ async def test_renewal_token(self): # Retrieve the expiration timestamp and renewal token and check that they're in # the right format. old_expiration_ts = await module._store.get_expiration_ts_for_user(user_id) - self.assertTrue(isinstance(old_expiration_ts, int)) + self.assertIsInstance(old_expiration_ts, int) renewal_token = await module._store.get_renewal_token_for_user(user_id) - self.assertTrue(isinstance(renewal_token, str)) + self.assertIsInstance(renewal_token, str) self.assertGreater(len(renewal_token), 0) # Sleep a bit so the new expiration timestamp isn't likely to be equal to the @@ -180,3 +180,28 @@ async def test_renewal_token(self): self.assertFalse(token_stale) self.assertEqual(expiration_ts, 0) + async def test_duplicate_token(self): + user_id_1 = "@izzy1:test" + user_id_2 = "@izzy2:test" + token = "sometoken" + + module = await create_account_validity_module() + + # Insert both users in the table. + await module._store.set_expiration_date_for_user(user_id_1) + await module._store.set_expiration_date_for_user(user_id_2) + + # Set the renewal token. + await module._store.set_renewal_token_for_user(user_id_1, token, True) + + # Try to set the same renewal token for another user. + exception = None + try: + await module._store.set_renewal_token_for_user(user_id_2, token, True) + except SynapseError as e: + exception = e + + # Check that an exception was raised and that it's the one we're expecting. + self.assertIsInstance(exception, SynapseError) + self.assertEqual(exception.code, 409) + From a1b57431993d8e746ed8f931d9dd3922ebd97cae Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 5 May 2021 18:13:38 +0100 Subject: [PATCH 06/26] call_args.kwargs was only introduced in Python 3.8 --- email_account_validity/_base.py | 4 +++- tests/test_account_validity.py | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/email_account_validity/_base.py b/email_account_validity/_base.py index dd0dc0f..02441ba 100644 --- a/email_account_validity/_base.py +++ b/email_account_validity/_base.py @@ -161,7 +161,9 @@ async def generate_renewal_token(self, user_id: str) -> str: # string), but also that we don't need to make the token unique across the # whole database. renewal_token = random_digit_string(8) - await self._store.set_renewal_token_for_user(user_id, renewal_token) + await self._store.set_renewal_token_for_user( + user_id, renewal_token, unique=False, + ) return renewal_token attempts = 0 diff --git a/tests/test_account_validity.py b/tests/test_account_validity.py index a84747d..efbc993 100644 --- a/tests/test_account_validity.py +++ b/tests/test_account_validity.py @@ -125,7 +125,7 @@ async def get_threepids(user_id): # Test that the email content contains a link; we haven't set send_links in the # module's config so its value should be the default (which is True). - kwargs = module._api.send_mail.call_args.kwargs + _, kwargs = module._api.send_mail.call_args path = "_synapse/client/email_account_validity/renew" self.assertNotEqual(kwargs["html"].find(path), -1) self.assertNotEqual(kwargs["text"].find(path), -1) @@ -236,7 +236,7 @@ async def get_threepids(user_id): await module.send_renewal_email_to_user(user_id) self.assertEqual(module._api.send_mail.call_count, 1) - kwargs = module._api.send_mail.call_args.kwargs + _, kwargs = module._api.send_mail.call_args path = "_synapse/client/email_account_validity/renew" self.assertEqual(kwargs["html"].find(path), -1) self.assertEqual(kwargs["text"].find(path), -1) From 04ed67b8c65897fd8aa579d510d46d29dd948d98 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 5 May 2021 18:16:20 +0100 Subject: [PATCH 07/26] Use the right assertion method --- tests/test_account_validity.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_account_validity.py b/tests/test_account_validity.py index efbc993..1d80b2e 100644 --- a/tests/test_account_validity.py +++ b/tests/test_account_validity.py @@ -244,5 +244,5 @@ async def get_threepids(user_id): # Check that the renewal token is in the right format. It should be a 8 digit # long string. token = await module._store.get_renewal_token_for_user(user_id) - self.assertTrue(isinstance(token, str)) + self.assertIsInstance(token, str) self.assertTrue(re.compile("^[0-9]{8}$").match(token)) From 083668c5150f7b87ac004b57f4bbd16a2d551f90 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 14 May 2021 12:16:49 +0100 Subject: [PATCH 08/26] Import from Synapse's module API --- email_account_validity/_base.py | 12 +++---- email_account_validity/_store.py | 4 +-- email_account_validity/_utils.py | 38 +++++++++++++++++++++- email_account_validity/account_validity.py | 10 +++--- email_account_validity/servlets.py | 18 +++++----- 5 files changed, 59 insertions(+), 23 deletions(-) diff --git a/email_account_validity/_base.py b/email_account_validity/_base.py index 02441ba..b5336ef 100644 --- a/email_account_validity/_base.py +++ b/email_account_validity/_base.py @@ -14,18 +14,16 @@ # limitations under the License. import logging +import time from typing import Any, Optional, Tuple from twisted.web.server import Request -from synapse.http.servlet import parse_json_object_from_request -from synapse.module_api import ModuleApi +from synapse.module_api import ModuleApi, parse_json_object_from_request, UserID from synapse.module_api.errors import SynapseError -from synapse.types import UserID -from synapse.util import stringutils from email_account_validity._store import EmailAccountValidityStore -from email_account_validity._utils import random_digit_string +from email_account_validity._utils import random_digit_string, random_string logger = logging.getLogger(__name__) @@ -169,7 +167,7 @@ async def generate_renewal_token(self, user_id: str) -> str: attempts = 0 while attempts < 5: try: - renewal_token = stringutils.random_string(32) + renewal_token = random_string(32) await self._store.set_renewal_token_for_user( user_id, renewal_token, unique=True, ) @@ -262,7 +260,7 @@ async def renew_account_for_user( New expiration date for this account, as a timestamp in milliseconds since epoch. """ - now = self._api.current_time_ms() + now = int(time.time() * 1000) if expiration_ts is None: expiration_ts = now + self._period diff --git a/email_account_validity/_store.py b/email_account_validity/_store.py index 3aaad03..0a689ad 100644 --- a/email_account_validity/_store.py +++ b/email_account_validity/_store.py @@ -17,10 +17,8 @@ import random from typing import Dict, List, Optional, Tuple, Union -from synapse.module_api import ModuleApi +from synapse.module_api import DatabasePool, LoggingTransaction, ModuleApi, cached from synapse.module_api.errors import SynapseError -from synapse.storage.database import DatabasePool, LoggingTransaction -from synapse.util.caches.descriptors import cached logger = logging.getLogger(__name__) diff --git a/email_account_validity/_utils.py b/email_account_validity/_utils.py index 1e3d122..fd0a0cc 100644 --- a/email_account_validity/_utils.py +++ b/email_account_validity/_utils.py @@ -16,6 +16,7 @@ import random import re import string +from typing import Union UNAUTHENTICATED_TOKEN_REGEX = re.compile('^[a-zA-Z]{32}$') @@ -24,5 +25,40 @@ rand = random.SystemRandom() +def random_string(length: int) -> str: + return "".join(rand.choice(string.ascii_letters) for _ in range(length)) + + def random_digit_string(length): - return "".join(rand.choice(string.digits) for _ in range(length)) \ No newline at end of file + return "".join(rand.choice(string.digits) for _ in range(length)) + + +def parse_duration(value: Union[str, int]) -> int: + """Convert a duration as a string or integer to a number of milliseconds. + + If an integer is provided it is treated as milliseconds and is unchanged. + + String durations can have a suffix of 's', 'm', 'h', 'd', 'w', or 'y'. + No suffix is treated as milliseconds. + + Args: + value: The duration to parse. + + Returns: + The number of milliseconds in the duration. + """ + if isinstance(value, int): + return value + second = 1000 + minute = 60 * second + hour = 60 * minute + day = 24 * hour + week = 7 * day + year = 365 * day + sizes = {"s": second, "m": minute, "h": hour, "d": day, "w": week, "y": year} + size = 1 + suffix = value[-1] + if suffix in sizes: + value = value[:-1] + size = sizes[suffix] + return int(value) * size diff --git a/email_account_validity/account_validity.py b/email_account_validity/account_validity.py index 8b5da44..de8ac88 100644 --- a/email_account_validity/account_validity.py +++ b/email_account_validity/account_validity.py @@ -14,15 +14,17 @@ # limitations under the License. import logging +import time from typing import Any, Tuple from twisted.web.server import Request -from synapse.config._base import Config, ConfigError +from synapse.config._base import ConfigError from synapse.module_api import ModuleApi, run_in_background from email_account_validity._base import EmailAccountValidityBase from email_account_validity._store import EmailAccountValidityStore +from email_account_validity._utils import parse_duration logger = logging.getLogger(__name__) @@ -54,8 +56,8 @@ def parse_config(config: dict): "'renew_at' is required when using email account validity" ) - config["period"] = Config.parse_duration(config.get("period") or 0) - config["renew_at"] = Config.parse_duration(config.get("renew_at") or 0) + config["period"] = parse_duration(config.get("period") or 0) + config["renew_at"] = parse_duration(config.get("renew_at") or 0) return config async def on_legacy_renew(self, renewal_token: str) -> Tuple[bool, bool, int]: @@ -111,7 +113,7 @@ async def user_expired(self, user_id: str) -> Tuple[bool, bool]: if expiration_ts is None: return False, False - now_ts = self._api.current_time_ms() + now_ts = int(time.time() * 1000) return now_ts >= expiration_ts, True async def on_user_registration(self, user_id: str): diff --git a/email_account_validity/servlets.py b/email_account_validity/servlets.py index 9a0e472..19b55ef 100644 --- a/email_account_validity/servlets.py +++ b/email_account_validity/servlets.py @@ -15,19 +15,21 @@ from twisted.web.resource import Resource -from synapse.api.errors import InvalidClientCredentialsError -from synapse.config._base import Config, ConfigError -from synapse.http.server import ( +from synapse.module_api import ( DirectServeHtmlResource, DirectServeJsonResource, + ModuleApi, respond_with_html, ) -from synapse.module_api import ModuleApi -from synapse.module_api.errors import SynapseError +from synapse.module_api.errors import ( + ConfigError, + InvalidClientCredentialsError, + SynapseError, +) from email_account_validity._base import EmailAccountValidityBase from email_account_validity._store import EmailAccountValidityStore -from email_account_validity._utils import UNAUTHENTICATED_TOKEN_REGEX +from email_account_validity._utils import UNAUTHENTICATED_TOKEN_REGEX, parse_duration class EmailAccountValidityServlet(Resource): @@ -39,7 +41,7 @@ def __init__(self, config: dict, api: ModuleApi): @staticmethod def parse_config(config: dict) -> dict: - config["period"] = Config.parse_duration(config.get("period") or 0) + config["period"] = parse_duration(config.get("period") or 0) return config @@ -54,7 +56,7 @@ def __init__(self, config: dict, api: ModuleApi): DirectServeHtmlResource.__init__(self) if "period" in config: - self._period = Config.parse_duration(config["period"]) + self._period = parse_duration(config["period"]) else: raise ConfigError("'period' is required when using account validity") From caa23fff8778a9061221c74925fe83dc466305c1 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 20 May 2021 10:10:07 +0100 Subject: [PATCH 09/26] Look for templates in the module; globalise config --- README.md | 9 +++--- email_account_validity/_base.py | 29 +++++++++++------- email_account_validity/_global.py | 29 ++++++++++++++++++ email_account_validity/_store.py | 18 ++++++----- email_account_validity/account_validity.py | 11 ++++++- email_account_validity/servlets.py | 35 ++++++++++------------ tests/__init__.py | 7 ----- 7 files changed, 90 insertions(+), 48 deletions(-) create mode 100644 email_account_validity/_global.py diff --git a/README.md b/README.md index b31d194..a1710c7 100644 --- a/README.md +++ b/README.md @@ -17,9 +17,10 @@ pip install synapse-email-account-validity ## Config -Add the following in your Synapse config under `account_validity`: +Add the following in your Synapse config: ```yaml +account_validity_modules: - module: email_account_validity.EmailAccountValidity config: # The maximum amount of time an account can stay valid for without being renewed. @@ -60,9 +61,7 @@ behave correctly. ## Templates -If they are not already there, copy the [templates](/email_account_validity/templates) -into Synapse's templates directory (or replace them with your own). The templates the -module will use are: +The templates the module will use are: * `notice_expiry.(html|txt)`: The content of the renewal email. It gets passed the following variables: @@ -86,6 +85,8 @@ module will use are: * `invalid_token.html`: The HTML to display to a user when they try to renew their account with the wrong token. It doesn't get passed any variable. +You can find and change the default templates [here](https://github.com/matrix-org/synapse-email-account-validity/tree/main/email_account_validity/templates). + Note that the templates directory contains two files that aren't templates (`mail.css` and `mail-expiry.css`), but are used by email templates to apply visual adjustments. diff --git a/email_account_validity/_base.py b/email_account_validity/_base.py index b5336ef..5a47600 100644 --- a/email_account_validity/_base.py +++ b/email_account_validity/_base.py @@ -14,6 +14,7 @@ # limitations under the License. import logging +import os import time from typing import Any, Optional, Tuple @@ -22,6 +23,7 @@ from synapse.module_api import ModuleApi, parse_json_object_from_request, UserID from synapse.module_api.errors import SynapseError +from email_account_validity import _global from email_account_validity._store import EmailAccountValidityStore from email_account_validity._utils import random_digit_string, random_string @@ -33,25 +35,32 @@ def __init__(self, config: Any, api: ModuleApi, store: EmailAccountValidityStore self._api = api self._store = store - self._period = config.get("period") - self._send_links = config.get("send_links", True) + if _global.config is None: + self._period = config.get("period") + self._send_links = config.get("send_links", True) - (self._template_html, self._template_text,) = api.read_templates( - ["notice_expiry.html", "notice_expiry.txt"], - ) - - if "renew_email_subject" in config: - renew_email_subject = config["renew_email_subject"] + if "renew_email_subject" in config: + renew_email_subject = config["renew_email_subject"] + else: + renew_email_subject = "Renew your %(app)s account" else: - renew_email_subject = "Renew your %(app)s account" + self._period = _global.config.period + self._send_links = _global.config.send_links + + renew_email_subject = _global.config.renew_email_subject try: app_name = self._api.email_app_name self._renew_email_subject = renew_email_subject % {"app": app_name} - except Exception: + except (KeyError, TypeError): # If substitution failed, fall back to the bare strings. self._renew_email_subject = renew_email_subject + (self._template_html, self._template_text,) = api.read_templates( + ["notice_expiry.html", "notice_expiry.txt"], + os.path.join(os.path.dirname(os.path.abspath(__file__)), "templates"), + ) + async def send_renewal_email_to_user(self, user_id: str) -> None: """ Send a renewal email for a specific user. diff --git a/email_account_validity/_global.py b/email_account_validity/_global.py new file mode 100644 index 0000000..8ebd3ae --- /dev/null +++ b/email_account_validity/_global.py @@ -0,0 +1,29 @@ +# -*- coding: utf-8 -*- +# 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. + +from typing import Optional + +import attr + + +@attr.s +class EmailAccountValidityConfig: + period = attr.ib(type=int) + renew_at = attr.ib(type=int) + renew_email_subject = attr.ib(type=str) + send_links = attr.ib(type=bool) + + +config: Optional[EmailAccountValidityConfig] = None \ No newline at end of file diff --git a/email_account_validity/_store.py b/email_account_validity/_store.py index 0a689ad..86f6600 100644 --- a/email_account_validity/_store.py +++ b/email_account_validity/_store.py @@ -15,19 +15,22 @@ import logging import random +import time from typing import Dict, List, Optional, Tuple, Union from synapse.module_api import DatabasePool, LoggingTransaction, ModuleApi, cached from synapse.module_api.errors import SynapseError +from email_account_validity import _global + logger = logging.getLogger(__name__) class EmailAccountValidityStore: - def __init__(self, config: dict, api: ModuleApi): + def __init__(self, api: ModuleApi): self._api = api - self._period = config.get("period", 0) - self._renew_at = config.get("renew_at") + self._period = _global.config.period + self._renew_at = _global.config.renew_at self._expiration_ts_max_delta = self._period * 10.0 / 100.0 self._rand = random.SystemRandom() @@ -98,7 +101,8 @@ def populate_table_txn(txn: LoggingTransaction, batch_size: int) -> int: # state includes an expiration timestamp close to now + validity period, but # is slightly randomised to avoid sending huge bursts of renewal emails at # once. - default_expiration_ts = self._api.current_time_ms() + self._period + now_ms = int(time.time() * 1000) + default_expiration_ts = now_ms + self._period for user in missing_users: if users_to_insert.get(user["name"]) is None: users_to_insert[user["name"]] = { @@ -149,8 +153,9 @@ async def get_users_expiring_soon(self) -> List[Dict[str, Union[str, int]]]: A list of dictionaries, each with a user ID and expiration time (in milliseconds). """ + def select_users_txn(txn, renew_at): + now_ms = int(time.time() * 1000) - def select_users_txn(txn, now_ms, renew_at): txn.execute( """ SELECT user_id, expiration_ts_ms FROM email_account_validity @@ -163,7 +168,6 @@ def select_users_txn(txn, now_ms, renew_at): return await self._api.run_db_interaction( "get_users_expiring_soon", select_users_txn, - self._api.current_time_ms(), self._renew_at, ) @@ -348,7 +352,7 @@ def set_expiration_date_for_user_txn( Args: user_id: User ID to set an expiration date for. """ - now_ms = self._api.current_time_ms() + now_ms = int(time.time() * 1000) expiration_ts = now_ms + self._period sql = """ diff --git a/email_account_validity/account_validity.py b/email_account_validity/account_validity.py index de8ac88..a2a44a2 100644 --- a/email_account_validity/account_validity.py +++ b/email_account_validity/account_validity.py @@ -22,6 +22,7 @@ from synapse.config._base import ConfigError from synapse.module_api import ModuleApi, run_in_background +from email_account_validity import _global from email_account_validity._base import EmailAccountValidityBase from email_account_validity._store import EmailAccountValidityStore from email_account_validity._utils import parse_duration @@ -31,7 +32,7 @@ class EmailAccountValidity(EmailAccountValidityBase): def __init__(self, config: Any, api: ModuleApi, populate_users: bool = True): - self._store = EmailAccountValidityStore(config, api) + self._store = EmailAccountValidityStore(api) self._api = api super().__init__(config, self._api, self._store) @@ -58,6 +59,14 @@ def parse_config(config: dict): config["period"] = parse_duration(config.get("period") or 0) config["renew_at"] = parse_duration(config.get("renew_at") or 0) + + _global.config = _global.EmailAccountValidityConfig( + config["period"], + config["renew_at"], + config.get("renewal_email_subject", "Renew your %(app)s account"), + config.get("send_links", True) + ) + return config async def on_legacy_renew(self, renewal_token: str) -> Tuple[bool, bool, int]: diff --git a/email_account_validity/servlets.py b/email_account_validity/servlets.py index 19b55ef..9b1caed 100644 --- a/email_account_validity/servlets.py +++ b/email_account_validity/servlets.py @@ -12,6 +12,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 os from twisted.web.resource import Resource @@ -34,32 +35,27 @@ class EmailAccountValidityServlet(Resource): def __init__(self, config: dict, api: ModuleApi): + print("-----------------------------SERVLET-----------------------------") super().__init__() - self.putChild(b'renew', EmailAccountValidityRenewServlet(config, api)) - self.putChild(b'send_mail', EmailAccountValiditySendMailServlet(config, api)) - self.putChild(b'admin', EmailAccountValidityAdminServlet(config, api)) + self.putChild(b'renew', EmailAccountValidityRenewServlet(api)) + self.putChild(b'send_mail', EmailAccountValiditySendMailServlet(api)) + self.putChild(b'admin', EmailAccountValidityAdminServlet(api)) @staticmethod def parse_config(config: dict) -> dict: - config["period"] = parse_duration(config.get("period") or 0) return config class EmailAccountValidityRenewServlet( EmailAccountValidityBase, DirectServeHtmlResource ): - def __init__(self, config: dict, api: ModuleApi): + def __init__(self, api: ModuleApi): self._api = api - self._store = EmailAccountValidityStore(config, api) + self._store = EmailAccountValidityStore(api) - EmailAccountValidityBase.__init__(self, config, self._api, self._store) + EmailAccountValidityBase.__init__(self, {}, self._api, self._store) DirectServeHtmlResource.__init__(self) - if "period" in config: - self._period = parse_duration(config["period"]) - else: - raise ConfigError("'period' is required when using account validity") - ( self._account_renewed_template, self._account_previously_renewed_template, @@ -69,7 +65,8 @@ def __init__(self, config: dict, api: ModuleApi): "account_renewed.html", "account_previously_renewed.html", "invalid_token.html", - ] + ], + os.path.join(os.path.dirname(os.path.abspath(__file__)), "templates"), ) async def _async_render_GET(self, request): @@ -120,10 +117,10 @@ class EmailAccountValiditySendMailServlet( EmailAccountValidityBase, DirectServeJsonResource, ): - def __init__(self, config: dict, api: ModuleApi): - store = EmailAccountValidityStore(config, api) + def __init__(self, api: ModuleApi): + store = EmailAccountValidityStore(api) - EmailAccountValidityBase.__init__(self, config, api, store) + EmailAccountValidityBase.__init__(self, {}, api, store) DirectServeJsonResource.__init__(self) if not api.public_baseurl: @@ -144,10 +141,10 @@ class EmailAccountValidityAdminServlet( EmailAccountValidityBase, DirectServeJsonResource, ): - def __init__(self, config: dict, api: ModuleApi): - store = EmailAccountValidityStore(config, api) + def __init__(self, api: ModuleApi): + store = EmailAccountValidityStore(api) - EmailAccountValidityBase.__init__(self, config, api, store) + EmailAccountValidityBase.__init__(self, {}, api, store) DirectServeJsonResource.__init__(self) async def _async_render_POST(self, request): diff --git a/tests/__init__.py b/tests/__init__.py index bb054f2..b7d9eb2 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -93,12 +93,6 @@ def _format_ts_filter(value: int, format: str): return [env.get_template(filename) for filename in filenames] -def current_time_ms(): - """Returns the current time in milliseconds. - """ - return int(time.time() * 1000) - - async def get_profile_for_user(user_id): ProfileInfo = namedtuple("ProfileInfo", ("avatar_url", "display_name")) return ProfileInfo(None, "Izzy") @@ -124,7 +118,6 @@ async def create_account_validity_module(config_override={}) -> EmailAccountVali module_api = mock.Mock(spec=ModuleApi) module_api.run_db_interaction.side_effect = store.run_db_interaction module_api.read_templates.side_effect = read_templates - module_api.current_time_ms.side_effect = current_time_ms module_api.get_profile_for_user.side_effect = get_profile_for_user module_api.send_mail.side_effect = send_mail From 56e484126b87587f37cf82b6638bbd3455f5256e Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 20 May 2021 10:23:03 +0100 Subject: [PATCH 10/26] Fix test infra --- tests/__init__.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/tests/__init__.py b/tests/__init__.py index b7d9eb2..fda7fef 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -69,13 +69,11 @@ def __next__(self): return self.cur.__next__() -def read_templates(filenames): +def read_templates(filenames, directory): """Reads Jinja templates from the templates directory. This function is mostly copied from Synapse. """ - loader = jinja2.FileSystemLoader( - pkg_resources.resource_filename("email_account_validity", "templates") - ) + loader = jinja2.FileSystemLoader(directory) env = jinja2.Environment( loader=loader, autoescape=jinja2.select_autoescape(), @@ -107,8 +105,8 @@ async def create_account_validity_module(config_override={}) -> EmailAccountVali ModuleApi. """ config = config_override - config.setdefault("period", 3628800000) # 6w - config.setdefault("renew_at", 604800000) # 1w + config.setdefault("period", "6w") + config.setdefault("renew_at", "1w") store = SQLiteStore() @@ -123,6 +121,7 @@ async def create_account_validity_module(config_override={}) -> EmailAccountVali # Make sure the table is created. Don't try to populate with users since we don't # have tables to populate from. + config = EmailAccountValidity.parse_config(config) module = EmailAccountValidity(config, module_api, populate_users=False) await module._store.create_and_populate_table(populate_users=False) From c02b93054fe2583b0a2af811fab89cfd9beda4a8 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 20 May 2021 15:07:35 +0100 Subject: [PATCH 11/26] Incorporate review --- README.md | 14 +---- email_account_validity/_base.py | 71 ++++++++++++++-------- email_account_validity/_store.py | 10 ++- email_account_validity/_utils.py | 9 +-- email_account_validity/account_validity.py | 2 +- email_account_validity/servlets.py | 18 ++---- tests/__init__.py | 1 - 7 files changed, 65 insertions(+), 60 deletions(-) diff --git a/README.md b/README.md index a1710c7..946cf0c 100644 --- a/README.md +++ b/README.md @@ -41,16 +41,6 @@ Also under the HTTP client `listener`, configure an `additional_resource` as per additional_resources: "/_synapse/client/email_account_validity": module: email_account_validity.EmailAccountValidityServlet - config: - # The maximum amount of time an account can stay valid for without being - # renewed. - period: 6w - # Whether to include a link to click in the emails sent to users. If false, - # only a renewal token is sent, in which case it is generated so it's simpler, - # and the user will need to copy it into a compatible client that will send an - # authenticated request to the server. - # Defaults to true. - send_links: true ``` The syntax for durations is the same as in the rest of Synapse's configuration file. @@ -65,13 +55,15 @@ The templates the module will use are: * `notice_expiry.(html|txt)`: The content of the renewal email. It gets passed the following variables: + * `app_name`: The value configured for `app_name` in the Synapse configuration file + (under the `email` section). * `display_name`: The display name of the user needing renewal. * `expiration_ts`: A timestamp in milliseconds representing when the account will expire. Templates can use the `format_ts` (with a date format as the function's parameter) to format this timestamp into a human-readable date. * `url`: The URL the user is supposed to click on to renew their account. If `send_links` is set to `false` in the module's configuration, the value of this - variable will be the token the user must copy into their client. + variable will be `None`. * `renewal_token`: The token to use in order to renew the user's account. If `send_links` is set to `false`, templates should prefer this variable to `url`. * `account_renewed.html`: The HTML to display to a user when they successfully renew diff --git a/email_account_validity/_base.py b/email_account_validity/_base.py index 5a47600..0e74042 100644 --- a/email_account_validity/_base.py +++ b/email_account_validity/_base.py @@ -25,7 +25,11 @@ from email_account_validity import _global from email_account_validity._store import EmailAccountValidityStore -from email_account_validity._utils import random_digit_string, random_string +from email_account_validity._utils import ( + random_digit_string, + random_string, + UNAUTHENTICATED_TOKEN_REGEX, +) logger = logging.getLogger(__name__) @@ -114,7 +118,14 @@ async def send_renewal_email(self, user_id: str, expiration_ts: int): except SynapseError: display_name = user_id - renewal_token = await self.generate_renewal_token(user_id) + # If the user isn't expected to click on a link, but instead to copy the token + # into their client, we generate a different kind of token, simpler and shorter, + # because a) we don't need it to be unique to the whole table and b) we want the + # user to be able to be easily type it back into their client. + if self._send_links: + renewal_token = await self.generate_unauthenticated_renewal_token(user_id) + else: + renewal_token = await self.generate_authenticated_renewal_token(user_id) if self._send_links: url = "%s_synapse/client/email_account_validity/renew?token=%s" % ( @@ -122,12 +133,10 @@ async def send_renewal_email(self, user_id: str, expiration_ts: int): renewal_token, ) else: - # If we're not supposed to send a URL, fallback to the URL being just the - # token. Templates should be using the `renewal_token` variable, but we do - # this so old templates don't break. - url = renewal_token + url = None template_vars = { + "app_name": self._api.email_app_name, "display_name": display_name, "expiration_ts": expiration_ts, "url": url, @@ -147,9 +156,11 @@ async def send_renewal_email(self, user_id: str, expiration_ts: int): await self._store.set_renewal_mail_status(user_id=user_id, email_sent=True) - async def generate_renewal_token(self, user_id: str) -> str: - """Generates a random string that will be inserted into the user's renewal email, - then saves it into the database. + async def generate_authenticated_renewal_token(self, user_id: str) -> str: + """Generates a 8-digit long random string then saves it into the database. + + This token is to be sent to the user over email so that the user can copy it into + their client to renew their account. Args: user_id: ID of the user to generate a string for. @@ -160,19 +171,27 @@ async def generate_renewal_token(self, user_id: str) -> str: Raises: SynapseError(500): Couldn't generate a unique string after 5 attempts. """ - if not self._send_links: - # If the user isn't expected to click on a link, they're expected to enter a - # token manually into their client, which in turn sends the renewal request - # to the server, authenticated with an access token. This means that in this - # case we need the token to be shorter and less complex (hence the 8 digits - # string), but also that we don't need to make the token unique across the - # whole database. - renewal_token = random_digit_string(8) - await self._store.set_renewal_token_for_user( - user_id, renewal_token, unique=False, - ) - return renewal_token + renewal_token = random_digit_string(8) + await self._store.set_renewal_token_for_user( + user_id, renewal_token, unique=False, + ) + return renewal_token + async def generate_unauthenticated_renewal_token(self, user_id: str) -> str: + """Generates a 32-letter long random string then saves it into the database. + + This token is to be sent to the user over email in a link that the user will then + click to renew their account. + + Args: + user_id: ID of the user to generate a string for. + + Returns: + The generated string. + + Raises: + SynapseError(500): Couldn't generate a unique string after 5 attempts. + """ attempts = 0 while attempts < 5: try: @@ -209,10 +228,10 @@ async def renew_account( * An int representing the user's expiry timestamp as milliseconds since the epoch, or 0 if the token was invalid. """ - if self._period is None: - # If a period hasn't been provided in the config, then it means this function - # was called from a place it shouldn't have been, e.g. the /send_mail servlet. - raise SynapseError(500, "Tried to renew account in unexpected place") + # If we were not able to authenticate the user requesting a renewal, and the + # token needs authentication, consider the token neither valid nor stale. + if user_id is None and not UNAUTHENTICATED_TOKEN_REGEX.match(renewal_token): + return False, False, 0 # Verify if the token, or the (token, user_id) tuple, exists. try: @@ -220,7 +239,7 @@ async def renew_account( user_id, current_expiration_ts, token_used_ts, - ) = await self._store.get_user_from_renewal_token(renewal_token, user_id) + ) = await self._store.validate_renewal_token(renewal_token, user_id) except SynapseError: return False, False, 0 diff --git a/email_account_validity/_store.py b/email_account_validity/_store.py index 86f6600..b918dd0 100644 --- a/email_account_validity/_store.py +++ b/email_account_validity/_store.py @@ -291,10 +291,12 @@ def set_renewal_token_for_user_txn(txn: LoggingTransaction): set_renewal_token_for_user_txn, ) - async def get_user_from_renewal_token( + async def validate_renewal_token( self, renewal_token: str, user_id: Optional[str] = None, ) -> Tuple[str, int, Optional[int]]: - """Get a user ID and renewal status from a renewal token. + """Check if the provided renewal token is associating with a user, optionally + validating the user it belongs to as well, and return the account renewal status + of the user it belongs to. Args: renewal_token: The renewal token to perform the lookup with. @@ -309,6 +311,10 @@ async def get_user_from_renewal_token( * 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. + + Raises: + SynapseError(404): The token could not be found (or does not belong to the + provided user, if any). """ def get_user_from_renewal_token_txn(txn: LoggingTransaction): diff --git a/email_account_validity/_utils.py b/email_account_validity/_utils.py index fd0a0cc..165fea7 100644 --- a/email_account_validity/_utils.py +++ b/email_account_validity/_utils.py @@ -13,24 +13,21 @@ # See the License for the specific language governing permissions and # limitations under the License. -import random import re +import secrets import string from typing import Union UNAUTHENTICATED_TOKEN_REGEX = re.compile('^[a-zA-Z]{32}$') -# We use SystemRandom to make sure we get cryptographically-secure randoms. -rand = random.SystemRandom() - def random_string(length: int) -> str: - return "".join(rand.choice(string.ascii_letters) for _ in range(length)) + return "".join(secrets.choice(string.ascii_letters) for _ in range(length)) def random_digit_string(length): - return "".join(rand.choice(string.digits) for _ in range(length)) + return "".join(secrets.choice(string.digits) for _ in range(length)) def parse_duration(value: Union[str, int]) -> int: diff --git a/email_account_validity/account_validity.py b/email_account_validity/account_validity.py index a2a44a2..7026110 100644 --- a/email_account_validity/account_validity.py +++ b/email_account_validity/account_validity.py @@ -106,7 +106,7 @@ async def on_legacy_admin_request(self, request: Request) -> int: """ return await self.set_account_validity_from_request(request) - async def user_expired(self, user_id: str) -> Tuple[bool, bool]: + async def is_user_expired(self, user_id: str) -> Tuple[bool, bool]: """Checks whether a user is expired. Args: diff --git a/email_account_validity/servlets.py b/email_account_validity/servlets.py index 9b1caed..6f76ecc 100644 --- a/email_account_validity/servlets.py +++ b/email_account_validity/servlets.py @@ -30,12 +30,10 @@ from email_account_validity._base import EmailAccountValidityBase from email_account_validity._store import EmailAccountValidityStore -from email_account_validity._utils import UNAUTHENTICATED_TOKEN_REGEX, parse_duration class EmailAccountValidityServlet(Resource): def __init__(self, config: dict, api: ModuleApi): - print("-----------------------------SERVLET-----------------------------") super().__init__() self.putChild(b'renew', EmailAccountValidityRenewServlet(api)) self.putChild(b'send_mail', EmailAccountValiditySendMailServlet(api)) @@ -78,17 +76,11 @@ async def _async_render_GET(self, request): renewal_token = request.args[b"token"][0].decode("utf-8") - user_id = None - if not UNAUTHENTICATED_TOKEN_REGEX.match(renewal_token): - # If the token doesn't look like one we might send as a clickable link via - # email, try to authenticate the request. - try: - requester = await self._api.get_user_by_req(request, allow_expired=True) - except InvalidClientCredentialsError: - respond_with_html(request, 404, self._invalid_token_template.render()) - return - + try: + requester = await self._api.get_user_by_req(request, allow_expired=True) user_id = requester.user.to_string() + except InvalidClientCredentialsError: + user_id = None ( token_valid, @@ -107,7 +99,7 @@ async def _async_render_GET(self, request): expiration_ts=expiration_ts ) else: - status_code = 404 + status_code = 400 response = self._invalid_token_template.render() respond_with_html(request, status_code, response) diff --git a/tests/__init__.py b/tests/__init__.py index fda7fef..b8c8ba5 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -15,7 +15,6 @@ from collections import namedtuple import sqlite3 -import pkg_resources import time from unittest import mock From 0efb4725eacd5d09ac6ce22b78255f5a025277c2 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 20 May 2021 15:15:34 +0100 Subject: [PATCH 12/26] Fix test --- tests/test_account_validity.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/test_account_validity.py b/tests/test_account_validity.py index 1d80b2e..f5f6556 100644 --- a/tests/test_account_validity.py +++ b/tests/test_account_validity.py @@ -39,7 +39,7 @@ async def test_user_expired(self): # Test that, if the user isn't known, the module says it can't determine whether # they've expired. - expired, success = await module.user_expired(user_id=user_id) + expired, success = await module.is_user_expired(user_id=user_id) self.assertFalse(expired) self.assertFalse(success) @@ -51,7 +51,7 @@ async def test_user_expired(self): expiration_ts=one_hour_ahead, ) - expired, success = await module.user_expired(user_id=user_id) + expired, success = await module.is_user_expired(user_id=user_id) self.assertFalse(expired) self.assertTrue(success) @@ -62,7 +62,7 @@ async def test_user_expired(self): expiration_ts=one_hour_ago, ) - expired, success = await module.user_expired(user_id=user_id) + expired, success = await module.is_user_expired(user_id=user_id) self.assertTrue(expired) self.assertTrue(success) @@ -142,7 +142,7 @@ async def test_renewal_token(self): # Insert a row with an expiration timestamp and a renewal token for this user. await module._store.set_expiration_date_for_user(user_id) - await module.generate_renewal_token(user_id) + await module.generate_unauthenticated_renewal_token(user_id) # Retrieve the expiration timestamp and renewal token and check that they're in # the right format. From 5663b3f6cc3ed1524a1417af27173653f2a036b2 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 24 May 2021 12:05:20 +0100 Subject: [PATCH 13/26] Fix is_user_expired to match expected API --- email_account_validity/account_validity.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/email_account_validity/account_validity.py b/email_account_validity/account_validity.py index 7026110..55b6de4 100644 --- a/email_account_validity/account_validity.py +++ b/email_account_validity/account_validity.py @@ -15,7 +15,7 @@ import logging import time -from typing import Any, Tuple +from typing import Any, Tuple, Optional from twisted.web.server import Request @@ -106,7 +106,7 @@ async def on_legacy_admin_request(self, request: Request) -> int: """ return await self.set_account_validity_from_request(request) - async def is_user_expired(self, user_id: str) -> Tuple[bool, bool]: + async def is_user_expired(self, user_id: str) -> Optional[bool]: """Checks whether a user is expired. Args: @@ -120,10 +120,10 @@ async def is_user_expired(self, user_id: str) -> Tuple[bool, bool]: """ expiration_ts = await self._store.get_expiration_ts_for_user(user_id) if expiration_ts is None: - return False, False + return None now_ts = int(time.time() * 1000) - return now_ts >= expiration_ts, True + return now_ts >= expiration_ts async def on_user_registration(self, user_id: str): """Set the expiration timestamp for a newly registered user. From d42ee31129f4f691681e9e3ba20fd7dd726bbee5 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 24 May 2021 12:21:20 +0100 Subject: [PATCH 14/26] Fix tests --- tests/test_account_validity.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/tests/test_account_validity.py b/tests/test_account_validity.py index f5f6556..f30cca2 100644 --- a/tests/test_account_validity.py +++ b/tests/test_account_validity.py @@ -39,10 +39,9 @@ async def test_user_expired(self): # Test that, if the user isn't known, the module says it can't determine whether # they've expired. - expired, success = await module.is_user_expired(user_id=user_id) + expired = await module.is_user_expired(user_id=user_id) - self.assertFalse(expired) - self.assertFalse(success) + self.assertIsNone(expired) # Test that, if the user has an expiration timestamp that's ahead of now, the # module says it can determine that they haven't expired. @@ -51,9 +50,8 @@ async def test_user_expired(self): expiration_ts=one_hour_ahead, ) - expired, success = await module.is_user_expired(user_id=user_id) + expired = await module.is_user_expired(user_id=user_id) self.assertFalse(expired) - self.assertTrue(success) # Test that, if the user has an expiration timestamp that's passed, the module # says it can determine that they have expired. @@ -62,9 +60,8 @@ async def test_user_expired(self): expiration_ts=one_hour_ago, ) - expired, success = await module.is_user_expired(user_id=user_id) + expired = await module.is_user_expired(user_id=user_id) self.assertTrue(expired) - self.assertTrue(success) async def test_on_user_registration(self): user_id = "@izzy:test" From 6978508e96559e875563ab19f0556c136ccef34a Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 1 Jul 2021 18:58:55 +0100 Subject: [PATCH 15/26] Move to the new module system --- email_account_validity/__init__.py | 3 +- email_account_validity/_base.py | 21 ++++------ email_account_validity/_global.py | 29 -------------- .../{servlets.py => _servlets.py} | 22 +++++----- email_account_validity/_store.py | 8 ++-- email_account_validity/account_validity.py | 40 ++++++++++++++----- 6 files changed, 52 insertions(+), 71 deletions(-) delete mode 100644 email_account_validity/_global.py rename email_account_validity/{servlets.py => _servlets.py} (89%) diff --git a/email_account_validity/__init__.py b/email_account_validity/__init__.py index 9cbbbbe..cb039a3 100644 --- a/email_account_validity/__init__.py +++ b/email_account_validity/__init__.py @@ -16,7 +16,6 @@ from pkg_resources import DistributionNotFound, get_distribution from email_account_validity.account_validity import EmailAccountValidity -from email_account_validity.servlets import EmailAccountValidityServlet try: __version__ = get_distribution(__name__).version @@ -24,4 +23,4 @@ # package is not installed pass -__all__ = ["EmailAccountValidity", "EmailAccountValidityServlet"] \ No newline at end of file +__all__ = ["EmailAccountValidity"] \ No newline at end of file diff --git a/email_account_validity/_base.py b/email_account_validity/_base.py index 0e74042..96e8080 100644 --- a/email_account_validity/_base.py +++ b/email_account_validity/_base.py @@ -23,7 +23,6 @@ from synapse.module_api import ModuleApi, parse_json_object_from_request, UserID from synapse.module_api.errors import SynapseError -from email_account_validity import _global from email_account_validity._store import EmailAccountValidityStore from email_account_validity._utils import ( random_digit_string, @@ -35,23 +34,17 @@ class EmailAccountValidityBase: - def __init__(self, config: Any, api: ModuleApi, store: EmailAccountValidityStore): + def __init__(self, config: dict, api: ModuleApi): self._api = api - self._store = store + self._store = EmailAccountValidityStore(config, api) - if _global.config is None: - self._period = config.get("period") - self._send_links = config.get("send_links", True) + self._period = config.get("period") + self._send_links = config.get("send_links", True) - if "renew_email_subject" in config: - renew_email_subject = config["renew_email_subject"] - else: - renew_email_subject = "Renew your %(app)s account" + if "renew_email_subject" in config: + renew_email_subject = config["renew_email_subject"] else: - self._period = _global.config.period - self._send_links = _global.config.send_links - - renew_email_subject = _global.config.renew_email_subject + renew_email_subject = "Renew your %(app)s account" try: app_name = self._api.email_app_name diff --git a/email_account_validity/_global.py b/email_account_validity/_global.py deleted file mode 100644 index 8ebd3ae..0000000 --- a/email_account_validity/_global.py +++ /dev/null @@ -1,29 +0,0 @@ -# -*- coding: utf-8 -*- -# 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. - -from typing import Optional - -import attr - - -@attr.s -class EmailAccountValidityConfig: - period = attr.ib(type=int) - renew_at = attr.ib(type=int) - renew_email_subject = attr.ib(type=str) - send_links = attr.ib(type=bool) - - -config: Optional[EmailAccountValidityConfig] = None \ No newline at end of file diff --git a/email_account_validity/servlets.py b/email_account_validity/_servlets.py similarity index 89% rename from email_account_validity/servlets.py rename to email_account_validity/_servlets.py index 6f76ecc..c53390e 100644 --- a/email_account_validity/servlets.py +++ b/email_account_validity/_servlets.py @@ -47,11 +47,11 @@ def parse_config(config: dict) -> dict: class EmailAccountValidityRenewServlet( EmailAccountValidityBase, DirectServeHtmlResource ): - def __init__(self, api: ModuleApi): + def __init__(self, config: dict, api: ModuleApi, store: EmailAccountValidityStore): self._api = api - self._store = EmailAccountValidityStore(api) + self._store = store - EmailAccountValidityBase.__init__(self, {}, self._api, self._store) + EmailAccountValidityBase.__init__(self, config, self._api) DirectServeHtmlResource.__init__(self) ( @@ -109,12 +109,12 @@ class EmailAccountValiditySendMailServlet( EmailAccountValidityBase, DirectServeJsonResource, ): - def __init__(self, api: ModuleApi): - store = EmailAccountValidityStore(api) - - EmailAccountValidityBase.__init__(self, {}, api, store) + def __init__(self, config: dict, api: ModuleApi, store: EmailAccountValidityStore): + EmailAccountValidityBase.__init__(self, config, api) DirectServeJsonResource.__init__(self) + self._store = store + if not api.public_baseurl: raise ConfigError("Can't send renewal emails without 'public_baseurl'") @@ -133,12 +133,12 @@ class EmailAccountValidityAdminServlet( EmailAccountValidityBase, DirectServeJsonResource, ): - def __init__(self, api: ModuleApi): - store = EmailAccountValidityStore(api) - - EmailAccountValidityBase.__init__(self, {}, api, store) + def __init__(self, config: dict, api: ModuleApi, store: EmailAccountValidityStore): + EmailAccountValidityBase.__init__(self, config, api) DirectServeJsonResource.__init__(self) + self._store = store + async def _async_render_POST(self, request): """On POST requests on /admin, update the given user with the given account validity state, if the requester is a server admin. diff --git a/email_account_validity/_store.py b/email_account_validity/_store.py index b918dd0..43c1be3 100644 --- a/email_account_validity/_store.py +++ b/email_account_validity/_store.py @@ -21,16 +21,14 @@ from synapse.module_api import DatabasePool, LoggingTransaction, ModuleApi, cached from synapse.module_api.errors import SynapseError -from email_account_validity import _global - logger = logging.getLogger(__name__) class EmailAccountValidityStore: - def __init__(self, api: ModuleApi): + def __init__(self, config: dict, api: ModuleApi): self._api = api - self._period = _global.config.period - self._renew_at = _global.config.renew_at + self._period = config.get("period") + self._renew_at = config.get("renew_at") self._expiration_ts_max_delta = self._period * 10.0 / 100.0 self._rand = random.SystemRandom() diff --git a/email_account_validity/account_validity.py b/email_account_validity/account_validity.py index 55b6de4..7379340 100644 --- a/email_account_validity/account_validity.py +++ b/email_account_validity/account_validity.py @@ -22,8 +22,12 @@ from synapse.config._base import ConfigError from synapse.module_api import ModuleApi, run_in_background -from email_account_validity import _global from email_account_validity._base import EmailAccountValidityBase +from email_account_validity._servlets import ( + EmailAccountValidityRenewServlet, + EmailAccountValiditySendMailServlet, + EmailAccountValidityAdminServlet, +) from email_account_validity._store import EmailAccountValidityStore from email_account_validity._utils import parse_duration @@ -32,10 +36,10 @@ class EmailAccountValidity(EmailAccountValidityBase): def __init__(self, config: Any, api: ModuleApi, populate_users: bool = True): - self._store = EmailAccountValidityStore(api) + self._store = EmailAccountValidityStore(config, api) self._api = api - super().__init__(config, self._api, self._store) + super().__init__(config, self._api) run_in_background(self._store.create_and_populate_table, populate_users) self._api.looping_background_call_async( @@ -45,6 +49,29 @@ def __init__(self, config: Any, api: ModuleApi, populate_users: bool = True): if not api.public_baseurl: raise ConfigError("Can't send renewal emails without 'public_baseurl'") + self._api.register_account_validity_callbacks( + is_user_expired=self.is_user_expired, + on_user_registration=self.on_user_registration, + on_legacy_send_mail=self.on_legacy_send_mail, + on_legacy_renew=self.on_legacy_renew, + on_legacy_admin_request=self.on_legacy_admin_request, + ) + + self._api.register_web_resource( + path="/_synapse/client/email_account_validity/renew", + resource=EmailAccountValidityRenewServlet(config, self._api, self._store) + ) + + self._api.register_web_resource( + path="/_synapse/client/email_account_validity/send_mail", + resource=EmailAccountValiditySendMailServlet(config, self._api, self._store) + ) + + self._api.register_web_resource( + path="/_synapse/client/email_account_validity/admin", + resource=EmailAccountValidityAdminServlet(config, self._api, self._store) + ) + @staticmethod def parse_config(config: dict): """Check that the configuration includes the required keys and parse the values @@ -60,13 +87,6 @@ def parse_config(config: dict): config["period"] = parse_duration(config.get("period") or 0) config["renew_at"] = parse_duration(config.get("renew_at") or 0) - _global.config = _global.EmailAccountValidityConfig( - config["period"], - config["renew_at"], - config.get("renewal_email_subject", "Renew your %(app)s account"), - config.get("send_links", True) - ) - return config async def on_legacy_renew(self, renewal_token: str) -> Tuple[bool, bool, int]: From 605337c8a204949644c36a77feba815b492d83e1 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 2 Jul 2021 12:54:07 +0100 Subject: [PATCH 16/26] Use indexes to check unicity on tokens --- email_account_validity/_base.py | 8 +-- email_account_validity/_store.py | 57 ++++++++++++---------- email_account_validity/account_validity.py | 2 +- 3 files changed, 33 insertions(+), 34 deletions(-) diff --git a/email_account_validity/_base.py b/email_account_validity/_base.py index 96e8080..c3a46e2 100644 --- a/email_account_validity/_base.py +++ b/email_account_validity/_base.py @@ -165,9 +165,7 @@ async def generate_authenticated_renewal_token(self, user_id: str) -> str: SynapseError(500): Couldn't generate a unique string after 5 attempts. """ renewal_token = random_digit_string(8) - await self._store.set_renewal_token_for_user( - user_id, renewal_token, unique=False, - ) + await self._store.set_renewal_token_for_user(user_id, renewal_token) return renewal_token async def generate_unauthenticated_renewal_token(self, user_id: str) -> str: @@ -189,9 +187,7 @@ async def generate_unauthenticated_renewal_token(self, user_id: str) -> str: while attempts < 5: try: renewal_token = random_string(32) - await self._store.set_renewal_token_for_user( - user_id, renewal_token, unique=True, - ) + await self._store.set_renewal_token_for_user(user_id, renewal_token) return renewal_token except SynapseError: attempts += 1 diff --git a/email_account_validity/_store.py b/email_account_validity/_store.py index 43c1be3..0d2614c 100644 --- a/email_account_validity/_store.py +++ b/email_account_validity/_store.py @@ -19,10 +19,12 @@ from typing import Dict, List, Optional, Tuple, Union from synapse.module_api import DatabasePool, LoggingTransaction, ModuleApi, cached -from synapse.module_api.errors import SynapseError logger = logging.getLogger(__name__) +_LONG_TOKEN_COLUMN_NAME = "long_renewal_token" +_SHORT_TOKEN_COLUMN_NAME = "short_renewal_token" + class EmailAccountValidityStore: def __init__(self, config: dict, api: ModuleApi): @@ -32,6 +34,13 @@ def __init__(self, config: dict, api: ModuleApi): self._expiration_ts_max_delta = self._period * 10.0 / 100.0 self._rand = random.SystemRandom() + use_long_tokens = config.get("send_links", True) + self._token_column_name = ( + _LONG_TOKEN_COLUMN_NAME + if use_long_tokens + else _SHORT_TOKEN_COLUMN_NAME + ) + async def create_and_populate_table(self, populate_users: bool = True): """Create the email_account_validity table and populate it from other tables from within Synapse. It populates users in it by batches of 100 in order not to clog up @@ -45,9 +54,16 @@ def create_table_txn(txn: LoggingTransaction): user_id TEXT PRIMARY KEY, expiration_ts_ms BIGINT NOT NULL, email_sent BOOLEAN NOT NULL, - renewal_token TEXT, + long_renewal_token TEXT, + short_renewal_token TEXT, token_used_ts_ms BIGINT - ) + ); + + CREATE UNIQUE INDEX IF NOT EXISTS long_renewal_token_idx + ON email_account_validity(long_renewal_token); + + CREATE UNIQUE INDEX IF NOT EXISTS short_renewal_token_idx + ON email_account_validity(short_renewal_token, user_id); """, (), ) @@ -200,7 +216,7 @@ def set_account_validity_for_user_txn(txn: LoggingTransaction): user_id, expiration_ts_ms, email_sent, - renewal_token, + %(token_column_name)s, token_used_ts_ms ) VALUES (?, ?, ?, ?, ?) @@ -208,9 +224,9 @@ def set_account_validity_for_user_txn(txn: LoggingTransaction): SET expiration_ts_ms = EXCLUDED.expiration_ts_ms, email_sent = EXCLUDED.email_sent, - renewal_token = EXCLUDED.renewal_token, + %(token_column_name)s = EXCLUDED.%(token_column_name)s, token_used_ts_ms = EXCLUDED.token_used_ts_ms - """, + """ % {"token_column_name": self._token_column_name}, (user_id, expiration_ts, email_sent, renewal_token, token_used_ts) ) @@ -251,37 +267,24 @@ async def set_renewal_token_for_user( self, user_id: str, renewal_token: str, - unique: bool, ): """Store the given renewal token for the given user. Args: user_id: The user ID to store the renewal token for. renewal_token: The renewal token to store for the user. - unique: Whether the token should be unique across the whole database, i.e. - whether it should be able to look the user up from the token. - - Raises: - SynapseError(409): unique is set to True and the token is already in use. """ def set_renewal_token_for_user_txn(txn: LoggingTransaction): - if unique: - ret = DatabasePool.simple_select_one_onecol_txn( - txn=txn, - table="email_account_validity", - keyvalues={"renewal_token": renewal_token}, - retcol="user_id", - allow_none=True, - ) - - if ret is not None: - raise SynapseError(409, "Renewal token already in use") - + # We don't need to check if the token is unique since we've got unique + # indexes to check that. DatabasePool.simple_update_one_txn( txn=txn, table="email_account_validity", keyvalues={"user_id": user_id}, - updatevalues={"renewal_token": renewal_token, "token_used_ts_ms": None}, + updatevalues={ + self._token_column_name: renewal_token, + "token_used_ts_ms": None, + }, ) await self._api.run_db_interaction( @@ -316,7 +319,7 @@ async def validate_renewal_token( """ def get_user_from_renewal_token_txn(txn: LoggingTransaction): - keyvalues = {"renewal_token": renewal_token} + keyvalues = {self._token_column_name: renewal_token} if user_id is not None: keyvalues["user_id"] = user_id @@ -410,7 +413,7 @@ def get_renewal_token_txn(txn: LoggingTransaction): txn=txn, table="email_account_validity", keyvalues={"user_id": user_id}, - retcol="renewal_token", + retcol=self._token_column_name, ) return await self._api.run_db_interaction( diff --git a/email_account_validity/account_validity.py b/email_account_validity/account_validity.py index 7379340..ba39637 100644 --- a/email_account_validity/account_validity.py +++ b/email_account_validity/account_validity.py @@ -42,7 +42,7 @@ def __init__(self, config: Any, api: ModuleApi, populate_users: bool = True): super().__init__(config, self._api) run_in_background(self._store.create_and_populate_table, populate_users) - self._api.looping_background_call_async( + self._api.looping_background_call( self._send_renewal_emails, 30 * 60 * 1000 ) From 41eaf8f57d596620ec018a2520d164d2fe910876 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 2 Jul 2021 15:15:27 +0100 Subject: [PATCH 17/26] Fix tests --- email_account_validity/_store.py | 38 ++++++++++++++++++++++---------- tests/test_account_validity.py | 4 ++-- 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/email_account_validity/_store.py b/email_account_validity/_store.py index 0d2614c..1c83f7e 100644 --- a/email_account_validity/_store.py +++ b/email_account_validity/_store.py @@ -19,6 +19,7 @@ from typing import Dict, List, Optional, Tuple, Union from synapse.module_api import DatabasePool, LoggingTransaction, ModuleApi, cached +from synapse.module_api.errors import SynapseError logger = logging.getLogger(__name__) @@ -57,13 +58,23 @@ def create_table_txn(txn: LoggingTransaction): long_renewal_token TEXT, short_renewal_token TEXT, token_used_ts_ms BIGINT - ); + ) + """, + (), + ) + txn.execute( + """ CREATE UNIQUE INDEX IF NOT EXISTS long_renewal_token_idx - ON email_account_validity(long_renewal_token); + ON email_account_validity(long_renewal_token) + """, + (), + ) + txn.execute( + """ CREATE UNIQUE INDEX IF NOT EXISTS short_renewal_token_idx - ON email_account_validity(short_renewal_token, user_id); + ON email_account_validity(short_renewal_token, user_id) """, (), ) @@ -277,15 +288,18 @@ async def set_renewal_token_for_user( def set_renewal_token_for_user_txn(txn: LoggingTransaction): # We don't need to check if the token is unique since we've got unique # indexes to check that. - DatabasePool.simple_update_one_txn( - txn=txn, - table="email_account_validity", - keyvalues={"user_id": user_id}, - updatevalues={ - self._token_column_name: renewal_token, - "token_used_ts_ms": None, - }, - ) + try: + DatabasePool.simple_update_one_txn( + txn=txn, + table="email_account_validity", + keyvalues={"user_id": user_id}, + updatevalues={ + self._token_column_name: renewal_token, + "token_used_ts_ms": None, + }, + ) + except Exception: + raise SynapseError(409, "Renewal token already in use") await self._api.run_db_interaction( "set_renewal_token_for_user", diff --git a/tests/test_account_validity.py b/tests/test_account_validity.py index f30cca2..f858727 100644 --- a/tests/test_account_validity.py +++ b/tests/test_account_validity.py @@ -202,12 +202,12 @@ async def test_duplicate_token(self): await module._store.set_expiration_date_for_user(user_id_2) # Set the renewal token. - await module._store.set_renewal_token_for_user(user_id_1, token, True) + await module._store.set_renewal_token_for_user(user_id_1, token) # Try to set the same renewal token for another user. exception = None try: - await module._store.set_renewal_token_for_user(user_id_2, token, True) + await module._store.set_renewal_token_for_user(user_id_2, token) except SynapseError as e: exception = e From 53778644637d522d69bf413151a034925a2b8dd4 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 2 Jul 2021 15:28:09 +0100 Subject: [PATCH 18/26] Update docs --- README.md | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 946cf0c..dfa77ab 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ A Synapse plugin module to manage account validity using validation emails. This module requires: -* Synapse >= 1.34.0 +* Synapse >= 1.38.0 * sqlite3 >= 3.24.0 (if using SQLite with Synapse) ## Installation @@ -20,7 +20,7 @@ pip install synapse-email-account-validity Add the following in your Synapse config: ```yaml -account_validity_modules: +modules: - module: email_account_validity.EmailAccountValidity config: # The maximum amount of time an account can stay valid for without being renewed. @@ -35,14 +35,6 @@ account_validity_modules: send_links: true ``` -Also under the HTTP client `listener`, configure an `additional_resource` as per below: - -```yaml - additional_resources: - "/_synapse/client/email_account_validity": - module: email_account_validity.EmailAccountValidityServlet -``` - The syntax for durations is the same as in the rest of Synapse's configuration file. Configuration parameters with matching names that appear both in `account_validity` and From ccc9a81ff3fbf292a52518a9de32b3467c93e318 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 19 Jul 2021 14:53:33 +0100 Subject: [PATCH 19/26] Migrate to new module interface --- email_account_validity/__init__.py | 3 +- email_account_validity/_base.py | 28 ++++------- .../{servlets.py => _servlets.py} | 40 +++------------ email_account_validity/_store.py | 15 +++--- email_account_validity/_utils.py | 42 ++++++++++++++++ email_account_validity/account_validity.py | 50 +++++++++++++++---- 6 files changed, 108 insertions(+), 70 deletions(-) rename email_account_validity/{servlets.py => _servlets.py} (75%) create mode 100644 email_account_validity/_utils.py diff --git a/email_account_validity/__init__.py b/email_account_validity/__init__.py index 9cbbbbe..cb039a3 100644 --- a/email_account_validity/__init__.py +++ b/email_account_validity/__init__.py @@ -16,7 +16,6 @@ from pkg_resources import DistributionNotFound, get_distribution from email_account_validity.account_validity import EmailAccountValidity -from email_account_validity.servlets import EmailAccountValidityServlet try: __version__ = get_distribution(__name__).version @@ -24,4 +23,4 @@ # package is not installed pass -__all__ = ["EmailAccountValidity", "EmailAccountValidityServlet"] \ No newline at end of file +__all__ = ["EmailAccountValidity"] \ No newline at end of file diff --git a/email_account_validity/_base.py b/email_account_validity/_base.py index 423eb1f..058923e 100644 --- a/email_account_validity/_base.py +++ b/email_account_validity/_base.py @@ -14,17 +14,16 @@ # limitations under the License. import logging +import time from typing import Any, Optional, Tuple from twisted.web.server import Request -from synapse.api.errors import StoreError, SynapseError -from synapse.http.servlet import parse_json_object_from_request -from synapse.module_api import ModuleApi -from synapse.types import UserID -from synapse.util import stringutils +from synapse.module_api import ModuleApi, UserID, parse_json_object_from_request +from synapse.module_api.errors import SynapseError from email_account_validity._store import EmailAccountValidityStore +from email_account_validity._utils import random_string logger = logging.getLogger(__name__) @@ -34,7 +33,7 @@ def __init__(self, config: Any, api: ModuleApi, store: EmailAccountValidityStore self._api = api self._store = store - self._period = config.get("period") + self._period = config["period"] (self._template_html, self._template_text,) = api.read_templates( ["notice_expiry.html", "notice_expiry.txt"], @@ -102,7 +101,7 @@ async def send_renewal_email(self, user_id: str, expiration_ts: int): display_name = profile.display_name if display_name is None: display_name = user_id - except StoreError: + except SynapseError: display_name = user_id renewal_token = await self.generate_renewal_token(user_id) @@ -147,12 +146,12 @@ async def generate_renewal_token(self, user_id: str) -> str: attempts = 0 while attempts < 5: try: - renewal_token = stringutils.random_string(32) + renewal_token = random_string(32) await self._store.set_renewal_token_for_user(user_id, renewal_token) return renewal_token - except StoreError: + except SynapseError: attempts += 1 - raise StoreError(500, "Couldn't generate a unique string as refresh string.") + raise SynapseError(500, "Couldn't generate a unique string as refresh string.") 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 @@ -171,18 +170,13 @@ async def renew_account(self, renewal_token: str) -> Tuple[bool, bool, int]: * An int representing the user's expiry timestamp as milliseconds since the epoch, or 0 if the token was invalid. """ - if self._period is None: - # If a period hasn't been provided in the config, then it means this function - # was called from a place it shouldn't have been, e.g. the /send_mail servlet. - raise SynapseError(500, "Tried to renew account in unexpected place") - try: ( user_id, current_expiration_ts, token_used_ts, ) = await self._store.get_user_from_renewal_token(renewal_token) - except StoreError: + except SynapseError: return False, False, 0 # Check whether this token has already been used. @@ -230,7 +224,7 @@ async def renew_account_for_user( New expiration date for this account, as a timestamp in milliseconds since epoch. """ - now = self._api.current_time_ms() + now = int(time.time() * 1000) if expiration_ts is None: expiration_ts = now + self._period diff --git a/email_account_validity/servlets.py b/email_account_validity/_servlets.py similarity index 75% rename from email_account_validity/servlets.py rename to email_account_validity/_servlets.py index 56e2452..ce0a2b1 100644 --- a/email_account_validity/servlets.py +++ b/email_account_validity/_servlets.py @@ -13,49 +13,27 @@ # See the License for the specific language governing permissions and # limitations under the License. -from twisted.web.resource import Resource - -from synapse.config._base import Config, ConfigError -from synapse.http.server import ( +from synapse.module_api import ( DirectServeHtmlResource, DirectServeJsonResource, + ModuleApi, respond_with_html, ) -from synapse.module_api import ModuleApi -from synapse.module_api.errors import SynapseError +from synapse.module_api.errors import ConfigError, SynapseError from email_account_validity._base import EmailAccountValidityBase from email_account_validity._store import EmailAccountValidityStore -class EmailAccountValidityServlet(Resource): - def __init__(self, config: dict, api: ModuleApi): - super().__init__() - self.putChild(b'renew', EmailAccountValidityRenewServlet(config, api)) - self.putChild(b'send_mail', EmailAccountValiditySendMailServlet(config, api)) - self.putChild(b'admin', EmailAccountValidityAdminServlet(config, api)) - - @staticmethod - def parse_config(config: dict) -> dict: - config["period"] = Config.parse_duration(config.get("period") or 0) - return config - - class EmailAccountValidityRenewServlet( EmailAccountValidityBase, DirectServeHtmlResource ): - def __init__(self, config: dict, api: ModuleApi): + def __init__(self, config: dict, api: ModuleApi, store: EmailAccountValidityStore): self._api = api - self._store = EmailAccountValidityStore(config, api) - EmailAccountValidityBase.__init__(self, config, self._api, self._store) + EmailAccountValidityBase.__init__(self, config, self._api, store) DirectServeHtmlResource.__init__(self) - if "period" in config: - self._period = Config.parse_duration(config["period"]) - else: - raise ConfigError("'period' is required when using account validity") - ( self._account_renewed_template, self._account_previously_renewed_template, @@ -104,9 +82,7 @@ class EmailAccountValiditySendMailServlet( EmailAccountValidityBase, DirectServeJsonResource, ): - def __init__(self, config: dict, api: ModuleApi): - store = EmailAccountValidityStore(config, api) - + def __init__(self, config: dict, api: ModuleApi, store: EmailAccountValidityStore): EmailAccountValidityBase.__init__(self, config, api, store) DirectServeJsonResource.__init__(self) @@ -128,9 +104,7 @@ class EmailAccountValidityAdminServlet( EmailAccountValidityBase, DirectServeJsonResource, ): - def __init__(self, config: dict, api: ModuleApi): - store = EmailAccountValidityStore(config, api) - + def __init__(self, config: dict, api: ModuleApi, store: EmailAccountValidityStore): EmailAccountValidityBase.__init__(self, config, api, store) DirectServeJsonResource.__init__(self) diff --git a/email_account_validity/_store.py b/email_account_validity/_store.py index 4eda21a..24a480d 100644 --- a/email_account_validity/_store.py +++ b/email_account_validity/_store.py @@ -15,11 +15,10 @@ import logging import random +import time from typing import Dict, List, Optional, Tuple, Union -from synapse.module_api import ModuleApi -from synapse.storage.database import DatabasePool, LoggingTransaction -from synapse.util.caches.descriptors import cached +from synapse.module_api import DatabasePool, LoggingTransaction, ModuleApi, cached logger = logging.getLogger(__name__) @@ -27,8 +26,8 @@ class EmailAccountValidityStore: def __init__(self, config: dict, api: ModuleApi): self._api = api - self._period = config.get("period", 0) - self._renew_at = config.get("renew_at") + self._period = config["period"] + self._renew_at = config["renew_at"] self._expiration_ts_max_delta = self._period * 10.0 / 100.0 self._rand = random.SystemRandom() @@ -99,7 +98,7 @@ def populate_table_txn(txn: LoggingTransaction, batch_size: int) -> int: # state includes an expiration timestamp close to now + validity period, but # is slightly randomised to avoid sending huge bursts of renewal emails at # once. - default_expiration_ts = self._api.current_time_ms() + self._period + default_expiration_ts = int(time.time() * 1000) + self._period for user in missing_users: if users_to_insert.get(user["name"]) is None: users_to_insert[user["name"]] = { @@ -164,7 +163,7 @@ def select_users_txn(txn, now_ms, renew_at): return await self._api.run_db_interaction( "get_users_expiring_soon", select_users_txn, - self._api.current_time_ms(), + int(time.time() * 1000), self._renew_at, ) @@ -315,7 +314,7 @@ def set_expiration_date_for_user_txn( Args: user_id: User ID to set an expiration date for. """ - now_ms = self._api.current_time_ms() + now_ms = int(time.time() * 1000) expiration_ts = now_ms + self._period sql = """ diff --git a/email_account_validity/_utils.py b/email_account_validity/_utils.py new file mode 100644 index 0000000..6f8e459 --- /dev/null +++ b/email_account_validity/_utils.py @@ -0,0 +1,42 @@ +import secrets +import string +from typing import Union + + +def parse_duration(value: Union[str, int]) -> int: + """Convert a duration as a string or integer to a number of milliseconds. + + If an integer is provided it is treated as milliseconds and is unchanged. + + String durations can have a suffix of 's', 'm', 'h', 'd', 'w', or 'y'. + No suffix is treated as milliseconds. + + Args: + value: The duration to parse. + + Returns: + The number of milliseconds in the duration. + """ + if isinstance(value, int): + return value + second = 1000 + minute = 60 * second + hour = 60 * minute + day = 24 * hour + week = 7 * day + year = 365 * day + sizes = {"s": second, "m": minute, "h": hour, "d": day, "w": week, "y": year} + size = 1 + suffix = value[-1] + if suffix in sizes: + value = value[:-1] + size = sizes[suffix] + return int(value) * size + + +def random_string(length: int) -> str: + """Generate a cryptographically secure string of random letters. + + Drawn from the characters: `a-z` and `A-Z` + """ + return "".join(secrets.choice(string.ascii_letters) for _ in range(length)) diff --git a/email_account_validity/account_validity.py b/email_account_validity/account_validity.py index 8b5da44..3c4a0dd 100644 --- a/email_account_validity/account_validity.py +++ b/email_account_validity/account_validity.py @@ -14,31 +14,61 @@ # limitations under the License. import logging -from typing import Any, Tuple +import time +from typing import Tuple, Optional from twisted.web.server import Request -from synapse.config._base import Config, ConfigError from synapse.module_api import ModuleApi, run_in_background +from synapse.module_api.errors import ConfigError from email_account_validity._base import EmailAccountValidityBase +from email_account_validity._servlets import ( + EmailAccountValidityAdminServlet, + EmailAccountValidityRenewServlet, + EmailAccountValiditySendMailServlet, +) from email_account_validity._store import EmailAccountValidityStore +from email_account_validity._utils import parse_duration logger = logging.getLogger(__name__) class EmailAccountValidity(EmailAccountValidityBase): - def __init__(self, config: Any, api: ModuleApi, populate_users: bool = True): + def __init__(self, config: dict, api: ModuleApi, populate_users: bool = True): self._store = EmailAccountValidityStore(config, api) self._api = api super().__init__(config, self._api, self._store) run_in_background(self._store.create_and_populate_table, populate_users) - self._api.looping_background_call_async( + self._api.looping_background_call( self._send_renewal_emails, 30 * 60 * 1000 ) + self._api.register_account_validity_callbacks( + is_user_expired=self.is_user_expired, + on_user_registration=self.on_user_registration, + on_legacy_send_mail=self.on_legacy_send_mail, + on_legacy_renew=self.on_legacy_renew, + on_legacy_admin_request=self.on_legacy_admin_request, + ) + + self._api.register_web_resource( + path="/_synapse/client/email_account_validity/renew", + resource=EmailAccountValidityRenewServlet(config, self._api, self._store) + ) + + self._api.register_web_resource( + path="/_synapse/client/email_account_validity/send_mail", + resource=EmailAccountValiditySendMailServlet(config, self._api, self._store) + ) + + self._api.register_web_resource( + path="/_synapse/client/email_account_validity/admin", + resource=EmailAccountValidityAdminServlet(config, self._api, self._store) + ) + if not api.public_baseurl: raise ConfigError("Can't send renewal emails without 'public_baseurl'") @@ -54,8 +84,8 @@ def parse_config(config: dict): "'renew_at' is required when using email account validity" ) - config["period"] = Config.parse_duration(config.get("period") or 0) - config["renew_at"] = Config.parse_duration(config.get("renew_at") or 0) + config["period"] = parse_duration(config["period"]) + config["renew_at"] = parse_duration(config["renew_at"]) return config async def on_legacy_renew(self, renewal_token: str) -> Tuple[bool, bool, int]: @@ -95,7 +125,7 @@ async def on_legacy_admin_request(self, request: Request) -> int: """ return await self.set_account_validity_from_request(request) - async def user_expired(self, user_id: str) -> Tuple[bool, bool]: + async def is_user_expired(self, user_id: str) -> Optional[bool]: """Checks whether a user is expired. Args: @@ -109,10 +139,10 @@ async def user_expired(self, user_id: str) -> Tuple[bool, bool]: """ expiration_ts = await self._store.get_expiration_ts_for_user(user_id) if expiration_ts is None: - return False, False + return None - now_ts = self._api.current_time_ms() - return now_ts >= expiration_ts, True + now_ts = int(time.time() * 1000) + return now_ts >= expiration_ts async def on_user_registration(self, user_id: str): """Set the expiration timestamp for a newly registered user. From f08686325d013d1fbdb22aff9201adad17f7448b Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 19 Jul 2021 15:19:16 +0100 Subject: [PATCH 20/26] Fix tests --- tests/__init__.py | 7 ------- tests/test_account_validity.py | 11 ++++------- 2 files changed, 4 insertions(+), 14 deletions(-) diff --git a/tests/__init__.py b/tests/__init__.py index cc71f36..f7c37ca 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -93,12 +93,6 @@ def _format_ts_filter(value: int, format: str): return [env.get_template(filename) for filename in filenames] -def current_time_ms(): - """Returns the current time in milliseconds. - """ - return int(time.time() * 1000) - - async def get_profile_for_user(user_id): ProfileInfo = namedtuple("ProfileInfo", ("avatar_url", "display_name")) return ProfileInfo(None, "Izzy") @@ -125,7 +119,6 @@ async def create_account_validity_module() -> EmailAccountValidity: module_api = mock.Mock(spec=ModuleApi) module_api.run_db_interaction.side_effect = store.run_db_interaction module_api.read_templates.side_effect = read_templates - module_api.current_time_ms.side_effect = current_time_ms module_api.get_profile_for_user.side_effect = get_profile_for_user module_api.send_mail.side_effect = send_mail diff --git a/tests/test_account_validity.py b/tests/test_account_validity.py index d38e523..b5ed9cb 100644 --- a/tests/test_account_validity.py +++ b/tests/test_account_validity.py @@ -38,10 +38,9 @@ async def test_user_expired(self): # Test that, if the user isn't known, the module says it can't determine whether # they've expired. - expired, success = await module.user_expired(user_id=user_id) + expired = await module.is_user_expired(user_id=user_id) - self.assertFalse(expired) - self.assertFalse(success) + self.assertIsNone(expired) # Test that, if the user has an expiration timestamp that's ahead of now, the # module says it can determine that they haven't expired. @@ -50,9 +49,8 @@ async def test_user_expired(self): expiration_ts=one_hour_ahead, ) - expired, success = await module.user_expired(user_id=user_id) + expired = await module.is_user_expired(user_id=user_id) self.assertFalse(expired) - self.assertTrue(success) # Test that, if the user has an expiration timestamp that's passed, the module # says it can determine that they have expired. @@ -61,9 +59,8 @@ async def test_user_expired(self): expiration_ts=one_hour_ago, ) - expired, success = await module.user_expired(user_id=user_id) + expired = await module.is_user_expired(user_id=user_id) self.assertTrue(expired) - self.assertTrue(success) async def test_on_user_registration(self): user_id = "@izzy:test" From ddc2640af6639d4fd1d9d9a02a677ffd5debcd5a Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 19 Jul 2021 17:01:26 +0100 Subject: [PATCH 21/26] Update README too --- README.md | 55 +++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 39 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index 1c01bbe..8f1531a 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ A Synapse plugin module to manage account validity using validation emails. This module requires: -* Synapse >= 1.34.0 +* Synapse >= 1.38.0 * sqlite3 >= 3.24.0 (if using SQLite with Synapse) ## Installation @@ -17,9 +17,10 @@ pip install synapse-email-account-validity ## Config -Add the following in your Synapse config under `account_validity`: +Add the following in your Synapse config: ```yaml +modules: - module: email_account_validity.EmailAccountValidity config: # The maximum amount of time an account can stay valid for without being renewed. @@ -28,22 +29,44 @@ Add the following in your Synapse config under `account_validity`: renew_at: 1w ``` -Also under the HTTP client `listener`, configure an `additional_resource` as per below: - -```yaml - additional_resources: - "/_synapse/client/email_account_validity": - module: email_account_validity.EmailAccountValidityServlet - config: - # The maximum amount of time an account can stay valid for without being - # renewed. - period: 6w -``` - The syntax for durations is the same as in the rest of Synapse's configuration file. -If they are not already there, copy the [templates](/email_account_validity/templates) -into Synapse's templates directory. +Configuration parameters with matching names that appear both in `account_validity` and +`listeners` __must__ have the same value in both places, otherwise the module will not +behave correctly. + +## Templates + +The templates the module will use are: + +* `notice_expiry.(html|txt)`: The content of the renewal email. It gets passed the + following variables: + * `app_name`: The value configured for `app_name` in the Synapse configuration file + (under the `email` section). + * `display_name`: The display name of the user needing renewal. + * `expiration_ts`: A timestamp in milliseconds representing when the account will + expire. Templates can use the `format_ts` (with a date format as the function's + parameter) to format this timestamp into a human-readable date. + * `url`: The URL the user is supposed to click on to renew their account. If + `send_links` is set to `false` in the module's configuration, the value of this + variable will be `None`. + * `renewal_token`: The token to use in order to renew the user's account. If + `send_links` is set to `false`, templates should prefer this variable to `url`. +* `account_renewed.html`: The HTML to display to a user when they successfully renew + their account. It gets passed the following vaiables: + * `expiration_ts`: A timestamp in milliseconds representing when the account will + expire. Templates can use the `format_ts` (with a date format as the function's + parameter) to format this timestamp into a human-readable date. +* `account_previously_renewed.html`: The HTML to display to a user when they try to renew + their account with a token that's valid but previously used. It gets passed the same + variables as `account_renewed.html`. +* `invalid_token.html`: The HTML to display to a user when they try to renew their account + with the wrong token. It doesn't get passed any variable. + +You can find and change the default templates [here](https://github.com/matrix-org/synapse-email-account-validity/tree/main/email_account_validity/templates). + +Note that the templates directory contains two files that aren't templates (`mail.css` +and `mail-expiry.css`), but are used by email templates to apply visual adjustments. ## Routes From a90ba0a1e98e462092305819c1655f09ac877724 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 19 Jul 2021 17:02:34 +0100 Subject: [PATCH 22/26] Update minimal required synapse version (for real this time) --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index dfa77ab..b142b5a 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ A Synapse plugin module to manage account validity using validation emails. This module requires: -* Synapse >= 1.38.0 +* Synapse >= 1.39.0 * sqlite3 >= 3.24.0 (if using SQLite with Synapse) ## Installation From 74bd1f5c7ef98008a43a5824a7b5da1cedfded6a Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 19 Jul 2021 17:02:34 +0100 Subject: [PATCH 23/26] Update minimal required synapse version (for real this time) --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 8f1531a..c8fe94e 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ A Synapse plugin module to manage account validity using validation emails. This module requires: -* Synapse >= 1.38.0 +* Synapse >= 1.39.0 * sqlite3 >= 3.24.0 (if using SQLite with Synapse) ## Installation From ccb68a64e78e88ce1253676ae07f1feb38830886 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 16 Aug 2021 15:36:16 +0100 Subject: [PATCH 24/26] Incorporate review --- email_account_validity/_base.py | 25 ++++++++----- email_account_validity/_store.py | 60 ++++++++++++++++++++++---------- email_account_validity/_utils.py | 9 +++++ tests/test_account_validity.py | 17 +++++---- 4 files changed, 79 insertions(+), 32 deletions(-) diff --git a/email_account_validity/_base.py b/email_account_validity/_base.py index 3bd05e2..51c7557 100644 --- a/email_account_validity/_base.py +++ b/email_account_validity/_base.py @@ -28,7 +28,7 @@ from email_account_validity._utils import ( random_digit_string, random_string, - UNAUTHENTICATED_TOKEN_REGEX, + UNAUTHENTICATED_TOKEN_REGEX, TokenFormat, ) logger = logging.getLogger(__name__) @@ -123,15 +123,13 @@ async def send_renewal_email(self, user_id: str, expiration_ts: int): # user to be able to be easily type it back into their client. if self._send_links: renewal_token = await self.generate_unauthenticated_renewal_token(user_id) - else: - renewal_token = await self.generate_authenticated_renewal_token(user_id) - if self._send_links: url = "%s_synapse/client/email_account_validity/renew?token=%s" % ( self._api.public_baseurl, renewal_token, ) else: + renewal_token = await self.generate_authenticated_renewal_token(user_id) url = None template_vars = { @@ -171,7 +169,9 @@ async def generate_authenticated_renewal_token(self, user_id: str) -> str: SynapseError(500): Couldn't generate a unique string after 5 attempts. """ renewal_token = random_digit_string(8) - await self._store.set_renewal_token_for_user(user_id, renewal_token) + await self._store.set_renewal_token_for_user( + user_id, renewal_token, TokenFormat.SHORT, + ) return renewal_token async def generate_unauthenticated_renewal_token(self, user_id: str) -> str: @@ -193,7 +193,9 @@ async def generate_unauthenticated_renewal_token(self, user_id: str) -> str: while attempts < 5: try: renewal_token = random_string(32) - await self._store.set_renewal_token_for_user(user_id, renewal_token) + await self._store.set_renewal_token_for_user( + user_id, renewal_token, TokenFormat.LONG, + ) return renewal_token except SynapseError: attempts += 1 @@ -223,9 +225,11 @@ async def renew_account( * An int representing the user's expiry timestamp as milliseconds since the epoch, or 0 if the token was invalid. """ + is_long_token = UNAUTHENTICATED_TOKEN_REGEX.match(renewal_token) + # If we were not able to authenticate the user requesting a renewal, and the # token needs authentication, consider the token neither valid nor stale. - if user_id is None and not UNAUTHENTICATED_TOKEN_REGEX.match(renewal_token): + if user_id is None and not is_long_token: return False, False, 0 # Verify if the token, or the (token, user_id) tuple, exists. @@ -234,7 +238,11 @@ async def renew_account( user_id, current_expiration_ts, token_used_ts, - ) = await self._store.validate_renewal_token(renewal_token, user_id) + ) = await self._store.validate_renewal_token( + renewal_token, + TokenFormat.LONG if is_long_token else TokenFormat.SHORT, + user_id, + ) except SynapseError: return False, False, 0 @@ -291,6 +299,7 @@ async def renew_account_for_user( user_id=user_id, expiration_ts=expiration_ts, email_sent=email_sent, + token_format=TokenFormat.LONG if self._send_links else TokenFormat.SHORT, renewal_token=renewal_token, token_used_ts=now, ) diff --git a/email_account_validity/_store.py b/email_account_validity/_store.py index 0b15a97..b8b5291 100644 --- a/email_account_validity/_store.py +++ b/email_account_validity/_store.py @@ -22,11 +22,15 @@ from synapse.module_api.errors import SynapseError from email_account_validity._config import EmailAccountValidityConfig +from email_account_validity._utils import TokenFormat logger = logging.getLogger(__name__) -_LONG_TOKEN_COLUMN_NAME = "long_renewal_token" -_SHORT_TOKEN_COLUMN_NAME = "short_renewal_token" +# The name of the column to look at for each type of renewal token. +_TOKEN_COLUMN_NAME = { + TokenFormat.LONG: "long_renewal_token", + TokenFormat.SHORT: "short_renewal_token", +} class EmailAccountValidityStore: @@ -37,13 +41,6 @@ def __init__(self, config: EmailAccountValidityConfig, api: ModuleApi): self._expiration_ts_max_delta = self._period * 10.0 / 100.0 self._rand = random.SystemRandom() - use_long_tokens = config.send_links - self._token_column_name = ( - _LONG_TOKEN_COLUMN_NAME - if use_long_tokens - else _SHORT_TOKEN_COLUMN_NAME - ) - async def create_and_populate_table(self, populate_users: bool = True): """Create the email_account_validity table and populate it from other tables from within Synapse. It populates users in it by batches of 100 in order not to clog up @@ -54,11 +51,22 @@ def create_table_txn(txn: LoggingTransaction): txn.execute( """ CREATE TABLE IF NOT EXISTS email_account_validity( + -- The user's Matrix ID. user_id TEXT PRIMARY KEY, + -- The expiration timestamp for this user in milliseconds. expiration_ts_ms BIGINT NOT NULL, + -- Whether a renewal email has already been sent to this user. email_sent BOOLEAN NOT NULL, + -- Long renewal tokens, which are unique to the whole table, so that + -- renewing an account using one doesn't require further + -- authentication. long_renewal_token TEXT, + -- Short renewal tokens, which aren't unique to the whole table, and + -- with which renewing an account requires authentication using an + -- access token. short_renewal_token TEXT, + -- Timestamp at which the renewal token for the user has been used, + -- or NULL if it hasn't been used yet. token_used_ts_ms BIGINT ) """, @@ -194,7 +202,6 @@ def select_users_txn(txn, renew_at): return await self._api.run_db_interaction( "get_users_expiring_soon", select_users_txn, - int(time.time() * 1000), self._renew_at, ) @@ -203,6 +210,7 @@ async def set_account_validity_for_user( user_id: str, expiration_ts: int, email_sent: bool, + token_format: TokenFormat, renewal_token: Optional[str] = None, token_used_ts: Optional[int] = None, ): @@ -216,6 +224,8 @@ async def set_account_validity_for_user( email_sent: True means a renewal email has been sent for this account and there's no need to send another one for the current validity period. + token_format: The configured token format, used to determine which + column to update. 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 @@ -239,7 +249,7 @@ def set_account_validity_for_user_txn(txn: LoggingTransaction): email_sent = EXCLUDED.email_sent, %(token_column_name)s = EXCLUDED.%(token_column_name)s, token_used_ts_ms = EXCLUDED.token_used_ts_ms - """ % {"token_column_name": self._token_column_name}, + """ % {"token_column_name": _TOKEN_COLUMN_NAME[token_format]}, (user_id, expiration_ts, email_sent, renewal_token, token_used_ts) ) @@ -280,12 +290,15 @@ async def set_renewal_token_for_user( self, user_id: str, renewal_token: str, + token_format: TokenFormat, ): """Store the given renewal token for the given user. Args: user_id: The user ID to store the renewal token for. renewal_token: The renewal token to store for the user. + token_format: The configured token format, used to determine which + column to update. """ def set_renewal_token_for_user_txn(txn: LoggingTransaction): # We don't need to check if the token is unique since we've got unique @@ -296,12 +309,12 @@ def set_renewal_token_for_user_txn(txn: LoggingTransaction): table="email_account_validity", keyvalues={"user_id": user_id}, updatevalues={ - self._token_column_name: renewal_token, + _TOKEN_COLUMN_NAME[token_format]: renewal_token, "token_used_ts_ms": None, }, ) except Exception: - raise SynapseError(409, "Renewal token already in use") + raise SynapseError(500, "Failed to update renewal token") await self._api.run_db_interaction( "set_renewal_token_for_user", @@ -309,7 +322,10 @@ def set_renewal_token_for_user_txn(txn: LoggingTransaction): ) async def validate_renewal_token( - self, renewal_token: str, user_id: Optional[str] = None, + self, + renewal_token: str, + token_format: TokenFormat, + user_id: Optional[str] = None, ) -> Tuple[str, int, Optional[int]]: """Check if the provided renewal token is associating with a user, optionally validating the user it belongs to as well, and return the account renewal status @@ -317,6 +333,8 @@ async def validate_renewal_token( Args: renewal_token: The renewal token to perform the lookup with. + token_format: The configured token format, used to determine which + column to update. user_id: The Matrix ID of the user to renew, if the renewal request was authenticated. @@ -330,12 +348,12 @@ async def validate_renewal_token( account has not been renewed using the current token yet. Raises: - SynapseError(404): The token could not be found (or does not belong to the + StoreError(404): The token could not be found (or does not belong to the provided user, if any). """ def get_user_from_renewal_token_txn(txn: LoggingTransaction): - keyvalues = {self._token_column_name: renewal_token} + keyvalues = {_TOKEN_COLUMN_NAME[token_format]: renewal_token} if user_id is not None: keyvalues["user_id"] = user_id @@ -414,11 +432,17 @@ def set_renewal_mail_status_txn(txn: LoggingTransaction): set_renewal_mail_status_txn, ) - async def get_renewal_token_for_user(self, user_id: str) -> str: + async def get_renewal_token_for_user( + self, + user_id: str, + token_format: TokenFormat, + ) -> str: """Retrieve the renewal token for the given user. Args: user_id: Matrix ID of the user to retrieve the renewal token of. + token_format: The configured token format, used to determine which + column to update. Returns: The renewal token for the user. @@ -429,7 +453,7 @@ def get_renewal_token_txn(txn: LoggingTransaction): txn=txn, table="email_account_validity", keyvalues={"user_id": user_id}, - retcol=self._token_column_name, + retcol=_TOKEN_COLUMN_NAME[token_format], ) return await self._api.run_db_interaction( diff --git a/email_account_validity/_utils.py b/email_account_validity/_utils.py index 8225e17..4d258c5 100644 --- a/email_account_validity/_utils.py +++ b/email_account_validity/_utils.py @@ -22,6 +22,15 @@ UNAUTHENTICATED_TOKEN_REGEX = re.compile('^[a-zA-Z]{32}$') +class TokenFormat: + """Supported formats for renewal tokens.""" + + # A LONG renewal token is a 32 letter-long string. + LONG = "long" + # A SHORT renewal token is a 8 digit-long string. + SHORT = "short" + + def random_digit_string(length): return "".join(secrets.choice(string.digits) for _ in range(length)) diff --git a/tests/test_account_validity.py b/tests/test_account_validity.py index f688b4c..c274abe 100644 --- a/tests/test_account_validity.py +++ b/tests/test_account_validity.py @@ -24,7 +24,7 @@ from synapse.module_api.errors import SynapseError -from email_account_validity._utils import UNAUTHENTICATED_TOKEN_REGEX +from email_account_validity._utils import UNAUTHENTICATED_TOKEN_REGEX, TokenFormat from tests import create_account_validity_module @@ -146,7 +146,10 @@ async def test_renewal_token(self): old_expiration_ts = await module._store.get_expiration_ts_for_user(user_id) self.assertIsInstance(old_expiration_ts, int) - renewal_token = await module._store.get_renewal_token_for_user(user_id) + renewal_token = await module._store.get_renewal_token_for_user( + user_id, + TokenFormat.LONG, + ) self.assertIsInstance(renewal_token, str) self.assertGreater(len(renewal_token), 0) self.assertTrue(UNAUTHENTICATED_TOKEN_REGEX.match(renewal_token)) @@ -202,18 +205,20 @@ async def test_duplicate_token(self): await module._store.set_expiration_date_for_user(user_id_2) # Set the renewal token. - await module._store.set_renewal_token_for_user(user_id_1, token) + await module._store.set_renewal_token_for_user(user_id_1, token, TokenFormat.LONG) # Try to set the same renewal token for another user. exception = None try: - await module._store.set_renewal_token_for_user(user_id_2, token) + await module._store.set_renewal_token_for_user( + user_id_2, token, TokenFormat.LONG, + ) except SynapseError as e: exception = e # Check that an exception was raised and that it's the one we're expecting. self.assertIsInstance(exception, SynapseError) - self.assertEqual(exception.code, 409) + self.assertEqual(exception.code, 500) async def test_send_link_false(self): user_id = "@izzy:test" @@ -240,6 +245,6 @@ async def get_threepids(user_id): # Check that the renewal token is in the right format. It should be a 8 digit # long string. - token = await module._store.get_renewal_token_for_user(user_id) + token = await module._store.get_renewal_token_for_user(user_id, TokenFormat.SHORT) self.assertIsInstance(token, str) self.assertTrue(re.compile("^[0-9]{8}$").match(token)) From f9e2c0f97508d666b8cf42833d6b478115c8f324 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 16 Aug 2021 16:36:45 +0200 Subject: [PATCH 25/26] Update README.md Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 6e9b528..2dc2139 100644 --- a/README.md +++ b/README.md @@ -28,7 +28,7 @@ modules: # How long before an account expires should Synapse send it a renewal email. renew_at: 1w # Whether to include a link to click in the emails sent to users. If false, only a - # renewal token is sent, in which case it is generated so it's simpler, and the + # renewal token is sent, in which case a shorter token is used, and the # user will need to copy it into a compatible client that will send an # authenticated request to the server. # Defaults to true. From ee8ee768ee0a9038f2fd2a50f33e35741848ce47 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 16 Aug 2021 16:24:45 +0100 Subject: [PATCH 26/26] Validate short tokens --- email_account_validity/_base.py | 18 ++++++++++++++---- email_account_validity/_servlets.py | 8 -------- email_account_validity/_utils.py | 3 ++- tests/test_account_validity.py | 6 +++--- 4 files changed, 19 insertions(+), 16 deletions(-) diff --git a/email_account_validity/_base.py b/email_account_validity/_base.py index 51c7557..9bddcaf 100644 --- a/email_account_validity/_base.py +++ b/email_account_validity/_base.py @@ -26,9 +26,11 @@ from email_account_validity._config import EmailAccountValidityConfig from email_account_validity._store import EmailAccountValidityStore from email_account_validity._utils import ( + LONG_TOKEN_REGEX, + SHORT_TOKEN_REGEX, random_digit_string, random_string, - UNAUTHENTICATED_TOKEN_REGEX, TokenFormat, + TokenFormat, ) logger = logging.getLogger(__name__) @@ -225,11 +227,19 @@ async def renew_account( * An int representing the user's expiry timestamp as milliseconds since the epoch, or 0 if the token was invalid. """ - is_long_token = UNAUTHENTICATED_TOKEN_REGEX.match(renewal_token) + # Try to match the token against a known format. + if LONG_TOKEN_REGEX.match(renewal_token): + token_format = TokenFormat.LONG + elif SHORT_TOKEN_REGEX.match(renewal_token): + token_format = TokenFormat.SHORT + else: + # If we can't figure out what format the renewal token is, consider it + # invalid. + return False, False, 0 # If we were not able to authenticate the user requesting a renewal, and the # token needs authentication, consider the token neither valid nor stale. - if user_id is None and not is_long_token: + if user_id is None and token_format == TokenFormat.SHORT: return False, False, 0 # Verify if the token, or the (token, user_id) tuple, exists. @@ -240,7 +250,7 @@ async def renew_account( token_used_ts, ) = await self._store.validate_renewal_token( renewal_token, - TokenFormat.LONG if is_long_token else TokenFormat.SHORT, + token_format, user_id, ) except SynapseError: diff --git a/email_account_validity/_servlets.py b/email_account_validity/_servlets.py index 1d996ff..b810502 100644 --- a/email_account_validity/_servlets.py +++ b/email_account_validity/_servlets.py @@ -142,14 +142,6 @@ class EmailAccountValidityAdminServlet( EmailAccountValidityBase, DirectServeJsonResource, ): - def __init__( - self, - config: EmailAccountValidityConfig, - api: ModuleApi, - store: EmailAccountValidityStore, - ): - EmailAccountValidityBase.__init__(self, config, api, store) - DirectServeJsonResource.__init__(self) async def _async_render_POST(self, request): """On POST requests on /admin, update the given user with the given account validity state, if the requester is a server admin. diff --git a/email_account_validity/_utils.py b/email_account_validity/_utils.py index 4d258c5..b1b7f1d 100644 --- a/email_account_validity/_utils.py +++ b/email_account_validity/_utils.py @@ -19,7 +19,8 @@ from typing import Union -UNAUTHENTICATED_TOKEN_REGEX = re.compile('^[a-zA-Z]{32}$') +LONG_TOKEN_REGEX = re.compile('^[a-zA-Z]{32}$') +SHORT_TOKEN_REGEX = re.compile('^[0-9]{8}$') class TokenFormat: diff --git a/tests/test_account_validity.py b/tests/test_account_validity.py index c274abe..ce75d40 100644 --- a/tests/test_account_validity.py +++ b/tests/test_account_validity.py @@ -24,7 +24,7 @@ from synapse.module_api.errors import SynapseError -from email_account_validity._utils import UNAUTHENTICATED_TOKEN_REGEX, TokenFormat +from email_account_validity._utils import LONG_TOKEN_REGEX, SHORT_TOKEN_REGEX, TokenFormat from tests import create_account_validity_module @@ -152,7 +152,7 @@ async def test_renewal_token(self): ) self.assertIsInstance(renewal_token, str) self.assertGreater(len(renewal_token), 0) - self.assertTrue(UNAUTHENTICATED_TOKEN_REGEX.match(renewal_token)) + self.assertTrue(LONG_TOKEN_REGEX.match(renewal_token)) # Sleep a bit so the new expiration timestamp isn't likely to be equal to the # previous one. @@ -247,4 +247,4 @@ async def get_threepids(user_id): # long string. token = await module._store.get_renewal_token_for_user(user_id, TokenFormat.SHORT) self.assertIsInstance(token, str) - self.assertTrue(re.compile("^[0-9]{8}$").match(token)) + self.assertTrue(SHORT_TOKEN_REGEX.match(token))