From 5b4a17086d72c6880d471396cb820f633ca8b114 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 6 Oct 2020 14:14:59 -0400 Subject: [PATCH 1/7] Add type hints for account validity. --- mypy.ini | 1 + synapse/handlers/account_validity.py | 13 ++++++++----- synapse/storage/databases/main/registration.py | 4 ++-- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/mypy.ini b/mypy.ini index 5e9f7b1259e4..aa0b2e4a1dd4 100644 --- a/mypy.ini +++ b/mypy.ini @@ -17,6 +17,7 @@ files = synapse/federation, synapse/handlers/_base.py, synapse/handlers/account_data.py, + synapse/handlers/account_validity.py, synapse/handlers/appservice.py, synapse/handlers/auth.py, synapse/handlers/cas_handler.py, diff --git a/synapse/handlers/account_validity.py b/synapse/handlers/account_validity.py index fd4f762f333d..33261ecb7cb7 100644 --- a/synapse/handlers/account_validity.py +++ b/synapse/handlers/account_validity.py @@ -18,7 +18,7 @@ import logging from email.mime.multipart import MIMEMultipart from email.mime.text import MIMEText -from typing import List +from typing import TYPE_CHECKING, List from synapse.api.errors import StoreError from synapse.logging.context import make_deferred_yieldable @@ -26,11 +26,14 @@ from synapse.types import UserID from synapse.util import stringutils +if TYPE_CHECKING: + from synapse.app.homeserver import HomeServer + logger = logging.getLogger(__name__) class AccountValidityHandler: - def __init__(self, hs): + def __init__(self, hs: "HomeServer"): self.hs = hs self.config = hs.config self.store = self.hs.get_datastore() @@ -67,7 +70,7 @@ def __init__(self, hs): self.clock.looping_call(self._send_renewal_emails, 30 * 60 * 1000) @wrap_as_background_process("send_renewals") - async def _send_renewal_emails(self): + async def _send_renewal_emails(self) -> None: """Gets the list of users whose account is expiring in the amount of time configured in the ``renew_at`` parameter from the ``account_validity`` configuration, and sends renewal emails to all of these users as long as they @@ -81,11 +84,11 @@ async def _send_renewal_emails(self): user_id=user["user_id"], expiration_ts=user["expiration_ts_ms"] ) - async def send_renewal_email_to_user(self, user_id: str): + async def send_renewal_email_to_user(self, user_id: str) -> None: expiration_ts = await self.store.get_expiration_ts_for_user(user_id) await self._send_renewal_email(user_id, expiration_ts) - async def _send_renewal_email(self, user_id: str, expiration_ts: int): + async def _send_renewal_email(self, user_id: str, expiration_ts: int) -> None: """Sends out a renewal email to every email address attached to the given user with a unique link allowing them to renew their account. diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index 4c843b76798c..f8886f3f5063 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -236,13 +236,13 @@ async def get_renewal_token_for_user(self, user_id: str) -> str: desc="get_renewal_token_for_user", ) - async def get_users_expiring_soon(self) -> List[Dict[str, int]]: + async def get_users_expiring_soon(self) -> List[Dict[str, Any]]: """Selects users whose account will expire in the [now, now + renew_at] time window (see configuration for account_validity for information on what renew_at refers to). Returns: - A list of dictionaries mapping user ID to expiration time (in milliseconds). + A list of dictionaries, each with a user ID and expiration time (in milliseconds). """ def select_users_txn(txn, now_ms, renew_at): From f2e97490e7ee91877f127d765b070fc78798ff1c Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 21 Oct 2020 14:55:27 -0400 Subject: [PATCH 2/7] Fix some optional strings. --- synapse/handlers/profile.py | 4 ++-- synapse/storage/databases/main/profile.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index 92700b589c82..5adec264216f 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -124,7 +124,7 @@ async def get_profile_from_cache(self, user_id: str) -> JsonDict: profile = await self.store.get_from_remote_profile_cache(user_id) return profile or {} - async def get_displayname(self, target_user: UserID) -> str: + async def get_displayname(self, target_user: UserID) -> Optional[str]: if self.hs.is_mine(target_user): try: displayname = await self.store.get_profile_displayname( @@ -211,7 +211,7 @@ async def set_displayname( await self._update_join_states(requester, target_user) - async def get_avatar_url(self, target_user: UserID) -> str: + async def get_avatar_url(self, target_user: UserID) -> Optional[str]: if self.hs.is_mine(target_user): try: avatar_url = await self.store.get_profile_avatar_url( diff --git a/synapse/storage/databases/main/profile.py b/synapse/storage/databases/main/profile.py index a6d1eb908a5f..0e25ca3d7aa8 100644 --- a/synapse/storage/databases/main/profile.py +++ b/synapse/storage/databases/main/profile.py @@ -39,7 +39,7 @@ async def get_profileinfo(self, user_localpart: str) -> ProfileInfo: avatar_url=profile["avatar_url"], display_name=profile["displayname"] ) - async def get_profile_displayname(self, user_localpart: str) -> str: + async def get_profile_displayname(self, user_localpart: str) -> Optional[str]: return await self.db_pool.simple_select_one_onecol( table="profiles", keyvalues={"user_id": user_localpart}, @@ -47,7 +47,7 @@ async def get_profile_displayname(self, user_localpart: str) -> str: desc="get_profile_displayname", ) - async def get_profile_avatar_url(self, user_localpart: str) -> str: + async def get_profile_avatar_url(self, user_localpart: str) -> Optional[str]: return await self.db_pool.simple_select_one_onecol( table="profiles", keyvalues={"user_id": user_localpart}, From f7c85d1ebdf6f00867d6e800ad17290a3273575e Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 21 Oct 2020 14:55:38 -0400 Subject: [PATCH 3/7] Not all accounts expire. --- synapse/handlers/account_validity.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/account_validity.py b/synapse/handlers/account_validity.py index 33261ecb7cb7..130762af9d59 100644 --- a/synapse/handlers/account_validity.py +++ b/synapse/handlers/account_validity.py @@ -85,8 +85,18 @@ async def _send_renewal_emails(self) -> None: ) async def send_renewal_email_to_user(self, user_id: str) -> None: + """ + Send a renewal email for a specific user. + + Note that if the user is not set to renew at some point in the future then + no email will be sent. + + Args: + user_id: The user ID to send a renewal email for. + """ expiration_ts = await self.store.get_expiration_ts_for_user(user_id) - await self._send_renewal_email(user_id, expiration_ts) + if expiration_ts is not None: + await self._send_renewal_email(user_id, expiration_ts) async def _send_renewal_email(self, user_id: str, expiration_ts: int) -> None: """Sends out a renewal email to every email address attached to the given user From 92431dac1e775ebb016f2b3feecfb08347a0c495 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 21 Oct 2020 14:57:36 -0400 Subject: [PATCH 4/7] Add a newsfragment. --- changelog.d/8620.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/8620.misc diff --git a/changelog.d/8620.misc b/changelog.d/8620.misc new file mode 100644 index 000000000000..35cbe52c8df5 --- /dev/null +++ b/changelog.d/8620.misc @@ -0,0 +1 @@ +Add type hints to the account validity handler. From e33fb0907a2c37b1a0734c01da066d72d7cf35f3 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 22 Oct 2020 12:43:54 -0400 Subject: [PATCH 5/7] Raise an error instead of silently failing. --- changelog.d/8620.bugfix | 1 + changelog.d/8620.misc | 1 - synapse/handlers/account_validity.py | 16 ++++++++++------ 3 files changed, 11 insertions(+), 7 deletions(-) create mode 100644 changelog.d/8620.bugfix delete mode 100644 changelog.d/8620.misc diff --git a/changelog.d/8620.bugfix b/changelog.d/8620.bugfix new file mode 100644 index 000000000000..2a76e4b763dc --- /dev/null +++ b/changelog.d/8620.bugfix @@ -0,0 +1 @@ +Fix a bug where the account validity endpoint would 500 if the user ID did not have an expiration time. It now returns a 400 error. diff --git a/changelog.d/8620.misc b/changelog.d/8620.misc deleted file mode 100644 index 35cbe52c8df5..000000000000 --- a/changelog.d/8620.misc +++ /dev/null @@ -1 +0,0 @@ -Add type hints to the account validity handler. diff --git a/synapse/handlers/account_validity.py b/synapse/handlers/account_validity.py index 130762af9d59..d395b5f5ea86 100644 --- a/synapse/handlers/account_validity.py +++ b/synapse/handlers/account_validity.py @@ -20,7 +20,7 @@ from email.mime.text import MIMEText from typing import TYPE_CHECKING, List -from synapse.api.errors import StoreError +from synapse.api.errors import StoreError, SynapseError from synapse.logging.context import make_deferred_yieldable from synapse.metrics.background_process_metrics import wrap_as_background_process from synapse.types import UserID @@ -88,15 +88,19 @@ async def send_renewal_email_to_user(self, user_id: str) -> None: """ Send a renewal email for a specific user. - Note that if the user is not set to renew at some point in the future then - no email will be sent. - Args: user_id: The user ID to send a renewal email for. + + Raises: + SynapseError if the user is not set to renew. """ expiration_ts = await self.store.get_expiration_ts_for_user(user_id) - if expiration_ts is not None: - await self._send_renewal_email(user_id, expiration_ts) + + # If this user isn't set to be expired, raise an error. + if expiration_ts is None: + raise SynapseError(400, "User has no expiration time: %s" % (user_id, )) + + await self._send_renewal_email(user_id, expiration_ts) async def _send_renewal_email(self, user_id: str, expiration_ts: int) -> None: """Sends out a renewal email to every email address attached to the given user From a3b6bf995077b3c6dcfba8833ed4d35c6db2a5ee Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 22 Oct 2020 12:45:11 -0400 Subject: [PATCH 6/7] Clarify changelog. --- changelog.d/8620.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/8620.bugfix b/changelog.d/8620.bugfix index 2a76e4b763dc..c1078a3fb571 100644 --- a/changelog.d/8620.bugfix +++ b/changelog.d/8620.bugfix @@ -1 +1 @@ -Fix a bug where the account validity endpoint would 500 if the user ID did not have an expiration time. It now returns a 400 error. +Fix a bug where the account validity endpoint would silently fail if the user ID did not have an expiration time. It now returns a 400 error. From 17d5c534220a7571d572e8e44ac4b309f31a4f19 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 22 Oct 2020 14:37:39 -0400 Subject: [PATCH 7/7] Lint --- synapse/handlers/account_validity.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/account_validity.py b/synapse/handlers/account_validity.py index d395b5f5ea86..664d09da1c28 100644 --- a/synapse/handlers/account_validity.py +++ b/synapse/handlers/account_validity.py @@ -98,7 +98,7 @@ async def send_renewal_email_to_user(self, user_id: str) -> None: # If this user isn't set to be expired, raise an error. if expiration_ts is None: - raise SynapseError(400, "User has no expiration time: %s" % (user_id, )) + raise SynapseError(400, "User has no expiration time: %s" % (user_id,)) await self._send_renewal_email(user_id, expiration_ts)