Address review comments

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(
+        email_trust_identity_server_for_password_resets = email_config.get(
-            "enable_password_reset_from_is", False,
+            "trust_identity_server_for_password_resets", False,
         )
-        self.enable_password_resets = (
+        self.email_password_reset_behaviour = (
-            self.email_enable_password_reset_from_is
+            "remote" if email_trust_identity_server_for_password_resets else "local"
-            or (not self.email_enable_password_reset_from_is and email_config != {})
         )
-        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(
+        # Get lifetime of a validation token in milliseconds
-            "validation_token_lifetime", 15 * 60,
+        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
+            or self.email_password_reset_behaviour == "local"
-                and self.email_enable_password_reset_from_is)
         ):
             # 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 "
+                    "email.password_reset_behaviour is set to 'local' but no "
-                    "public_baseurl is set"
+                    "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 <noreply@example.com>"
         #   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
+        #   # Configure the time that a validation email or text message code
-        #   # message code will expire after sending
+        #   # 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

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(

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)
         )

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 (
+        if self.config.email_password_reset_behaviour == "local":
-            hs.config.enable_password_resets
-            and not hs.config.email_enable_password_reset_from_is
-        ):
             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:
+        if session and session.get("validated_at") is None:
-            session_id = ret['session_id']
+            session_id = session['session_id']
-            last_send_attempt = ret['last_send_attempt']
+            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(
+        yield self.datastore.start_or_continue_validation_session(
-            session_id, token, kwargs.get("next_link"), token_expires,
+            "email", email, session_id, client_secret, send_attempt,
-        )
+            next_link, token, 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,
         )
 
         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)

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
+            validated_at (int|None): The unix timestamp in milliseconds of
-                the session was marked as valid
+                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",