From 3478213392a231fb5c44698a1df129b0bd7b695f Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 6 Jun 2019 14:16:24 +0100 Subject: [PATCH] Address review comments --- synapse/config/emailconfig.py | 64 ++++++++++----------- synapse/handlers/identity.py | 13 ++++- synapse/push/mailer.py | 2 +- synapse/rest/client/v2_alpha/account.py | 65 ++++++++++----------- synapse/storage/registration.py | 75 +++++++++++++++++++++++-- 5 files changed, 145 insertions(+), 74 deletions(-) diff --git a/synapse/config/emailconfig.py b/synapse/config/emailconfig.py index 06f230781b..296aad943e 100644 --- a/synapse/config/emailconfig.py +++ b/synapse/config/emailconfig.py @@ -50,6 +50,11 @@ class EmailConfig(Config): else: self.email_app_name = "Matrix" + # TODO: Rename notif_from to something more generic, or have a separate + # from for password resets, message notifications, etc? + # Currently the email section is a bit bogged down with settings for + # multiple functions. Would be good to split it out into separate + # sections and only put the common ones under email: self.email_notif_from = email_config.get("notif_from", None) if self.email_notif_from is not None: # make sure it's valid @@ -74,27 +79,27 @@ class EmailConfig(Config): "account_validity", {}, ).get("renew_at") - self.email_enable_password_reset_from_is = email_config.get( - "enable_password_reset_from_is", False, + email_trust_identity_server_for_password_resets = email_config.get( + "trust_identity_server_for_password_resets", False, ) - self.enable_password_resets = ( - self.email_enable_password_reset_from_is - or (not self.email_enable_password_reset_from_is and email_config != {}) + self.email_password_reset_behaviour = ( + "remote" if email_trust_identity_server_for_password_resets else "local" ) - if email_config == {} and not self.email_enable_password_reset_from_is: + if not email_trust_identity_server_for_password_resets and email_config == {}: logger.warn( - "User password resets have been disabled due to lack of email config." + "User password resets have been disabled due to lack of email config" ) + self.email_password_reset_behaviour = "off" - self.email_validation_token_lifetime = email_config.get( - "validation_token_lifetime", 15 * 60, + # Get lifetime of a validation token in milliseconds + self.email_validation_token_lifetime = self.parse_duration( + email_config.get("validation_token_lifetime", "1h") ) if ( self.email_enable_notifs or account_validity_renewal_enabled - or (self.enable_password_resets - and self.email_enable_password_reset_from_is) + or self.email_password_reset_behaviour == "local" ): # make sure we can import the required deps import jinja2 @@ -103,7 +108,7 @@ class EmailConfig(Config): jinja2 bleach - if self.enable_password_resets and not self.email_enable_password_reset_from_is: + if self.email_password_reset_behaviour == "local": required = [ "smtp_host", "smtp_port", @@ -117,7 +122,7 @@ class EmailConfig(Config): if (len(missing) > 0): raise RuntimeError( - "email.enable_password_reset_from_is is False " + "email.password_reset_behaviour is set to 'local' " "but required keys are missing: %s" % (", ".join(["email." + k for k in missing]),) ) @@ -139,8 +144,9 @@ class EmailConfig(Config): if config.get("public_baseurl") is None: raise RuntimeError( - "email.enable_password_reset_from_is is False but no " - "public_baseurl is set" + "email.password_reset_behaviour is set to 'local' but no " + "public_baseurl is set. This is necessary to generate password " + "reset links" ) if self.email_enable_notifs: @@ -200,17 +206,6 @@ class EmailConfig(Config): if not os.path.isfile(p): raise ConfigError("Unable to find email template file %s" % (p, )) - def _get_template_content(self, template_dir, path): - fullpath = os.path.join(template_dir, path) - - try: - with open(fullpath) as f: - return f.read() - except Exception as e: - raise ConfigError( - "Unable to read content of template: %s - %s", fullpath, e, - ) - def default_config(self, config_dir_path, server_name, **kwargs): return """ # Enable sending emails for password resets, notification events or @@ -220,6 +215,7 @@ class EmailConfig(Config): # smtp_pass variables should be used # #email: + # enable_notifs: False # smtp_host: "localhost" # smtp_port: 25 # SSL: 465, STARTTLS: 587 # smtp_user: "exampleusername" @@ -228,10 +224,6 @@ class EmailConfig(Config): # notif_from: "Your Friendly %(app)s Home Server " # app_name: Matrix # - # # Enable sending email notifications for new chat messages - # # - # enable_notifs: False - # # # Enable email notifications by default # notif_for_new_users: True # @@ -240,7 +232,7 @@ class EmailConfig(Config): # # the "app_name" setting is ignored # riot_base_url: "http://localhost/riot" # - # # Disable sending password reset emails via the configured, trusted + # # Enable sending password reset emails via the configured, trusted # # identity servers # # # # IMPORTANT! This will give a malicious or overtaken identity server @@ -248,13 +240,15 @@ class EmailConfig(Config): # # that you want to do this! It is strongly recommended that password # # reset emails be sent by the homeserver instead # # - # #enable_password_reset_from_is: False + # # If this option is set to false and SMTP options have not been + # # configured, resetting user passwords via email will be disabled + # #trust_identity_server_for_password_resets: false # - # # Configure the time in seconds that a validation email or text - # # message code will expire after sending + # # Configure the time that a validation email or text message code + # # will expire after sending # # # # This is currently used for password resets - # #validation_token_lifetime: 900 # 15 minutes + # #validation_token_lifetime: 1h # # # Template directory. All template files should be stored within this # # directory diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py index 22469486d7..04caf65793 100644 --- a/synapse/handlers/identity.py +++ b/synapse/handlers/identity.py @@ -247,7 +247,14 @@ class IdentityHandler(BaseHandler): defer.returnValue(changed) @defer.inlineCallbacks - def requestEmailToken(self, id_server, email, client_secret, send_attempt, **kwargs): + def requestEmailToken( + self, + id_server, + email, + client_secret, + send_attempt, + next_link=None, + ): if not self._should_trust_id_server(id_server): raise SynapseError( 400, "Untrusted ID server '%s'" % id_server, @@ -259,7 +266,9 @@ class IdentityHandler(BaseHandler): 'client_secret': client_secret, 'send_attempt': send_attempt, } - params.update(kwargs) + + if next_link: + params.update({'next_link': next_link}) try: data = yield self.http_client.post_json_get_json( diff --git a/synapse/push/mailer.py b/synapse/push/mailer.py index 005fbc06e2..4bc9eb7313 100644 --- a/synapse/push/mailer.py +++ b/synapse/push/mailer.py @@ -117,7 +117,7 @@ class Mailer(object): link = ( self.hs.config.public_baseurl + - "_matrix/identity/api/v1/validate/email/submitToken" + "_synapse/password_reset/email/submit_token" "?token=%s&client_secret=%s&sid=%s" % (token, client_secret, sid) ) diff --git a/synapse/rest/client/v2_alpha/account.py b/synapse/rest/client/v2_alpha/account.py index bb0c5ed97e..aa75a820bb 100644 --- a/synapse/rest/client/v2_alpha/account.py +++ b/synapse/rest/client/v2_alpha/account.py @@ -46,10 +46,7 @@ class EmailPasswordRequestTokenRestServlet(RestServlet): self.config = hs.config self.identity_handler = hs.get_handlers().identity_handler - if ( - hs.config.enable_password_resets - and not hs.config.email_enable_password_reset_from_is - ): + if self.config.email_password_reset_behaviour == "local": from synapse.push.mailer import Mailer, load_jinja2_templates templates = load_jinja2_templates( config=hs.config, @@ -63,15 +60,9 @@ class EmailPasswordRequestTokenRestServlet(RestServlet): template_text=templates[1], ) - # Create a background job for culling expired 3PID validity tokens - # every minute - hs.get_clock().looping_call( - self.datastore.cull_expired_threepid_validation_tokens, 60 * 1000, - ) - @defer.inlineCallbacks def on_POST(self, request): - if not self.config.enable_password_resets: + if self.config.email_password_reset_behaviour == "off": raise SynapseError(400, "Password resets have been disabled on this server") body = parse_json_object_from_request(request) @@ -80,7 +71,13 @@ class EmailPasswordRequestTokenRestServlet(RestServlet): 'client_secret', 'email', 'send_attempt' ]) - if not check_3pid_allowed(self.hs, "email", body['email']): + # Extract params from body + client_secret = body["client_secret"] + email = body["email"] + send_attempt = body["send_attempt"] + next_link = body.get("next_link") # Optional param + + if not check_3pid_allowed(self.hs, "email", email): raise SynapseError( 403, "Your email domain is not authorized on this server", @@ -88,21 +85,25 @@ class EmailPasswordRequestTokenRestServlet(RestServlet): ) existingUid = yield self.hs.get_datastore().get_user_id_by_threepid( - 'email', body['email'] + 'email', email, ) if existingUid is None: raise SynapseError(400, "Email not found", Codes.THREEPID_NOT_FOUND) - if self.config.email_enable_password_reset_from_is: + if self.config.email_password_reset_behaviour == "remote": if 'id_server' not in body: raise SynapseError(400, "Missing 'id_server' param in body") # Have the identity server handle the password reset flow - ret = yield self.identity_handler.requestEmailToken(**body) + ret = yield self.identity_handler.requestEmailToken( + body["id_server"], email, client_secret, send_attempt, next_link, + ) else: # Send password reset emails from Synapse - sid = yield self.send_password_reset(**body) + sid = yield self.send_password_reset( + email, client_secret, send_attempt, next_link, + ) # Wrap the session id in a JSON object ret = {"sid": sid} @@ -110,7 +111,13 @@ class EmailPasswordRequestTokenRestServlet(RestServlet): defer.returnValue((200, ret)) @defer.inlineCallbacks - def send_password_reset(self, email, client_secret, send_attempt, **kwargs): + def send_password_reset( + self, + email, + client_secret, + send_attempt, + next_link=None, + ): """Send a password reset email Args: @@ -126,17 +133,15 @@ class EmailPasswordRequestTokenRestServlet(RestServlet): """ # Check that this email/client_secret/send_attempt combo is new or # greater than what we've seen previously - ret = yield self.datastore.get_threepid_validation_session( + session = yield self.datastore.get_threepid_validation_session( "email", client_secret, address=email, validated=False, ) - logger.info("Ret is %s", ret) - # Check to see if a session already exists and that it is not yet # marked as validated - if ret and ret.get("validated_at") is None: - session_id = ret['session_id'] - last_send_attempt = ret['last_send_attempt'] + if session and session.get("validated_at") is None: + session_id = session['session_id'] + last_send_attempt = session['last_send_attempt'] # Check that the send_attempt is higher than previous attempts if send_attempt <= last_send_attempt: @@ -165,15 +170,11 @@ class EmailPasswordRequestTokenRestServlet(RestServlet): ) token_expires = (self.hs.clock.time_msec() + - self.config.email_validation_token_lifetime * 1000) + self.config.email_validation_token_lifetime) - yield self.datastore.insert_threepid_validation_token( - session_id, token, kwargs.get("next_link"), token_expires, - ) - - # Save the session_id and send_attempt to the database - yield self.datastore.upsert_threepid_validation_session( - "email", email, client_secret, send_attempt, session_id, + yield self.datastore.start_or_continue_validation_session( + "email", email, session_id, client_secret, send_attempt, + next_link, token, token_expires, ) defer.returnValue(session_id) @@ -190,7 +191,7 @@ class MsisdnPasswordRequestTokenRestServlet(RestServlet): @defer.inlineCallbacks def on_POST(self, request): - if not self.config.enable_password_resets: + if not self.config.email_password_reset_behaviour == "off": raise SynapseError(400, "Password resets have been disabled on this server") body = parse_json_object_from_request(request) diff --git a/synapse/storage/registration.py b/synapse/storage/registration.py index 54200d621d..8ad1911164 100644 --- a/synapse/storage/registration.py +++ b/synapse/storage/registration.py @@ -29,6 +29,8 @@ from synapse.storage._base import SQLBaseStore from synapse.types import UserID from synapse.util.caches.descriptors import cached, cachedInlineCallbacks +THIRTY_MINUTES_IN_MS = 30 * 60 * 1000 + class RegistrationWorkerStore(SQLBaseStore): def __init__(self, db_conn, hs): @@ -37,6 +39,11 @@ class RegistrationWorkerStore(SQLBaseStore): self.config = hs.config self.clock = hs.get_clock() + # Create a background job for culling expired 3PID validity tokens + hs.get_clock().looping_call( + self.cull_expired_threepid_validation_tokens, THIRTY_MINUTES_IN_MS, + ) + @cached() def get_user_by_id(self, user_id): return self._simple_select_one( @@ -1136,15 +1143,15 @@ class RegistrationStore( validated_at=None, ): """Upsert a threepid validation session - Args: medium (str): The medium of the 3PID address (str): The address of the 3PID client_secret (str): A unique string provided by the client to help identify this validation attempt + send_attempt (int): The latest send_attempt on this session session_id (str): The id of this validation session - validated_at (int): The unix timestamp in milliseconds of when - the session was marked as valid + validated_at (int|None): The unix timestamp in milliseconds of + when the session was marked as valid """ insertion_values = { "medium": medium, @@ -1163,12 +1170,70 @@ class RegistrationStore( desc="upsert_threepid_validation_session", ) + def start_or_continue_validation_session( + self, + medium, + address, + session_id, + client_secret, + send_attempt, + next_link, + token, + token_expires, + ): + """Creates a new threepid validation session if it does not already + exist and associates a new validation token with it + + Args: + medium (str): The medium of the 3PID + address (str): The address of the 3PID + session_id (str): The id of this validation session + client_secret (str): A unique string provided by the client to + help identify this validation attempt + send_attempt (int): The latest send_attempt on this session + next_link (str|None): The link to redirect the user to upon + successful validation + token (str): The validation token + token_expires (int): The timestamp for which after the token + will no longer be valid + """ + def start_or_continue_validation_session_txn(txn): + # Create or update a validation session + self._simple_upsert_txn( + txn, + table="threepid_validation_session", + keyvalues={"session_id": session_id}, + values={"last_send_attempt": send_attempt}, + insertion_values={ + "medium": medium, + "address": address, + "client_secret": client_secret, + }, + ) + + # Create a new validation token with this session ID + self._simple_insert_txn( + txn, + table="threepid_validation_token", + values={ + "session_id": session_id, + "token": token, + "next_link": next_link, + "expires": token_expires, + }, + ) + + return self.runInteraction( + "start_or_continue_validation_session", + start_or_continue_validation_session_txn, + ) + def insert_threepid_validation_token( self, session_id, token, - next_link, expires, + next_link=None, ): """Insert a new 3PID validation token and details @@ -1178,6 +1243,8 @@ class RegistrationStore( token (str): The validation token expires (int): The timestamp for which after this token will no longer be valid + next_link (str|None): The link to redirect the user to upon successful + validation """ return self._simple_insert( table="threepid_validation_token",